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

NFT Buyout Multiple Purchase Vulnerability

0xblackadam

Description

The buyOutEstateNFT() function in the InheritanceManager contract contains a critical vulnerability that allows the same NFT to be purchased multiple times and also allows the purchase of Nonexistent NFT. This occurs due to an early return statement inside the beneficiary distribution loop that prevents the NFT burning operation from being executed.

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; // Early return skips NFT burning
} else {
IERC20(assetToPay).safeTransfer(beneficiaries[i], finalAmount / divisor);
}
}
nft.burnEstate(_nftID); // This line is never reached if caller is a beneficiary
}

When a beneficiary calls this function, the code executes the token transfer, then loops through the beneficiaries to distribute funds. When it finds the caller in the beneficiary list, it returns early, skipping the nft.burnEstate(_nftID) call that would prevent the NFT from being purchased again.

Impact

This vulnerability has severe consequences:

  1. Multiple Purchases: The same NFT can be bought out multiple times, as confirmed by the test case where im.buyOutEstateNFT(1) is called twice successfully.

  2. Inconsistent State: The contract state becomes inconsistent as the NFT remains active despite being "bought out."

  3. Economic Exploitation: A malicious actor could trick other beneficiaries into believing an NFT has been properly bought out when it hasn't been burned and can be purchased again.

  4. Contract Integrity: The core functionality of the estate buyout mechanism is fundamentally compromised.

Proof of Concept

The test case clearly demonstrates the vulnerability:

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);
usdc.mint(user2, 4e6);
vm.warp(1 + 90 days);
vm.startPrank(user3);
usdc.approve(address(im), 4e6);
im.inherit();
im.buyOutEstateNFT(1); // First purchase
im.buyOutEstateNFT(1); // Second purchase of the same NFT - should fail but doesn't
vm.stopPrank();
}

Ouptut:

[481390] 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]
├─ [69020] InheritanceManager::addBeneficiery(user1: [0x29E3b139f4393aDda86303fcdAa35F60Bb7092bF])
│ └─ ← [Stop]
├─ [23120] InheritanceManager::addBeneficiery(user2: [0x537C8f3d3E18dF5517a58B3fB9D9143697996802])
│ └─ ← [Stop]
├─ [23120] InheritanceManager::addBeneficiery(user3: [0xc0A55e2205B289a967823662B841Bd67Aa362Aec])
│ └─ ← [Stop]
├─ [145826] 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]
├─ [83206] 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]
├─ [17506] InheritanceManager::buyOutEstateNFT(1)
│ ├─ [4158] ERC20Mock::transferFrom(user3: [0xc0A55e2205B289a967823662B841Bd67Aa362Aec], InheritanceManager: [0x88F59F8826af5e695B13cA934d6c7999875A9EeA], 2000000 [2e6])
│ │ ├─ emit Transfer(from: user3: [0xc0A55e2205B289a967823662B841Bd67Aa362Aec], to: InheritanceManager: [0x88F59F8826af5e695B13cA934d6c7999875A9EeA], value: 2000000 [2e6])
│ │ └─ ← [Return] true
│ ├─ [3304] ERC20Mock::transfer(user1: [0x29E3b139f4393aDda86303fcdAa35F60Bb7092bF], 666666 [6.666e5])
│ │ ├─ emit Transfer(from: InheritanceManager: [0x88F59F8826af5e695B13cA934d6c7999875A9EeA], to: user1: [0x29E3b139f4393aDda86303fcdAa35F60Bb7092bF], value: 666666 [6.666e5])
│ │ └─ ← [Return] true
│ ├─ [3304] 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]

Recommendation

Restructure the function to ensure the NFT is always burned after purchase, regardless of which beneficiary path is taken:

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);
// Distribute funds to all beneficiaries except the caller
for (uint256 i = 0; i < beneficiaries.length; i++) {
if (msg.sender != beneficiaries[i]) {
IERC20(assetToPay).safeTransfer(beneficiaries[i], finalAmount / divisor);
}
}
// Always burn the NFT
nft.burnEstate(_nftID);
}

This revision ensures that:

  1. Only non-caller beneficiaries receive funds

  2. The NFT is always burned after a successful purchase

  3. The function's logic remains intact while fixing the vulnerability

Tools Used

  • Foundry Testing Framework

  • Transaction Trace Analysis

  • Manual Code Review

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!