Submission Details
Severity:
`InheritanceManager.sol::buyOutEstateNFT()` may end prematurely and not pay remaining beneficiaries
Summary
In InheritanceManager.sol::buyOutEstateNFT(), if the buyer beneficiary is not the last beneficiary, the function will return and end the loop. Causing remaining beneficiaries to not receive their payouts.
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
User2 who is the buyer and also first beneficiary. After purchasing the NFT, the function ends and does not pay remaining beneficiary (user1).
function test_buyOutEstateNFTDoesNotPayRemainingBeneficiaries() public {
// Create 1 more beneficiary
address user2 = makeAddr("user2");
// Add the beneficiaries, in this case we add user2 first
vm.startPrank(owner);
im.addBeneficiery(user2);
im.addBeneficiery(user1);
// 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);
// user2 has bought the NFT, and because user2 is first beneficiary, the tx return and ends
// User1 did not receive any payment.
vm.assertEq(usdc.balanceOf(user1), 0);
console.log("user1.USDCbalance:", usdc.balanceOf(user1));
}
Results
[PASS] test_buyOutEstateNFTDoesNotPayRemainingBeneficiaries() (gas: 391490)
Logs:
user1.USDCbalance: 0
Traces:
[391490] InheritanceManagerTest::test_buyOutEstateNFTDoesNotPayRemainingBeneficiaries()
├─ [0] VM::addr(<pk>) [staticcall]
│ └─ ← [Return] user2: [0x537C8f3d3E18dF5517a58B3fB9D9143697996802]
├─ [0] VM::label(user2: [0x537C8f3d3E18dF5517a58B3fB9D9143697996802], "user2")
│ └─ ← [Return]
├─ [0] VM::startPrank(owner: [0x7c8999dC9a822c1f0Df42023113EDB4FDd543266])
│ └─ ← [Return]
├─ [69020] InheritanceManager::addBeneficiery(user2: [0x537C8f3d3E18dF5517a58B3fB9D9143697996802])
│ └─ ← [Stop]
├─ [23120] InheritanceManager::addBeneficiery(user1: [0x29E3b139f4393aDda86303fcdAa35F60Bb7092bF])
│ └─ ← [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]
├─ [28634] 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
│ └─ ← [Stop]
├─ [2559] ERC20Mock::balanceOf(user1: [0x29E3b139f4393aDda86303fcdAa35F60Bb7092bF]) [staticcall]
│ └─ ← [Return] 0
├─ [0] VM::assertEq(0, 0) [staticcall]
│ └─ ← [Return]
├─ [559] ERC20Mock::balanceOf(user1: [0x29E3b139f4393aDda86303fcdAa35F60Bb7092bF]) [staticcall]
│ └─ ← [Return] 0
├─ [0] console::log("user1.USDCbalance:", 0) [staticcall]
│ └─ ← [Stop]
└─ ← [Stop]
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 939.59µs (253.00µ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;
+ continue;
} 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:
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.