Eggstravaganza

Summary

The EggVault.depositEgg function can be front-run by a malicious actor to steal NFTs that users intend to deposit, by designating themselves as the depositor.

Vulnerability Details

The depositEgg function in EggVault.sol is declared public and takes a depositor address as an argument:

The intended deposit flow is via the EggHuntGame.depositEggToVault function, which atomically transfers the NFT to the vault and then calls EggVault.depositEgg. However, a user might mistakenly first transfer their NFT directly to the EggVault contract and then call EggVault.depositEgg themselves.

This two-step process (transfer then call depositEgg) opens a window for a front-running attack:

1. A legitimate user transfers their NFT (e.g., tokenId 123) to the EggVault contract address.

2. The user then submits a transaction calling depositEgg(123, userAddress).

3. An attacker observing the mempool sees this transaction.

4. The attacker copies the transaction data, replaces userAddress with attackerAddress, and submits their own transaction calling depositEgg(123, attackerAddress) with a higher gas price.

5. The attacker's transaction is mined first, registering the attacker as the eggDepositor for tokenId 123.

6. The attacker can now call withdrawEgg(123) to transfer the NFT to themselves.

Impact

Users can permanently lose their NFTs if they attempt to deposit them directly into the vault instead of using the EggHuntGame.depositEggToVault function, due to this front-running vulnerability.

Tools Used

Manual Review

Recommendations

Restrict the depositEgg function so that it can only be called by the EggHuntGame contract. This ensures that deposits only happen through the intended, secure EggHuntGame::depositEggToVault flow.

1. Add a state variable in EggVault to store the EggHuntGame contract address. This should ideally be set immutably in the constructor or with a dedicated setter function with appropriate access control.

   // In EggVault.sol

   address public eggHuntGameAddress;

   ​

   // Example: Set in constructor or via a setter

   // constructor(address _gameAddress) Ownable(msg.sender) {

   // eggHuntGameAddress = _gameAddress;

   // }

   ​

   // OR add a setter function

   function setEggHuntGameAddress(address _gameAddress) external onlyOwner {

   require(_gameAddress != address(0), "Invalid game address");

   eggHuntGameAddress = _gameAddress;

   // Consider adding an event emission here

   }

2. Add an inline require statement at the beginning of the depositEgg function to check the caller:

   function depositEgg(uint256 tokenId, address depositor) public {

   require(msg.sender == eggHuntGameAddress, "Caller is not the EggHuntGame contract"); // Add this check

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

   }

This change enforces that only the EggHuntGame contract can dictate the depositor, eliminating the front-running vulnerability.