Eggstravaganza

Summary

The EggVault contract has a critical front-running vulnerability in its deposit process due to the separation between the NFT transfer and recording the depositor's ownership. This allows attackers to monitor the mempool and claim ownership of NFTs that have been transferred to the vault but not yet registered.

Vulnerability Details

The EggVault contract lacks the standard onERC721Received hook. The deposit mechanism relies on a two-step process implicitly (user approves game/vault, user initiates transfer, then depositEgg is called). Specifically, in EggHuntGame.depositEggToVault, the eggNFT.transferFrom happens first, sending the NFT to the vault. After this transfer is successful but before the eggVault.depositEgg(tokenId, msg.sender) call within the same function completes, the NFT resides in the vault but isn't officially recorded. The EggVault.depositEgg function itself is public and takes the depositor address as an argument. An attacker monitoring the mempool can see the successful transferFrom to the vault. They can then immediately call EggVault.depositEgg(tokenId, attackerAddress) before the original transaction's call to depositEgg executes. Because require(eggNFT.ownerOf(tokenId) == address(this)) will pass (the NFT is in the vault) and require(!storedEggs[tokenId]) will pass (it hasn't been recorded yet), the attacker's transaction can succeed, recording them as the eggDepositor.

POC:

Impact

An attacker can steal NFTs deposited into the vault by front-running the depositEgg call and registering themselves as the depositor. They can then use withdrawEgg to take the NFT.

Tools Used

• Manual code review

• Forge testing framework to validate the vulnerability

Recommendations

• Implement onERC721Received: The EggVault should inherit ERC721Holder (or manually implement onERC721Received).

• Modify Deposit Flow: Users should call safeTransferFrom on the EggstravaganzaNFT contract, transferring the NFT directly to the EggVault. The onERC721Received function in the vault will then automatically be called. Inside onERC721Received, record the _from address (the original owner transferring the NFT) as the depositor and the _tokenId.

• Remove depositEgg: The separate depositEgg function becomes unnecessary and should be removed.

• Adjust EggHuntGame.depositEggToVault: This function might be removed entirely if users interact directly with the NFT contract and vault for deposits. Alternatively, it could be kept but would need the user to have approved the Vault contract first, and then this function would simply call eggNFT.safeTransferFrom(msg.sender, address(eggVault), tokenId).