Submission Details
Severity:
Early Return in BuyoutEstateNFT Causes Complete Payment Distribution Failure
Summary
When the first beneficiary executes buyOutEstateNFT, an early return statement prevents any fund distribution to other beneficiaries, despite the contract successfully collecting payment. This creates an immediate financial loss for other beneficiaries making them receives absolutely nothing from their entitled shares from the buy out of the estate nft.
Vulnerability Details
Affected Code :
Root Cause :
for (uint256 i = 0; i < beneficiaries.length; i++) {
// Hits return immediately in first loop iteration
// preventing fund distribution for all the remaining beneficiaries
if (msg.sender == beneficiaries[i]) {
return;
} else {
IERC20(assetToPay).safeTransfer(beneficiaries[i], finalAmount / divisor);
}
For a 1000 USDC NFT with 2 beneficiaries:
-
First beneficiary pays: 500 USDC
-
Loop starts, finds beneficiary at index 0
-
Returns immediately
-
Results:
No distribution to second beneficiary
Full payment remains in contract
POC :
function test_firstBeneficiaryBuyoutDistributionFailure() public {
// Setup
address user2 = makeAddr("user2");
vm.startPrank(owner);
im.addBeneficiery(user1); // First beneficiary
im.addBeneficiery(user2);
im.createEstateNFT("valuable estate", 1000e6, address(usdc));
vm.stopPrank();
// Enable inheritance
vm.warp(block.timestamp + 91 days);
vm.prank(user1);
im.inherit();
// First beneficiary buys out
usdc.mint(user1, 1000e6);
vm.startPrank(user1);
usdc.approve(address(im), 1000e6);
uint256 user2BalanceBefore = usdc.balanceOf(user2);
im.buyOutEstateNFT(1);
assertEq(
usdc.balanceOf(user2),
user2BalanceBefore,
"Second beneficiary received no payment"
);
}
POC Result :
Ran 1 test for test/InheritanceManagerTest.t.sol:InheritanceManagerTest
[PASS] test_firstBeneficiaryBuyoutDistributionFailure() (gas: 389228)
Traces:
[389228] InheritanceManagerTest::test_firstBeneficiaryBuyoutDistributionFailure()
├─ [0] VM::addr(<pk>) [staticcall]
│ └─ ← [Return] user2: [0x537C8f3d3E18dF5517a58B3fB9D9143697996802]
├─ [0] VM::label(user2: [0x537C8f3d3E18dF5517a58B3fB9D9143697996802], "user2")
│ └─ ← [Return]
├─ [0] VM::startPrank(owner: [0x7c8999dC9a822c1f0Df42023113EDB4FDd543266])
│ └─ ← [Return]
├─ [69020] InheritanceManager::addBeneficiery(user1: [0x29E3b139f4393aDda86303fcdAa35F60Bb7092bF])
│ └─ ← [Stop]
├─ [23120] InheritanceManager::addBeneficiery(user2: [0x537C8f3d3E18dF5517a58B3fB9D9143697996802])
│ └─ ← [Stop]
├─ [145826] InheritanceManager::createEstateNFT("valuable estate", 1000000000 [1e9], ERC20Mock: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f])
│ ├─ [95512] NFTFactory::createEstate("valuable estate")
│ │ ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: InheritanceManager: [0x88F59F8826af5e695B13cA934d6c7999875A9EeA], tokenId: 1)
│ │ ├─ emit MetadataUpdate(_tokenId: 1)
│ │ └─ ← [Return] 1
│ └─ ← [Stop]
├─ [0] VM::stopPrank()
│ └─ ← [Return]
├─ [0] VM::warp(7862401 [7.862e6])
│ └─ ← [Return]
├─ [0] VM::prank(user1: [0x29E3b139f4393aDda86303fcdAa35F60Bb7092bF])
│ └─ ← [Return]
├─ [22686] InheritanceManager::inherit()
│ └─ ← [Stop]
├─ [46784] ERC20Mock::mint(user1: [0x29E3b139f4393aDda86303fcdAa35F60Bb7092bF], 1000000000 [1e9])
│ ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: user1: [0x29E3b139f4393aDda86303fcdAa35F60Bb7092bF], value: 1000000000 [1e9])
│ └─ ← [Stop]
├─ [0] VM::startPrank(user1: [0x29E3b139f4393aDda86303fcdAa35F60Bb7092bF])
│ └─ ← [Return]
├─ [24735] ERC20Mock::approve(InheritanceManager: [0x88F59F8826af5e695B13cA934d6c7999875A9EeA], 1000000000 [1e9])
│ ├─ emit Approval(owner: user1: [0x29E3b139f4393aDda86303fcdAa35F60Bb7092bF], spender: InheritanceManager: [0x88F59F8826af5e695B13cA934d6c7999875A9EeA], value: 1000000000 [1e9])
│ └─ ← [Return] true
├─ [2559] ERC20Mock::balanceOf(user2: [0x537C8f3d3E18dF5517a58B3fB9D9143697996802]) [staticcall]
│ └─ ← [Return] 0
├─ [28634] InheritanceManager::buyOutEstateNFT(1)
│ ├─ [26058] ERC20Mock::transferFrom(user1: [0x29E3b139f4393aDda86303fcdAa35F60Bb7092bF], InheritanceManager: [0x88F59F8826af5e695B13cA934d6c7999875A9EeA], 500000000 [5e8])
│ │ ├─ emit Transfer(from: user1: [0x29E3b139f4393aDda86303fcdAa35F60Bb7092bF], to: InheritanceManager: [0x88F59F8826af5e695B13cA934d6c7999875A9EeA], value: 500000000 [5e8])
│ │ └─ ← [Return] true
│ └─ ← [Stop]
├─ [559] ERC20Mock::balanceOf(user2: [0x537C8f3d3E18dF5517a58B3fB9D9143697996802]) [staticcall]
│ └─ ← [Return] 0
├─ [0] VM::assertEq(0, 0, "Second beneficiary received no payment") [staticcall]
│ └─ ← [Return]
└─ ← [Stop]
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 5.25ms (1.18ms CPU time)
Impact
Complete payment loss for other beneficiaries as they receive nothing from the buyout.
Breaks estate settlement process
Violates project's core purpose: "settle the financial claims fair on-chain"
Position in beneficiary array determines ability to exploit
Tools Used
Manual Review and Foundry
Recommendations
Use continue instead of return to prevent this :
function buyOutEstateNFT(uint256 _nftID) external onlyBeneficiaryWithIsInherited {
uint256 value = nftValue[_nftID];
uint256 divisor = beneficiaries.length;
uint256 multiplier = beneficiaries.length - 1;
uint256 finalAmount = (value / divisor) * multiplier;
IERC20(assetToPay).safeTransferFrom(msg.sender, address(this), finalAmount);
for (uint256 i = 0; i < beneficiaries.length; i++) {
if (msg.sender == beneficiaries[i]) {
- return;
+ continue;
}
IERC20(assetToPay).safeTransfer(beneficiaries[i], finalAmount / divisor);
}
nft.burnEstate(_nftID);
}
Updates
Lead Judging Commences
0xtimefliez 6 months ago
Submission Judgement Published Assigned finding tags:
buyOutNFT has return instead of continue
Rewards breakdown
nSLOC
211This contest has a flat reward structure with the following payouts:
100 EXP
20 EXP
2 EXP
Live
The contest is live. Earn rewards by submitting a finding.
Appeals
This is your time to appeal against judgements on your submissions.
Appeals Review
Appeals are being carefully reviewed by our judges.