NFT Dealers
First Flight #58
Beginner FriendlyFoundry
100 EXP
View results
Previous Next
Submission Details
Severity: high
Valid

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

altshar

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);
}
Updates

Lead Judging Commences

rubik0n Lead Judge 16 days ago
Submission Judgement Published
Validated
Assigned finding tags:

drain-protocol-risk

collateral is not reset to zero after collecting USDC from sold NFT. No accounting for collected USDC

Support

FAQs

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

Give us feedback!