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

collectUsdcFromSelling() missing state reset allows seller to drain the entire protocol by claiming sale proceeds and collateral multiple times

Author Revealed upon completion

Description

The expected behavior of collectUsdcFromSelling() is that a seller calls it once after their NFT is sold to receive their sale proceeds (minus fees) plus their minting collateral returned. After this single call the funds should be marked as disbursed.

What actually happens is that the function has no mechanism to prevent it from being called multiple times. The only guard is require(!listing.isActive), which is satisfied once a sale completes and never changes again. Critically, collateralForMinting[listing.tokenId] — the value re-sent on every call — is never zeroed inside this function. Every subsequent call sends the same listing.price - fees + collateral amount to the seller, draining funds that belong to other protocol users.

function collectUsdcFromSelling(uint256 _listingId) external onlySeller(_listingId) {
Listing memory listing = s_listings[_listingId];
require(!listing.isActive, "Listing must be inactive to collect USDC"); // @> only guard — satisfied permanently after one sale
​
uint256 fees = _calculateFees(listing.price);
uint256 amountToSeller = listing.price - fees;
uint256 collateralToReturn = collateralForMinting[listing.tokenId]; // @> read every call — never zeroed here
​
totalFeesCollected += fees; // @> inflated on every call
amountToSeller += collateralToReturn;
usdc.safeTransfer(address(this), fees); // @> no-op self-transfer — does nothing
usdc.safeTransfer(msg.sender, amountToSeller); // @> no state update follows — nothing prevents re-calling
}

Note the additional bug on the safeTransfer(address(this), fees) line: this transfers from the contract to itself, which is a no-op in standard ERC20. The fees are already in the contract after buy() — this line achieves nothing and only adds confusion about fee accounting.

Risk

Likelihood:

  • Occurs any time a seller realises the function can be called more than once — requires no special access or tooling, just multiple transactions

  • The attack is more profitable as more users have locked collateral in the contract, which is the normal operating state of the protocol

Impact:

  • Complete drain of all USDC held by the contract, up to the full contract balance

  • Other users' minting collateral is stolen and permanently unrecoverable — victims cannot call cancelListing() (they never listed) and cannot call collectUsdcFromSelling() (they are not the seller of the attacker's listing)

  • totalFeesCollected is over-inflated on every call, so withdrawFees() could also attempt to send more than the remaining balance to the owner

Proof of Concept

/**
* @notice H-01 PoC: Seller drains alice and bob's collateral by calling
* collectUsdcFromSelling() twice on the same listing.
*
* Steps:
* 1. alice and bob each mint an NFT, locking 20 USDC collateral in the contract
* 2. attacker mints and lists their NFT for 5 USDC
* 3. A buyer purchases the attacker's NFT
* 4. attacker calls collectUsdcFromSelling() — legitimate first claim (~25 USDC)
* 5. attacker calls collectUsdcFromSelling() again — collateralForMinting was
* never zeroed, so the call succeeds and pays out another ~25 USDC
* 6. attacker has stolen alice and bob's locked collateral (40 USDC total)
*/
function testCollectUsdcFromSellingDoubleClaim() public { ... }

Run with:

forge test --mt testCollectUsdcFromSellingDoubleClaim -vv

Expected console output:

--- Initial state ---
Contract USDC (3x collateral): 60000000
Contract USDC after sale (collateral + price): 65000000
Legitimate claim amount: 24950000
--- After claim 1 (legitimate) ---
Attacker USDC balance: 24950000
Contract USDC balance: 40050000
--- After claim 2 (THEFT - should have reverted) ---
Attacker USDC balance: 49900000
Contract USDC balance: 15100000
Alice collateral stolen: 20000000
Bob collateral stolen: 20000000

Recommended Mitigation

Zero collateralForMinting[listing.tokenId] before the transfer and remove the no-op self-transfer. This follows the Checks-Effects-Interactions pattern — state is cleared before any external call, preventing any re-entry or repeated call from finding the collateral still set.

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];
+ uint256 collateralToReturn = collateralForMinting[listing.tokenId];
+ collateralForMinting[listing.tokenId] = 0; // clear before transfer
​
totalFeesCollected += fees;
amountToSeller += collateralToReturn;
- usdc.safeTransfer(address(this), fees); // remove no-op self-transfer
usdc.safeTransfer(msg.sender, amountToSeller);
}

This ensures that on any second call, collateralToReturn will be 0 and amountToSeller will equal listing.price - fees — the sale proceeds that have already been transferred to the seller on the first call — causing the contract to revert due to insufficient balance, or at worst sending a second (incorrect) amount of just the sale price with no collateral. A cleaner guard is to add an explicit "already claimed" check:

+ mapping(uint256 => bool) public hasClaimed; // tokenId => claimed
​
function collectUsdcFromSelling(uint256 _listingId) external onlySeller(_listingId) {
Listing memory listing = s_listings[_listingId];
require(!listing.isActive, "Listing must be inactive to collect USDC");
+ require(!hasClaimed[listing.tokenId], "Already collected");
+ hasClaimed[listing.tokenId] = true;
...
}

Support

FAQs

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

Give us feedback!