Eggstravaganza

Summary

The depositEgg function in EggVault.sol is marked as public, allowing anyone to call it as long as the NFT (with the specified tokenId) has been transferred to the vault contract. The function takes two parameters: tokenId and depositor and it sets eggDepositors[tokenId] to the provided depositor address after verifying that the NFT is owned by the vault. In the intended flow, EggHuntGame.depositEggToVault transfers the NFT from the user to the vault using transferFrom and then calls eggVault.depositEgg(tokenId, msg.sender) to register the user as the depositor. However, because depositEgg is public and independent of the caller’s identity, an attacker can exploit this via front-running.

Vulnerability Details

The attack scenario is as follows:

Alice calls depositEggToVault on EggHuntGame, which transfers her NFT to the vault. Before the subsequent depositEgg call is mined, Bob monitors the mempool, sees the NFT transfer and submits a transaction calling vault.depositEgg(tokenId, Bob). If Bob’s transaction is mined first, storedEggs[tokenId] is set to true and eggDepositors[tokenId] is set to Bob. Alice’s transaction then fails due to the require(!storedEggs[tokenId], "Egg already deposited") check, and Bob can later withdraw Alice’s NFT using withdrawEgg.

Impact

An attacker can steal NFTs intended for deposit into the vault, resulting in direct asset loss for users. This undermines the security of the vault

Recommendations

To fix this I would integrate the deposit logic with the NFT transfer using safeTransferFrom and the onERC721Received callback. Update EggVault to implement the IERC721Receiver interface with an onERC721Received function that sets storedEggs[tokenId] and eggDepositors[tokenId] based on the data provided during the transfer.