Eggstravaganza

Summary

When players choose to deposit their egg(s) to the EggVault via the EggVault::depositEgg function, they must first transfer the egg to the vault in a separate transaction. Due to missing access control, an attacker can then front-run the call to EggVault::depositEgg and use a different depositor address to which the egg can later be withdrawn.

Vulnerability Details

Players can deposit and secure found eggs in the EggVault through calling the EggHuntGame::despositEggToVault or directly via the EggVault::depositEgg function. The latter case requires the player to first transfer the egg NFT to the EggVault in a separate transaction. Once the NFT is transferred, the player calls EggVault::depositEgg which allows for an arbitrary depositor address. Between the transfer of the NFT and the call of EggVault::depositEgg, an attacker could front-run the call with their own depositor address. The attacker can use that address to withdraw the egg with the EggVault::withdrawEgg function and steal the egg from the player.

Proof of Concept

The following scenario may lead to stolen eggs:

1. User finds egg

2. User transfers egg to the vault by calling EggstravaganzaNFT::transferFrom

3. User initiates call to EggVault::depositEgg

4. Attacker front-runs user's call using their own address for depositor

5. Deposited egg is registred to attacker's address

6. Attacker withdraws egg from the vault via EggVault::withdrawEgg and steals egg

Code:

Place following code into EggHuntGameTest.t.sol:

Impact

The impact of this vulnerability is medium to high, as funds (egg NFTs) are directly at risk to be stolen from users and would significantly disrupt the user experience of players. Note, however, that this assessment is based on the assumption that egg NFTs are valuable assets within, and possibly outside of, the game ecosystem. Nothing in the documentation would speak for or against this assumption. One could also argue that the egg NFTs themselves do not have intrinsic value and are merely the means to play the game, and therefore the impact would only be on user experience and not financial.

Tools Used

Manual review, custom test

Recommendations

To prevent front-running attacks, the EggVault::depositEgg function should be access restricted to the EggHuntGame contract, which would force users to deposit their eggs through the EggHuntGame::despositEggToVault function. This would prevent any front-running attacks as the egg would be transferred and deposited in one transaction.