Eggstravaganza

Vulnerability Details

The depositEgg function takes the depositor address as a parameter. Crucially, it checks if the NFT tokenId is already held by the vault contract (eggNFT.ownerOf(tokenId) == address(this)) but does not verify that the msg.sender calling the function has any relation to the depositor address being provided or the NFT being deposited.

The attack scenario is as follows: A legitimate user finds an egg (NFT) and decides to deposit it. They first approve the EggVault contract and then transfer the NFT to the EggVault's address, intending to call depositEgg afterwards. Then an attacker monitoring the blockchain sees the NFT transfer to the EggVault address.

Before the legitimate user (or the EggHuntGame contract) calls depositEgg, the attacker calls depositEgg(tokenId, attackerAddress). Since the NFT is already in the vault, the first require passes. If the egg wasn't previously recorded as deposited, the second require also passes.

The vault now incorrectly records the attackerAddress as the eggDepositors[tokenId].

Finally the attacker can now call withdrawEgg(tokenId). The check eggDepositors[tokenId] == msg.sender will pass because the attacker's address was recorded, allowing them to steal the NFT.

Impact

Direct theft of user NFTs deposited into the vault

Recommendation

Make depositEgg callable only by EggHuntGame. Also modify depositEgg to require msg.sender to be the depositor or have prior authorization.