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

`return` keyword in for loop in `InheritanceManager::buyOutEstateNFT` will cause beneficiaries to not receive their payments

uddercover

Summary

The function uses a for loop to distribute beneficiary payments but includes an if block that returns if beneficiary[i] is the msg.sender. This is an issue because the return keyword completely exits the function, causing:

  • msg.sender to lose their funds used for payment

  • some or all of the remaining beneficiaries to not receive their payments

  • the nft to never be burnt

Vulnerability Details

The issue occurs here:

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;
} else {
IERC20(assetToPay).safeTransfer(beneficiaries[i], finalAmount / divisor);
}
}
nft.burnEstate(_nftID);
}

POC

Add the below test function to `InheritanceManagerTest.t.sol`

function testFundDistributionIssueInBuyingEstateNFT() public {
address user2 = makeAddr("user2");
address user3 = makeAddr("user3");
uint256 nftValue = 4e6;
string memory tokenUri = "our beach-house";
​
vm.warp(1);
vm.startPrank(owner);
im.addBeneficiery(user1);
im.addBeneficiery(user2);
im.addBeneficiery(user3);
im.createEstateNFT(tokenUri, nftValue, address(usdc));
vm.stopPrank();
​
usdc.mint(user1, nftValue);
usdc.mint(user2, nftValue);
usdc.mint(user3, nftValue);
vm.warp(1 + 90 days);
​
//The first user tries to buy out NFT but loses their funds instead
uint256 user1BalanceBefore = usdc.balanceOf(user1);
uint256 user2BalanceBefore = usdc.balanceOf(user2);
uint256 user3BalanceBefore = usdc.balanceOf(user3);
uint256 imBalanceBefore = usdc.balanceOf(address(im));
​
vm.startPrank(user1);
usdc.approve(address(im), nftValue);
im.inherit();
im.buyOutEstateNFT(1);
vm.stopPrank();
​
uint256 user1BalanceAfter = usdc.balanceOf(user1);
uint256 user2BalanceAfter = usdc.balanceOf(user2);
uint256 user3BalanceAfter = usdc.balanceOf(user3);
uint256 imBalanceAfter = usdc.balanceOf(address(im));
​
assertGt(imBalanceAfter, imBalanceBefore);
assertLt(user1BalanceAfter, user1BalanceBefore);
assertEq(user2BalanceAfter, user1BalanceBefore);
assertEq(user3BalanceAfter, user3BalanceBefore);
​
//The second user tries to buy out NFT but loses some of their funds as only user1 gets paid
user1BalanceBefore = usdc.balanceOf(user1);
user2BalanceBefore = usdc.balanceOf(user2);
user3BalanceBefore = usdc.balanceOf(user3);
imBalanceBefore = usdc.balanceOf(address(im));
​
vm.startPrank(user2);
usdc.approve(address(im), nftValue);
im.inherit();
im.buyOutEstateNFT(1);
vm.stopPrank();
​
user1BalanceAfter = usdc.balanceOf(user1);
user2BalanceAfter = usdc.balanceOf(user2);
user3BalanceAfter = usdc.balanceOf(user3);
imBalanceAfter = usdc.balanceOf(address(im));
​
assertGt(imBalanceAfter, imBalanceBefore);
assertLt(user2BalanceAfter, user2BalanceBefore);
assertGt(user1BalanceAfter, user1BalanceBefore);
assertEq(user3BalanceAfter, user3BalanceBefore);
​
//Only the last user can successfully buy out nft
user1BalanceBefore = usdc.balanceOf(user1);
user2BalanceBefore = usdc.balanceOf(user2);
user3BalanceBefore = usdc.balanceOf(user3);
imBalanceBefore = usdc.balanceOf(address(im));
​
vm.startPrank(user3);
usdc.approve(address(im), nftValue);
im.inherit();
im.buyOutEstateNFT(1);
vm.stopPrank();
​
user1BalanceAfter = usdc.balanceOf(user1);
user2BalanceAfter = usdc.balanceOf(user2);
user3BalanceAfter = usdc.balanceOf(user3);
imBalanceAfter = usdc.balanceOf(address(im));
​
assertGt(imBalanceAfter, imBalanceBefore);
assertLt(user3BalanceAfter, user3BalanceBefore);
assertGt(user1BalanceAfter, user1BalanceBefore);
assertGt(user2BalanceAfter, user2BalanceBefore);
}

Impact

  • The paying beneficiary msg.sender, loses their funds used for payment

  • Some or all of the remaining beneficiaries would not receive their payments

  • The estate nft would never be burnt

Tools Used

Manual review, foundry test suite

Recommendations

Use the continue keyword instead

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
} else {
IERC20(assetToPay).safeTransfer(beneficiaries[i], finalAmount / divisor);
}
}
nft.burnEstate(_nftID);
}
```
Updates

Lead Judging Commences

0xtimefliez Lead Judge 6 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.