Summary
When the NFT is settled on-chain, the beneficiaries will use a special function called InheritanceManager.sol::buyOutEstateNFT(uint256). However, the distributed reward is wrongly divided among the remaining beneficiaries.
Vulnerability Details
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);
}
As per the document state:
4.If the beneficiaries settle the NFTs on-chain the amount to pay is (Value / Number Of Beneficiaries) * (Number Of Beneficiaries - 1) since the paying beneficiary does not need to pay his own share. The above calculation is equally distributed between the other beneficiaries.
The finalAmountshould be distributed equally between the remaining beneficiaries, but using the divisorvariable won't give back a correct amount since it still includes the one that bought out the asset.
Scenario
add this to the test
function test_buyOutMiscalculation() 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", 3e6, address(usdc));
vm.stopPrank();
usdc.mint(user3, 4e6);
vm.warp(1 + 90 days);
vm.startPrank(user3);
console.log('user3 original balance: ',usdc.balanceOf(address(user3)));
usdc.approve(address(im), 4e6);
im.inherit();
im.buyOutEstateNFT(1);
vm.stopPrank();
console.log('user1 usdc balance: ', usdc.balanceOf(address(user1)));
console.log('user2 usdc balance: ', usdc.balanceOf(address(user2)));
console.log('Inheritance Manager usdc balance: ', usdc.balanceOf(address(im)));
console.log('user3 usdc balance: ', usdc.balanceOf(address(user3)));
assertEq(usdc.balanceOf(address(user2)), 666666);
assertEq(usdc.balanceOf(address(user1)), 666666);
}
and modify the InheritanceManager.sol::buyOutEstateNFT(uint)to print out some variables
function buyOutEstateNFT(uint256 _nftID) external onlyBeneficiaryWithIsInherited {
uint256 value = nftValue[_nftID];
uint256 divisor = beneficiaries.length;
uint256 multiplier = beneficiaries.length - 1;
uint256 finalAmount = (value / divisor) * multiplier;
console.log("Beneficiaries Count: ", beneficiaries.length);
console.log("Multiplier : ", multiplier);
console.log("Divisor : ", divisor);
console.log("Final Amount : ", finalAmount);
console.log("Amount Received by the rest of beneficiaries: ", finalAmount / divisor);
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);
}
We can see this result:
Ran 1 test for test/InheritanceManagerTest.t.sol:InheritanceManagerTest
[PASS] test_buyOutMiscalculation() (gas: 503304)
Logs:
user3 original balance: 4000000
Beneficiaries Count: 3
Multiplier : 2
Divisor : 3
Final Amount : 2000000
Amount Received by the rest of beneficiaries: 666666
user1 usdc balance: 666666
user2 usdc balance: 666666
user3 usdc balance: 2000000
Inheritance Manager usdc balance: 666668
Where the final Amount user3has to pay is 2e6 to buy out the NFT, which is correct, but then the distributed payment to the rest of the beneficiaries (user1 and user2) are 666666which leads to the remaining third being kept in the InheritanceManagercontract.
Impact
The impact of this miscalculation is:
Remaining beneficiaries will not receive the correct payment for the buy out process
A third of the actual funds that were sent to buy out the NFT is forever locked in theInheritanceManagerContract.
Tools Used
Recommendations
To fix this miscalculation, the divisorvariable can be replaced with the actual remaining beneficiaries, which is the multipliervariable. To make it more visible, a new variable toPayEachBeneficiariesis introduced and used to calculate the amount the remaining beneficiaries should receive.
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 toPayEachBeneficiaries = finalAmount / 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], toPayEachBeneficiaries);
}
}
nft.burnEstate(_nftID);
}
Here is the result using the test_buyOutEstateNFTSuccess(), which shows the correct amount being distributed to the remaining beneficiaries and that there is no remaining balance stuck in the InheritanceManagerContract
Ran 1 test for test/InheritanceManagerTest.t.sol:InheritanceManagerTest
[PASS] test_buyOutEstateNFTSuccess() (gas: 467512)
Logs:
Beneficiaries Count : 3
Multiplier : 2
Divisor : 3
Final Amount : 2000000
toPayEachBeneficiaries : 1000000
IM USDC Balance: : 0
