Submission Details
Severity:
Wrong divisor used in calculating beneficiary payments causes beneficiaries to lose a significant part of their payments.
Summary
Wrong divisor used in calculating beneficiary payments causes beneficiaries to lose a significant part of their payments.
Vulnerability Details
The issue occurs 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);
}
}
nft.burnEstate(_nftID);
}
The finalAmount should be divided by `multiplier` instead
POC
Add the below test function to `InheritanceManagerTest.t.sol`
function testWrongFundDistributionMath() public {
address user2 = makeAddr("user2");
uint256 nftValue = 4e6;
string memory tokenUri = "our beach-house";
vm.warp(1);
vm.startPrank(owner);
im.addBeneficiery(user1);
im.addBeneficiery(user2);
im.createEstateNFT(tokenUri, nftValue, address(usdc));
vm.stopPrank();
usdc.mint(user1, nftValue);
usdc.mint(user2, nftValue);
vm.warp(1 + 90 days);
uint256 user1BalanceBefore = usdc.balanceOf(user1);
//user2 buys nft
vm.startPrank(user2);
usdc.approve(address(im), nftValue);
im.inherit();
im.buyOutEstateNFT(1);
vm.stopPrank();
uint256 imBalanceAfter = usdc.balanceOf(address(im));
uint256 user1BalanceAfter = usdc.balanceOf(user1);
uint256 expectedUser1BalanceAfter = user1BalanceBefore + (nftValue / 2);
assertLt(user1BalanceAfter, expectedUser1BalanceAfter);
assertGt(imBalanceAfter, 0);
console.log("Balance stuck in contract: ", imBalanceAfter);
}
Impact
Beneficiaries lose a large percentage of their payments
Tools Used
Manual review, foundry test suite
Recommendations
The finalAmount should be divided by `multiplier` instead
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 / divisor);
}
}
nft.burnEstate(_nftID);
}
Updates
Lead Judging Commences
0xtimefliez 6 months ago
Submission Judgement Published Assigned finding tags:
buyOutNFT has wrong denominator
Rewards breakdown
nSLOC
211This contest has a flat reward structure with the following payouts:
100 EXP
20 EXP
2 EXP
Live
The contest is live. Earn rewards by submitting a finding.
Appeals
This is your time to appeal against judgements on your submissions.
Appeals Review
Appeals are being carefully reviewed by our judges.