Description
The buyOutEstateNFT() function in the InheritanceManager contract contains a critical vulnerability related to rounding errors during division operations. These rounding issues can lead to fund leakage or insufficient payment distribution to beneficiaries.
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;
} else {
IERC20(assetToPay).safeTransfer(beneficiaries[i], finalAmount / divisor);
}
}
nft.burnEstate(_nftID);
}
The issue occurs due to two consecutive integer division operations:
First division: value / divisor - This rounds down to the nearest integer
Second division: finalAmount / divisor - Another rounding down occurs
These operations cause several critical problems:
Funds get "stuck" in the contract due to rounding errors
Beneficiaries receive less than their fair share
The total amount transferred to beneficiaries is less than the amount transferred from the buyer
Impact
This vulnerability has significant financial implications:
-
Trapped Funds: Due to rounding errors, tokens remain stuck in the contract after the transaction completes.
-
Uneven Distribution: Beneficiaries receive slightly less than they should, with the discrepancy growing as the number of beneficiaries increases.
-
Contract Integrity: The core functionality of fairly distributing funds among beneficiaries is compromised.
Proof of Concept
The following test case demonstrates the vulnerability:
function test_buyOutEstateNFTRoundingIssue() public {
address user2 = makeAddr("user2");
address user3 = makeAddr("user3");
vm.warp(1);
vm.startPrank(owner);
im.addBeneficiery(user1);
im.addBeneficiery(user2);
im.addBeneficiery(user3);
uint256 nftValue = 1000;
im.createEstateNFT("test property", nftValue, address(usdc));
vm.stopPrank();
usdc.mint(user3, 1000);
uint256 initialContractBalance = usdc.balanceOf(address(im));
uint256 initialUser1Balance = usdc.balanceOf(user1);
uint256 initialUser2Balance = usdc.balanceOf(user2);
vm.warp(1 + 90 days);
vm.startPrank(user3);
usdc.approve(address(im), 1000);
im.inherit();
im.buyOutEstateNFT(1);
vm.stopPrank();
uint256 finalContractBalance = usdc.balanceOf(address(im));
uint256 finalUser1Balance = usdc.balanceOf(user1);
uint256 finalUser2Balance = usdc.balanceOf(user2);
uint256 totalPaid = (finalUser1Balance - initialUser1Balance) + (finalUser2Balance - initialUser2Balance);
uint256 stuckTokens = finalContractBalance - initialContractBalance;
console.log("Actual balance increase user1:", finalUser1Balance - initialUser1Balance);
console.log("Actual balance increase user2:", finalUser2Balance - initialUser2Balance);
console.log("Total paid to beneficiaries:", totalPaid);
console.log("Tokens stuck in contract:", stuckTokens);
assert(stuckTokens > 0);
}
Output:
│ └─ ← [Return]
├─ [559] ERC20Mock::balanceOf(InheritanceManager: [0x88F59F8826af5e695B13cA934d6c7999875A9EeA]) [staticcall]
│ └─ ← [Return] 222
├─ [559] ERC20Mock::balanceOf(user1: [0x29E3b139f4393aDda86303fcdAa35F60Bb7092bF]) [staticcall]
│ └─ ← [Return] 222
├─ [559] ERC20Mock::balanceOf(user2: [0x537C8f3d3E18dF5517a58B3fB9D9143697996802]) [staticcall]
│ └─ ← [Return] 222
├─ [0] console::log("Actual balance increase user1:", 222) [staticcall]
│ └─ ← [Stop]
├─ [0] console::log("Actual balance increase user2:", 222) [staticcall]
│ └─ ← [Stop]
├─ [0] console::log("Total paid to beneficiaries:", 444) [staticcall]
│ └─ ← [Stop]
├─ [0] console::log("Tokens stuck in contract:", 222) [staticcall]
│ └─ ← [Stop]
└─ ← [Stop]
In this example with an NFT value of 1000 and 3 beneficiaries:
The contract calculates finalAmount = (1000 / 3) * 2 = 666
Each non-buyer beneficiary receives 666 / 3 = 222
Total paid to beneficiaries: 222 * 2 = 444
The contract receives 666 tokens but only distributes 444
Result: 222 tokens remain permanently stuck in the contract
Recommendation
Restructure the calculation to minimize or eliminate rounding errors:
function buyOutEstateNFT(uint256 _nftID) external onlyBeneficiaryWithIsInherited {
uint256 value = nftValue[_nftID];
uint256 beneficiaryCount = beneficiaries.length;
uint256 individualShare = value / beneficiaryCount;
uint256 amountToPay = individualShare * (beneficiaryCount - 1);
IERC20(assetToPay).safeTransferFrom(msg.sender, address(this), amountToPay);
for (uint256 i = 0; i < beneficiaryCount; i++) {
if (msg.sender != beneficiaries[i]) {
IERC20(assetToPay).safeTransfer(beneficiaries[i], individualShare);
}
}
nft.burnEstate(_nftID);
}
This revised implementation:
Calculates the individual share once using a single division
Transfers the exact amount needed based on that calculation
Distributes the exact individual share to each non-buyer beneficiary
Avoids the double division that causes compounding rounding errors
Also fixes the early return issue noted in the previous vulnerability report
Tools Used
