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

Buying out an estate nft does not properly distribute proceeds realized from sale to remaining beneficiaries

yeahchibyke

Summary

In the event that an owner sets up an estate for their beneficiaries, any of the beneficiaries can buy the estate for a reduced cost, and the proceeds will be shared between the remaining beneficiaries.

Vulnerability Details

A coupe of errors in the InheritanceManager::buyOutEstateNFT() function results in proceeds from the sale of the estate not being properly distributed between the remaining beneficiaries.

Impact

There are 3 impacts of this bug:

  • Where there are more than 2 beneficiaries -let's assume 3-, when one of the beneficiaries buys out the estate, the proceeds is shared into 3(instead of 2) portions

  • Only one of the remaining beneficiaries gets a portion, the remaining portions remain in the portfolio

  • Whoever is the owner can withdraw these remaining portions out

Thus, leaving the remaining beneficiaries without an inheritance.

Tools Used

  • Manual Review

  • Foundry

PoS

Consider this scenario:

  • Jack has a really sick Porsche 911 GT which he sets up as an estate for his 3 beneficiaries (wife, son, and daughter) to inherit by calling ::createEstateNFT()

  • He sets the value of the estate at 150k USDC

  • Son opts to buy out the estate for 100k USDC by calling ::buyOutEstateNFT()

  • Wife and daughter should get 50k USDC each, but wife gets 33.3k USDC, and the balance remains in the portfolio

  • Jack (or whoever is the owner) can withdraw the portfolio

  • Daughter is left with no inheritance

PoC

Add the following test to the InheritanceManagerTest contract:

function test_poc() public {
address wife = makeAddr("wife");
address son = makeAddr("son");
address daughter = makeAddr("daughter");
​
usdc.mint(son, 100_000e18);
​
vm.startPrank(owner);
im.createEstateNFT("Porsche GT 911", 150_000e18, address(usdc)); // owner sets up estate
im.addBeneficiery(wife);
im.addBeneficiery(son);
im.addBeneficiery(daughter);
vm.stopPrank();
​
vm.warp(1 + 90 days);
​
vm.startPrank(son);
im.inherit();
usdc.approve(address(im), 100_000e18);
im.buyOutEstateNFT(1); // son buys out estate for 100k, but only wife gets paid, and she gets 33.3k
vm.stopPrank();
​
uint256 bal = usdc.balanceOf(address(im));
assert(bal > 0); // 66.6k still remains in the portfolio
assert(owner == im.getOwner());
​
vm.prank(owner);
im.sendERC20(address(usdc), bal, owner); // owner withdraws the portfolio balance
}

Recommendations

The security flaw exists in the buyOutEstateNFT() function, and particularly in the way it calculates distribution of proceeds from sale of estate.

Make the following corrections:

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

Changes made:

  • Changed return to continue

  • For distribution of proceeds, divided finalAmount by multiplier instead of divisor

Updates

Lead Judging Commences

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