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

Silent fund sink - Payable Functions Accept ETH Without Withdrawal Mechanism Leading to Permanent Fund Lock

Author Revealed upon completion

Silent fund sink

Description

  • Functions mintNft() and buy() accept msg.value but do not use or refund ETH.

  • The contract lacks any receive(), fallback(), or explicit ETH withdrawal function, making ETH irrecoverable once sent.

function mintNft() external payable onlyWhenRevealed onlyWhitelisted {
// @> Function is payable but does not use msg.value anywhere
​
require(
usdc.transferFrom(msg.sender, address(this), lockAmount),
"USDC transfer failed"
);
​
tokenIdCounter++;
​
collateralForMinting[tokenIdCounter] = lockAmount;
​
_safeMint(msg.sender, tokenIdCounter);
​
// @> ETH sent alongside this call is silently accepted and stuck in contract
}
​
function buy(uint256 _listingId) external payable {
// @> Function is payable but ETH is not used in purchase logic
​
Listing memory listing = s_listings[_listingId];
​
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;
​
// @> Any ETH sent remains in contract balance with no withdrawal path
}

Risk

  • Impact: Medium

    • Users can permanently lose ETH if mistakenly sent.

    • No recovery path exists, even for the contract owner.

  • Likelihood: Medium

    • Wallets (e.g., MetaMask) allow users to attach ETH to any transaction.

    • Malicious or buggy frontends can trick users into sending ETH.

    • This is a realistic and common user error vector.

Proof of Concept

The following Forge test demonstrates that both mintNft() and buy() accept ETH due to being marked as payable, while the protocol logic remains entirely USDC-based. The ETH sent is neither used nor refunded and becomes permanently locked in the contract.

Key points demonstrated:

  • ETH can be sent alongside function calls without reverting.

  • Core functionality (minting and buying NFTs) executes normally.

  • Contract balance increases by msg.value.

  • No mechanism exists to withdraw or recover ETH.

path: test/NFTDealers.PayableLock.t.sol

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.34;
​
import "forge-std/Test.sol";
import "../src/NFTDealers.sol";
import "../src/MockUSDC.sol";
​
contract PayableETHLockPoC is Test {
NFTDealers internal nft;
MockUSDC internal usdc;
​
address internal owner = address(0xA11CE);
address internal user = address(0xB0B);
​
uint256 internal constant LOCK_AMOUNT = 20e6; // 20 USDC
​
function setUp() public {
vm.deal(owner, 10 ether);
vm.deal(user, 10 ether);
​
usdc = new MockUSDC();
​
nft = new NFTDealers(
owner,
address(usdc),
"NFT Dealers",
"DEAL",
"ipfs://collection-image",
LOCK_AMOUNT
);
​
// @> Setup required state
vm.startPrank(owner);
nft.revealCollection();
nft.whitelistWallet(user);
vm.stopPrank();
​
// @> Fund user with USDC and approve contract
usdc.mint(user, 1000e6);
​
vm.prank(user);
usdc.approve(address(nft), type(uint256).max);
}
​
function test_ETH_is_accepted_and_permanently_locked_in_mint() public {
uint256 ethAmount = 1 ether;
​
// @> Ensure contract starts with 0 ETH
assertEq(address(nft).balance, 0);
​
// @> User calls mintNft sending ETH (unexpected but allowed)
vm.prank(user);
nft.mintNft{value: ethAmount}();
​
// @> ETH is accepted because function is payable
assertEq(address(nft).balance, ethAmount);
​
// @> NFT mint still succeeds → silent failure pattern
assertEq(nft.ownerOf(1), user);
​
// @> No function exists to withdraw ETH
// @> Even owner cannot recover funds
vm.prank(owner);
vm.expectRevert("No fees to withdraw");
nft.withdrawFees();
​
// @> ETH remains permanently locked
assertEq(address(nft).balance, ethAmount);
}
​
function test_ETH_is_accepted_and_locked_in_buy() public {
uint256 ethAmount = 0.5 ether;
​
// @> Mint NFT first
vm.prank(user);
nft.mintNft();
​
// @> List NFT
vm.prank(user);
nft.list(1, 100e6);
​
// @> Another user buys NFT sending ETH
address buyer = address(0xCAFE);
vm.deal(buyer, 10 ether);
​
usdc.mint(buyer, 1000e6);
vm.prank(buyer);
usdc.approve(address(nft), type(uint256).max);
​
uint256 beforeBalance = address(nft).balance;
​
vm.prank(buyer);
nft.buy{value: ethAmount}(1);
​
// @> ETH is accepted and trapped
assertEq(
address(nft).balance,
beforeBalance + ethAmount
);
​
// @> Ownership transfer still works
assertEq(nft.ownerOf(1), buyer);
​
// @> No withdrawal path exists → ETH permanently locked
}
}

Results

forge test --match-contract PayableETHLockPoC -vvvv
[⠊] Compiling...
No files changed, compilation skipped
​
Ran 2 tests for test/NFTDealers.PayableLock.t.sol:PayableETHLockPoC
[PASS] test_ETH_is_accepted_and_locked_in_buy() (gas: 313220)
Traces:
[353020] PayableETHLockPoC::test_ETH_is_accepted_and_locked_in_buy()
├─ [0] VM::prank(0x0000000000000000000000000000000000000B0b)
│ └─ ← [Return]
├─ [136077] NFTDealers::mintNft()
│ ├─ [33161] MockUSDC::transferFrom(0x0000000000000000000000000000000000000B0b, NFTDealers: [0x2e234DAe75C793f67A35089C9d99245E1C58470b], 20000000 [2e7])
│ │ ├─ emit Transfer(from: 0x0000000000000000000000000000000000000B0b, to: NFTDealers: [0x2e234DAe75C793f67A35089C9d99245E1C58470b], value: 20000000 [2e7])
│ │ └─ ← [Return] true
│ ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: 0x0000000000000000000000000000000000000B0b, tokenId: 1)
│ └─ ← [Stop]
├─ [0] VM::prank(0x0000000000000000000000000000000000000B0b)
│ └─ ← [Return]
├─ [93886] NFTDealers::list(1, 100000000 [1e8])
│ ├─ emit NFT_Dealers_Listed(listedBy: 0x0000000000000000000000000000000000000B0b, listingId: 1)
│ └─ ← [Stop]
├─ [0] VM::deal(0x000000000000000000000000000000000000cafE, 10000000000000000000 [1e19])
│ └─ ← [Return]
├─ [30191] MockUSDC::mint(0x000000000000000000000000000000000000cafE, 1000000000 [1e9])
│ ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: 0x000000000000000000000000000000000000cafE, value: 1000000000 [1e9])
│ └─ ← [Stop]
├─ [0] VM::prank(0x000000000000000000000000000000000000cafE)
│ └─ ← [Return]
├─ [25296] MockUSDC::approve(NFTDealers: [0x2e234DAe75C793f67A35089C9d99245E1C58470b], 115792089237316195423570985008687907853269984665640564039457584007913129639935 [1.157e77])
│ ├─ emit Approval(owner: 0x000000000000000000000000000000000000cafE, spender: NFTDealers: [0x2e234DAe75C793f67A35089C9d99245E1C58470b], value: 115792089237316195423570985008687907853269984665640564039457584007913129639935 [1.157e77])
│ └─ ← [Return] true
├─ [0] VM::prank(0x000000000000000000000000000000000000cafE)
│ └─ ← [Return]
├─ [37961] NFTDealers::buy{value: 500000000000000000}(1)
│ ├─ [4461] MockUSDC::transferFrom(0x000000000000000000000000000000000000cafE, NFTDealers: [0x2e234DAe75C793f67A35089C9d99245E1C58470b], 100000000 [1e8])
│ │ ├─ emit Transfer(from: 0x000000000000000000000000000000000000cafE, to: NFTDealers: [0x2e234DAe75C793f67A35089C9d99245E1C58470b], value: 100000000 [1e8])
│ │ └─ ← [Return] true
│ ├─ emit Transfer(from: 0x0000000000000000000000000000000000000B0b, to: 0x000000000000000000000000000000000000cafE, tokenId: 1)
│ ├─ emit NFT_Dealers_Sold(soldTo: 0x000000000000000000000000000000000000cafE, price: 100000000 [1e8])
│ └─ ← [Stop]
├─ [0] VM::assertEq(500000000000000000 [5e17], 500000000000000000 [5e17]) [staticcall]
│ └─ ← [Return]
├─ [1049] NFTDealers::ownerOf(1) [staticcall]
│ └─ ← [Return] 0x000000000000000000000000000000000000cafE
├─ [0] VM::assertEq(0x000000000000000000000000000000000000cafE, 0x000000000000000000000000000000000000cafE) [staticcall]
│ └─ ← [Return]
└─ ← [Stop]
​
[PASS] test_ETH_is_accepted_and_permanently_locked_in_mint() (gas: 165369)
Traces:
[165369] PayableETHLockPoC::test_ETH_is_accepted_and_permanently_locked_in_mint()
├─ [0] VM::assertEq(0, 0) [staticcall]
│ └─ ← [Return]
├─ [0] VM::prank(0x0000000000000000000000000000000000000B0b)
│ └─ ← [Return]
├─ [136077] NFTDealers::mintNft{value: 1000000000000000000}()
│ ├─ [33161] MockUSDC::transferFrom(0x0000000000000000000000000000000000000B0b, NFTDealers: [0x2e234DAe75C793f67A35089C9d99245E1C58470b], 20000000 [2e7])
│ │ ├─ emit Transfer(from: 0x0000000000000000000000000000000000000B0b, to: NFTDealers: [0x2e234DAe75C793f67A35089C9d99245E1C58470b], value: 20000000 [2e7])
│ │ └─ ← [Return] true
│ ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: 0x0000000000000000000000000000000000000B0b, tokenId: 1)
│ └─ ← [Stop]
├─ [0] VM::assertEq(1000000000000000000 [1e18], 1000000000000000000 [1e18]) [staticcall]
│ └─ ← [Return]
├─ [1049] NFTDealers::ownerOf(1) [staticcall]
│ └─ ← [Return] 0x0000000000000000000000000000000000000B0b
├─ [0] VM::assertEq(0x0000000000000000000000000000000000000B0b, 0x0000000000000000000000000000000000000B0b) [staticcall]
│ └─ ← [Return]
├─ [0] VM::prank(0x00000000000000000000000000000000000A11cE)
│ └─ ← [Return]
├─ [0] VM::expectRevert(custom error 0xf28dceb3: No fees to withdraw)
│ └─ ← [Return]
├─ [2808] NFTDealers::withdrawFees()
│ └─ ← [Revert] No fees to withdraw
├─ [0] VM::assertEq(1000000000000000000 [1e18], 1000000000000000000 [1e18]) [staticcall]
│ └─ ← [Return]
└─ ← [Stop]
​
Suite result: ok. 2 passed; 0 failed; 0 skipped; finished in 1.26ms (675.94µs CPU time)

Recommended Mitigation

  • Removing payable ensures ETH cannot be accidentally sent

  • Non-payable functions automatically revert if ETH is sent

-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!