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

mintNft() is marked payable but lacks msg.value validation, permanently locking any ETH sent with the call

Author Revealed upon completion

Description

The expected behavior is that mintNft() collects the minting collateral exclusively in USDC via transferFrom, and any ETH accidentally included in the transaction should be rejected. The function does not need to accept ETH at any point.

What actually happens is that mintNft() is declared payable, meaning the EVM will accept any native ETH sent alongside the call without complaint. The function body never reads msg.value, never returns it, and the contract has no withdrawETH() function or ETH-capable fallback. ETH sent to this function is permanently locked.

// @> payable keyword allows ETH to be sent — but there is no handling for it
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");
​
// @> only USDC is collected — msg.value is silently ignored
require(usdc.transferFrom(msg.sender, address(this), lockAmount), "USDC transfer failed");
​
tokenIdCounter++;
collateralForMinting[tokenIdCounter] = lockAmount;
_safeMint(msg.sender, tokenIdCounter);
// @> function ends with no reference to msg.value — ETH is now stuck
}

The payable modifier is entirely unnecessary here — the function's payment mechanism is USDC, not ETH.

Risk

Likelihood:

  • Any user who calls mintNft via a script, multisig, or wallet UI that populates a native value field by default is at risk

  • Etherscan's "Write Contract" interface exposes a payableAmount field for payable functions — users interacting directly may enter a value there thinking it is required

  • Probability per individual user is low, but the protocol has a fixed MAX_SUPPLY of 1,000 mints, so the cumulative exposure across all minters is non-trivial

Impact:

  • ETH sent is permanently lost — there is no recovery path for affected users

  • The owner cannot rescue it either, as there is no ETH withdrawal function

  • Loss scales linearly with the amount sent: a user who sends 1 ETH loses 1 ETH with no recourse

Proof of Concept

/**
* @notice M-02 PoC: Demonstrates that ETH sent alongside mintNft() is
* silently accepted and permanently locked in the contract.
*
* The user sends 0.5 ETH with their mint call. The mint succeeds,
* the USDC collateral is taken, the NFT is issued, and the 0.5 ETH
* disappears into the contract with no way to recover it.
*/
function testMintNftLocksEth() public { ... }

Run with:

forge test --mt testMintNftLocksEth -vv

Expected console output:

--- Before mint ---
User ETH balance: 1000000000000000000
Contract ETH balance: 0
--- After mint ---
User ETH balance (0.5 ETH lost): 500000000000000000
Contract ETH balance (permanently locked): 500000000000000000

Recommended Mitigation

Remove the payable modifier from mintNft(). This makes the function reject any transaction that includes ETH at the EVM level — no additional validation code is needed.

- function mintNft() external payable onlyWhenRevealed onlyWhitelisted {
+ function mintNft() external onlyWhenRevealed onlyWhitelisted {

Why this works: a non-payable function in Solidity will automatically revert with a built-in error if msg.value > 0 is detected. No ETH can enter the function, so no ETH can be trapped.

Alternative: if there is ever a legitimate reason to accept ETH (e.g. a future native-token payment option), add an explicit guard at the top instead:

function mintNft() external payable onlyWhenRevealed onlyWhitelisted {
+ require(msg.value == 0, "ETH not accepted");
...
}

The simpler and preferred fix is removing payable entirely.

Support

FAQs

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

Give us feedback!