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

L05. `mintNft()` uses raw `require(usdc.transferFrom(...))` instead of `safeTransferFrom`, inconsistent with the rest of the contract

Author Revealed upon completion

Root + Impact

Description

  • The contract declares using SafeERC20 for IERC20 and uses usdc.safeTransfer in every other function that moves USDC.

  • mintNft() calls usdc.transferFrom directly with a require on the return value instead of using usdc.safeTransferFrom. Some ERC20 tokens do not return a boolean on transferFrom (non-compliant tokens), which causes the require to behave unexpectedly. In those cases the minting collateral transfer silently fails or reverts with an ABI-decode error rather than the intended error message.

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");
​
// @> raw transferFrom with require — inconsistent with SafeERC20 used elsewhere
require(usdc.transferFrom(msg.sender, address(this), lockAmount), "USDC transfer failed");
​
tokenIdCounter++;
collateralForMinting[tokenIdCounter] = lockAmount;
_safeMint(msg.sender, tokenIdCounter);
}

Risk

Likelihood: Low

  • Standard USDC always returns true on a successful transfer, so this causes no practical issue with the intended deployment token.

  • The risk increases only when a non-standard ERC20 (returning void instead of bool) is used as the payment token.

Impact: Low

  • For standard USDC the behavior is identical to safeTransferFrom.

  • For a token that returns no data on transferFrom (void return), Solidity 0.8 will attempt to ABI-decode the empty returndata as bool and revert with a decoding error — the revert message will be a generic ABI error rather than the intended "USDC transfer failed" string, making the failure harder to diagnose.

  • Code inconsistency also makes auditing and reasoning about the contract harder.

Proof of Concept

The inconsistency is visible by comparing mintNft() with every other USDC transfer in the contract:

// In mintNft() — raw call with require:
require(usdc.transferFrom(msg.sender, address(this), lockAmount), "USDC transfer failed");
​
// In cancelListing(), collectUsdcFromSelling(), withdrawFees() — SafeERC20:
usdc.safeTransfer(listing.seller, collateralForMinting[listing.tokenId]);
usdc.safeTransfer(msg.sender, amountToSeller);
usdc.safeTransfer(owner, amount);

For a token that returns void on transferFrom, Solidity 0.8 tries to ABI-decode the empty returndata as bool. This triggers a decoding revert rather than the intended "USDC transfer failed" message, obscuring the failure cause. SafeERC20.safeTransferFrom explicitly handles this case by checking returndata.length == 0 || abi.decode(returndata, (bool)), correctly accepting both void-returning and bool-returning tokens.

Recommended Mitigation

Replace the raw require(usdc.transferFrom(...)) in mintNft() with usdc.safeTransferFrom(), consistent with the pattern used throughout the rest of the contract.

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");
+ usdc.safeTransferFrom(msg.sender, address(this), lockAmount);
​
tokenIdCounter++;
collateralForMinting[tokenIdCounter] = lockAmount;
_safeMint(msg.sender, tokenIdCounter);
}

Support

FAQs

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

Give us feedback!