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

The burnEstate function allows burning NFTs without ownership verification

0xWithdrawers

Description

The burnEstate() function in the NFTFactory contract allows the inheritanceManager to burn any NFT without verifying that the caller is actually the owner of the token. The function only checks that the caller is the inheritanceManager address but fails to validate ownership of the NFT being burned.

function burnEstate(uint256 _id) external onlyInheritanceManager {
_burn(_id);
}

This violates a fundamental principle of ERC721 tokens where only the owner or an approved address should be able to transfer or destroy a token. Since the inheritanceManager can mint and then transfer NFTs to beneficiaries, the ability to burn these tokens after transfer represents a serious vulnerability.

Impact

  1. Unauthorized token destruction: The inheritanceManager can burn any NFT at any time, even after legitimate ownership transfer to a beneficiary or other party.

  2. Asset permanence violation: NFTs in this system are meant to represent real estate or other valuable assets as indicated by the createEstateNFT function, which links them to real-world values. The ability to destroy these tokens undermines their value as a representation of ownership.

  3. Trust violation: Beneficiaries and other token holders cannot trust that their NFT ownership is secure, which undermines the entire inheritance management system.

  4. Potential financial loss: If an NFT represents a valuable real-world asset or has been purchased for a significant amount, its unauthorized destruction could result in substantial financial loss to the legitimate owner.

Mitigating factors

  • Only the inheritanceManager contract can call this function (enforced by the onlyInheritanceManager modifier).

  • If the system is designed with the assumption that the inheritanceManager should have this level of control over all tokens, then this may be intentional behavior.

Proof of Concept (PoC)

The following code demonstrates how the inheritanceManager can burn an NFT that has been transferred to another owner:

// SPDX-License-Identifier: MIT
pragma solidity 0.8.26;
​
import "forge-std/Test.sol";
import "../src/NFTFactory.sol";
​
// This PoC demonstrates that the inheritanceManager can burn any token without having to check that it is the owner.
​
contract NFTFactoryPoCTest is Test {
NFTFactory nftFactory;
address inheritanceManager;
address user1;
uint256 tokenId;
​
function setUp() public {
// Set up addresses
inheritanceManager = makeAddr("inheritanceManager");
user1 = makeAddr("user1");
// Deploy NFT factory with inheritance manager address
nftFactory = new NFTFactory(inheritanceManager);
// Mint an NFT to the inheritance manager
vm.startPrank(inheritanceManager);
tokenId = nftFactory.createEstate("Test Estate");
// Transfer the NFT to user1
nftFactory.transferFrom(inheritanceManager, user1, tokenId);
vm.stopPrank();
}
​
function testBurnTokenNotOwned() public {
// Verify user1 owns the token
assertEq(nftFactory.ownerOf(tokenId), user1);
console.log("Token %d is owned by: %s", tokenId, user1);
// The inheritance manager can burn the token despite not owning it
vm.prank(inheritanceManager);
nftFactory.burnEstate(tokenId);
console.log("Token burned by inheritance manager: %s", inheritanceManager);
// Verify the token is burned (should revert when checking ownership)
vm.expectRevert(abi.encodeWithSignature("ERC721NonexistentToken(uint256)", tokenId));
nftFactory.ownerOf(tokenId);
console.log("Token no longer exists");
}
}

Place the test in the test folder and run it with the following command

forge test --match-test test_BurnTokenNotOwned -vv

When running this test, we can observe that the token is successfully burned by the inheritance manager even though it is owned by user1:

Token 1 is owned by: 0x29E3b139f4393aDda86303fcdAa35F60Bb7092bF
Token burned by inheritance manager: 0x17009c6C57Bd56E3C086D0C5ECd5522176e9a708

Recommendation

To fix this vulnerability, the burnEstate function should be modified to include ownership verification:

function burnEstate(uint256 _id) external onlyInheritanceManager {
// Ensure the inheritance manager owns the token before burning
require(ownerOf(_id) == inheritanceManager, "Not the token owner");
_burn(_id);
}

Alternatively, if the design intention is to allow the inheritance manager to burn tokens regardless of ownership (which would be unusual for an NFT system), this ability should be clearly documented as it poses significant trust concerns for token holders.

Concrete Impact Example

To illustrate the potential impact of this vulnerability, consider this scenario:

  • A beneficiary receives an NFT representing a valuable real estate property worth $500,000

  • The beneficiary has full ownership of this NFT according to the blockchain

  • At any time, the inheritance manager can unilaterally destroy this NFT

  • If the NFT is the only proof of ownership for the underlying asset, the beneficiary could lose their claim to the property

This vulnerability essentially means that any NFT in the system can be rendered worthless at the discretion of the inheritance manager, regardless of who rightfully owns it.

Updates

Lead Judging Commences

0xtimefliez Lead Judge 3 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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