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

payable Modifier in the `mintNft` func is unused which can cause eth to be accidentally stuck in the contract

Author Revealed upon completion

payable Modifier in the mintNft func is unused which can cause eth to be accidentally stuck in the contract

Description

  • The mintNft() function allows whitelisted users to mint an NFT by transferring a fixed amount of USDC (lockAmount) to the contract as collateral. The function should only accept the required USDC payment for minting and should not accept or retain any ETH, since ETH is not used in the minting process.

  • The function is marked payable, which allows users to send ETH when calling it, but the contract does not use or refund any ETH sent. As a result, if a user accidentally sends ETH along with the mint transaction, the ETH will be accepted and permanently locked in the contract because there is no mechanism to reject or withdraw it.

// Root cause in the codebase with @> marks to highlight the relevant section
@> 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:

  • This occurs when a user calls mintNft() through a wallet or script that automatically includes a non-zero msg.value, since the function is marked payable and does not reject ETH.

  • Users interacting with smart contracts through custom scripts, frontends, or copied transaction parameters may unintentionally include ETH in the transaction, causing the contract to accept and lock the funds without any refund mechanism.

Impact:

  • Any ETH sent to mintNft() is permanently trapped in the contract, leading to accidental financial loss for users.

  • Users may lose confidence in the protocol if they inadvertently lose ETH while interacting with the mint function, which can harm adoption and reputation.

  • If the contract includes a withdrawal function, the owner could access the trapped ETH, turning a UX issue into a financial risk.

Proof of Concept

In the NFTDealersTest.t.sol file, add this test func:

function test_sendEthAccidentallyDuringMint() public revealed whitelisted {
vm.deal(userWithCash, 2 ether);
vm.startBroadcast(userWithCash);
usdc.approve(address(nftDealers), 20e6);
nftDealers.mintNft{value: 1 ether}();
vm.stopBroadcast();
​
assertEq(address(nftDealers).balance, 1 ether);
assertEq(userWithCash.balance, 1 ether);
}

Then run this: forge test --mt test_sendEthAccidentallyDuringMint -vvv

output:

```
[⠊] Compiling...
[⠊] Compiling 1 files with Solc 0.8.34
[⠒] Solc 0.8.34 finished in 1.89s
Compiler run successful!

Ran 1 test for test/NFTDealersTest.t.sol:NFTDealersTest
[PASS] test_sendEthAccidentallyDuringMint() (gas: 203661)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 33.40ms (7.32ms CPU time)

Ran 1 test suite in 256.42ms (33.40ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

```

Recommended Mitigation

Remove the payable modifier:

+ function mintNft() external onlyWhenRevealed onlyWhitelisted {
...
+ }
​

Support

FAQs

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

Give us feedback!