Beginner FriendlySolidity
100 EXP
View results
Submission Details
Severity: high
Valid

Premature return in `InheritanceManager::buyOutEstateNFT` can lead to loss of Fund

Summary

The buyOutEstateNFT function in the InheritanceManager contract contains a logic error that causes premature termination of the function's execution. This occurs when a beneficiary attempts to buy out an estate NFT, and the loop to distribute funds to other beneficiaries terminates prematurely after identifying the calling beneficiary, without transferring to other beneficiaries and without burning the NFT. This leads to an inconsistent state where some beneficiaries may not receive their share and the NFT remains active.

Vulnerability Details

https://github.com/CodeHawks-Contests/2025-03-inheritable-smart-contract-wallet/blob/main/src/InheritanceManager.sol#L263-L277

  • Location: InheritanceManager.so

  • Function: buyOutEstateNFT

  • Vulnerability: Premature Return

  • Description:

    • The buyOutEstateNFT function is intended to allow a beneficiary to buy out an estate NFT from the other beneficiaries.

    • The function calculates the amount to be paid to the other beneficiaries, transfers it from the buyer to the contract, and then iterates through the list of beneficiaries to distribute the funds.

    • Critical Flaw: Inside the loop, there's a check: if (msg.sender == beneficiaries[i]) { return; }. This return statement is executed as soon as the function encounters the address of the calling beneficiary in beneficiaries.

    • Consequence: When the calling beneficiary is found, the return statement immediately exits the function. The function never proceeds to transfer funds to the remaining beneficiaries, nor does it execute the nft.burnEstate(_nftID) function call. This function is called at the end of the for loop, and since the loop exits early, the NFT is never burned.

    • Incorrect Logic: The intended logic likely meant to skip the beneficiary who was buying out the NFT, but the return statement abruptly halts the entire process.

    • State Inconsistency: This results in the NFT remaining active, and the contract holding some funds that should be distributed.

Proof of Concept

Consider a scenario where the first user in the beneficiaries array buyouts estate NFT.

  • Add this test to the test suite InheritanceManagerTest at path test/InheritanceManagerTest.t.sol and run the command forge test --mt test_buyOutBug:

function test_buyOutBug() public {
address user2 = makeAddr("user2");
address user3 = makeAddr("user3");
vm.warp(1);
vm.startPrank(owner);
im.addBeneficiery(user1);
im.addBeneficiery(user2);
im.addBeneficiery(user3);
im.createEstateNFT("our beach-house", 3e6, address(usdc));
vm.stopPrank();
usdc.mint(user1, 4e6);
vm.warp(1 + 90 days);
uint256 contractBalanceBefore = usdc.balanceOf(address(im));
vm.startPrank(user1);
usdc.approve(address(im), 4e6);
im.inherit();
im.buyOutEstateNFT(1);
vm.stopPrank();
uint256 contractBalanceAfter = usdc.balanceOf(address(im));
uint256 user2BalanceAfter = usdc.balanceOf(address(user2));
uint256 user3BalanceAfter = usdc.balanceOf(address(user3));
assertGt(contractBalanceAfter, contractBalanceBefore); // Contract balance is now greater
assertEq(contractBalanceAfter, 2e6); // Funds locked in contract
assertEq(user2BalanceAfter, 0); // User 2 did not receive any fund
assertEq(user3BalanceAfter, 0); //User 3 did not receive any fund
}

After buyout, funds are locked in the contract and other users were not paid any fund

Impact

  • Partial Fund Distribution: Beneficiaries may not receive their rightful share of the funds when an NFT is bought out.

  • NFT Remains Active: The estate NFT is not burned after a buyout, leading to potential confusion and errors if the same NFT is subject to multiple buyout attempts or other operations.

  • Loss of Funds: The funds intended for remaining beneficiaries are held in the contract without being properly transferred.

Tools Used

  • Manual Code Review

  • Foundry

Recommendations

  1. Option 1: Refactor buyOutEstateNFT Logic:

    • Replace the return statement with continue. This will skip the current iteration (when the calling beneficiary is encountered) but will allow the loop to continue processing other beneficiaries.

    • Move nft.burnEstate(_nftID) before the for loop, so it will burn the NFT regardless of who is buying.

  2. Option 2: Revised Code Snippet: Skip if its the beneficiary buying the 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);
    nft.burnEstate(_nftID); //burn nft before distributing fund
    for (uint256 i = 0; i < beneficiaries.length; i++) {
    if (msg.sender != beneficiaries[i]) { // skip if its the beneficiary buying the nft
    IERC20(assetToPay).safeTransfer(beneficiaries[i], finalAmount / divisor);
    }
    }
    }
Updates

Lead Judging Commences

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