NFT Dealers

Root + Impact

Description

The collectUsdcFromSelling() function (L171-183) only checks that a listing is inactive (!listing.isActive at L173), but does not distinguish between a listing that was cancelled via cancelListing() and one that was sold via buy(). Both paths set s_listings[_listingId].isActive = false -- cancelListing() at L162 and buy() at L152 -- making them indistinguishable to collectUsdcFromSelling().

The root cause is that the Listing struct (L54-60) has no field to record whether a sale occurred. The isActive boolean is overloaded to mean both "listing was cancelled" and "listing was sold", but collectUsdcFromSelling() should only pay out in the sold case.

After a user calls cancelListing() (L157-169):

1. s_listings[_listingId].isActive is set to false at L162

2. collateralForMinting[listing.tokenId] is zeroed at L166 and the collateral is returned at L165

3. But s_listings[_listingId].seller and s_listings[_listingId].price are not cleared

The seller then calls collectUsdcFromSelling() (L171-183):

1. onlySeller modifier (L72-75): s_listings[_listingId].seller == msg.sender -- passes because seller was never cleared

2. require(!listing.isActive, ...) at L173 -- passes because cancelListing set it to false

3. _calculateFees(listing.price) at L175 uses the original listing price (never cleared)

4. amountToSeller = listing.price - fees at L176 -- the full sale payout is calculated

5. collateralToReturn = collateralForMinting[listing.tokenId] at L177 -- is 0 (already zeroed by cancel)

6. usdc.safeTransfer(address(this), fees) at L181 -- self-transfer, no-op on balance

7. usdc.safeTransfer(msg.sender, amountToSeller) at L182 -- transfers listing.price - fees USDC to the attacker

The attacker receives listing.price - fees USDC even though no buyer ever paid. These funds are drained from other users' collateral held in the contract.

This is distinct from H-01 (repeated collectUsdcFromSelling calls on a sold listing). H-01 requires a real buyer to purchase the NFT first. This vulnerability requires no buyer at all -- a single attacker can list, cancel, and collect to drain the contract unilaterally.

Risk

Likelihood:

• Any whitelisted user who has minted an NFT can exploit this (list() requires onlyWhitelisted at L127)

• The attack requires only 3 transactions: list(), cancelListing(), collectUsdcFromSelling() -- no buyer or external cooperation needed

• No timing constraints, front-running, or special conditions required

• The attack is profitable on every call: attacker receives listing.price - fees USDC from other users' collateral

Impact:

• Attacker steals listing.price - fees USDC from the contract per exploit cycle, funded by other users' locked collateral

• The attacker can set listing.price up to type(uint32).max (~4294 USDC) to maximize the stolen amount per cycle

• Combined with H-01 (no state reset in collectUsdcFromSelling), the same cancelled listing can be collected repeatedly, compounding the drain

• Victims permanently lose their collateral with no recovery mechanism -- the contract becomes insolvent

Proof of Concept

Attack steps:

1. Victims mint NFTs via mintNft(), each locking 20 USDC collateral into the contract

2. Attacker mints an NFT via mintNft(), locking 20 USDC collateral (tokenId 3)

3. Attacker lists the NFT for 10 USDC via list(3, 10e6)

4. Attacker calls cancelListing(3) -- receives 20 USDC collateral back, isActive set to false at L162, collateralForMinting[3] zeroed at L166

5. Attacker calls collectUsdcFromSelling(3) -- onlySeller passes (seller not cleared), !isActive passes (set by cancel), contract sends 10 - 0.1 = 9.9 USDC to attacker

6. Contract drops from 40 USDC to 30.1 USDC while still owing 40 USDC to victims -- insolvent

The following Foundry test demonstrates the full attack. Run with forge test --match-test test_collectAfterCancelStealsFromVictims -vv:

Recommended Mitigation

Delete the listing in cancelListing() after returning collateral. This zeros the seller field, causing the onlySeller modifier (L72-75) to revert with address(0) != msg.sender on any subsequent collectUsdcFromSelling() call. This mirrors the approach recommended for buy() in H-02.

The delete s_listings[_listingId] statement:

1. Zeros seller to address(0), causing onlySeller to revert on any collectUsdcFromSelling call

2. Zeros price to 0, so even if reached, the payout would be 0

3. Zeros isActive to false (equivalent to the removed explicit assignment)

4. Fully cleans up the listing, preventing any residual state from being exploited