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

mintNft() uses transferFrom instead of safeTransferFrom, inconsistent with the rest of the contract

Author Revealed upon completion

Root + Impact

Description

  • The contract imports and declares using SafeERC20 for IERC20 to safely handle ERC20 transfers, including tokens that do not return a boolean value on transfer/transferFrom. Every USDC transfer in the contract — in collectUsdcFromSelling(), cancelListing(), and withdrawFees() — correctly uses safeTransfer or safeTransferFrom from this library.

  • mintNft() is the sole exception: it calls usdc.transferFrom() directly and checks the return value manually via require. This bypasses SafeERC20's protective wrapper entirely. For a non-standard ERC20 token that does not return a bool, transferFrom would revert due to ABI decoding failure, or in edge cases silently succeed without an actual transfer — neither of which is handled correctly by the raw call.

using SafeERC20 for IERC20; // declared at contract level
​
function mintNft() external payable onlyWhenRevealed onlyWhitelisted {
...
@> require(usdc.transferFrom(msg.sender, address(this), lockAmount), "USDC transfer failed");
// ^^^ raw transferFrom — does not use SafeERC20
...
}
​
// every other transfer in the contract uses SafeERC20 correctly:
// collectUsdcFromSelling() → usdc.safeTransfer(owner, fees)
// collectUsdcFromSelling() → usdc.safeTransfer(msg.sender, amountToSeller)
// cancelListing() → usdc.safeTransfer(listing.seller, ...)
// withdrawFees() → usdc.safeTransfer(owner, amount)

Risk

Likelihood:

  • USDC on Ethereum mainnet correctly returns a bool, so this does not trigger in the primary deployment scenario.

  • The protocol states compatibility with "Ethereum/Any EVM" — on some EVM chains, the token at the USDC address may behave differently, or the protocol may be deployed with a different ERC20 as the payment token, where the non-standard behavior applies.

Impact:

  • On chains or configurations where the payment token does not return a bool, mintNft() fails while all other transfer functions continue to work correctly — breaking minting in isolation.

  • A successful transferFrom that does not return a value would cause the require check to revert due to ABI decoding, meaning collateral is never collected and the NFT is never minted, even though the transfer may have succeeded on-chain.

  • The inconsistency creates a maintenance risk: future developers may follow the pattern in mintNft() and introduce more unsafe transfer calls elsewhere in the contract.

Proof of Concept

The contract declares using SafeERC20 for IERC20 but mintNft() does not use it. Comparing the two patterns side by side:

// mintNft() — raw call, manually checks bool return value
require(usdc.transferFrom(msg.sender, address(this), lockAmount), "USDC transfer failed");
​
// SafeERC20 wrapper — handles missing return value, reverts on failure automatically
usdc.safeTransferFrom(msg.sender, address(this), lockAmount);

SafeERC20.safeTransferFrom uses a low-level call and checks both the return data length and value, correctly handling tokens that return nothing. The raw transferFrom call in mintNft() does not — if the token returns no data, the ABI decoder reverts when trying to decode the expected bool return value.

Recommended Mitigation

Replace the raw transferFrom call with safeTransferFrom to match every other transfer in the contract and correctly handle non-standard ERC20 tokens.

- require(usdc.transferFrom(msg.sender, address(this), lockAmount), "USDC transfer failed");
+ usdc.safeTransferFrom(msg.sender, address(this), lockAmount);

Support

FAQs

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

Give us feedback!