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

Wrong Transfer Logic

Author Revealed upon completion

The contract has a logic flaw in transfering USDC

Description

In the function collectUsdcFromSelling, the contract attempts to transfer USDC from the contract back to itself.

Since the USDC was already pulled into the contract during the buy() function (via transferFrom), the tokens are already sitting in the contract's balance. Executing a transfer where the sender and recipient are the same address changes nothing regarding the balance but still triggers the full execution logic of the USDC ERC20 contract.

// Root cause in the codebase with @> marks to highlight the relevant section
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;
amountToSeller += collateralToReturn;
@> usdc.safeTransfer(address(this), fees);
usdc.safeTransfer(msg.sender, amountToSeller);
}

Risk

There is unnnecessary loss of gas

Likelihood

  • Basically everytime,

  • The Flow of Funds:

  • Buyer calls buy(): usdc.transferFrom(Buyer, NFTDealers, price) is executed. The contract now holds the full price

  • ** Seller calls collectUsdcFromSelling():**

    • The contract calculates fees

    • The Issue: It calls usdc.safeTransfer(address(this), fees).

    • The Result: The USDC contract subtracts 5 from NFTDealers and adds 5 to NFTDealers. The balance remains 100.

    • The Waste: This operation costs approximately ** gas** (standard ERC20 for a zero-sum result.

Impact

  • Financial Waste: Every seller who collects their funds pays an unnecessary gas premium. On a high-traffic marketplace, this totals significant capital wasted on "do-nothing" operations.

  • Contract Complexity: It adds unnecessary external calls, which slightly increases the risk of reentrancy or unexpected reverts if the underlying token (USDC) has non-standard behavior for self-transfers.

Proof of Concept

* The Flow of Funds:
​
* **Buyer calls `buy()`:** `usdc.transferFrom(Buyer, NFTDealers, price)` is executed. The contract now holds the full `price`
​
* ** Seller calls `collectUsdcFromSelling()`:**
* The contract calculates `fees`
* **The Issue:** It calls `usdc.safeTransfer(address(this), fees)`.
* **The Result:** The USDC contract subtracts 5 from `NFTDealers` and adds 5 to `NFTDealers`. The balance remains 100.
* **The Waste:** This operation costs approximately ** gas** (standard ERC20 for a zero-sum result.
​
<br />
​

Recommended Mitigation

Just remove the reduntant line 'usdc.safeTransfer(address(this), fees);'


Support

FAQs

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

Give us feedback!