Summary
The InheritManager::buyOutEstateNFT function uses divisor (total number of beneficiaries) instead of multiplier (number of beneficiaries excluding the buyer) when distributing funds to beneficiaries. This results in beneficiaries receiving less than their fair share of the payment, while the contract retains unallocated funds.
POC
Place this test in the test/InheritanceManagerTest.t.sol:
function test_unfair_share_due_to_wrong_denominator() external {
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(user3, 4e6);
vm.warp(1 + 90 days);
assertEq(usdc.balanceOf(user1), 0);
assertEq(usdc.balanceOf(user2), 0);
vm.startPrank(user3);
usdc.approve(address(im), 4e6);
im.inherit();
im.buyOutEstateNFT(1);
vm.stopPrank();
uint256 expectedShare = im.getNftValue(1) / 3;
assertLt(usdc.balanceOf(user1), expectedShare);
uint256 amountLost = 333334;
assertEq(expectedShare, usdc.balanceOf(user1) + amountLost);
}
Impact
Beneficiaries receive less than their fair share of the payment, leading to financial losses.
Tools Used
Recommendations
Use multiplier (not divisor) when distributing funds to beneficiaries. Update the distribution logic as follows:
function buyOutEstateNFT(uint256 _nftID) external onlyBeneficiaryWithIsInherited {
uint256 value = nftValue[_nftID];
uint256 divisor = beneficiaries.length;
uint256 multiplier = beneficiaries.length - 1;
- uint256 finalAmount = (value / divisor) * multiplier;
+ uint256 finalAmount = (value * multiplier) / divisor; // this is another issue
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);
- }
+ if (msg.sender != beneficiaries[i]) {
+ IERC20(assetToPay).safeTransfer(beneficiaries[i], finalAmount / multiplier); // main issue
+ }
}
nft.burnEstate(_nftID);
}
