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

Front-running Vulnerability in EggVault::depositEgg

molaratai

Summary

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);
​
}
function depositEgg(uint256 tokenId, address depositor) public {
require(eggNFT.ownerOf(tokenId) == address(this), "NFT not transferred to vault");
//No checks performed on function caller
require(!storedEggs[tokenId], "Egg already deposited");
storedEggs[tokenId] = true;
eggDepositors[tokenId] = depositor;
emit EggDeposited(depositor, tokenId);
}

The EggVault::depositEgg function is publicly accessible and allows any address to register themselves as the depositor of an NFT after it has been transferred to the vault but before the EggHuntGame contract executes the depositEgg function. This opens the door for front-running attacks through mempool monitoring.

Vulnerability Details

  1. A legitimate user submits a transaction calling depositEggToVault(tokenId), which first transfers the NFT to the vault, then calls depositEgg.

  2. An attacker monitoring the mempool observes this pending transaction.

  3. The attacker frontruns the user's transaction by directly calling eggVault.depositEgg(tokenId, attackerAddress) with a higher gas fee.

  4. If the attacker's transaction is mined first, they become the recorded depositor of the NFT.

  5. When the original user's transaction executes, the NFT is transferred to the vault, but the attacker has already registered themselves as the depositor.

  6. The attacker can later invoke withdrawEgg(tokenId) and claim the NFT.

Impact

  • Loss of assets: Attackers can steal NFTs by becoming the recorded depositor.

  • Trust violation: Players lose ownership of NFTs they believe were safely deposited.

  • Game integrity: The logic of the EggHuntGame can be undermined, allowing attackers to manipulate the system.

Tools Used

Manual code review

Recommendations

Consider using safeTransferFrom() and the onERC721Received() callback in the vault contract and also moving depositEgg() logic into onERC721Received(). This prevents external calls to depositEgg() and ensures only legitimate transfers register the depositor.

  1. In 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);
+ eggNFT.safeTransferFrom(msg.sender, address(eggVault), tokenId);
​
- eggVault.depositEgg(tokenId, msg.sender);
​
}

In EggVault contract

import "@openzeppelin/contracts/access/Ownable.sol";
import "./EggstravaganzaNFT.sol";
+import "@openzeppelin/contracts/token/ERC721/IERC721Receiver.sol";
​
- contract EggVault is Ownable {
+ contract EggVault is Ownable, IERC721Receiver {
/// @notice Reference to the EggstravaganzaNFT contract.
EggstravaganzaNFT public eggNFT;
​
/// @notice Mapping to track deposited eggs (tokenId => deposited status).
mapping(uint256 => bool) public storedEggs;
/// @notice Mapping to record who deposited each egg.
mapping(uint256 => address) public eggDepositors;
​
event EggDeposited(address indexed depositor, uint256 tokenId);
event EggWithdrawn(address indexed withdrawer, uint256 tokenId);
​
constructor()Ownable(msg.sender){}
​
+ function onERC721Received(
+ address operator,
+ address from,
+ uint256 tokenId,
+ bytes calldata
+ ) external override returns (bytes4) {
+ require(msg.sender == address(eggNFT), "Invalid NFT");
​
+ require(!storedEggs[tokenId], "Egg already deposited");
+ storedEggs[tokenId] = true;
+ eggDepositors[tokenId] = from;
​
+ emit EggDeposited(from, tokenId);
+ return this.onERC721Received.selector;
+ }
/// @notice Set the NFT contract address.
function setEggNFT(address _eggNFTAddress) external onlyOwner {...}
​
/// @notice Records the deposit of an egg (NFT).
/// The NFT must already have been transferred to the vault.
- function depositEgg(uint256 tokenId, address depositor) public {...}
Updates

Lead Judging Commences

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