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

NFTDealers::collectUsdcFromSelling attempts a self-transfer of USDC, leading to potential transaction reverts and accounting errors

Author Revealed upon completion

Description

Formatted Version:

The collectUsdcFromSelling function is responsible for distributing funds to the seller after a successful transaction. The function calculates the fees for the protocol and the amountToSeller (which includes the price minus fees plus the collateral).

The core issue is a logic error where the contract attempts to transfer USDC from itself to itself: usdc.safeTransfer(address(this), fees);

In the previous step (buy function), the buyer already sent the full price to the contract. Therefore, the fees are already sitting in the contract's balance. Attempting a self-transfer will cause the transaction to revert in most environments because the contract does not have an "allowance" to spend its own tokens. As a result, the seller's funds (amountToSeller) and their collateral remain permanently trapped in the contract.

@> usdc.safeTransfer(address(this), fees);

Risk

Likelihood:

  • The issue occurs every time a seller attempts to collect their funds after a sale.

  • Many USDC implementations or security wrappers revert on self-transfers or if the contract's "allowance" to itself isn't set (which it isn't).

Impact:

  • Sellers cannot claim their USDC or their collateral, effectively freezing user funds.

  • The protocol's fee accounting becomes unreliable.

Proof of Concept

Step-by-step scenario:

  1. Seller mints an NFT and lists it for 1000 USDC. The contract holds 20 USDC as collateral.

  2. Buyer calls buy(), transferring the full 1000 USDC to the NFTDealers contract.

  3. Seller calls collectUsdcFromSelling() to get their money.

  4. The contract calculates fees = 10 USDC (1% of 1000).

  5. The contract tries to execute usdc.safeTransfer(address(this), 10).

  6. The transaction reverts because the contract cannot transfer tokens to itself without prior approval or due to ERC20 implementation restrictions.

  7. The seller can never reach the next line of code, and their 1010 USDC is stuck forever.

Recommended Mitigation:\

Remove the redundant self-transfer. The protocol already holds the USDC; updating the state variable totalFeesCollected is sufficient for accounting.

## Recommended Mitigation
​
The redundant self-transfer should be removed entirely. The protocol correctly "collects" the fees by simply updating the `totalFeesCollected` state variable and keeping that portion of the USDC in the contract's balance while sending only the remainder to the seller.
​
```diff
function collectUsdcFromSelling(uint256 _listingId) external onlySeller(_listingId) {
...
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!