Submission Details
Severity:
`InheritanceManager.sol::buyOutEstateNFT()` pays wrong amount to beneficiaries
Summary
The InheritanceManager.sol::buyOutEstateNFT() function calculates wroung amount of ERC20 token to be transferred to the beneficiaries. As the totalAmount is divided by divisor instead of multiplier.
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);
}
Impact
In this POC, assuming NFT value is 2 USDC and there are 2 beneficiaries. After the 2nd beneficiary pays 2 USDC to buy the NFT, the 1st beneficiary should receive 1 USDC, but instead only receives 0.5 USDC
function test_buyOutEstateNFTPaysWrongAmount() public {
// Create 1 more beneficiary
address user2 = makeAddr("user2");
// Add the beneficiaries
vm.startPrank(owner);
im.addBeneficiery(user1);
im.addBeneficiery(user2);
// Create Estate NFT
uint256 NFTvalue = 2e6;
im.createEstateNFT("condominium", NFTvalue, address(usdc));
vm.stopPrank();
// Fast forward 90 days
vm.warp(90 days + 1);
// Give the buyer funds to purchase estate
usdc.mint(user2, 2e6);
vm.startPrank(user2);
usdc.approve(address(im), 2e6);
im.inherit(); // This set `isInherited` as True to allow buying EstateNFT
im.buyOutEstateNFT(1);
// NFT value is 2 USDC, excluding user2's share, user1 should receive 1 USDC
// However, user1 only receive 5e5 of USDC
vm.assertNotEq(usdc.balanceOf(user1), 1e6);
console.log("user1.USDCbalance:", usdc.balanceOf(user1));
}
Results
[PASS] test_buyOutEstateNFTPaysWrongAmount() (gas: 416766)
Logs:
user1.USDCbalance: 500000
Traces:
[416766] InheritanceManagerTest::test_buyOutEstateNFTPaysWrongAmount()
├─ [0] VM::addr(<pk>) [staticcall]
│ └─ ← [Return] user2: [0x537C8f3d3E18dF5517a58B3fB9D9143697996802]
├─ [0] VM::label(user2: [0x537C8f3d3E18dF5517a58B3fB9D9143697996802], "user2")
│ └─ ← [Return]
├─ [0] VM::startPrank(owner: [0x7c8999dC9a822c1f0Df42023113EDB4FDd543266])
│ └─ ← [Return]
├─ [69020] InheritanceManager::addBeneficiery(user1: [0x29E3b139f4393aDda86303fcdAa35F60Bb7092bF])
│ └─ ← [Stop]
├─ [23120] InheritanceManager::addBeneficiery(user2: [0x537C8f3d3E18dF5517a58B3fB9D9143697996802])
│ └─ ← [Stop]
├─ [145826] InheritanceManager::createEstateNFT("condominium", 2000000 [2e6], 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]
├─ [46784] ERC20Mock::mint(user2: [0x537C8f3d3E18dF5517a58B3fB9D9143697996802], 2000000 [2e6])
│ ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: user2: [0x537C8f3d3E18dF5517a58B3fB9D9143697996802], value: 2000000 [2e6])
│ └─ ← [Stop]
├─ [0] VM::startPrank(user2: [0x537C8f3d3E18dF5517a58B3fB9D9143697996802])
│ └─ ← [Return]
├─ [24735] ERC20Mock::approve(InheritanceManager: [0x88F59F8826af5e695B13cA934d6c7999875A9EeA], 2000000 [2e6])
│ ├─ emit Approval(owner: user2: [0x537C8f3d3E18dF5517a58B3fB9D9143697996802], spender: InheritanceManager: [0x88F59F8826af5e695B13cA934d6c7999875A9EeA], value: 2000000 [2e6])
│ └─ ← [Return] true
├─ [22686] InheritanceManager::inherit()
│ └─ ← [Stop]
├─ [55920] InheritanceManager::buyOutEstateNFT(1)
│ ├─ [26058] ERC20Mock::transferFrom(user2: [0x537C8f3d3E18dF5517a58B3fB9D9143697996802], InheritanceManager: [0x88F59F8826af5e695B13cA934d6c7999875A9EeA], 1000000 [1e6])
│ │ ├─ emit Transfer(from: user2: [0x537C8f3d3E18dF5517a58B3fB9D9143697996802], to: InheritanceManager: [0x88F59F8826af5e695B13cA934d6c7999875A9EeA], value: 1000000 [1e6])
│ │ └─ ← [Return] true
│ ├─ [25204] ERC20Mock::transfer(user1: [0x29E3b139f4393aDda86303fcdAa35F60Bb7092bF], 500000 [5e5])
│ │ ├─ emit Transfer(from: InheritanceManager: [0x88F59F8826af5e695B13cA934d6c7999875A9EeA], to: user1: [0x29E3b139f4393aDda86303fcdAa35F60Bb7092bF], value: 500000 [5e5])
│ │ └─ ← [Return] true
│ └─ ← [Stop]
├─ [559] ERC20Mock::balanceOf(user1: [0x29E3b139f4393aDda86303fcdAa35F60Bb7092bF]) [staticcall]
│ └─ ← [Return] 500000 [5e5]
├─ [0] VM::assertNotEq(500000 [5e5], 1000000 [1e6]) [staticcall]
│ └─ ← [Return]
├─ [559] ERC20Mock::balanceOf(user1: [0x29E3b139f4393aDda86303fcdAa35F60Bb7092bF]) [staticcall]
│ └─ ← [Return] 500000 [5e5]
├─ [0] console::log("user1.USDCbalance:", 500000 [5e5]) [staticcall]
│ └─ ← [Stop]
└─ ← [Stop]
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.23ms (272.89µs CPU time)
Tools Used
Manual review
Recommendations
Change to the following
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);
}
Updates
Lead Judging Commences
0xtimefliez 6 months ago
Submission Judgement Published Assigned finding tags:
buyOutNFT has wrong denominator
buyOutNFT has return instead of continue
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.