Root + Impact
Description
Both mintNft() (L114) and buy() (L141) are marked payable, but the protocol exclusively uses USDC for all payments. Neither function references msg.value, and the contract has no receive() function, no fallback() function, and no ETH withdrawal mechanism of any kind.
The payable modifier allows these functions to accept ETH alongside the call. Since the functions only process USDC via usdc.transferFrom (L119, L148), any ETH attached to the transaction is silently accepted and permanently locked in the contract with no recovery path.
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);
}
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--;
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;
emit NFT_Dealers_Sold(msg.sender, listing.price);
}
The contract defines no ETH handling anywhere:
-
No receive() or fallback() function
-
No ETH withdrawal function
-
withdrawFees() (L195-201) only transfers USDC via usdc.safeTransfer
-
The owner has no mechanism to recover trapped ETH
Risk
Likelihood:
-
Both mintNft() (L114) and buy() (L141) accept ETH via the unnecessary payable modifier
-
A user who mistakenly sends ETH alongside a USDC marketplace transaction will permanently lose it
-
The contract has no receive(), fallback(), or ETH withdrawal function to recover trapped funds
Impact:
-
ETH sent to either function is permanently locked in the contract with no recovery mechanism
-
The owner cannot withdraw the trapped ETH -- withdrawFees() (L195-201) only handles USDC
-
While this requires user error (sending ETH to a USDC-based protocol), the unnecessary payable modifier creates an avoidable footgun that could be prevented at the compiler level
Proof of Concept
Two Foundry tests demonstrate that ETH sent via both functions is permanently trapped. Run with forge test --match-contract L02PoCTest -vv:
pragma solidity 0.8.34;
import {Test, console} from "forge-std/Test.sol";
import {NFTDealers} from "../src/NFTDealers.sol";
import {MockUSDC} from "../src/MockUSDC.sol";
contract L02PoCTest is Test {
NFTDealers public nftDealers;
MockUSDC public usdc;
address public owner = makeAddr("owner");
address public seller = makeAddr("seller");
address public buyer = makeAddr("buyer");
uint256 constant LOCK_AMOUNT = 20e6;
function setUp() public {
usdc = new MockUSDC();
nftDealers = new NFTDealers(owner, address(usdc), "NFTDealers", "NFTD", "ipfs://test/", LOCK_AMOUNT);
vm.startPrank(owner);
nftDealers.revealCollection();
nftDealers.whitelistWallet(seller);
vm.stopPrank();
usdc.mint(seller, 1000e6);
usdc.mint(buyer, 1000e6);
vm.prank(seller);
usdc.approve(address(nftDealers), type(uint256).max);
vm.prank(buyer);
usdc.approve(address(nftDealers), type(uint256).max);
}
function test_mintNftTrapsEth() public {
assertEq(address(nftDealers).balance, 0);
vm.deal(seller, 1 ether);
vm.prank(seller);
nftDealers.mintNft{value: 0.5 ether}();
assertEq(address(nftDealers).balance, 0.5 ether);
console.log("ETH trapped in contract after mintNft:", address(nftDealers).balance);
}
function test_buyTrapsEth() public {
vm.prank(seller);
nftDealers.mintNft();
vm.prank(seller);
nftDealers.list(1, uint32(100e6));
assertEq(address(nftDealers).balance, 0);
vm.deal(buyer, 2 ether);
vm.prank(buyer);
nftDealers.buy{value: 1 ether}(1);
assertEq(address(nftDealers).balance, 1 ether);
console.log("ETH trapped in contract after buy:", address(nftDealers).balance);
}
}
Recommended Mitigation
Remove the payable modifier from both functions. The protocol uses USDC exclusively -- without payable, Solidity rejects any transaction that includes ETH at the compiler level, preventing accidental loss.
-function mintNft() external payable onlyWhenRevealed onlyWhitelisted {
+function mintNft() external 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");
...
}
-function buy(uint256 _listingId) external payable {
+function buy(uint256 _listingId) external {
Listing memory listing = s_listings[_listingId];
if (!listing.isActive) revert ListingNotActive(_listingId);
require(listing.seller != msg.sender, "Seller cannot buy their own NFT");
...
}
This fix:
Prevents ETH from being accepted by either function -- Solidity automatically reverts transactions with msg.value > 0 on non-payable functions
Has no side effects -- neither function uses msg.value for any logic
Aligns the function signatures with the USDC-only payment model
