Summary
The buyOutEstateNFT function incorrectly divides the NFT value by the total number of beneficiaries instead of the number of non-buying beneficiaries, leading to miscalculated share payouts and locked token shares.
Vulnerability Details
The buyOutEstateNFT function calculates and distributes the value of an NFT among beneficiaries when one of them wants to buy it out.
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);
}
finalAmount, which corresponds to the buyout amount, is calculated and transferred into the contract ([1]). Then, the contract distributes this amount among all other beneficiaries (excluding the caller) ([2]). However, the amount per-beneficiary is calculated as finalAmount/divisor, which is wrong. Consider the following setup:
three beneficiaries [0xA, 0xB, 0xC], asset value 100 USDC
0xC buys out the NFT, paying the other two (0xA and 0xB) their corresponding shares
value / divisor = 100 / 3, 33 USDC
finalAmount = (100 / 3) * 2, where 2 is multiplier, 66 USDC
0xC pays 66 USDC into the contract via safeTransferFrom
distribution per-beneficiary is finalAmount / divisor = 66 / 3, 22 USDC
each beneficiary gets a pay of 22 USDC while it should've been 33 USDC
InheritanceManager contract keeps 22 USDC (22% is effectively lost)
Proof of Concept
Place test_buyOutEstateNFTSharesLoss in InheritanceManager.t.sol:
function test_buyOutEstateNFTSharesLoss() 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("beach-house", 100e6, address(usdc));
vm.stopPrank();
usdc.mint(user3, 100e6);
vm.warp(1 + 90 days);
vm.startPrank(user3);
usdc.approve(address(im), 100e6);
im.inherit();
im.buyOutEstateNFT(1);
vm.stopPrank();
assertEq(usdc.balanceOf(user1) / 1e6, 22);
assertEq(usdc.balanceOf(user2) / 1e6, 22);
assertEq(usdc.balanceOf(address(im)) / 1e6, 22);
}
And run the test:
$ forge test --mp test/InheritanceManagerTest.t.sol --mt test_buyOutEstateNFTSharesLoss
[⠊] Compiling...
No files changed, compilation skipped
Ran 1 test for test/InheritanceManagerTest.t.sol:InheritanceManagerTest
[PASS] test_buyOutEstateNFTSharesLoss() (gas: 479768)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.03ms (248.58µs CPU time)
Ran 1 test suite in 134.01ms (1.03ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)
Note how user1 and user2 end up with 22 token shares each instead of 33., while the InheritanceManager contract keeps 22.
Impact
Beneficiaries receive less than their fair share and funds can get locked, which breaks one of the core assumptions/invariants of the InheritanceManager contract.
Recommendations
When calculating the amount per-beneficiary, divide by multiplier (the number of beneficiaries except for the buyer), instead of divisor (the total number of 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);
+ IERC20(assetToPay).safeTransfer(beneficiaries[i], finalAmount / multiplier);
}
}
nft.burnEstate(_nftID);
}
The following test should now pass:
function test_buyOutEstateNFTShareDistributionFix() 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("beach-house", 100e6, address(usdc));
vm.stopPrank();
usdc.mint(user3, 100e6);
vm.warp(1 + 90 days);
vm.startPrank(user3);
usdc.approve(address(im), 100e6);
im.inherit();
im.buyOutEstateNFT(1);
vm.stopPrank();
console.log(usdc.balanceOf(user1));
console.log(usdc.balanceOf(user2));
console.log(usdc.balanceOf(address(im)));
assertEq(usdc.balanceOf(user1) / 1e6, 33);
assertEq(usdc.balanceOf(user2) / 1e6, 33);
assertEq(usdc.balanceOf(address(im)) / 1e6, 0);
}
$ forge test --mp test/InheritanceManagerTest.t.sol --mt test_buyOutEstateNFTShareDistributionFix
Ran 1 test for test/InheritanceManagerTest.t.sol:InheritanceManagerTest
[PASS] test_buyOutEstateNFTShareDistributionFix() (gas: 459855)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.09ms (296.79µs CPU time)
Ran 1 test suite in 135.74ms (1.09ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)
