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

L03. `collectUsdcFromSelling` transfers fees to `address(this)`, which is a no-op and will fail with non-standard tokens

Author Revealed upon completion

Root + Impact

Description

  • collectUsdcFromSelling is intended to retain protocol fees inside the contract and forward the remainder to the seller.

  • The function calls usdc.safeTransfer(address(this), fees) — a transfer from the contract to itself. For standard ERC20 tokens this is a no-op (the balance does not change), but the fees are already inside the contract from the earlier buy() call and need not be moved at all. The explicit self-transfer is misleading and will revert on tokens that disallow transfers to the sender (some non-standard ERC20 implementations), making collectUsdcFromSelling completely non-functional in those deployments.

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;
// @> contract transfers to itself — fees are already here, this call is redundant
usdc.safeTransfer(address(this), fees);
usdc.safeTransfer(msg.sender, amountToSeller);
}

Risk

Likelihood: Low

  • With the intended USDC token, this line executes on every collectUsdcFromSelling call but causes no observable effect — the transfer completes as a no-op.

  • Impact only materialises when a non-standard ERC20 that reverts on self-transfers is configured as the payment token at deploy time.

Impact: Low

  • With standard USDC the call wastes gas and misrepresents intent but causes no fund loss.

  • With an ERC20 that disallows self-transfers, collectUsdcFromSelling becomes permanently non-functional for all sellers, preventing them from ever collecting sale proceeds.

Proof of Concept

The test confirms that the self-transfer is a no-op with standard USDC: the contract balance after the call equals exactly the fees, meaning fees stayed in the contract naturally and the safeTransfer(address(this), ...) line contributed nothing.

function test_selfTransferFees_isNoOp() public {
uint32 nftPrice = 500e6;
uint256 fees = nftDealers.calculateFees(nftPrice);
​
vm.prank(seller);
nftDealers.list(1, nftPrice);
​
vm.startPrank(buyer);
usdc.approve(address(nftDealers), nftPrice);
nftDealers.buy(1);
vm.stopPrank();
​
vm.prank(seller);
nftDealers.collectUsdcFromSelling(1);
​
// Contract holds exactly the fees — self-transfer was a no-op
assertEq(usdc.balanceOf(address(nftDealers)), fees);
assertEq(nftDealers.totalFeesCollected(), fees);
}

Recommended Mitigation

Remove the self-transfer line from collectUsdcFromSelling. Fees remain in the contract automatically because the buyer's USDC was already transferred to the contract in buy() and only the seller's portion needs to be explicitly forwarded.

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

Support

FAQs

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

Give us feedback!