NFT Dealers
First Flight #58
Beginner FriendlyFoundry
100 EXP
View results
Previous Next
Submission Details
Severity: high
Valid

Fee Self-Transfer Renders Protocol Fee Collection Insolvent

perun84

Fee Self-Transfer Renders Protocol Fee Collection Insolvent

Description

  • The expected behavior is that protocol fees are retained in the contract when a seller settles a completed sale, tracked via totalFeesCollected, and later withdrawn by the owner through withdrawFees().

  • The issue is that collectUsdcFromSelling() executes usdc.safeTransfer(address(this), fees) — a transfer from the contract to itself — which is a no-op. totalFeesCollected is incremented as though fees were segregated, but no USDC is actually set aside. When the owner calls withdrawFees(), the payout draws from other users' locked collateral and uncollected sale proceeds, making the contract progressively insolvent.

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); // no-op: contract sends fees to itself
usdc.safeTransfer(msg.sender, amountToSeller);
}

Risk

Likelihood:

  • This triggers on every completed sale — collectUsdcFromSelling() is the only settlement path, and withdrawFees() is the normal revenue collection flow.

Impact:

  • The owner's fee withdrawal consumes USDC belonging to other users (collateral and uncollected proceeds), causing permanent loss of funds for later sellers and minters.

Proof of Concept

function testFeeInsolvency() public revealed {
vm.prank(owner);
nftDealers.whitelistWallet(userWithEvenMoreCash);
​
// User A mints + lists token 1
vm.startPrank(userWithCash);
usdc.approve(address(nftDealers), 20e6);
nftDealers.mintNft();
nftDealers.list(1, 1000e6);
vm.stopPrank();
​
// User B mints token 2 (20 USDC collateral locked)
vm.startPrank(userWithEvenMoreCash);
usdc.approve(address(nftDealers), type(uint256).max);
nftDealers.mintNft();
// User B buys token 1
nftDealers.buy(1);
vm.stopPrank();
​
// Contract holds: 20 (A collateral) + 20 (B collateral) + 1000 (sale) = 1040
​
// User A settles — self-transfer is no-op, gets 1010, contract left with 30
vm.prank(userWithCash);
nftDealers.collectUsdcFromSelling(1);
​
// Owner withdraws 10 USDC "fees" — actually taken from User B's collateral pool
vm.prank(owner);
nftDealers.withdrawFees();
​
// Only 20 USDC remains — exactly User B's collateral, zero margin
assertEq(usdc.balanceOf(address(nftDealers)), 20e6);
}

Recommended Mitigation

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;
+ collateralForMinting[listing.tokenId] = 0;
+ delete s_listings[_listingId];
- usdc.safeTransfer(address(this), fees);
usdc.safeTransfer(msg.sender, amountToSeller);
}
Updates

Lead Judging Commences

rubik0n Lead Judge 16 days ago
Submission Judgement Published
Validated
Assigned finding tags:

drain-protocol-risk

collateral is not reset to zero after collecting USDC from sold NFT. No accounting for collected USDC

Support

FAQs

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

Give us feedback!