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

M03. `buy()` and `mintNft()` are `payable` but have no ETH recovery, permanently locking any ETH sent

Author Revealed upon completion

Root + Impact

Description

  • buy() and mintNft() both accept ETH (payable) even though all payment flows use USDC exclusively. Neither function reads or refunds msg.value.

  • Any ETH sent alongside either call is accepted by the contract and becomes permanently inaccessible. There is no ETH withdrawal function anywhere in the contract.

// @> payable declared in both functions, but msg.value is never used or refunded
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);
// @> no refund of msg.value, no ETH withdrawal path anywhere in the contract
}

Risk

Likelihood: Medium

  • Users who interact with NFT marketplaces often send ETH by habit or mistake, particularly when using wallets or scripts that do not distinguish between ETH- and ERC20-denominated purchases.

  • Front-end interfaces that do not explicitly zero value on the transaction can silently pass non-zero ETH.

Impact: Medium

  • Every ETH sent to buy() is permanently lost from the sender's perspective; the contract provides no way to recover it.

  • The owner also cannot retrieve trapped ETH: withdrawFees() only moves USDC.

Proof of Concept

A buyer sends 1 ETH alongside a USDC purchase. The buy completes successfully, the NFT is transferred, but the 1 ETH is now permanently held by the contract with no withdrawal path.

function test_ethLockedInBuy() public {
uint32 nftPrice = 500e6;
​
vm.prank(seller);
nftDealers.list(1, nftPrice);
​
vm.startPrank(buyer);
usdc.approve(address(nftDealers), nftPrice);
vm.deal(buyer, 1 ether);
// Buyer accidentally sends 1 ETH alongside the USDC purchase
nftDealers.buy{value: 1 ether}(1);
vm.stopPrank();
​
// The ETH is now locked in the contract forever
assertEq(address(nftDealers).balance, 1 ether);
​
// No function exists to retrieve it:
// withdrawFees() only transfers USDC
// There is no receive(), fallback() ETH handler that routes funds out
}

Recommended Mitigation

Remove payable from both buy() and mintNft(). Since neither function uses ETH, there is no reason to accept it. Accidental ETH sends will then revert immediately rather than being silently trapped.

- function buy(uint256 _listingId) external payable {
+ function buy(uint256 _listingId) external {
Listing memory listing = s_listings[_listingId];
// ...
}
​
- function mintNft() external payable onlyWhenRevealed onlyWhitelisted {
+ function mintNft() external onlyWhenRevealed onlyWhitelisted {
// ...
}

Support

FAQs

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

Give us feedback!