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

Early return in `InheritanceManager::buyOutEstateNFT` can lead to beneficiaries not receiving their shares

stratan

Summary

buyOutEstateNFT allows one beneficiary (the caller) to take control of an underlying asset by distributing to the rest of the beneficiaries their corresponding shares (buying them out). However, unless the caller is the last beneficiary, later beneficiaries will not receive their shares.

Vulnerability Details

A beneficiary can call InheritanceManager::buyOutEstateNFT to buy out a Real World Assert (e.g real estate) represented by an NFT.

function buyOutEstateNFT(
uint256 _nftID
) external onlyBeneficiaryWithIsInherited {
uint256 value = nftValue[_nftID];
uint256 divisor = beneficiaries.length;
uint256 multiplier = beneficiaries.length - 1;
uint256 finalAmount = (value / divisor) * multiplier;
// [1]
IERC20(assetToPay).safeTransferFrom(
msg.sender,
address(this),
finalAmount
);
for (uint256 i = 0; i < beneficiaries.length; i++) {
// bug
if (msg.sender == beneficiaries[i]) {
return;
} else {
// [2]
IERC20(assetToPay).safeTransfer(
beneficiaries[i],
finalAmount / divisor
);
}
}
// [3]
nft.burnEstate(_nftID);
}

The caller-beneficiary pays finalAmount tokens into the contract ([1]). Then, the contract distributes this amount equally among all other beneficiaries (excluding the caller) ([2]). Finally, the NFT is burned, removing it from circulation ([3]). The for-loop will check if beneficiaries[i] corresponds to msg.sender. If that's the case, buyOutEstateNFT will return. However, unless the caller is the last beneficiary, later beneficiaries will not receive their shares.

Proof of Concept

Consider the following scenario:

  1. owner calls createEstateNFT

  2. owner adds alice as beneficiary

    • beneficiary[0] == alice

  3. owner adds bob as beneficiary

    • beneficiary[1] == bob

  4. deadline expires

  5. alice calls inherit

  6. alice calls buyOutEstateNFT

  7. buyOutEstateNFT will return early since beneficiary[0] == alice

    • bob doesn't receive his token share

Code

Place test_buyOutEstateTokenShareLoss in InheritanceManagerTest.t.sol:

function test_buyOutEstateTokenShareLoss() public {
address bob = makeAddr("bob");
address alice = makeAddr("alice");
vm.startPrank(owner);
im.addBeneficiery(alice);
im.addBeneficiery(bob);
im.createEstateNFT("beach-house", 4e6, address(usdc));
vm.stopPrank();
usdc.mint(alice, 4e6);
vm.warp(1 + 90 days);
vm.startPrank(alice);
usdc.approve(address(im), 4e6);
im.inherit();
im.buyOutEstateNFT(1);
vm.stopPrank();
assertEq(usdc.balanceOf(address(bob)), 0);
}

And run the test:

$ forge test --mp test/InheritanceManagerTest.t.sol --mt test_buyOutEstateTokenShareLoss
[⠊] Compiling...
No files changed, compilation skipped
​
Ran 1 test for test/InheritanceManagerTest.t.sol:InheritanceManagerTest
[PASS] test_buyOutEstateTokenShareLoss() (gas: 386820)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.37ms (272.42µs CPU time)
​
Ran 1 test suite in 139.99ms (1.37ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

Impact

Fund inheritance is a core feature of the InheritanceManager contract. The premature loop-exit return in buyOutEstateNFT can break the distribution logic.

Tools Used

  • Manual review

  • Foundry

Recommendations

Adjust the if-check and remove the early return.

function buyOutEstateNFT(uint256 _nftID) external onlyBeneficiaryWithIsInherited {
// ...
​
for (uint256 i = 0; i < divisor; i++) {
// fix
if (msg.sender != beneficiaries[i]) {
IERC20(assetToPay).safeTransfer(beneficiaries[i], finalAmount / divisor);
}
}
​
// ...
}
Updates

Lead Judging Commences

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

Give us feedback!