Description
-
The marketplace is designed to operate entirely in USDC, not ETH: mintNft() locks lockAmount USDC via transferFrom, and buy() charges the buyer in USDC via transferFrom. There is no business logic in either function that uses msg.value, and the only owner withdrawal path, withdrawFees(), transfers USDC fees - not ETH.
-
The issue is that both mintNft() and buy() are marked payable, so users can accidentally attach ETH to these transactions and the calls will still succeed. Because the contract has no ETH accounting and no ETH withdrawal/recovery function, any ETH sent this way becomes stuck in the contract balance indefinitely.
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);
}
function withdrawFees() external onlyOwner {
require(totalFeesCollected > 0, "No fees to withdraw");
uint256 amount = totalFeesCollected;
totalFeesCollected = 0;
usdc.safeTransfer(owner, amount);
emit NFT_Dealers_Fees_Withdrawn(amount);
}
Risk
Likelihood: Low
-
This occurs whenever a user submits mintNft() or buy() through a wallet/UI/script that accidentally includes a nonzero msg.value, because both functions are explicitly marked payable and do not reject ETH. [contracts | Txt]
-
This occurs during ordinary usage because minting and buying are core flows, while the contract provides no indication on-chain that attached ETH is ignored and unrecoverable.
Impact: High
-
Users can permanently lose ETH by sending it to the contract through mintNft() or buy(), even though ETH is not part of the marketplace’s intended payment model.
-
The contract can accumulate stranded ETH that neither users nor the owner can recover through any existing function, creating stuck funds and avoidable operational confusion.
Proof of Concept
-
Add import {console2} from "forge-std/console2.sol"; at the top of NFTDealersTest.t.sol.
-
Copy the code below to NFTDealersTest contract.
-
Run command forge test --mt testPayableInMintAndBuyAllowsEthToBecomeStuck -vv --via-ir.
function testPayableInMintAndBuyAllowsEthToBecomeStuck() public revealed whitelisted {
uint256 tokenId = 1;
uint32 price = 1000e6;
uint256 accidentalMintEth = 1 ether;
uint256 accidentalBuyEth = 0.5 ether;
vm.deal(userWithCash, 10 ether);
vm.deal(userWithEvenMoreCash, 10 ether);
console2.log("Initial contract ETH balance:", address(nftDealers).balance);
console2.log("Initial seller ETH balance:", userWithCash.balance);
console2.log("Initial buyer ETH balance:", userWithEvenMoreCash.balance);
vm.startPrank(userWithCash);
usdc.approve(address(nftDealers), 20e6);
nftDealers.mintNft{value: accidentalMintEth}();
nftDealers.list(tokenId, price);
vm.stopPrank();
console2.log("Contract ETH after mintNft{value: ...}:", address(nftDealers).balance);
console2.log("Seller ETH after accidental mint payment:", userWithCash.balance);
console2.log("Owner of token after mint:", nftDealers.ownerOf(tokenId));
assertEq(address(nftDealers).balance, accidentalMintEth, "BUG: ETH sent to mintNft() became trapped");
assertEq(nftDealers.ownerOf(tokenId), userWithCash, "sanity: mint still succeeded");
vm.startPrank(userWithEvenMoreCash);
usdc.approve(address(nftDealers), price);
nftDealers.buy{value: accidentalBuyEth}(tokenId);
vm.stopPrank();
console2.log("Contract ETH after buy{value: ...}:", address(nftDealers).balance);
console2.log("Buyer ETH after accidental buy payment:", userWithEvenMoreCash.balance);
console2.log("Owner of token after buy:", nftDealers.ownerOf(tokenId));
assertEq(
address(nftDealers).balance,
accidentalMintEth + accidentalBuyEth,
"BUG: ETH sent to buy() also became trapped"
);
assertEq(nftDealers.ownerOf(tokenId), userWithEvenMoreCash, "sanity: buy still succeeded");
uint256 fee = nftDealers.calculateFees(price);
vm.prank(userWithCash);
nftDealers.collectUsdcFromSelling(tokenId);
console2.log("Contract ETH before owner withdrawFees():", address(nftDealers).balance);
console2.log("totalFeesCollected before withdraw:", nftDealers.totalFeesCollected());
console2.log("Expected USDC fee:", fee);
uint256 ownerEthBefore = owner.balance;
uint256 contractEthBeforeWithdraw = address(nftDealers).balance;
vm.prank(owner);
nftDealers.withdrawFees();
uint256 ownerEthAfter = owner.balance;
uint256 contractEthAfterWithdraw = address(nftDealers).balance;
console2.log("Owner ETH before withdrawFees():", ownerEthBefore);
console2.log("Owner ETH after withdrawFees():", ownerEthAfter);
console2.log("Contract ETH after withdrawFees():", contractEthAfterWithdraw);
assertEq(ownerEthAfter, ownerEthBefore, "owner did not recover any ETH");
assertEq(
contractEthAfterWithdraw,
contractEthBeforeWithdraw,
"BUG: ETH remains stuck even after the normal fee withdrawal flow"
);
assertEq(
contractEthAfterWithdraw,
accidentalMintEth + accidentalBuyEth,
"BUG: all accidentally sent ETH remains trapped in the contract"
);
}
Output:
[⠊] Compiling...
No files changed, compilation skipped
Ran 1 test for test/NFTDealersTest.t.sol:NFTDealersTest
[PASS] testPayableInMintAndBuyAllowsEthToBecomeStuck() (gas: 488059)
Logs:
Initial contract ETH balance: 0
Initial seller ETH balance: 10000000000000000000
Initial buyer ETH balance: 10000000000000000000
Contract ETH after mintNft{value: ...}: 1000000000000000000
Seller ETH after accidental mint payment: 9000000000000000000
Owner of token after mint: 0x22CdC71E987473D657FCe79C9C0C0B1A62148056
Contract ETH after buy{value: ...}: 1500000000000000000
Buyer ETH after accidental buy payment: 9500000000000000000
Owner of token after buy: 0x533575789af8F38A73C7747E36C17C1835FDF44a
Contract ETH before owner withdrawFees(): 1500000000000000000
totalFeesCollected before withdraw: 10000000
Expected USDC fee: 10000000
Owner ETH before withdrawFees(): 0
Owner ETH after withdrawFees(): 0
Contract ETH after withdrawFees(): 1500000000000000000
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 4.51ms (1.11ms CPU time)
Ran 1 test suite in 12.77ms (4.51ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)
Recommended Mitigation
-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");
tokenIdCounter++;
collateralForMinting[tokenIdCounter] = lockAmount;
_safeMint(msg.sender, tokenIdCounter);
}
-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");
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);
}
