Submission Details
Severity:
[M-3] `InheritanceManager::buyOutEstateNFT` has precision loss due to operations order
Description:
In Solidity it is advisable to perform multiplication before division. This is in order to prevent precision loss in the calculations.
Impact:
Medium - the beneficiaries will not receive the correct amount, but will be able to withdraw with another transaction by calling InheritanceManager::withdrawInheritedFunds.
This also means that the buyer is paying less.
Likelihood: High. Occurs in every buyOutEstateNFT call, as it’s inherent to the function’s arithmetic logic.
Proof of Concept:
Even from the provided test if we inspect the events we can see that the NFT is listed for 3e6 USDC.
User 3 transfers 2e6 USDC
[26058] ERC20Mock::transferFrom(user3: [0xc0A55e2205B289a967823662B841Bd67Aa362Aec], InheritanceManager: [0x88F59F8826af5e695B13cA934d6c7999875A9EeA], 2000000 [2e6])
│ │ ├─ emit Transfer(from: user3: [0xc0A55e2205B289a967823662B841Bd67Aa362Aec], to: InheritanceManager: [0x88F59F8826af5e695B13cA934d6c7999875A9EeA], value: 2000000 [2e6])
│ │ └─ ← [Return] true
And then less is transferred to the beneficiaries
[25204] ERC20Mock::transfer(user1: [0x29E3b139f4393aDda86303fcdAa35F60Bb7092bF], 666666 [6.666e5])
│ │ ├─ emit Transfer(from: InheritanceManager: [0x88F59F8826af5e695B13cA934d6c7999875A9EeA], to: user1: [0x29E3b139f4393aDda86303fcdAa35F60Bb7092bF], value: 666666 [6.666e5])
│ │ └─ ← [Return] true
│ ├─ [25204] ERC20Mock::transfer(user2: [0x537C8f3d3E18dF5517a58B3fB9D9143697996802], 666666 [6.666e5])
│ │ ├─ emit Transfer(from: InheritanceManager: [0x88F59F8826af5e695B13cA934d6c7999875A9EeA], to: user2: [0x537C8f3d3E18dF5517a58B3fB9D9143697996802], value: 666666 [6.666e5])
│ │ └─ ← [Return] true
Tests
function test_buyOutEstateNFTSuccess() 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);
usdc.approve(address(im), 4e6);
im.inherit();
im.buyOutEstateNFT(1);
vm.stopPrank();
}
[PASS] test_buyOutEstateNFTSuccess() (gas: 461887)
Traces:
[461887] InheritanceManagerTest::test_buyOutEstateNFTSuccess()
├─ [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]
├─ [0] VM::warp(1)
│ └─ ← [Return]
├─ [0] VM::startPrank(owner: [0x7c8999dC9a822c1f0Df42023113EDB4FDd543266])
│ └─ ← [Return]
├─ [69042] InheritanceManager::addBeneficiery(user1: [0x29E3b139f4393aDda86303fcdAa35F60Bb7092bF])
│ └─ ← [Stop]
├─ [23142] InheritanceManager::addBeneficiery(user2: [0x537C8f3d3E18dF5517a58B3fB9D9143697996802])
│ └─ ← [Stop]
├─ [23142] InheritanceManager::addBeneficiery(user3: [0xc0A55e2205B289a967823662B841Bd67Aa362Aec])
│ └─ ← [Stop]
├─ [143726] InheritanceManager::createEstateNFT("our beach-house", 3000000 [3e6], ERC20Mock: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f])
│ ├─ [95512] NFTFactory::createEstate("our beach-house")
│ │ ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: InheritanceManager: [0x88F59F8826af5e695B13cA934d6c7999875A9EeA], tokenId: 1)
│ │ ├─ emit MetadataUpdate(_tokenId: 1)
│ │ └─ ← [Return] 1
│ └─ ← [Stop]
├─ [0] VM::stopPrank()
│ └─ ← [Return]
├─ [46784] ERC20Mock::mint(user3: [0xc0A55e2205B289a967823662B841Bd67Aa362Aec], 4000000 [4e6])
│ ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: user3: [0xc0A55e2205B289a967823662B841Bd67Aa362Aec], value: 4000000 [4e6])
│ └─ ← [Stop]
├─ [0] VM::warp(7776001 [7.776e6])
│ └─ ← [Return]
├─ [0] VM::startPrank(user3: [0xc0A55e2205B289a967823662B841Bd67Aa362Aec])
│ └─ ← [Return]
├─ [24735] ERC20Mock::approve(InheritanceManager: [0x88F59F8826af5e695B13cA934d6c7999875A9EeA], 4000000 [4e6])
│ ├─ emit Approval(owner: user3: [0xc0A55e2205B289a967823662B841Bd67Aa362Aec], spender: InheritanceManager: [0x88F59F8826af5e695B13cA934d6c7999875A9EeA], value: 4000000 [4e6])
│ └─ ← [Return] true
├─ [22686] InheritanceManager::inherit()
│ └─ ← [Stop]
├─ [83049] InheritanceManager::buyOutEstateNFT(1)
│ ├─ [26058] ERC20Mock::transferFrom(user3: [0xc0A55e2205B289a967823662B841Bd67Aa362Aec], InheritanceManager: [0x88F59F8826af5e695B13cA934d6c7999875A9EeA], 2000000 [2e6])
│ │ ├─ emit Transfer(from: user3: [0xc0A55e2205B289a967823662B841Bd67Aa362Aec], to: InheritanceManager: [0x88F59F8826af5e695B13cA934d6c7999875A9EeA], value: 2000000 [2e6])
│ │ └─ ← [Return] true
│ ├─ [25204] ERC20Mock::transfer(user1: [0x29E3b139f4393aDda86303fcdAa35F60Bb7092bF], 666666 [6.666e5])
│ │ ├─ emit Transfer(from: InheritanceManager: [0x88F59F8826af5e695B13cA934d6c7999875A9EeA], to: user1: [0x29E3b139f4393aDda86303fcdAa35F60Bb7092bF], value: 666666 [6.666e5])
│ │ └─ ← [Return] true
│ ├─ [25204] ERC20Mock::transfer(user2: [0x537C8f3d3E18dF5517a58B3fB9D9143697996802], 666666 [6.666e5])
│ │ ├─ emit Transfer(from: InheritanceManager: [0x88F59F8826af5e695B13cA934d6c7999875A9EeA], to: user2: [0x537C8f3d3E18dF5517a58B3fB9D9143697996802], value: 666666 [6.666e5])
│ │ └─ ← [Return] true
│ └─ ← [Stop]
├─ [0] VM::stopPrank()
│ └─ ← [Return]
└─ ← [Stop]
Recommended Mitigation:
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 finalAmount = (value * multiplier) / 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);
}
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.