NFT Dealers

First Flight #58
Beginner FriendlyFoundry
100 EXP
Submission Details
Impact: medium
Likelihood: high

Incorrect fee handling due to self-transfer leads to broken accounting

Author Revealed upon completion

Root + Impact

Root cause

The function `collectUsdcFromSelling()` attempts to transfer the calculated fees using:

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

However, this is a self-transfer, meaning the contract is transferring tokens to itself. Since the contract already holds the funds received from the buyer (via transferFrom in the buy function), this operation does not change the token balance or isolate the fee amount in any way.

As a result, the contract incorrectly assumes that fees are being handled or separated, while in reality, they remain mixed with user funds inside the same balance.

This creates a mismatch between the accounting variable `totalFeesCollected` and the actual token distribution within the contract.

Impact

This issue leads to incorrect financial accounting within the contract.

Specifically:

- The variable `totalFeesCollected` increases as if fees are properly collected.

- However, no actual separation of fees from user funds occurs.

- All funds (user payments and fees) remain pooled together in the contract balance.

As a result:

- The owner may withdraw fees based on `totalFeesCollected`, which may not accurately reflect the true available balance.

- The protocol loses clear tracking of which funds belong to users versus fees.

- This can lead to accounting inconsistencies and potential mismanagement of funds.

While this does not immediately allow an attacker to steal funds, it breaks the correctness of the protocol’s financial logic, which is critical for a marketplace handling user assets.

Description

  • The function `collectUsdcFromSelling()` attempts to transfer fees to the contract using `usdc.safeTransfer(address(this), fees)`.

  • However, since the contract is already the holder of the tokens, this transfer has no effect and does not isolate or track the fees properly.

  • As a result, the contract incorrectly assumes fees are being handled, while they remain mixed with user funds.

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

Risk

Likelihood:

  • Reason 1 This logic executes on every sale and is not dependent on special conditions.

Impact:

  • Impact 1 - Fees are not properly separated from user funds

  • Impact 2 - Accounting using totalFeesCollected becomes misleading

  • Impact 3 - Owner may withdraw incorrect amounts

  • Impact 4 - Leads to incorrect financial state of the contract

Proof of Concept

Explanation:

Fees are not properly separated from user funds.

1. User sells NFT
2. Fees are calculated
3. Contract attempts to transfer fees to itself
4. No actual separation occurs
5. totalFeesCollected increases
6. Owner withdraws fees based on incorrect accounting

Recommended Mitigation

Explanation:

The self-transfer of fees should be removed, as it does not serve any functional purpose.

Since the contract already receives the full payment from the buyer, the fee portion is implicitly held within the contract balance. Therefore, explicitly transferring fees to the contract again is redundant.

Instead, the contract should rely on accurate accounting using `totalFeesCollected` and ensure that fee withdrawals are consistent with the actual contract balance.

If fee isolation is required, a more robust approach would be to:

- Maintain clear accounting variables

- Avoid redundant token transfers

- Ensure all fee-related logic is consistent with actual token balances.

- usdc.safeTransfer(address(this), fees);
Remove the redundant self-transfer. Fees are already held within the contract balance.
Only track fees via state variables and ensure withdrawals are consistent with actual balances.

Support

FAQs

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

Give us feedback!