Submission Details
Severity:
`InheritanceManager::buyOutEstateNFT` does not payout to other beneficiaries
Summary
The function is intended to payout the share from the value of the NFT to each other beneficiary. This can be skipped if the msg.sender is the first beneficiary. Even more, any beneficiary after his index will not get paid due to the return statement in the loop.
Vulnerability Details
Likelihood: High. Any beneficiary can trigger this by buying an NFT, making it a guaranteed flaw in normal operation.
The test is buying the real estate NFT via user1 who is the first beneficiary.
From the logs we can see that there were no Transfer events to the other beneficiaries.
function test_buyOutEstateNFTDoesNotPayToBeneficiaries() 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", 100e18, address(weth));
im.createEstateNFT("our beach-house 2", 2000000e18, address(bnb));
vm.stopPrank();
bnb.mint(user1, 100e18);
vm.warp(1 + 90 days);
vm.startPrank(user1);
bnb.approve(address(im), 100e18);
im.inherit();
im.buyOutEstateNFT(1);
vm.stopPrank();
}
[PASS] test_buyOutEstateNFTDoesNotPayToBeneficiaries() (gas: 483949)
Traces:
[483949] InheritanceManagerTest::test_buyOutEstateNFTDoesNotPayToBeneficiaries()
├─ [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", 100000000000000000000 [1e20], ERC20Mock: [0x2e234DAe75C793f67A35089C9d99245E1C58470b])
│ ├─ [95512] NFTFactory::createEstate("our beach-house")
│ │ ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: InheritanceManager: [0x88F59F8826af5e695B13cA934d6c7999875A9EeA], tokenId: 1)
│ │ ├─ emit MetadataUpdate(_tokenId: 1)
│ │ └─ ← [Return] 1
│ └─ ← [Stop]
├─ [73526] InheritanceManager::createEstateNFT("our beach-house 2", 2000000000000000000000000 [2e24], ERC20Mock: [0xF62849F9A0B5Bf2913b396098F7c7019b51A820a])
│ ├─ [49712] NFTFactory::createEstate("our beach-house 2")
│ │ ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: InheritanceManager: [0x88F59F8826af5e695B13cA934d6c7999875A9EeA], tokenId: 2)
│ │ ├─ emit MetadataUpdate(_tokenId: 2)
│ │ └─ ← [Return] 2
│ └─ ← [Stop]
├─ [0] VM::stopPrank()
│ └─ ← [Return]
├─ [46784] ERC20Mock::mint(user1: [0x29E3b139f4393aDda86303fcdAa35F60Bb7092bF], 100000000000000000000 [1e20])
│ ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: user1: [0x29E3b139f4393aDda86303fcdAa35F60Bb7092bF], value: 100000000000000000000 [1e20])
│ └─ ← [Stop]
├─ [0] VM::warp(7776001 [7.776e6])
│ └─ ← [Return]
├─ [0] VM::startPrank(user1: [0x29E3b139f4393aDda86303fcdAa35F60Bb7092bF])
│ └─ ← [Return]
├─ [24735] ERC20Mock::approve(InheritanceManager: [0x88F59F8826af5e695B13cA934d6c7999875A9EeA], 100000000000000000000 [1e20])
│ ├─ emit Approval(owner: user1: [0x29E3b139f4393aDda86303fcdAa35F60Bb7092bF], spender: InheritanceManager: [0x88F59F8826af5e695B13cA934d6c7999875A9EeA], value: 100000000000000000000 [1e20])
│ └─ ← [Return] true
├─ [22686] InheritanceManager::inherit()
│ └─ ← [Stop]
├─ [28683] InheritanceManager::buyOutEstateNFT(1)
│ ├─ [26058] ERC20Mock::transferFrom(user1: [0x29E3b139f4393aDda86303fcdAa35F60Bb7092bF], InheritanceManager: [0x88F59F8826af5e695B13cA934d6c7999875A9EeA], 66666666666666666666 [6.666e19])
│ │ ├─ emit Transfer(from: user1: [0x29E3b139f4393aDda86303fcdAa35F60Bb7092bF], to: InheritanceManager: [0x88F59F8826af5e695B13cA934d6c7999875A9EeA], value: 66666666666666666666 [6.666e19])
│ │ └─ ← [Return] true
│ └─ ← [Stop]
├─ [0] VM::stopPrank()
│ └─ ← [Return]
└─ ← [Stop]
Impact
High - a beneficiary with malicious intents can buyout the NFT without paying out to other beneficiaries.
It is still possible for beneficiaries to receive the funds using InheritanceManager::withdrawInheritedFunds but it will not be the correct amount as the buyer would get a payout as well.
Tools Used
Manual Review
Recommendations
Reverse the if statement to avoid statements such as continue or return.
for (uint256 i = 0; i < beneficiaries.length; i++) {
- if (msg.sender == beneficiaries[i]) {
- return;
- } else {
- IERC20(assetToPay).safeTransfer(beneficiaries[i], finalAmount / divisor);
- }
+ if (msg.sender != beneficiaries[i]) {
+ IERC20(assetToPay).safeTransfer(beneficiaries[i], finalAmount / divisor);
+ }
}
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.