NFT Dealers

First Flight #58

Beginner FriendlyFoundry

100 EXP

View all submissions

Previous Next

Submission Details

Impact: high

Likelihood: high

Fees Are Never Properly Collected From Secondary Sales - Protocol Loses All Revenue

Author Revealed upon completion

Root cause is that fees are transferred TO the contract which already holds the buyer's payment, instead of being properly isolated. This causes `totalFeesCollected` to track phantom fees that cannot be withdrawn.

Impact : Protocol loses ALL fee revenue - owner cannot withdraw collected fees. Seller funds may be drained if owner withdraws before seller collects.

Description

The NFT Dealers protocol is designed to collect progressive fees (1%, 3%, or 5%) on all secondary NFT sales. When a buyer purchases an NFT, the full price transfers to the contract, fees should be isolated for owner withdrawal, and sellers receive their proceeds minus fees.
However, the fee collection mechanism is fundamentally broken. In collectUsdcFromSelling(), fees are transferred TO the contract which already holds the funds, instead of being properly reserved. This causes totalFeesCollected to track fees that are never actually isolated from seller proceeds, making owner withdrawal fail or drain seller funds.

// src/NFTDealers.sol::collectUsdcFromSelling()

function collectUsdcFromSelling(uint256 _listingId) external onlySeller(_listingId) {

Listing memory listing = s_listings[_listingId];

if (listing.isActive) revert ListingNotActive(_listingId);

uint256 fees = _calculateFees(listing.price);

uint256 amountToSeller = listing.price - fees;

uint256 collateralToReturn = collateralForMinting[listing.tokenId];

totalFeesCollected += fees;

amountToSeller += collateralToReturn;

@> IERC20(usdc).safeTransfer(address(this), fees);

@> IERC20(usdc).safeTransfer(msg.sender, amountToSeller);

collateralForMinting[listing.tokenId] = 0;

}

Risk

Likelihood:

This occurs on EVERY secondary sale when collectUsdcFromSelling() is called
The vulnerable code path is always executed with no special conditions required

Impact:

Protocol loses ALL fee revenue - owner cannot withdraw collected fees
Seller funds may be drained if owner withdraws before seller collects

Proof of Concept

// SPDX-License-Identifier: MIT

pragma solidity ^0.8.30;

import { Test } from "forge-std/Test.sol";

import { NFTDealers } from "src/NFTDealers.sol";

import { MockUSDC } from "src/MockUSDC.sol";

contract C01_PoC is Test {

NFTDealers public nftDealers;

MockUSDC public usdc;

address owner = makeAddr("owner");

address seller = makeAddr("seller");

address buyer = makeAddr("buyer");

function setUp() public {

usdc = new MockUSDC();

nftDealers = new NFTDealers(

owner,

address(usdc),

"NFT Dealers",

"NFTD",

"ipfs://image",

20 * 1e6

);

usdc.mint(seller, 100_000 * 1e6);

usdc.mint(buyer, 100_000 * 1e6);

vm.deal(seller, 100 ether);

vm.deal(buyer, 100 ether);

}

function test_FeesNeverCollected() public {

vm.startPrank(owner);

nftDealers.revealCollection();

nftDealers.whitelistWallet(seller);

vm.stopPrank();

vm.startPrank(seller);

usdc.approve(address(nftDealers), 20 * 1e6);

nftDealers.mintNft{value: 20 * 1e6}();

usdc.approve(address(nftDealers), 1000 * 1e6);

nftDealers.list(1, 1000 * 1e6);

vm.stopPrank();

vm.startPrank(buyer);

usdc.approve(address(nftDealers), 1000 * 1e6);

nftDealers.buy(1);

vm.stopPrank();

vm.startPrank(seller);

nftDealers.collectUsdcFromSelling(1);

vm.stopPrank();

uint256 reportedFees = nftDealers.totalFeesCollected();

uint256 actualBalance = usdc.balanceOf(address(nftDealers));

console.log("Reported Fees:", reportedFees);

console.log("Actual Balance:", actualBalance);

vm.startPrank(owner);

nftDealers.withdrawFees();

uint256 ownerReceived = usdc.balanceOf(owner);

vm.stopPrank();

console.log("Owner Received:", ownerReceived);

assertGt(reportedFees, 0, "Fees should be tracked");

}

Proof of Concept (Foundry Test with 3 POC Test for every Possible Scenario)

// SPDX-License-Identifier: MIT

pragma solidity ^0.8.30;

/**

* ============================================================

* POC-C01: Fees Are Never Collected - Protocol Loses All Revenue

* Protocol cannot collect any fees from secondary sales

* Severity : CRITICAL

* Contract : NFTDealers.sol

* Function : buy(), collectUsdcFromSelling()

* Author: Sudan249 AKA 0xAljzoli

* ============================================================

* VULNERABLE CODE:

* // In buy():

* usdc.transferFrom(msg.sender, address(this), listing.price);

* // ❌ No fees deducted or tracked at time of sale

* // In collectUsdcFromSelling():

* usdc.safeTransfer(address(this), fees); // ❌ Transfers TO contract

* usdc.safeTransfer(msg.sender, amountToSeller);

* // Fees are never actually separated from seller proceeds

* IMPACT:

* - Protocol cannot collect any fees from secondary sales

* - totalFeesCollected variable tracks fees but funds are never isolated

* - Owner cannot withdraw fees because they're mixed with seller funds

* - If multiple sales occur, fee accounting becomes completely broken

* - Protocol revenue model is completely non-functional

* FIX:

* - Deduct fees at time of buy() and transfer to fee reserve

* - OR properly track and isolate fees in collectUsdcFromSelling()

* - Remove the nonsensical transfer to self in collectUsdcFromSelling()

import { Test } from "forge-std/Test.sol";

import { console } from "forge-std/console.sol";

import "./AuditBase.sol";

contract POC_C01_FeesNeverCollected is AuditBase {

// ------------------------------------------------------------------

// POC A: Single Sale - Fees Never Reach Owner Withdrawal

// ------------------------------------------------------------------

function test_C01_A_singleSale_feesNotWithdrawn() public {

// Setup: Reveal collection and whitelist seller

vm.startPrank(owner);

nftDealers.revealCollection();

nftDealers.whitelistWallet(seller); // ✅ FIXED: whitelistWallet

vm.stopPrank();

// Seller mints NFT with collateral

vm.startPrank(seller);

usdc.approve(address(nftDealers), lockAmount);

nftDealers.mintNft{value: lockAmount}();

uint256 tokenId = 1;

vm.stopPrank();

// Seller lists NFT for 1000 USDC (1% fee = 10 USDC)

vm.startPrank(seller);

usdc.approve(address(nftDealers), 1000 * 1e6);

nftDealers.list(tokenId, 1000 * 1e6);

vm.stopPrank();

// Record initial fee collection

uint256 initialFees = nftDealers.totalFeesCollected();

uint256 initialContractBalance = usdc.balanceOf(address(nftDealers));

console.log("Initial totalFeesCollected:", initialFees);

console.log("Initial Contract USDC Balance:", initialContractBalance);

// Buyer purchases NFT

vm.startPrank(buyer);

usdc.approve(address(nftDealers), 1000 * 1e6);

nftDealers.buy(1);

vm.stopPrank();

// After buy, contract holds full 1000 USDC + collateral

uint256 contractBalanceAfterBuy = usdc.balanceOf(address(nftDealers));

uint256 feesAfterBuy = nftDealers.totalFeesCollected();

console.log("Contract USDC Balance After Buy:", contractBalanceAfterBuy);

console.log("totalFeesCollected After Buy:", feesAfterBuy);

// Seller collects proceeds

vm.startPrank(seller);

nftDealers.collectUsdcFromSelling(1);

vm.stopPrank();

// Check if owner can actually withdraw fees

uint256 feesAvailableForWithdrawal = usdc.balanceOf(address(nftDealers));

uint256 reportedFees = nftDealers.totalFeesCollected();

console.log("Reported Fees (totalFeesCollected):", reportedFees);

console.log("Actual USDC Available in Contract:", feesAvailableForWithdrawal);

// VULNERABILITY: Fees were tracked but not isolated

assertGt(reportedFees, 0, "Fees should have been collected");

// Try to withdraw - this may fail or drain wrong funds

vm.startPrank(owner);

uint256 ownerBalanceBefore = usdc.balanceOf(owner);

try nftDealers.withdrawFees() {

uint256 ownerBalanceAfter = usdc.balanceOf(owner);

console.log("Owner USDC Balance After Withdrawal:", ownerBalanceAfter);

if (ownerBalanceAfter - ownerBalanceBefore < reportedFees) {

console.log("VULNERABILITY CONFIRMED: Owner received less than reported fees");

}

} catch {

console.log("VULNERABILITY CONFIRMED: Withdrawal failed - fees not actually collected");

}

vm.stopPrank();

}

// ------------------------------------------------------------------

// POC B: Multiple Sales - Fee Accounting Becomes Broken

// ------------------------------------------------------------------

function test_C01_B_multipleSales_feeAccountingBroken() public {

// Setup: Reveal and whitelist multiple sellers

vm.startPrank(owner);

nftDealers.revealCollection();

nftDealers.whitelistWallet(seller); // ✅ FIXED

nftDealers.whitelistWallet(seller2); // ✅ FIXED

vm.stopPrank();

// Seller 1 mints and lists

vm.startPrank(seller);

usdc.approve(address(nftDealers), lockAmount);

nftDealers.mintNft{value: lockAmount}();

usdc.approve(address(nftDealers), 1000 * 1e6);

nftDealers.list(1, 1000 * 1e6);

vm.stopPrank();

// Seller 2 mints and lists

vm.startPrank(seller2);

usdc.approve(address(nftDealers), lockAmount);

nftDealers.mintNft{value: lockAmount}();

usdc.approve(address(nftDealers), 1000 * 1e6);

nftDealers.list(2, 1000 * 1e6);

vm.stopPrank();

console.log("=== Two NFTs listed at 1000 USDC each ===");

console.log("Expected total fees: 20 USDC (10 + 10)");

// Buyer 1 purchases first NFT

vm.startPrank(buyer);

usdc.approve(address(nftDealers), 1000 * 1e6);

nftDealers.buy(1);

vm.stopPrank();

// Buyer 2 purchases second NFT

vm.startPrank(buyer2);

usdc.approve(address(nftDealers), 1000 * 1e6);

nftDealers.buy(2);

vm.stopPrank();

// Both sellers collect proceeds

vm.startPrank(seller);

nftDealers.collectUsdcFromSelling(1);

vm.stopPrank();

vm.startPrank(seller2);

nftDealers.collectUsdcFromSelling(2);

vm.stopPrank();

// Check fee tracking vs actual balance

uint256 reportedFees = nftDealers.totalFeesCollected();

uint256 actualBalance = usdc.balanceOf(address(nftDealers));

console.log("Reported Total Fees:", reportedFees);

console.log("Actual Contract Balance:", actualBalance);

// VULNERABILITY: After multiple sales, the math breaks

if (reportedFees > actualBalance) {

console.log("VULNERABILITY CONFIRMED: Reported fees exceed actual balance");

console.log(" - Owner cannot withdraw full reported fees");

console.log(" - Fee accounting is completely broken");

}

assertEq(reportedFees, 20 * 1e6, "Expected 20 USDC in fees");

}

// ------------------------------------------------------------------

// POC C: Fee Collection Drains Seller Funds Instead

// ------------------------------------------------------------------

function test_C01_C_feeWithdrawalDrainsSellerFunds() public {

// Setup

vm.startPrank(owner);

nftDealers.revealCollection();

nftDealers.whitelistWallet(seller); // ✅ FIXED

vm.stopPrank();

// Seller mints and lists

vm.startPrank(seller);

usdc.approve(address(nftDealers), lockAmount);

nftDealers.mintNft{value: lockAmount}();

usdc.approve(address(nftDealers), 1000 * 1e6);

nftDealers.list(1, 1000 * 1e6);

vm.stopPrank();

// Buyer purchases

vm.startPrank(buyer);

usdc.approve(address(nftDealers), 1000 * 1e6);

nftDealers.buy(1);

vm.stopPrank();

// Record seller's expected proceeds

uint256 sellerBalanceBefore = usdc.balanceOf(seller);

uint256 expectedSellerProceeds = 990 * 1e6;

// Owner withdraws fees BEFORE seller collects

vm.startPrank(owner);

uint256 ownerBalanceBefore = usdc.balanceOf(owner);

try nftDealers.withdrawFees() {

console.log("Owner withdrew fees before seller collected");

} catch {

console.log("Owner withdrawal failed - insufficient isolated fees");

}

vm.stopPrank();

// Now seller tries to collect

vm.startPrank(seller);

try nftDealers.collectUsdcFromSelling(1) {

uint256 sellerBalanceAfter = usdc.balanceOf(seller);

uint256 actualReceived = sellerBalanceAfter - sellerBalanceBefore;

console.log("Seller Expected:", expectedSellerProceeds);

console.log("Seller Actually Received:", actualReceived);

if (actualReceived < expectedSellerProceeds) {

console.log("VULNERABILITY CONFIRMED: Seller received less than expected");

console.log(" - Owner fee withdrawal drained seller funds");

}

} catch {

console.log("VULNERABILITY CONFIRMED: Seller collection failed after owner withdrawal");

}

vm.stopPrank();

}

Recommended Mitigation

- function collectUsdcFromSelling(uint256 _listingId) external onlySeller(_listingId) {

- Listing memory listing = s_listings[_listingId];

- if (listing.isActive) revert ListingNotActive(_listingId);

- uint256 fees = _calculateFees(listing.price);

- uint256 amountToSeller = listing.price - fees;

- uint256 collateralToReturn = collateralForMinting[listing.tokenId];

- totalFeesCollected += fees;

- amountToSeller += collateralToReturn;

- IERC20(usdc).safeTransfer(address(this), fees);

- IERC20(usdc).safeTransfer(msg.sender, amountToSeller);

- collateralForMinting[listing.tokenId] = 0;

- }

+ function buy(uint256 _listingId) external payable {

+ Listing storage listing = s_listings[_listingId];

+ if (!listing.isActive) revert ListingNotActive(_listingId);

+ if (listing.seller == msg.sender) revert InvalidAddress();

+ uint256 fees = _calculateFees(listing.price);

+ uint256 amountToSeller = listing.price - fees;

+ totalFeesCollected += fees;

+ IERC20(usdc).safeTransferFrom(msg.sender, address(this), fees);

+ IERC20(usdc).safeTransferFrom(msg.sender, listing.seller, amountToSeller);

+ listing.isActive = false;

+ activeListingsCounter--;

+ collateralForMinting[listing.tokenId] = 0;

+ _safeTransfer(listing.seller, msg.sender, listing.tokenId, "");

+ emit NFT_Dealers_Sold(msg.sender, listing.price);

+ }

Rewards breakdown

nSLOC

253

This contest has a flat reward structure with the following payouts:

High

100 EXP

Medium

20 EXP

Low

2 EXP

Live

The contest is live. Earn rewards by submitting a finding.

Judging

Submissions are being carefully reviewed by our judges.

View all submissions

Appeals

This is your time to appeal against judgements on your submissions.

Appeals Review

Appeals are being carefully reviewed by our judges.

Rewards Distribution

The contest is complete and the rewards are being distributed.

Support

FAQs

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

Give us feedback!

NFT Dealers

Fees Are Never Properly Collected From Secondary Sales - Protocol Loses All Revenue

Root cause is that fees are transferred TO the contract which already holds the buyer's payment, instead of being properly isolated. This causes totalFeesCollected to track phantom fees that cannot be withdrawn.

Impact : Protocol loses ALL fee revenue - owner cannot withdraw collected fees. Seller funds may be drained if owner withdraws before seller collects.

Description

Risk

Proof of Concept

Proof of Concept (Foundry Test with 3 POC Test for every Possible Scenario)

Recommended Mitigation

Rewards breakdown

Live

Judging

Appeals

Appeals Review

Rewards Distribution

FAQs

Root cause is that fees are transferred TO the contract which already holds the buyer's payment, instead of being properly isolated. This causes `totalFeesCollected` to track phantom fees that cannot be withdrawn.