Description: In InheritanceManager::buyOutEstateNFT, the total amount needed for a buyout is calculated and then divided up and sent to the other beneficiaries. The issue is when dividing the total amount for disbursement, it includes the paying beneficiary in the calculation even though their share isn't included. This incorrectly reduces the amount paid to the other beneficiaries in the buyout.
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);
}
Impact: The beneficiaries being bought out are shorted the amount owed. What's more critical is if the owner actually passed away and these funds will be locked in the InheritanceManager manager contract with no way to get them out.
Proof of Concept:
Add the following unit test to InheritanceManagerTest.t.sol:
function test_buyOutEstateNFTLossOfFunds() 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("our beach-house", 6e6, address(usdc));
vm.stopPrank();
usdc.mint(user3, 4e6);
vm.warp(1 + 90 days);
vm.startPrank(user3);
usdc.approve(address(im), 4e6);
im.inherit();
im.buyOutEstateNFT(1);
vm.stopPrank();
console.log("usdc.balanceOf(user1)", usdc.balanceOf(user1));
console.log("usdc.balanceOf(user2)", usdc.balanceOf(user2));
assertNotEq(usdc.balanceOf(user1), 2e6);
assertNotEq(usdc.balanceOf(user2), 2e6);
console.log("usdc.balanceOf(address(im))", usdc.balanceOf(address(im)));
assertNotEq(usdc.balanceOf(address(im)), 0);
Run the unit test: forge test --mt test_buyOutEstateNFTLossOfFunds:
Ran 1 test for test/InheritanceManagerTest.t.sol:InheritanceManagerTest
[PASS] test_buyOutEstateNFTLossOfFunds() (gas: 457255)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 6.87ms (1.19ms CPU time)
Recommended Mitigation: Fix the divisor by replacing with (Number Of Beneficiaries - 1). This value is calculated in the multiplier variable in InheritanceManager::buyOutEstateNFT.
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);
}
