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

collectUsdcFromSelling is a No-Op , Fees Are Not Segregated

Author Revealed upon completion

Root + Impact

collectUsdcFromSelling is a No-Op — Fees Are Not Segregated

Description

  • collectUsdcFromSelling is intended to separate the fee portion from the seller's proceeds and hold fees within the contract for later withdrawal by the owner.

  • The function calls usdc.safeTransfer(address(this), fees), which transfers USDC from the contract to itself ,a no-op that wastes gas. Fees are tracked only via the totalFeesCollected counter with no actual token segregation, meaning withdrawFees() depends entirely on the contract having sufficient USDC balance.

function collectUsdcFromSelling(uint256 _listingId) external onlySeller(_listingId) {
// ...
uint256 fees = _calculateFees(listing.price);
uint256 amountToSeller = listing.price - fees;
totalFeesCollected += fees;
amountToSeller += collateralToReturn;
​
@> usdc.safeTransfer(address(this), fees); // self-transfer: contract -> contract = no-op
usdc.safeTransfer(msg.sender, amountToSeller);
}

Risk

Likelihood:

  • Every call to collectUsdcFromSelling executes the self-transfer — this is not conditional and occurs on 100% of collection calls

Impact:

  • Gas waste on every collection (unnecessary ERC20 transfer + event emission)

totalFeesCollected can inflate beyond actual available balance (especially combined with NM-001/NM-002), causing withdrawFees() to revert

  • Creates a false impression of fee segregation — fees share the same pool as user collateral and proceeds

Proof of Concept

function test_NM005_SelfTransferNoOp() public {
address alice = makeAddr("alice");
_mintAndDepositCollateral(alice, 1);
vm.prank(alice);
nftDealers.list(1, 100e6);
​
address bob = makeAddr("bob");
deal(address(usdc), bob, 100e6);
vm.startPrank(bob);
usdc.approve(address(nftDealers), 100e6);
nftDealers.buy(1);
vm.stopPrank();
​
uint256 contractBalanceBefore = usdc.balanceOf(address(nftDealers));
​
vm.prank(alice);
nftDealers.collectUsdcFromSelling(1);
​
// The self-transfer did not change contract balance beyond the seller payout
// Fees remain in the same pool as user collateral — no segregation
uint256 contractBalanceAfter = usdc.balanceOf(address(nftDealers));
uint256 expectedFees = nftDealers.calculateFees(100e6);
assertEq(contractBalanceAfter, expectedFees); // fees sit in general pool
}

Recommended Mitigation

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!