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

Reentrancy and Whitelist Abuse in NFT Marketplace

Author Revealed upon completion

Root + Impact

Description

Normal behavior: NFT Dealers is an NFT marketplace where whitelisted users can mint NFTs by paying a USDC collateral, list them for resale, and collect proceeds. Non-whitelisted users can only buy listed NFTs. The owner can manage the whitelist, reveal the collection, and withdraw fees.

The contract has multiple security risks: owner abuse, reentrancy in USDC transfers, fee manipulation based on listing price, race conditions in listings, and redundant internal token transfers. These issues could lead to unauthorized minting, loss of funds, and inconsistent contract state.

Problem: The contract has multiple security risks:

  • Excessive owner privileges: the owner can manipulate the whitelist, reveal the collection, and withdraw all fees without restrictions.

  • Reentrancy in USDC payments: collectUsdcFromSelling and cancelListing transfer tokens before updating state, allowing recursive attacks.

  • Fee and price manipulation: _calculateFees depends directly on the listing price, allowing optimization attacks and potential inconsistencies in totalFeesCollected.

  • Race conditions in listings: multiple users interacting with the same NFT simultaneously can cause inconsistent state or payments.

  • Incorrect token transfers: usdc.safeTransfer(address(this), fees) is unnecessary and can confuse internal accounting.

// Root cause in the codebase with @> marks to highlight the relevant section
function whitelistWallet(address _wallet) external onlyOwner {
@>whitelistedUsers[_wallet] = true;
}
​
function removeWhitelistedWallet(address _wallet) external onlyOwner {
@>whitelistedUsers[_wallet] = false;
}
​
function withdrawFees() external onlyOwner {
@>uint256 amount = totalFeesCollected;
@>totalFeesCollected = 0;
@>usdc.safeTransfer(owner, amount);
}
​
function collectUsdcFromSelling(uint256 _listingId) external onlySeller(_listingId) {
Listing memory listing = s_listings[_listingId];
@>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);
}

Risk

Likelihood:

  • If the owner's wallet is compromised, an attacker can add malicious wallets to the whitelist and mint NFTs without limits.

  • Malicious users could attempt reentrancy attacks in collectUsdcFromSelling or cancelListing to steal funds or collaterals.

  • Price and fee manipulation could allow users to optimize commissions, causing losses in totalFeesCollected.

Impact:

  • Theft of NFTs or unauthorized minting.

  • Loss of USDC due to incorrect or duplicated fees.

  • Inconsistent listing states causing accounting errors or NFT lock-ups.

Proof of Concept

  1. An attacker controls the owner account.

  2. They whitelist their own wallet using whitelistWallet.

  3. They mint NFTs multiple times using mintNft() without paying legitimate collateral

  4. They call withdrawFees() to drain all accumulated fees.

  5. Alternatively, a malicious user can deploy a contract that repeatedly calls collectUsdcFromSelling before state updates, stealing funds via reentrancy.

    This demonstrates that both owner abuse and reentrancy attacks are feasible with the current code.

// Exploit demonstrating owner abuse and reentrancy
contract Exploit {
NFTDealers nft;
​
constructor(address _nft) {
nft = NFTDealers(_nft);
}
​
function ownerAttack(address attacker) public {
// Add a malicious wallet to the whitelist
nft.whitelistWallet(attacker);
// Mint NFT without restrictions
nft.mintNft();
// Withdraw all accumulated fees
nft.withdrawFees();
}
​
function reentrancyAttack(uint256 listingId) public {
// Recursive call on collectUsdcFromSelling to try stealing funds
nft.collectUsdcFromSelling(listingId);
// In a malicious contract, fallback or receive would call this function again
}
}

Recommended Mitigation

Explanation of mitigation:
These steps reduce risks by limiting owner powers through timelocks, preventing reentrancy with proper state updates, removing redundant transfers that confuse accounting, and validating prices/fees to protect financial correctness

- function whitelistWallet(address _wallet) external onlyOwner { whitelistedUsers[_wallet] = true; }
- function removeWhitelistedWallet(address _wallet) external onlyOwner { whitelistedUsers[_wallet] = false; }
- function withdrawFees() external onlyOwner { uint256 amount = totalFeesCollected; totalFeesCollected = 0; usdc.safeTransfer(owner, amount); }
- usdc.safeTransfer(address(this), fees);
+ // Mitigation: use multisig and timelock for all critical owner functions
+ function whitelistWallet(address _wallet) external onlyOwner onlyWithTimelock { whitelistedUsers[_wallet] = true; }
+ function removeWhitelistedWallet(address _wallet) external onlyOwner onlyWithTimelock { whitelistedUsers[_wallet] = false; }
+ function withdrawFees() external onlyOwner onlyWithTimelock { uint256 amount = totalFeesCollected; totalFeesCollected = 0; usdc.safeTransfer(owner, amount); }
+ // Mitigation: apply ReentrancyGuard to functions that transfer USDC
+ // Update state before transferring funds
+ // Remove unnecessary internal transfers (e.g., usdc.safeTransfer(address(this), fees))
+ // Validate fee and price limits before calculating and transferring

Support

FAQs

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

Give us feedback!