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

InheritanceManager::buyOutEstateNFT() divides finalAmount by the wrong number, leading to incorrect division of funds

vanshikag

Summary

InheritanceManager::buyOutEstateNFT() can be used by one of the beneficiaries to buy out an NFT by paying the other beneficiaries their share of NFT value. Two variables are defined in the scope of this function that are relevant to this bug. divisor is the total number of beneficiaries. multiplier is the total number of beneficiaries minus msg.sender. The finalAmount is the amount to be payed by the msg.sender. This value should be divided by multiplier but is instead divided by divisor to settle the purchase of the NFT.

Impact

Incorrect fund allocation, leading to users receiving lower funds than they should. This breaks a contract invariant.

Proof Of Concept

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

function test_buyOutEstateNFTMultiplierBug() public {
// set-up
vm.startPrank(owner);
im.addBeneficiery(alice);
im.addBeneficiery(bob);
im.addBeneficiery(charlie);
im.createEstateNFT("NFT1", 9e6, address(usdc));
im.createEstateNFT("NFT2", 10e6, address(usdc));
vm.stopPrank();
vm.warp(1 + 90 days);
usdc.mint(charlie, 6e6);
vm.startPrank(charlie);
usdc.approve(address(im), 9e6);
im.inherit();
// transaction to be tested:
im.buyOutEstateNFT(1);
vm.stopPrank();
// vulnerable code: IERC20(assetToPay).safeTransfer(beneficiaries[i], finalAmount / divisor);
// correct: IERC20(assetToPay).safeTransfer(beneficiaries[i], finalAmount / multiplier);
assertEq(2e6, usdc.balanceOf(bob)); // should be 3e6
assertEq(2e6, usdc.balanceOf(alice)); // should be 3e6
assertEq(2e6, usdc.balanceOf(address(im))); // should be 0
}

Expected Result:

forge test --mt test_buyOutEstateNFTMultiplierBug -vvv
[⠢] Compiling...
No files changed, compilation skipped
​
Ran 1 test for test/ProofOfConcept.t.sol:InheritanceManagerTest
[PASS] test_buyOutEstateNFTMultiplierBug() (gas: 429954)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 13.24ms (962.58µs CPU time)
​
Ran 1 test suite in 707.13ms (13.24ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

Recommendation

Bug fix:

- IERC20(assetToPay).safeTransfer(beneficiaries[i], finalAmount / divisor);
+ IERC20(assetToPay).safeTransfer(beneficiaries[i], finalAmount / multiplier);

Give variable names that are reflective of what the variables are and not where they're used, in case they need to be used in multiple contexts. For example, you could rename divisor as numberOfBeneficiaries and multiplier as beneficiariesMinusOne, which would make this kind of bug less likely.

Updates

Lead Judging Commences

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

buyOutNFT has wrong denominator

Support

FAQs

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