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

A malicious actor can steal user's NFTs by frontrunning `EggVault::depositEgg` function.

medinamv

Summary

The EggVault::depositEgg function can be frontrun claiming an user NFT and then it can be sent to the attacker wallet using the EggVault::withdrawEgg function.

Vulnerability Details

The way to deposit a NFT into EggVault is this:

  1. UserA approves the EggHuntGame contract to transfer the NFT.

  2. The EggHuntGame::depositEggToVault function transfer the NFT to EggVault.

  3. The owner of the NFT is now EggVault.

  4. Then the EggVault::depositEgg function is called to officially tell the EggVault that the NFT belongs to UserA.

This is very risky because the transaction can be seen in the mempool so, an attacker can call the EggHuntGame::depositEggToVault before the real owner, providing the attacker address and claim ownership of the NFT.

As we can see, in order to claim ownership of the NFT an address of the depositor is required and this address can be any address. So anyone can call this function by pretending to be the owner.

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

Ability to steal all NFTs that users send to the EggVault.

Tools Used

  1. Manual Review

  2. Foundry

Recommendations

Remove functionality of sending the NFTs from the EggHuntGame to the EggVault. It is not useful and brings more risks than benefits.

If a user wants to deposit his NFT, he should only do it directly calling the Vault.

Remove this function from EggHuntGame

- function depositEggToVault(uint256 tokenId) external {
- require(eggNFT.ownerOf(tokenId) == msg.sender, "Not owner of this egg");
- // The player must first approve the transfer on the NFT contract.
- eggNFT.transferFrom(msg.sender, address(eggVault), tokenId);
- eggVault.depositEgg(tokenId, msg.sender);
}

Modify this function in EggVault

- function depositEgg(uint256 tokenId, address depositor) public {
+ function depositEgg(uint256 tokenId) external {
- require(eggNFT.ownerOf(tokenId) == address(this), "NFT not transferred to vault");
+ require(eggNFT.ownerOf(tokenId) == msg.sender, "Unauthorized user");
require(!storedEggs[tokenId], "Egg already deposited");
+ eggNFT.safeTransferFrom(msg.sender, address(this), tokenId);
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 6 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.