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

collectUsdcFromSelling has no re-call protection — seller drains entire contract USDC balance by calling repeatedly

Author Revealed upon completion

Root + Impact

Description

  • After a successful sale via buy(), the seller calls collectUsdcFromSelling() to receive their sale proceeds (price - fees) plus minting collateral.

  • The function never marks the listing as "collected", never zeroes collateralForMinting[tokenId], and never zeroes listing.price. Since the only checks are !listing.isActive (true after sale) and onlySeller (always true for original seller), the function can be called unlimited times, draining the entire USDC balance.

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 zeroed
uint256 collateralToReturn = collateralForMinting[listing.tokenId];
​
// @> totalFeesCollected accumulates each call
totalFeesCollected += fees;
amountToSeller += collateralToReturn;
usdc.safeTransfer(address(this), fees);
// @> No state update prevents re-calling — sends (price - fees + collateral) every time
usdc.safeTransfer(msg.sender, amountToSeller);
}
​

Risk

Likelihood:

  • Any whitelisted seller who has completed at least one sale can exploit this immediately — no special conditions required

  • The function has zero state mutation to prevent a second call — listing.price, listing.seller, collateralForMinting[tokenId] all remain unchanged between calls

Impact:

  • Complete loss of all USDC held by the contract — every user's minting collateral, unsettled sale proceeds, and accumulated protocol fees

  • A single malicious seller can drain funds belonging to all other users in a single block

Proof of Concept

Alice mints and lists at $500, Bob buys (contract holds 520 USDC). Alice calls collectUsdcFromSelling twice — the second call succeeds because no state was updated to prevent it, draining funds belonging to other users.


function test_repeatedCollectDrainsContract() public {
// Setup: owner whitelists alice and bob
vm.prank(owner);
dealers.whitelistWallet(alice);
vm.prank(owner);
dealers.whitelistWallet(bob);
​
// Alice mints tokenId 1 (deposits 20 USDC collateral)
vm.startPrank(alice);
usdc.approve(address(dealers), type(uint256).max);
dealers.mintNft();
dealers.list(1, uint32(500e6)); // list at $500
vm.stopPrank();
​
// Bob buys — contract now holds 520 USDC (20 collateral + 500 price)
vm.startPrank(bob);
usdc.approve(address(dealers), type(uint256).max);
dealers.buy(1);
vm.stopPrank();
​
uint256 contractBalBefore = usdc.balanceOf(address(dealers));
assertEq(contractBalBefore, 520e6);
​
// ATTACK: Alice calls collectUsdcFromSelling twice
vm.startPrank(alice);
dealers.collectUsdcFromSelling(1); // collects ~515 USDC (500 - 5 fee + 20 collateral)
dealers.collectUsdcFromSelling(1); // collects another ~515 USDC — steals other users' funds
vm.stopPrank();
​
// Contract drained far beyond the legitimate 515 USDC alice was owed
assertLt(usdc.balanceOf(address(dealers)), 5e6);
}

Recommended Mitigation

Replace bool isActive with a ListingStatus enum so the contract can distinguish Sold from Cancelled and track collection. Set status to Collected and zero collateral before the transfer (CEI pattern).

+ enum ListingStatus { Inactive, Active, Sold, Cancelled, Collected }
​
struct Listing {
address seller;
- uint32 price;
+ uint256 price;
address nft;
uint256 tokenId;
- bool isActive;
+ ListingStatus status;
}
​
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.status == ListingStatus.Sold, "Listing must be sold to collect");
​
uint256 fees = _calculateFees(listing.price);
uint256 amountToSeller = listing.price - fees;
uint256 collateralToReturn = collateralForMinting[listing.tokenId];
​
+ // Mark as collected BEFORE transfers (CEI pattern)
+ s_listings[_listingId].status = ListingStatus.Collected;
+ collateralForMinting[listing.tokenId] = 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!