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

Unsold listing can collect sale proceeds and drain shared USDC balance

Author Revealed upon completion

Description

Root + Impact

Normal behavior: sellers should only collect USDC proceeds after a real sale has occurred for their listing.

Issue: collectUsdcFromSelling allows collection whenever a listing is inactive. A listing becomes inactive after both buy() and cancelListing(), so canceled (unsold) listings can still withdraw listing.price - fees.

Description Explanation

The listing lifecycle currently conflates two different terminal states under a single boolean:

  • sold listing: payout should be allowed

  • canceled listing: payout should not be allowed

Because both states map to isActive = false, the claim function cannot differentiate a valid settlement from a canceled listing. This lets an attacker reuse canceled listing data (listing.price) to pull USDC from the contract's shared balance, even when no sale occurred for that listing.

// Root cause in the codebase with @> marks to highlight the relevant section
function collectUsdcFromSelling(uint256 _listingId) external onlySeller(_listingId) {
Listing memory listing = s_listings[_listingId];
// @> Inactive does not imply sold
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);
// @> Transfers payout even when listing was canceled and never sold
usdc.safeTransfer(msg.sender, amountToSeller);
}

Risk

Likelihood:

  • Listing cancellation is a standard user flow and deterministically sets isActive = false.

  • Any later USDC funded by honest users can be stolen by calling the claim path on a canceled listing.

Impact:

  • Unauthorized transfer of USDC from protocol-held pooled balance.

  • Honest participants can be indirectly robbed because balances are commingled.

Proof of Concept

  1. sellerA creates listing #1 and cancels it, which makes it inactive without a sale.

  2. sellerB creates listing #2; another user buys it, adding USDC to contract balance.

  3. sellerA calls collectUsdcFromSelling(1) on canceled listing #1.

  4. The function accepts because it only checks !isActive.

  5. sellerA receives payout for a listing that never sold, proving theft from pooled funds.

// test/NFTDealersFindingsPoC.t.sol
function testPoC_CancelThenCollectStealsFromContractBalance() public {
_mintAndList(sellerA, 1, PRICE_A);
_mintAndList(sellerB, 2, PRICE_B);
​
vm.prank(sellerA);
nftDealers.cancelListing(1); // unsold listing becomes inactive
​
vm.startPrank(nonWhitelistedBuyer);
usdc.approve(address(nftDealers), type(uint256).max);
nftDealers.buy(2); // funds contract
vm.stopPrank();
​
vm.prank(sellerA);
nftDealers.collectUsdcFromSelling(1); // steals from shared pool
}

Recommended Mitigation

The fix enforces explicit settlement state:

  • isSold ensures only actually sold listings can be claimed.

  • claimed ensures payout happens once.

With these guards, canceled listings are permanently ineligible for sale-proceeds withdrawal. This removes the cross-listing fund-drain vector and aligns payout authorization with real sale completion.

struct Listing {
address seller;
uint32 price;
address nft;
uint256 tokenId;
bool isActive;
+ bool isSold;
+ bool claimed;
}
function buy(uint256 _listingId) external payable {
- Listing memory listing = s_listings[_listingId];
+ Listing storage listing = s_listings[_listingId];
if (!listing.isActive) revert ListingNotActive(_listingId);
+ listing.isActive = false;
+ listing.isSold = true;
...
- s_listings[_listingId].isActive = false;
}
function collectUsdcFromSelling(uint256 _listingId) external onlySeller(_listingId) {
- Listing memory listing = s_listings[_listingId];
- require(!listing.isActive, "Listing must be inactive to collect USDC");
+ Listing storage listing = s_listings[_listingId];
+ require(listing.isSold, "Listing not sold");
+ require(!listing.claimed, "Already claimed");
+ listing.claimed = true;
...
}

Support

FAQs

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

Give us feedback!