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

buy() and mintNft() Are Payable but Lock ETH Permanently

Author Revealed upon completion

buy() and mintNft() Are Payable but Lock ETH Permanently

Description

buy() and mintNft() are USDC-denominated functions but both are marked payble. The protocol silently accepts any ETH sent alongside the call, but ETH is permanently locked with no recovery path.

// @> payable but msg.value is never read or returned
function mintNft() external payable onlyWhenRevealed onlyWhitelisted {
...
// ETH sent here is accepted and trapped
}
​
// @> same issue
function buy(uint256 _listingId) external payable {
...
// ETH sent here is accepted and trapped
}

Risk

Likelihood: High

Scripts, aggregators, and front-ends that build multicall bundles often forward the full msg.value to every sub-call, so a user transacting through such an interface will silently lose ETH without any error.

Impact: High

Any ETH sent to either functions is irrecoverable as there is no withdrawal path. Also, the calls succeed, so neither the sender nor monitoring tooling will detect the fund loss from the transaction receipt.

Proof of Concept

function test_LockedETH() public {
// Initialize test
vm.startBroadcast(owner);
nftDealers.whitelistWallet(seller);
nftDealers.revealCollection();
vm.stopBroadcast();
​
// Assert state after initialization
assertEq(nftDealers.whitelistedUsers(seller), true);
assertEq(nftDealers.isCollectionRevealed(), true);
​
// Mint
vm.startBroadcast(seller);
usdc.approve(address(nftDealers), USDC_COLLATERAL);
nftDealers.mintNft();
vm.stopBroadcast();
​
// Assert state after mint
uint256 tokenId = 1;
assertEq(nftDealers.ownerOf(tokenId), seller);
assertEq(nftDealers.collateralForMinting(tokenId), USDC_COLLATERAL);
assertEq(usdc.balanceOf(address(nftDealers)), INITIAL_USER_BALANCE + USDC_COLLATERAL);
assertEq(usdc.balanceOf(seller), INITIAL_USER_BALANCE - USDC_COLLATERAL);
​
// List NFT
uint32 sellingPrice = 40e6;
vm.prank(seller);
nftDealers.list(tokenId, sellingPrice);
​
// Assert state after list
(address _seller, uint32 _price, address _nft, uint256 _tokenId, bool _isActive) =
nftDealers.s_listings(tokenId);
assertEq(_seller, seller);
assertEq(_price, sellingPrice);
assertEq(_nft, address(nftDealers));
assertEq(_tokenId, tokenId);
assertEq(_isActive, true);
assertEq(nftDealers.ownerOf(tokenId), seller);
​
// Buyer approving amount
vm.prank(buyer);
usdc.approve(address(nftDealers), sellingPrice);
​
// Assert state before call
assertEq(buyer.balance, 10 ether);
​
// Buy the NFT listing and forward ETH
vm.prank(buyer);
nftDealers.buy{value: 5 ether}(tokenId);
​
// Assert state after trade succeeds
assertEq(nftDealers.ownerOf(tokenId), buyer);
assertEq(buyer.balance, 5 ether);
assertEq(address(nftDealers).balance, 5 ether);
}

Recommended Mitigation

Remove payable from both functions. Neither accepts ETH; the keyword serves no purpose and actively creates a fund-loss vector.

- function mintNft() external payable onlyWhenRevealed onlyWhitelisted {
+ function mintNft() external onlyWhenRevealed onlyWhitelisted {
​
- function buy(uint256 _listingId) external payable {
+ function buy(uint256 _listingId) external {

Support

FAQs

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

Give us feedback!