Description
-
Under the intended settlement flow, the protocol fee for a sale should become owner-withdrawable as soon as the sale is completed, because the buyer’s full USDC payment already enters the contract during buy(). The seller’s later claim should only release the seller’s portion of that already-received payment (plus any collateral return), while the fee portion should remain recognized as protocol revenue.
-
The issue is that the contract does not account protocol fees at sale time. Instead, it only increments totalFeesCollected inside collectUsdcFromSelling(), which is a seller-triggered action executed later. As a result, after a successful buy(), the fee portion already sits inside the contract balance, but the owner still cannot withdraw it because withdrawFees() only looks at totalFeesCollected. When the seller never calls collectUsdcFromSelling(), the fee remains trapped inside the pooled contract balance indefinitely and is never recognized as withdrawable protocol profit.
function buy(uint256 _listingId) external payable {
Listing memory listing = s_listings[_listingId];
if (!listing.isActive) revert ListingNotActive(_listingId);
require(listing.seller != msg.sender, "Seller cannot buy their own NFT");
activeListingsCounter--;
bool success = usdc.transferFrom(msg.sender, address(this), listing.price);
require(success, "USDC transfer failed");
_safeTransfer(listing.seller, msg.sender, listing.tokenId, "");
s_listings[_listingId].isActive = false;
emit NFT_Dealers_Sold(msg.sender, listing.price);
}
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);
}
function withdrawFees() external onlyOwner {
require(totalFeesCollected > 0, "No fees to withdraw");
uint256 amount = totalFeesCollected;
totalFeesCollected = 0;
usdc.safeTransfer(owner, amount);
emit NFT_Dealers_F
Risk
Likelihood: High
-
This occurs whenever a sale completes and the seller delays or never performs collectUsdcFromSelling(), because buy() itself does not record any protocol fees into totalFeesCollected.
-
This occurs during normal marketplace usage because seller collection is intentionally split into a separate later transaction rather than happening atomically during buy().
Impact: High
-
The owner cannot withdraw fee revenue from completed sales until the seller claims, even though the fee funds are already present in the contract.
-
In practical scenarios where the seller loses access, disappears, or simply never claims, the fee portion can remain stuck indefinitely inside the contract balance instead of being withdrawable as protocol revenue.
Proof of Concept
-
Add import {console2} from "forge-std/console2.sol"; at the top of NFTDealersTest.t.sol.
-
Copy the code below to NFTDealersTest contract.
-
Run command forge test --mt testFeesOnlyAccountedOnSellerClaimStuckFeesIfSellerNeverCollects -vv --via-ir.
function testFeesOnlyAccountedOnSellerClaimStuckFeesIfSellerNeverCollects() public revealed whitelisted {
uint256 tokenId = 1;
uint32 salePrice = 1000e6;
uint256 expectedFee = nftDealers.calculateFees(salePrice);
uint256 expectedSellerProceeds = uint256(salePrice) - expectedFee;
uint256 expectedContractBalanceAfterBuy = nftDealers.lockAmount() + uint256(salePrice);
console2.log("salePrice:", uint256(salePrice));
console2.log("expectedFee:", expectedFee);
console2.log("expectedSellerProceeds:", expectedSellerProceeds);
console2.log("lockAmount:", nftDealers.lockAmount());
mintAndListNFTForTesting(tokenId, salePrice);
console2.log("Contract USDC after mint+list:", usdc.balanceOf(address(nftDealers)));
console2.log("Seller USDC after mint+list:", usdc.balanceOf(userWithCash));
console2.log("totalFeesCollected before sale:", nftDealers.totalFeesCollected());
assertEq(
usdc.balanceOf(address(nftDealers)), nftDealers.lockAmount(), "sanity: only collateral held before sale"
);
assertEq(nftDealers.totalFeesCollected(), 0, "sanity: no fees before sale");
vm.startPrank(userWithEvenMoreCash);
usdc.approve(address(nftDealers), salePrice);
nftDealers.buy(tokenId);
vm.stopPrank();
uint256 contractBalanceAfterBuy = usdc.balanceOf(address(nftDealers));
uint256 sellerBalanceAfterBuy = usdc.balanceOf(userWithCash);
uint256 totalFeesAfterBuy = nftDealers.totalFeesCollected();
console2.log("Contract USDC after buy:", contractBalanceAfterBuy);
console2.log("Seller USDC after buy (before collect):", sellerBalanceAfterBuy);
console2.log("totalFeesCollected after buy:", totalFeesAfterBuy);
console2.log("Owner USDC before withdraw attempt:", usdc.balanceOf(owner));
assertEq(
contractBalanceAfterBuy,
expectedContractBalanceAfterBuy,
"sanity: contract holds collateral + full buyer payment after sale"
);
assertEq(sellerBalanceAfterBuy, 0, "sanity: seller has not claimed yet, so proceeds remain in the contract");
assertEq(totalFeesAfterBuy, 0, "BUG: fee from completed sale is not accounted until seller claim");
vm.prank(owner);
vm.expectRevert("No fees to withdraw");
nftDealers.withdrawFees();
uint256 ownerBalanceAfterFailedWithdraw = usdc.balanceOf(owner);
uint256 contractBalanceAfterFailedWithdraw = usdc.balanceOf(address(nftDealers));
console2.log("Owner USDC after failed withdraw:", ownerBalanceAfterFailedWithdraw);
console2.log("Contract USDC after failed withdraw:", contractBalanceAfterFailedWithdraw);
assertEq(ownerBalanceAfterFailedWithdraw, 0, "BUG: owner could not withdraw realized sale fee");
assertEq(
contractBalanceAfterFailedWithdraw,
expectedContractBalanceAfterBuy,
"fee remains trapped in pooled contract funds until seller claim"
);
}
Output:
[⠊] Compiling...
No files changed, compilation skipped
Ran 1 test for test/NFTDealersTest.t.sol:NFTDealersTest
[PASS] testFeesOnlyAccountedOnSellerClaimStuckFeesIfSellerNeverCollects() (gas: 454388)
Logs:
salePrice: 1000000000
expectedFee: 10000000
expectedSellerProceeds: 990000000
lockAmount: 20000000
Contract USDC after mint+list: 20000000
Seller USDC after mint+list: 0
totalFeesCollected before sale: 0
Contract USDC after buy: 1020000000
Seller USDC after buy (before collect): 0
totalFeesCollected after buy: 0
Owner USDC before withdraw attempt: 0
Owner USDC after failed withdraw: 0
Contract USDC after failed withdraw: 1020000000
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 4.32ms (1.15ms CPU time)
Ran 1 test suite in 15.62ms (4.32ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)
Recommended Mitigation
The fix is to account protocol fees at sale time rather than at seller-claim time. Since the contract already receives the full buyer payment during buy(), it can compute the fee immediately and increment totalFeesCollected there.
Then collectUsdcFromSelling() should only pay the seller’s net proceeds plus any valid collateral return, without separately “creating” the fee accounting later.
function buy(uint256 _listingId) external payable {
Listing memory listing = s_listings[_listingId];
if (!listing.isActive) revert ListingNotActive(_listingId);
require(listing.seller != msg.sender, "Seller cannot buy their own NFT");
activeListingsCounter--;
bool success = usdc.transferFrom(msg.sender, address(this), listing.price);
require(success, "USDC transfer failed");
+ uint256 fees = _calculateFees(listing.price);
+ totalFeesCollected += fees;
+
_safeTransfer(listing.seller, msg.sender, listing.tokenId, "");
s_listings[_listingId].isActive = false;
emit NFT_Dealers_Sold(msg.sender, listing.price);
}
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 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);
}
