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

The buyOutEstateNFT function has severe logic flaws causing incomplete asset distribution and failed NFT burning

0xWithdrawers

Description

The buyOutEstateNFT() function in the InheritanceManager contract has multiple implementation flaws that prevent it from functioning as intended. The function is designed to allow a beneficiary to buy out an estate NFT and distribute the payment to other beneficiaries in an equal way.

However, the function contains a misplaced return statement, incorrect calculation and distribution of funds, and fails to complete critical operations such as burning the bought 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;
IERC20(assetToPay).safeTransferFrom(msg.sender, address(this), finalAmount);
for (uint256 i = 0; i < beneficiaries.length; i++) {
if (msg.sender == beneficiaries[i]) {
return; // Early return that breaks the distribution
} else {
IERC20(assetToPay).safeTransfer(beneficiaries[i], finalAmount / divisor);
}
}
nft.burnEstate(_nftID); // Never reached if early return happens
}

The key issues are:

  • The return statement causes the function to exit prematurely once the buyer is found in the beneficiaries array. All beneficiaries which are at an
    index AFTER the beneficiary calling the function won't be paid because of this function early return.

  • The calculation and distribution of funds results in incorrect amounts being paid to beneficiaries because of a logical issue in the amount computation to be paid to each beneficiary.

  • The NFT never gets burned because the early return prevents execution from reaching the burn statement.

Impact

These issues have various impacts:

  1. Incomplete asset distribution: Beneficiaries that come after the buyer in the array never receive their share of the payment. This leads to a fundamentally unfair distribution system where some beneficiaries are paid while others receive nothing.

  2. Incorrect payment amounts: Even for beneficiaries who do receive payments, the amounts are incorrect. The test below shows that beneficiaries receive 3.75 MOCK tokens instead of the expected 5 MOCK tokens, meaning they receive 25% less than they should (in this example, it depends on the actual count of beneficiaries)

  3. NFT state inconsistency: The NFT is never burned or marked as processed despite the buyer having paid for it. This creates a problematic state where:

  4. Funds locked in contract: A significant portion of funds paid by the buyer remains locked in the contract with no mechanism to distribute them.

Proof-Of-Concept

// SPDX-License-Identifier: MIT
pragma solidity 0.8.26;
​
import "forge-std/Test.sol";
import "../../src/InheritanceManager.sol";
import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import "@openzeppelin/contracts/token/ERC20/ERC20.sol";
​
/** [VULN 11]
* This PoC demonstrates an incomplete buyout process implementation in the `buyOutEstateNFT()`function
*/
​
// Mock ERC20 token for testing
contract MockToken is ERC20 {
constructor() ERC20("Mock Token", "MOCK") {
_mint(msg.sender, 100 * 10**18);
}
}
​
contract PoCTest is Test {
InheritanceManager inheritanceManager;
address inheritanceOwner = address(0x1);
address benef1 = address(0x2);
address benef2 = address(0x3);
address benef3 = address(0x4);
address benef4 = address(0x5);
MockToken mockToken;
​
function setUp() public {
/* - Deployer is the owner of the contract
* - Add 4 beneficiaries to the contract
* - Creates a 20 MOCK estate asset valued
*/
vm.startPrank(inheritanceOwner);
​
inheritanceManager = new InheritanceManager();
mockToken = new MockToken();
​
inheritanceManager.addBeneficiery(benef1);
inheritanceManager.addBeneficiery(benef2);
inheritanceManager.addBeneficiery(benef3);
inheritanceManager.addBeneficiery(benef4);
​
inheritanceManager.createEstateNFT("Trump Tower", 20 * 10**18, address(mockToken));
vm.stopPrank();
}
function testbuyOutEstateNFT() public {
// Fast forward time to pass the TIMELOCK period (90 days)
vm.warp(block.timestamp + 90 days);
​
// Call inherit() to make the contract marked as inherited
vm.prank(benef1);
inheritanceManager.inherit();
​
// Ensure isInherited is true
bool isInheritedAfter = inheritanceManager.getIsInherited();
console.log("Value of 'isInherited' after inherit():", isInheritedAfter);
assertEq(isInheritedAfter, true, "isInherited should be true");
// Ensure the estate asset value is set correctly
uint256 assetValue = inheritanceManager.getNftValue(1);
console.log("Value of the estate asset in the contract: ", assetValue);
console.log("------------------------------");
assertEq(assetValue, 20 * 10**18);
​
// Transfer some MockTokens from the test contract to benef2 which will call the buyOutEstateNFT() function
vm.prank(inheritanceOwner);
mockToken.transfer(benef2, 40 * 10**18);
​
// Log balances of all beneficiaries
console.log("Beneficiary 1 balance before buyOutEstateNFT(): ", mockToken.balanceOf(benef1));
console.log("Beneficiary 2 balance before buyOutEstateNFT(): ", mockToken.balanceOf(benef2));
console.log("Beneficiary 3 balance before buyOutEstateNFT(): ", mockToken.balanceOf(benef3));
console.log("Beneficiary 4 balance before buyOutEstateNFT(): ", mockToken.balanceOf(benef4));
console.log("------------------------------");
​
// Approve the contract to spend benef2's MockTokens
vm.prank(benef2);
mockToken.approve(address(inheritanceManager), assetValue);
​
// Make benef2 buy the NFT previously minted
vm.prank(benef2);
inheritanceManager.buyOutEstateNFT(1);
​
// Log balances of all beneficiaries
console.log("Beneficiary 1 balance after buyOutEstateNFT(): ", mockToken.balanceOf(benef1));
console.log("Beneficiary 2 balance after buyOutEstateNFT(): ", mockToken.balanceOf(benef2));
console.log("Beneficiary 3 balance after buyOutEstateNFT(): ", mockToken.balanceOf(benef3));
console.log("Beneficiary 4 balance after buyOutEstateNFT(): ", mockToken.balanceOf(benef4));
console.log("------------------------------");
​
// Checks that the benef1 is receiving part of the asset value but not the right one.
// IERC20(assetToPay).safeTransfer(beneficiaries[i], finalAmount / divisor) makes the actual amount paid
// not correct. It should be 5 MOCK instead of 3.75
assertEq(mockToken.balanceOf(benef1), 3.75 * 10**18);
​
// Because of the early return in the function, benef3 and benef4 won't receive their tokens so their balance
// will remain equals to 0
assertEq(mockToken.balanceOf(benef3), 0 * 10**18);
assertEq(mockToken.balanceOf(benef4), 0 * 10**18);
​
// Because of the early return in the function, benef3 and benef4 won't receive their tokens so their balance
// will remain equals to 0
assertEq(mockToken.balanceOf(benef3), 0 * 10**18);
assertEq(mockToken.balanceOf(benef4), 0 * 10**18);
​
// We can see that the NFT has not been burned because it still has its origina value of 20 MOCK
uint256 nftValue = inheritanceManager.getNftValue(1);
console.log("NFT Value: ", nftValue);
assertEq(nftValue, 20 * 10**18);
}
}

Recommendation

The function should be rewritten to fix all the identified issues. A suggestion is described as following:

function buyOutEstateNFT(uint256 _nftID) external onlyBeneficiaryWithIsInherited {
uint256 value = nftValue[_nftID];
require(value > 0, "NFT does not exist or has no value");
uint256 divisor = beneficiaries.length;
uint256 multiplier = beneficiaries.length - 1;
uint256 totalAmount = (value / divisor) * multiplier;
// Transfer tokens from buyer to contract
IERC20(assetToPay).safeTransferFrom(msg.sender, address(this), totalAmount);
// Distribute tokens to other beneficiaries
uint256 amountPerBeneficiary = value / divisor;
for (uint256 i = 0; i < beneficiaries.length; i++) {
if (msg.sender == beneficiaries[i]) {
// Move to the next iteration rather than returning
continue;
}
// Transfer the correct amount to each beneficiary
IERC20(assetToPay).safeTransfer(beneficiaries[i], amountPerBeneficiary);
}
// Burn the NFT after all distributions are complete and delete its value in the mapping
nft.burnEstate(_nftID);
delete nftValue[_nftID];
}
Updates

Lead Judging Commences

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

buyOutNFT has wrong denominator

buyOutNFT has return instead of continue

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

buyOutNFT has wrong denominator

buyOutNFT has return instead of continue

Support

FAQs

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