Root + Impact
Description
-
After a sale, the contract is supposed to separate the protocol fee from the seller's proceeds, track it in totalFeesCollected, and allow the owner to later withdraw it
via withdrawFees().
Line 181 executes usdc.safeTransfer(address(this), fees) — a self-transfer from the contract to itself. This is a no-op: no tokens move, no reservation occurs.
totalFeesCollected is incremented as if the fee was reserved, but the USDC is never actually separated. When the owner calls withdrawFees(), it sends tokens that are
already owed to other users (collateral, pending proceeds), causing protocol insolvency
**Root Cause**: On line 181, `usdc.safeTransfer(address(this), fees)` transfers USDC from the contract to itself. This is a no-op -- the contract's balance does not change. The intent was clearly to separate fees from the seller's payment, but since all funds are already in the contract, this transfer accomplishes nothing. Meanwhile, `totalFeesCollected` is incremented by `fees`, creating a phantom accounting entry that does not correspond to any actual reserved tokens.
When the owner calls `withdrawFees()`, it attempts to transfer `totalFeesCollected` worth of USDC to the owner. But those tokens were never actually reserved -- they were already sent to the seller as part of `amountToSeller + collateralToReturn` (or were already in the contract from the buyer's payment). This means `withdrawFees()` will send USDC that belongs to other users (other sellers' pending payments, other users' collateral), causing protocol insolvency.
Combined with NFTD-C-001 (repeated collection), this compounds into total fund loss. Even without repeated collection, the fee accounting is fundamentally broken.// Root cause in the codebase with @> marks to highlight the relevant section
Impact:
Proof of Concept
1. Alice mints (20 USDC collateral). Bob mints (20 USDC collateral). Contract holds 40 USDC.
2. Alice lists for 1000 USDC. Charlie buys. Contract now holds 1040 USDC.
3. Alice calls `collectUsdcFromSelling`: receives 990 + 20 = 1010 USDC. `totalFeesCollected` = 10 USDC. Contract holds 30 USDC.
4. The self-transfer of 10 USDC fees did nothing -- those 10 USDC are part of the 30 remaining.
5. Owner calls `withdrawFees()` -- sends 10 USDC to owner. Contract now holds 20 USDC.
6. But Bob's collateral (20 USDC) is still locked! The protocol just sent Bob's collateral to the owner as "fees."
7. When Bob tries to cancel his listing or sell and collect, the contract may not have enough USDC, depending on additional activity.
In scenarios with many sales, `totalFeesCollected` accumulates to a large number, and `withdrawFees` drains other users' collateral and pending sale proceeds.
Recommendation:
Remove the self-transfer entirely. Fees are implicitly retained in the contract when only amountToSeller (price - fees + collateral) is sent out. The fee amount stays in the contract by not being sent to the seller.
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);
}
