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

`createEstateNFT()` overwrites the global `assetToPay` variable for ALL NFTs

0xWithdrawers

Description

The InheritanceManager contract inherits from the Trustee contract, which contains a global variable assetToPay that specifies which token should be used to purchase NFTs. The createEstateNFT() function in InheritanceManager sets this global variable each time a new NFT is created:

function createEstateNFT(string memory _description, uint256 _value, address _asset) external onlyOwner {
uint256 nftID = nft.createEstate(_description);
nftValue[nftID] = _value;
assetToPay = _asset; // <-- This overwrites the payment asset for ALL NFTs
}

This design flaw means that the payment token is not associated with individual NFTs, but is a global setting that affects all NFTs in the system. As a result, each time a new NFT is created with a different payment token, it changes the required payment token for all previously created NFTs.

Impact

This vulnerability has several significant impacts:

  1. Payment Asset Mismatch: Beneficiaries expecting to buy out an NFT with a specific token (e.g., a stablecoin like USDC) may suddenly be required to use a different, potentially less valuable token.

  2. Value Manipulation: An owner could intentionally manipulate the payment asset just before inheritance, changing it from a valuable token to a less valuable one, thereby reducing the effective compensation other beneficiaries would receive.

  3. Contract Intent Violation: The contract's intent appears to be that each NFT should have its own associated payment token, but the implementation fails to achieve this.

Proof of Concept

The following test demonstrates this vulnerability:

// SPDX-License-Identifier: MIT
pragma solidity 0.8.26;
​
import "forge-std/Test.sol";
import "../src/InheritanceManager.sol";
​
// @audit-note - [VULN 09]
// This PoC demonstrates that calling createEstateNFT() overwrites the assetToPay variable
// for ALL existing NFTs, allowing the owner to change the payment currency for previously
// created NFTs to a cheaper or less valuable asset.
​
contract PoCTest is Test {
InheritanceManager public inheritanceManager;
address public owner;
address public tokenA;
address public tokenB;
function setUp() public {
// Deploy the InheritanceManager contract
inheritanceManager = new InheritanceManager();
owner = inheritanceManager.getOwner();
// Create mock addresses for two different tokens
tokenA = address(0xA1);
tokenB = address(0xB2);
}
function test_AssetToPayOverwrite() public {
// Step 1: Create first NFT with TokenA as payment asset
vm.prank(owner);
inheritanceManager.createEstateNFT("Asset 1", 1000, tokenA);
​
// Step 2: Verify assetToPay is set to tokenA
address paymentAssetAfterFirstNFT = inheritanceManager.getAssetToPay();
console.log("After creating NFT #0:");
console.log("- Payment token (assetToPay):", paymentAssetAfterFirstNFT);
assertEq(paymentAssetAfterFirstNFT, tokenA, "assetToPay should be set to tokenA");
​
// Step 3: Create second NFT with TokenB as payment asset
vm.prank(owner);
inheritanceManager.createEstateNFT("Asset 2", 500, tokenB);
// Step 4: Verify assetToPay has been changed to tokenB for ALL NFTs
address paymentAssetAfterSecondNFT = inheritanceManager.getAssetToPay();
console.log("\nAfter creating NFT #1:");
console.log("- Payment token (assetToPay):", paymentAssetAfterSecondNFT);
assertEq(paymentAssetAfterSecondNFT, tokenB, "assetToPay should now be set to tokenB");
assertTrue(paymentAssetAfterSecondNFT != tokenA, "assetToPay should no longer be tokenA");
​
console.log("\n----- VULNERABILITY DEMONSTRATION -----");
console.log("Initial payment token required for first NFT:", tokenA);
console.log("Current payment token required for ALL NFTs:", tokenB);
}
}

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

forge test --match-test test_AssetToPayOverwrite -vv

Recommendation

There are two potential solutions to this issue:

  1. Associate payment assets with individual NFTs: Store the payment token for each NFT in a mapping rather than using a global variable:

// Add this mapping to the Trustee contract
mapping(uint256 nftID => address paymentToken) public nftPaymentAsset;
​
// Modify createEstateNFT to use the mapping
function createEstateNFT(string memory _description, uint256 _value, address _asset) external onlyOwner {
uint256 nftID = nft.createEstate(_description);
nftValue[nftID] = _value;
nftPaymentAsset[nftID] = _asset; // Store payment asset per NFT
}
​
// Update buyOutEstateNFT to use the NFT-specific payment asset
function buyOutEstateNFT(uint256 _nftID) external onlyBeneficiaryWithIsInherited {
uint256 value = nftValue[_nftID];
address paymentToken = nftPaymentAsset[_nftID]; // Get the correct token for this NFT
// Rest of the function remains the same but uses paymentToken instead of assetToPay
...
}

  1. Make assetToPay immutable after first set: If having a single payment token for all NFTs is intended, consider making it immutable after the first NFT is created:

// Add this variable to Trustee
bool public assetToPayLocked = false;
​
// Modify createEstateNFT to prevent changes after first set
function createEstateNFT(string memory _description, uint256 _value, address _asset) external onlyOwner {
uint256 nftID = nft.createEstate(_description);
nftValue[nftID] = _value;
if (!assetToPayLocked) {
assetToPay = _asset;
assetToPayLocked = true;
}
}

The first approach is preferred as it provides more flexibility and correctly implements the apparent intention of the contract design.

Updates

Lead Judging Commences

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

global asset in NFT values

Support

FAQs

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

Give us feedback!