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

State Updates After External Token Calls Violate Checks-Effects-Interactions

Author Revealed upon completion

State Updates After External Token Calls Violate Checks-Effects-Interactions

Description

The expected pattern in functions that interact with external contracts is to finalize internal state before making external calls, following the checks-effects-interactions pattern.

In both mintNft() and buy(), the contract performs an external ERC20 call before completing all related state updates. While the protocol is intended to work with USDC, and a standard USDC implementation is not expected to reenter, this ordering still weakens the contract's safety assumptions and creates unnecessary exposure if the configured token is non-standard, upgradeable, or behaves unexpectedly.

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");
​
//@> External call happens before state updates
require(usdc.transferFrom(msg.sender, address(this), lockAmount), "USDC transfer failed");
​
tokenIdCounter++;
collateralForMinting[tokenIdCounter] = lockAmount;
_safeMint(msg.sender, tokenIdCounter);
}
​
function buy(uint256 _listingId) external payable {
Listing memory listing = s_listings[_listingId];
if (!listing.isActive) revert ListingNotActive(_listingId);
require(listing.seller != msg.sender, "Seller cannot buy their own NFT");
​
activeListingsCounter--;
​
//@> External call happens before listing state is fully finalized
bool success = usdc.transferFrom(msg.sender, address(this), listing.price);
require(success, "USDC transfer failed");
_safeTransfer(listing.seller, msg.sender, listing.tokenId, "");
s_listings[_listingId].isActive = false;
}

Risk

Likelihood:

  • The issue becomes relevant whenever the configured token does not behave like a simple, non-reentrant ERC20 implementation.

  • The risk increases if the token address is changed to an upgradeable, hook-enabled, or otherwise non-standard token in a future deployment.

Impact:

  • Unexpected control flow during token transfer can observe or interact with partially updated protocol state.

  • The contract becomes harder to reason about and more fragile to integration changes, even if no immediate exploit exists with standard USDC.

Proof of Concept

The issue is visible directly from the execution order in mintNft() and buy(): both functions call usdc.transferFrom(...) before all related protocol state has been finalized. This breaks the checks-effects-interactions pattern and leaves the contract in a partially updated state during the external token call.

Recommended Mitigation

Refactor these flows to follow checks-effects-interactions more closely. Finalize all internal state before making external calls where possible, and consider using a reentrancy guard on functions that transfer tokens and NFTs in the same execution path.

function buy(uint256 _listingId) external nonReentrant payable {
Listing memory listing = s_listings[_listingId];
if (!listing.isActive) revert ListingNotActive(_listingId);
require(listing.seller != msg.sender, "Seller cannot buy their own NFT");
​
activeListingsCounter--;
s_listings[_listingId].isActive = false;
​
bool success = usdc.transferFrom(msg.sender, address(this), listing.price);
require(success, "USDC transfer failed");
_safeTransfer(listing.seller, msg.sender, listing.tokenId, "");
}

Support

FAQs

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

Give us feedback!