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

`usdc.safeTransfer(address(this), fees)` in `collectUsdcFromSelling()` is a no-op self-transfer that wastes gas and misleads about fee segregation

Author Revealed upon completion

Root + Impact

Description

In collectUsdcFromSelling() (L171-183), fees are ostensibly "collected" via usdc.safeTransfer(address(this), fees) at L181 -- a self-transfer where the contract sends USDC to itself. This has zero effect on the contract's USDC balance. The fee amount is only tracked virtually via totalFeesCollected += fees at L179.

The self-transfer is a no-op because the from address (the contract, via SafeERC20's safeTransfer which calls IERC20.transfer) and the to address (address(this)) are the same. The contract's balance does not change -- it was already holding the fees from the buyer's payment in buy() at L148.

function collectUsdcFromSelling(uint256 _listingId) external onlySeller(_listingId) { // @audit L171
Listing memory listing = s_listings[_listingId]; // @audit L172
require(!listing.isActive, "Listing must be inactive to collect USDC"); // @audit L173
​
uint256 fees = _calculateFees(listing.price); // @audit L175
uint256 amountToSeller = listing.price - fees; // @audit L176
uint256 collateralToReturn = collateralForMinting[listing.tokenId]; // @audit L177
​
totalFeesCollected += fees; // @audit L179: virtual counter only
amountToSeller += collateralToReturn; // @audit L180
usdc.safeTransfer(address(this), fees); // @audit L181: NO-OP self-transfer
usdc.safeTransfer(msg.sender, amountToSeller); // @audit L182: actual payout
}

The fees are implicitly retained because amountToSeller = listing.price - fees + collateral -- the fee portion is simply not sent to the seller. The self-transfer at L181 adds nothing: the contract already holds the fees. It wastes gas on a redundant ERC-20 transfer call (~7000+ gas for the storage read/write in the token contract) on every collection.

The withdrawFees() function (L195-201) later sends totalFeesCollected to the owner at L199 via usdc.safeTransfer(owner, amount). This works correctly because the fee amount is retained in the contract's balance after collectUsdcFromSelling sends only amountToSeller (which excludes fees). The self-transfer at L181 is entirely redundant to this mechanism.

function withdrawFees() external onlyOwner { // @audit L195
require(totalFeesCollected > 0, "No fees to withdraw"); // @audit L196
uint256 amount = totalFeesCollected; // @audit L197
totalFeesCollected = 0; // @audit L198
usdc.safeTransfer(owner, amount); // @audit L199: draws from shared pool
emit NFT_Dealers_Fees_Withdrawn(amount); // @audit L200
}

Note: Under normal operation (without the H-01 repeated-collection bug), the accounting is correct -- totalFeesCollected accurately reflects fees retained in the contract. The self-transfer itself does not cause fund loss or accounting errors. It is a gas-wasting no-op with misleading intent.

Risk

Likelihood:

  • Every call to collectUsdcFromSelling() executes the self-transfer at L181 -- this affects 100% of sale collections

  • The self-transfer usdc.safeTransfer(address(this), fees) always sends USDC from the contract to itself, producing zero net balance change

Impact:

  • Wasted gas: Each collection pays for a redundant ERC-20 transfer (~7000+ gas for token storage operations) that has no effect

  • Misleading code: The self-transfer suggests fees are being "moved" or "segregated" when they are not -- they remain in the same shared USDC balance alongside collateral and pending payouts

  • No fund loss: Under normal operation, the virtual accounting via totalFeesCollected is sufficient and correct. The self-transfer is redundant, not harmful

Proof of Concept

The following test proves the self-transfer has zero effect on the contract's USDC balance. Run with forge test --match-test test_feeSelfTransferIsNoOp -vv:

// SPDX-License-Identifier: UNLICENSED
pragma solidity 0.8.34;
​
import {Test, console} from "forge-std/Test.sol";
import {NFTDealers} from "../src/NFTDealers.sol";
import {MockUSDC} from "../src/MockUSDC.sol";
​
contract L04PoCTest is Test {
NFTDealers public nftDealers;
MockUSDC public usdc;
​
address public owner = makeAddr("owner");
address public seller = makeAddr("seller");
address public buyer = makeAddr("buyer");
​
uint256 constant LOCK_AMOUNT = 20e6; // 20 USDC
​
function setUp() public {
usdc = new MockUSDC();
nftDealers = new NFTDealers(owner, address(usdc), "NFTDealers", "NFTD", "ipfs://test/", LOCK_AMOUNT);
​
vm.startPrank(owner);
nftDealers.revealCollection();
nftDealers.whitelistWallet(seller);
vm.stopPrank();
​
usdc.mint(seller, 1000e6);
usdc.mint(buyer, 1000e6);
​
vm.prank(seller);
usdc.approve(address(nftDealers), type(uint256).max);
vm.prank(buyer);
usdc.approve(address(nftDealers), type(uint256).max);
}
​
function test_feeSelfTransferIsNoOp() public {
// Seller mints and lists
vm.prank(seller);
nftDealers.mintNft(); // tokenId 1, locks 20 USDC
vm.prank(seller);
nftDealers.list(1, uint32(100e6)); // list for 100 USDC
​
// Buyer purchases
vm.prank(buyer);
nftDealers.buy(1);
​
// Contract holds: 20 USDC (collateral) + 100 USDC (sale) = 120 USDC
uint256 balanceBefore = usdc.balanceOf(address(nftDealers));
assertEq(balanceBefore, 120e6);
​
// Seller collects
vm.prank(seller);
nftDealers.collectUsdcFromSelling(1);
​
uint256 balanceAfter = usdc.balanceOf(address(nftDealers));
uint256 fees = nftDealers.calculateFees(100e6); // 1% of 100 = 1 USDC
​
// Self-transfer at L181 had NO effect on balance
// Only the seller payout (price - fees + collateral = 119 USDC) left the contract
uint256 sellerPayout = 100e6 - fees + LOCK_AMOUNT; // 119 USDC
assertEq(balanceBefore - balanceAfter, sellerPayout);
​
// Contract retains exactly the fee amount (1 USDC)
assertEq(balanceAfter, fees);
​
// totalFeesCollected matches retained amount
assertEq(nftDealers.totalFeesCollected(), fees);
​
console.log("Balance before collect:", balanceBefore);
console.log("Balance after collect: ", balanceAfter);
console.log("Seller payout: ", sellerPayout);
console.log("Fee (retained): ", fees);
console.log("Self-transfer effect: 0 (no-op)");
}
}

Recommended Mitigation

Remove the self-transfer entirely. Fees are already retained in the contract because amountToSeller excludes the fee portion (listing.price - fees). The totalFeesCollected virtual counter is sufficient for withdrawFees() to track and withdraw the correct amount.

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);
}

This fix:

  1. Removes the redundant self-transfer, saving ~7000+ gas per collection

  2. Does not change the contract's USDC balance behavior -- fees are already retained by not including them in amountToSeller

  3. withdrawFees() (L195-201) continues to work correctly since totalFeesCollected tracks the virtual amount and the actual USDC is still in the contract

  4. No side effects on any other function

Support

FAQs

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

Give us feedback!