Description:
In collectUsdcFromSelling(), after computing fees and amountToSeller, the function calls:
usdc.safeTransfer(address(this), fees);
usdc.safeTransfer(msg.sender, amountToSeller);
Transferring to address(this) is effectively a no-op in most ERC-20 implementations, but the accounting line totalFeesCollected += fees is executed regardless. This means totalFeesCollected grows with each sale but the actual fee USDC is sent straight to the seller (included in amountToSeller which is calculated as listing.price - fees + collateral). The owner can then call withdrawFees() which will attempt to transfer totalFeesCollected from a balance that was never actually set aside, causing either an incorrect payment or an underflow revert.
function collectUsdcFromSelling(uint256 _listingId) external onlySeller(_listingId) {
...
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);
}
Impact: Fee accounting is broken. The owner will be unable to withdraw correct fee amounts, and sellers may receive more USDC than intended.
Recommended Mitigation: Remove the erroneous self-transfer. The fees are already held in the contract from the buy() call:
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 collateralToReturn = collateralForMinting[listing.tokenId];
uint256 amountToSeller = listing.price - fees + collateralToReturn;
totalFeesCollected += fees;
collateralForMinting[listing.tokenId] = 0;
usdc.safeTransfer(msg.sender, amountToSeller);
}
Proof of Concept (Forge):
function test_collectUsdc_feeAccountingBroken() public {
uint256 tokenId = 1;
uint32 nftPrice = 1000e6;
vm.prank(owner); nftDealers.revealCollection();
vm.prank(owner); nftDealers.whitelistWallet(userWithCash);
usdc.mint(userWithCash, 20e6);
usdc.mint(userWithEvenMoreCash, 200_000e6);
vm.startPrank(userWithCash);
usdc.approve(address(nftDealers), 20e6);
nftDealers.mintNft();
nftDealers.list(tokenId, nftPrice);
vm.stopPrank();
vm.startPrank(userWithEvenMoreCash);
usdc.approve(address(nftDealers), nftPrice);
nftDealers.buy(1);
vm.stopPrank();
uint256 contractBalanceBeforeCollect = usdc.balanceOf(address(nftDealers));
vm.prank(userWithCash);
nftDealers.collectUsdcFromSelling(tokenId);
uint256 fees = nftDealers.totalFeesCollected();
uint256 contractBalanceAfterCollect = usdc.balanceOf(address(nftDealers));
assertEq(contractBalanceAfterCollect, fees,
"Contract balance does not match totalFeesCollected — fee accounting is broken");
}
