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

Unequal Fund Distribution in buyOutEstateNFT Function

Description

In the buyOutEstateNFT() function, there is a critical distribution issue where any beneficiaries listed after the buyer in the beneficiaries array do not receive their share of funds. This occurs because the function uses an early return statement in the distribution loop when it finds the buyer, preventing the loop from reaching any subsequent beneficiaries.

Affected Code

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 - exits function completely
} else {
IERC20(assetToPay).safeTransfer(beneficiaries[i], finalAmount / divisor);
}
}
nft.burnEstate(_nftID);
}

Impact

This vulnerability has several significant consequences:

  1. Unfair Distribution: Only beneficiaries listed before the buyer in the array receive funds; those after the buyer receive nothing.

  2. Unpredictable Outcomes: The distribution outcome depends on the arbitrary ordering of beneficiaries in the array.

  3. Financial Loss: Beneficiaries positioned after the buyer in the array are deprived of their rightful inheritance.

Root Cause

The issue stems from using a function-level return statement inside the distribution loop instead of using a loop-level continue statement. When the code identifies the buyer in the loop, it immediately exits the entire function rather than just skipping that iteration and continuing to process the remaining beneficiaries.

Proof of Concept

Consider a scenario with three beneficiaries (A, B, C) where B is the buyer:

function test_buyOutEstateNFTPartialDistribution() public {
address userA = makeAddr("userA");
address userB = makeAddr("userB");
address userC = makeAddr("userC");
// Setup
vm.startPrank(owner);
im.addBeneficiery(userA); // First beneficiary
im.addBeneficiery(userB); // Second beneficiary (buyer)
im.addBeneficiery(userC); // Third beneficiary
im.createEstateNFT("estate", 3e6, address(usdc));
vm.stopPrank();
usdc.mint(userB, 4e6);
vm.warp(1 + 90 days);
// UserB buys the NFT
vm.startPrank(userB);
usdc.approve(address(im), 4e6);
im.inherit();
// Record balances before
uint256 balanceA_before = usdc.balanceOf(userA);
uint256 balanceC_before = usdc.balanceOf(userC);
im.buyOutEstateNFT(1);
vm.stopPrank();
// Check balances after
assertEq(usdc.balanceOf(userA), balanceA_before + 666666); // userA gets paid
assertEq(usdc.balanceOf(userC), balanceC_before); // userC gets nothing
}

Recommended Fix

Restructure the distribution logic to ensure all beneficiaries except the buyer receive funds, regardless of their position in the array:

function buyOutEstateNFT(uint256 _nftID) external onlyBeneficiaryWithIsInherited {
uint256 value = nftValue[_nftID];
uint256 divisor = beneficiaries.length;
uint256 multiplier = beneficiaries.length - 1;
uint256 finalAmount = (value / divisor) * multiplier;
// Transfer funds from buyer to contract
IERC20(assetToPay).safeTransferFrom(msg.sender, address(this), finalAmount);
// Distribute funds to all beneficiaries except the buyer
uint256 amountPerBeneficiary = finalAmount / multiplier;
for (uint256 i = 0; i < beneficiaries.length; i++) {
if (msg.sender != beneficiaries[i]) {
IERC20(assetToPay).safeTransfer(beneficiaries[i], amountPerBeneficiary);
}
}
nft.burnEstate(_nftID);
}

Tools Used

  • Foundry Testing Framework

  • Manual Code Review

Updates

Lead Judging Commences

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

Give us feedback!