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

`buyOutEstateNFT` Early Return and Distribution calculation causes permanent loss of funds

abhishekthakur

Summary

The buyOutEstateNFT function has two separate critical issues:

  1. Early Return Issue: The function can exit prematurely with return, preventing ANY token distribution if the buyer is found early in the beneficiaries array

  2. Calculation Error: Even when distributions occur, each beneficiary receives finalAmount / divisor instead of finalAmount / multiplier, resulting in stuck funds

Vulnerability Details

Affected code - https://github.com/CodeHawks-Contests/2025-03-inheritable-smart-contract-wallet/blob/9de6350f3b78be35a987e972a1362e26d8d5817d/src/InheritanceManager.sol#L263C5-L277C6

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]) {
///@audit: early return
@> return;
} else {
/// @audit: wrong calculation of share
@> IERC20(assetToPay).safeTransfer(beneficiaries[i], finalAmount / divisor);
}
}
nft.burnEstate(_nftID);
}

Issue 1: Early Return

If the buyer is early in the beneficiaries array (e.g., the first or second beneficiary), the function will exit entirely when it finds the buyer:

  • The return statement immediately exits the function

  • No tokens are distributed to ANY other beneficiaries

  • The NFT is not burned

  • ALL collected tokens remain stuck in the contract

Issue 2: Calculation Error

When distributions do occur (i.e., when the buyer is later in the array):

  • Each beneficiary receives finalAmount / divisor

  • The correct amount should be finalAmount / multiplier (or value / divisor)

  • This results in part of the collected funds being stuck in the contract

POC

Add following test to existing InheritanceManagerTest.t.sol

function test_buyOutEstateNFTIssues() public {
// Create additional users for testing
address user2 = makeAddr("user2");
address user3 = makeAddr("user3");
​
// Set up inheritance with three beneficiaries
vm.startPrank(owner);
im.addBeneficiery(user1); // First beneficiary will be our buyer in first test
im.addBeneficiery(user2);
im.addBeneficiery(user3);
​
// Create NFT with value 100 tokens
im.createEstateNFT("Beach House", 100e18, address(usdc));
vm.stopPrank();
​
// Fast forward time and trigger inheritance
vm.warp(block.timestamp + 91 days);
vm.prank(user1);
im.inherit();
​
// Verify inheritance is activated
assertEq(true, im.getIsInherited());
​
// PART 1: Test with first beneficiary as buyer (early return issue)
console.log("\n=== TEST 1: EARLY RETURN ISSUE ===");
console.log("Scenario: First beneficiary (index 0) buys the NFT");
​
// Prepare buyer with tokens
usdc.mint(user1, 1000e18);
​
// Calculate expected values
uint256 nftValue = 100e18;
uint256 numBeneficiaries = 3;
uint256 sharePerBeneficiary = nftValue / numBeneficiaries;
uint256 finalAmount = sharePerBeneficiary * (numBeneficiaries - 1);
​
console.log("NFT value:", nftValue / 1e18);
console.log("Fair share per beneficiary:", sharePerBeneficiary / 1e18);
console.log("Buyer pays:", finalAmount / 1e18);
​
// Check initial balances
console.log("\nInitial balances:");
console.log(
"Contract token balance:",
usdc.balanceOf(address(im)) / 1e18
);
console.log(
"Buyer (user1) token balance:",
usdc.balanceOf(user1) / 1e18
);
console.log(
"Beneficiary2 token balance:",
usdc.balanceOf(user2) / 1e18
);
console.log(
"Beneficiary3 token balance:",
usdc.balanceOf(user3) / 1e18
);
​
// Execute buyout
vm.startPrank(user1);
usdc.approve(address(im), finalAmount);
im.buyOutEstateNFT(1);
vm.stopPrank();
​
// Check final balances
console.log("\nFinal balances:");
console.log(
"Contract token balance:",
usdc.balanceOf(address(im)) / 1e18
);
// cache user 1 balance, used in test 2 calculation
uint256 user1balanceAfterTest1 = usdc.balanceOf(user1);
console.log(
"Buyer (user1) token balance:",
user1balanceAfterTest1 / 1e18
);
console.log(
"Beneficiary2 token balance:",
usdc.balanceOf(user2) / 1e18
);
console.log(
"Beneficiary3 token balance:",
usdc.balanceOf(user3) / 1e18
);
​
// Verify Issue 1: Early return prevents ANY distributions
assertEq(
usdc.balanceOf(address(im)),
finalAmount,
"All tokens should remain in contract"
);
assertEq(
usdc.balanceOf(user2),
0,
"Beneficiary2 should receive nothing due to early return"
);
assertEq(
usdc.balanceOf(user3),
0,
"Beneficiary3 should receive nothing due to early return"
);
​
console.log(
"\nVerdict for Test 1: CRITICAL ISSUE - Early return prevents any distributions"
);
console.log(
"Expected: Tokens should be distributed to beneficiaries 2 and 3"
);
console.log("Actual: All tokens remain stuck in the contract");
​
// Reset for next test - clear the state
vm.warp(1); // Reset time
vm.prank(owner);
im = new InheritanceManager(); // Create a fresh instance
​
// PART 2: Test with last beneficiary as buyer (calculation issue)
console.log("\n=== TEST 2: CALCULATION ISSUE ===");
console.log("Scenario: Last beneficiary (index 2) buys the NFT");
​
// Set up inheritance again but in different order
vm.startPrank(owner);
im.addBeneficiery(user1);
im.addBeneficiery(user2);
im.addBeneficiery(user3); // Last beneficiary will be our buyer in second test
​
// Create NFT again
im.createEstateNFT("Beach House", 100e18, address(usdc));
vm.stopPrank();
​
// Fast forward time and trigger inheritance
vm.warp(block.timestamp + 91 days);
vm.prank(user1);
im.inherit();
​
// Prepare buyer with tokens
usdc.mint(user3, 1000e18);
​
// Calculate expected values
nftValue = 100e18; // Using the same variable from above
numBeneficiaries = 3;
sharePerBeneficiary = nftValue / numBeneficiaries;
finalAmount = sharePerBeneficiary * (numBeneficiaries - 1);
uint256 currentDistribution = finalAmount / numBeneficiaries;
uint256 correctDistribution = finalAmount / (numBeneficiaries - 1);
uint256 expectedStuck = finalAmount -
(currentDistribution * (numBeneficiaries - 1));
​
console.log("NFT value:", nftValue / 1e18);
console.log("Fair share per beneficiary:", sharePerBeneficiary / 1e18);
console.log("Buyer pays:", finalAmount / 1e18);
console.log(
"Current distribution per beneficiary:",
currentDistribution / 1e18
);
console.log(
"Correct distribution per beneficiary:",
correctDistribution / 1e18
);
​
// Check initial balances
console.log("\nInitial balances:");
console.log(
"Contract token balance:",
usdc.balanceOf(address(im)) / 1e18
);
console.log(
"Buyer (user3) token balance:",
usdc.balanceOf(user3) / 1e18
);
console.log(
"Beneficiary1 token balance:",
usdc.balanceOf(user1) / 1e18
);
console.log(
"Beneficiary2 token balance:",
usdc.balanceOf(user2) / 1e18
);
​
// Execute buyout
vm.startPrank(user3);
usdc.approve(address(im), finalAmount);
im.buyOutEstateNFT(1);
vm.stopPrank();
​
// Check final balances
console.log("\nFinal balances:");
console.log(
"Contract token balance:",
usdc.balanceOf(address(im)) / 1e18
);
console.log(
"Buyer (user3) token balance:",
usdc.balanceOf(user3) / 1e18
);
console.log(
"Beneficiary1 token balance:",
usdc.balanceOf(user1) / 1e18
);
console.log(
"Beneficiary2 token balance:",
usdc.balanceOf(user2) / 1e18
);
​
// Verify Issue 2: Calculation error leads to stuck funds
assertEq(
usdc.balanceOf(user1) - user1balanceAfterTest1,
currentDistribution,
"Beneficiary1 should receive the incorrect amount"
);
assertEq(
usdc.balanceOf(user2),
currentDistribution,
"Beneficiary2 should receive the incorrect amount"
);
​
// Calculate percentage of funds stuck
uint256 stuckPercentage = (expectedStuck * 100) / finalAmount;
​
console.log(
"\nVerdict for Test 2: HIGH SEVERITY ISSUE - Calculation error causes stuck funds"
);
console.log(
"Expected: Each beneficiary should receive",
correctDistribution / 1e18,
"tokens"
);
console.log(
"Actual: Each beneficiary receives",
currentDistribution / 1e18,
"tokens"
);
console.log(
"Result:",
stuckPercentage,
"% of funds remain stuck in contract"
);
}

run forge test --mt test_buyOutEstateNFTIssuesImpact-vv` in the terminal and we'll get following output, which confirms the issues.

[⠊] Compiling...
[⠘] Compiling 1 files with Solc 0.8.26
[⠃] Solc 0.8.26 finished in 712.61ms
Compiler run successful!
​
Ran 1 test for test/InheritanceManagerTest.t.sol:InheritanceManagerTest
[PASS] test_buyOutEstateNFTIssues() (gas: 4785728)
Logs:
=== TEST 1: EARLY RETURN ISSUE ===
Scenario: First beneficiary (index 0) buys the NFT
NFT value: 100
Fair share per beneficiary: 33
Buyer pays: 66
Initial balances:
Contract token balance: 0
Buyer (user1) token balance: 1000
Beneficiary2 token balance: 0
Beneficiary3 token balance: 0
Final balances:
Contract token balance: 66
Buyer (user1) token balance: 933
Beneficiary2 token balance: 0
Beneficiary3 token balance: 0
Verdict for Test 1: CRITICAL ISSUE - Early return prevents any distributions
Expected: Tokens should be distributed to beneficiaries 2 and 3
Actual: All tokens remain stuck in the contract
=== TEST 2: CALCULATION ISSUE ===
Scenario: Last beneficiary (index 2) buys the NFT
NFT value: 100
Fair share per beneficiary: 33
Buyer pays: 66
Current distribution per beneficiary: 22
Correct distribution per beneficiary: 33
Initial balances:
Contract token balance: 0
Buyer (user3) token balance: 1000
Beneficiary1 token balance: 933
Beneficiary2 token balance: 0
Final balances:
Contract token balance: 22
Buyer (user3) token balance: 933
Beneficiary1 token balance: 955
Beneficiary2 token balance: 22
Verdict for Test 2: HIGH SEVERITY ISSUE - Calculation error causes stuck funds
Expected: Each beneficiary should receive 33 tokens
Actual: Each beneficiary receives 22 tokens
Result: 33 % of funds remain stuck in contract
​
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 9.07ms (2.63ms CPU time)
​
Ran 1 test suite in 158.38ms (9.07ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

Impact

Permanent Loss of funds

Tools Used

Foundry

Recommendations

Here is the recommendation which should solve the issue -

function buyOutEstateNFT(uint256 _nftID) external onlyBeneficiaryWithIsInherited {
uint256 value = nftValue[_nftID];
uint256 beneficiaryCount = beneficiaries.length;
uint256 sharePerBeneficiary = value / beneficiaryCount;
uint256 multiplier = beneficiaryCount - 1;
​
// Calculate payment amount
uint256 paymentAmount = sharePerBeneficiary * multiplier;
​
// Take payment from buyer
IERC20(assetToPay).safeTransferFrom(msg.sender, address(this), paymentAmount);
​
// Distribute to all beneficiaries except the buyer
for (uint256 i = 0; i < beneficiaryCount; i++) {
// Skip the buyer and any address(0) entries
if (msg.sender != beneficiaries[i] && beneficiaries[i] != address(0)) {
// Distribute correct share (either approach works):
// Option 1: IERC20(assetToPay).safeTransfer(beneficiaries[i], sharePerBeneficiary);
// Option 2: IERC20(assetToPay).safeTransfer(beneficiaries[i], paymentAmount / multiplier);
IERC20(assetToPay).safeTransfer(beneficiaries[i], sharePerBeneficiary);
}
}
​
// Always burn the NFT
nft.burnEstate(_nftID);
}
Updates

Lead Judging Commences

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

buyOutNFT has return instead of continue

Appeal created

abhishekthakur Submitter
5 months ago
0xtimefliez Lead Judge 5 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.