Inheritable Smart Contract Wallet
First Flight #35
Beginner FriendlySolidity
100 EXP
View results
Previous Next
Submission Details
Severity: high
Valid

Incorrect return in InheritanceManager::buyOutEstateNFT() exits function before funds can be transferred to the correct users

vanshikag

Summary

InheritanceManager::buyOutEstateNFT() loops through the beneficiaries array and sends each user their allocated funds. When it reaches msg.sender, it performs an incorrect return statement, preventing any funds being transferred to users after that index. The vulnerable for loop in buyOutEstateNFT():

for (uint256 i = 0; i < beneficiaries.length; i++) {
if (msg.sender == beneficiaries[i]) {
return; // ends the function call
} else {
IERC20(assetToPay).safeTransfer(beneficiaries[i], finalAmount / divisor);
}
}

Impact

Incorrect fund allocation during on-chain NFT settlement, breaking a contract invariant.

Proof of Concept

Copy the following into InheritanceManager.t.sol and run the test:

function test_buyOutEstateNFTIndexBug() public {
// set-up
vm.startPrank(owner);
im.addBeneficiery(alice);
im.addBeneficiery(bob);
im.addBeneficiery(charlie);
im.createEstateNFT("NFT1", 9e6, address(usdc));
vm.stopPrank();
vm.warp(1 + 90 days);
usdc.mint(alice, 6e6);
vm.startPrank(alice); // index 0
usdc.approve(address(im), 6e6);
im.inherit();
// transaction to be tested:
im.buyOutEstateNFT(1);
vm.stopPrank();
assertEq(0, usdc.balanceOf(bob)); // should be 3e6
assertEq(0, usdc.balanceOf(charlie)); // should be 3e6
assertEq(6e6, usdc.balanceOf(address(im))); // should be 0
}

Expected result:

forge test --mt test_buyOutEstateNFTIndexBug -vvv
[⠢] Compiling...
[⠢] Compiling 1 files with Solc 0.8.26
[⠆] Solc 0.8.26 finished in 3.95s
Compiler run successful!
​
Ran 1 test for test/ProofOfConcept.t.sol:InheritanceManagerTest
[PASS] test_buyOutEstateNFTIndexBug() (gas: 379547)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 14.36ms (6.35ms CPU time)
​
Ran 1 test suite in 467.67ms (14.36ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

Recommendation

Remove the return statement. Use continue instead, or leave the if block empty. Alternatively, you can use if (beneficiaries[i] != msg.sender) { //code } and get rid of the else block.

Updates

Lead Judging Commences

0xtimefliez Lead Judge 8 months ago
Submission Judgement Published
Validated
Assigned finding tags:

buyOutNFT has return instead of continue

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.