Eggstravaganza
First Flight #37
Beginner FriendlySolidity
100 EXP
View results
Previous Next
Submission Details
Severity: high
Invalid

Unrestricted NFT Address Update in EggVault Leads to Permanent Loss of User Deposits

0xluckyluke

Summary

The EggVault::setEggNFT function allows the owner to change the NFT contract address without any restrictions, even when there are active deposits. This can result in users permanently losing access to their deposited NFTs, as the vault will attempt to interact with the new NFT contract address which has no knowledge of the previously deposited tokens.

Vulnerability Details

https://github.com/CodeHawks-Contests/2025-04-eggstravaganza/blob/f83ed7dff700c4319bdfd0dff796f74db5be4538/src/EggVault.sol#L22-L25

The vulnerability exists in the EggVault::setEggNFT function:

function setEggNFT(address _eggNFTAddress) external onlyOwner {
require(_eggNFTAddress != address(0), "Invalid NFT address");
eggNFT = EggstravaganzaNFT(_eggNFTAddress);
}

The function only checks that the new address is not zero but fails to:

  1. Check if there are any active deposits in the vault

  2. Implement any time-delay or notification mechanism for users

  3. Have any mechanism to migrate existing deposits

When the NFT contract address is changed:

  • The vault still holds the original NFTs

  • The vault's state (storedEggs and eggDepositors mappings) remains unchanged

  • All withdrawal attempts fail because the new NFT contract has no knowledge of the original tokens

This creates a permanent lock of user funds as:

  • The new NFT contract will revert on transferFrom calls for non-existent tokens

  • The original NFT contract is no longer referenced by the vault

  • There is no mechanism to recover or migrate the locked tokens

Add this test file EggVaultUpgradeTest.t:

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.23;
​
import "forge-std/Test.sol";
import "../src/EggstravaganzaNFT.sol";
import "../src/EggVault.sol";
import "../src/EggHuntGame.sol";
​
contract EggVaultUpgradeTest is Test {
EggstravaganzaNFT nft;
EggstravaganzaNFT newNft;
EggVault vault;
EggHuntGame game;
address owner;
address alice;
​
function setUp() public {
owner = address(this);
alice = address(0xA11CE);
// Deploy original contracts
nft = new EggstravaganzaNFT("Eggstravaganza", "EGG");
vault = new EggVault();
game = new EggHuntGame(address(nft), address(vault));
// Configure contracts
nft.setGameContract(address(game));
vault.setEggNFT(address(nft));
// Mint an NFT to Alice via the game contract authority
vm.prank(address(game));
nft.mintEgg(alice, 1);
// Verify Alice owns the token
assertEq(nft.ownerOf(1), alice);
}
/// @notice Test demonstrating the risk of changing the NFT contract address
function testChangeNFTContractAddress() public {
// Alice approves the game contract to transfer her NFT
vm.prank(alice);
nft.approve(address(game), 1);
// Alice deposits her NFT into the vault
vm.prank(address(game));
nft.transferFrom(alice, address(vault), 1);
vm.prank(address(game));
vault.depositEgg(1, alice);
// Verify Alice is recorded as the depositor
assertTrue(vault.isEggDeposited(1), "NFT should be marked as deposited");
assertEq(vault.eggDepositors(1), alice, "Alice should be the depositor");
// Deploy a new NFT contract
newNft = new EggstravaganzaNFT("NewEggstravaganza", "NEGG");
// Owner changes the NFT contract address in the vault
vm.prank(owner);
vault.setEggNFT(address(newNft));
// Attempt to withdraw the NFT using the old contract should fail
vm.prank(alice);
vm.expectRevert(abi.encodeWithSignature("ERC721NonexistentToken(uint256)", 1));
vault.withdrawEgg(1);
console.log("VULNERABLE: Changing the NFT contract address locks existing deposits!");
// Additional verification that the NFT is effectively locked
assertEq(nft.ownerOf(1), address(vault), "Original NFT still owned by vault");
vm.expectRevert(abi.encodeWithSignature("ERC721NonexistentToken(uint256)", 1));
newNft.ownerOf(1);
}
/// @notice Test demonstrating that deposits should be protected before changing NFT
function testShouldNotChangeNFTWithDeposits() public {
// Alice deposits her NFT
vm.prank(alice);
nft.approve(address(game), 1);
vm.prank(address(game));
nft.transferFrom(alice, address(vault), 1);
vm.prank(address(game));
vault.depositEgg(1, alice);
// Deploy a new NFT contract
newNft = new EggstravaganzaNFT("NewEggstravaganza", "NEGG");
// Change NFT address with active deposits (dangerous!)
vm.prank(owner);
vault.setEggNFT(address(newNft));
// Show that Alice's NFT is now inaccessible
vm.prank(alice);
vm.expectRevert(abi.encodeWithSignature("ERC721NonexistentToken(uint256)", 1));
vault.withdrawEgg(1);
console.log("WARNING: NFT address should not be changeable while deposits exist!");
console.log("User funds are now locked in the vault!");
}
​
/// @notice Test demonstrating the proper sequence of operations
function testProperNFTAddressChangeSequence() public {
// Show that NFT address can be changed when there are no deposits
newNft = new EggstravaganzaNFT("NewEggstravaganza", "NEGG");
vm.prank(owner);
vault.setEggNFT(address(newNft));
assertEq(
address(vault.eggNFT()),
address(newNft),
"NFT address should be updated when there are no deposits"
);
console.log("SAFE: NFT address can be changed when there are no active deposits");
}
}

Run forge test --match-contract "EggVaultUpgradeTest" -vvv :

2025-04-eggstravaganza$ forge test --match-contract "EggVaultUpgradeTest" -vvv
[⠊] Compiling...
[⠃] Compiling 1 files with Solc 0.8.28
[⠊] Solc 0.8.28 finished in 807.23ms
Compiler run successful!
​
Ran 3 tests for test/EggVaultUpgradeTest.t.sol:EggVaultUpgradeTest
[PASS] testChangeNFTContractAddress() (gas: 2020950)
Logs:
VULNERABLE: Changing the NFT contract address locks existing deposits!
​
[PASS] testProperNFTAddressChangeSequence() (gas: 1933262)
Logs:
SAFE: NFT address can be changed when there are no active deposits
​
[PASS] testShouldNotChangeNFTWithDeposits() (gas: 2007568)
Logs:
WARNING: NFT address should not be changeable while deposits exist!
User funds are now locked in the vault!
​
Suite result: ok. 3 passed; 0 failed; 0 skipped; finished in 3.60ms (3.06ms CPU time)
​
Ran 1 test suite in 11.19ms (3.60ms CPU time): 3 tests passed, 0 failed, 0 skipped (3 total tests)
​

Impact

  • High severity due to permanent loss of user funds

  • No recovery mechanism available

  • Affects all users with active deposits when the NFT address is changed

  • Could be exploited maliciously or triggered accidentally by admin

Tools Used

Recommendations

Short term:

  1. Add a check for active deposits before allowing NFT address changes:

function setEggNFT(address _eggNFTAddress) external onlyOwner {
require(_eggNFTAddress != address(0), "Invalid NFT address");
// Check for any active deposits
require(getTotalDeposits() == 0, "Cannot change NFT address with active deposits");
eggNFT = EggstravaganzaNFT(_eggNFTAddress);
}

Long term:

  1. Implement an upgradeable pattern that includes:

    • Time-delay for critical parameter changes

    • User notification mechanism

    • Emergency withdrawal functionality

  2. Consider implementing a token migration mechanism if NFT contract updates are necessary

  3. Add events for important state changes to improve transparency

  4. Consider making the NFT address immutable after initial setup

Updates

Lead Judging Commences

m3dython Lead Judge 3 months ago
Submission Judgement Published
Invalidated
Reason: Design choice
Assigned finding tags:

Trusted Owner

Owner is trusted and is not expected to interact in ways that would compromise security

Support

FAQs

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