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

Missing NFT Existence Check in buyOutEstateNFT

osuolale

Summary

The buyOutEstateNFT function lacks a check to verify if the specified _nftID corresponds to an existing NFT. This allows the function to process payments for non-existent NFTs, potentially affecting operations if the NFT is minted later, as the nftValue mapping might be overwritten or misinterpreted without proper validation.

Vulnerability Details

The vulnerable code is in the buyOutEstateNFT function:

solidity

function buyOutEstateNFT(uint256 _nftID) external onlyBeneficiaryWithIsInherited {
//@audit-info k no check to know if nft exist or not, and if you buyout unexisted nft will that later affect it operation after being minted
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);
}

POC

// 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";
​
​
​
/// @title General InheritanceManager Tests
/// @notice Runs additional functional tests on `InheritanceManager`
contract InheritanceManagerTest is Test {
InheritanceManager im;
ERC20Mock usdc;
ERC20Mock weth;
​
address owner = makeAddr("owner");
address user1 = makeAddr("user1");
​
function setUp() public {
vm.prank(owner);
im = new InheritanceManager();
​
// ERC20Mock requires a name, symbol, and initial supply
usdc = new ERC20Mock();
weth = new ERC20Mock();
}
​
function test_buyOutEstateNFTForNonExistentNFT() 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);
vm.warp(1 + 90 days);
vm.startPrank(user3);
usdc.approve(address(im), 4e6);
im.inherit();
im.buyOutEstateNFT(10);
vm.stopPrank();
}
​
}

Issues:

  1. No Existence Validation:

    • The function uses nftValue[_nftID] without checking if the NFT exists. If _nftID hasn’t been minted, nftValue[_nftID] defaults to 0, yet the function proceeds (though payments may be 0).

    • nft.burnEstate(_nftID) will revert if the NFT doesn’t exist (due to ERC721._burn), but funds may already be transferred.

  2. Future Impact:

    • If a non-existent _nftID is "bought out" (with value = 0), a later minting of that ID could confuse the state, as nftValue[_nftID] might be set afterward, but the buyout already occurred.

  3. Early Fund Transfer:

    • Funds are transferred to the contract before the burn, risking partial execution if the burn reverts.

Impact

Funds at Risk: Payments for non-existent NFTs could occur if nftValue[_nftID] is non-zero (e.g., from a previous bug), with the burn reverting, leaving funds stuck.

  • State Confusion: Buying out a non-existent NFT could mislead users, and later minting the same ID might create ownership disputes or duplicate payments.

  • Low Severity: Currently mitigated by burnEstate reverting, but still a logic flaw.

Tools Used

Manual Review and foundry

Recommendations

Add an existence check before processing:

solidity

function buyOutEstateNFT(uint256 _nftID) external onlyBeneficiaryWithIsInherited {
require(nft.ownerOf(_nftID) != address(0), "NFT does not exist"); // Check existence
uint256 value = nftValue[_nftID];
require(value > 0, "NFT has no value");
uint256 divisor = beneficiaries.length - 1;
uint256 totalAmount = value;
IERC20(assetToPay).safeTransferFrom(msg.sender, address(this), totalAmount);
for (uint256 i = 0; i < beneficiaries.length; i++) {
if (msg.sender != beneficiaries[i]) {
IERC20(assetToPay).safeTransfer(beneficiaries[i], value / divisor);
}
}
nft.burnEstate(_nftID);
}
Updates

Lead Judging Commences

0xtimefliez Lead Judge 5 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

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