NFT Dealers

Root + Impact

Description

Normal behavior dictates that a listing system must use a consistent, unique identifier to store, retrieve, and interact with active listings (e.g., buying, updating, or canceling).

The specific issue is a critical state storage mismatch in the list function. It stores the listing struct in the s_listings mapping using _tokenId as the key, but the event emits listingsCounter as the listing ID. Because all subsequent functions (buy, updatePrice, cancelListing, collectUsdcFromSelling) expect a _listingId, relying on the emitted event data will cause users to interact with the wrong storage slot, resulting in reverted transactions or unauthorized interactions with different NFTs.

Solidity
function list(uint256 _tokenId, uint32 _price) external onlyWhitelisted {
require(_price >= MIN_PRICE, "Price must be at least 1 USDC");
require(ownerOf(_tokenId) == msg.sender, "Not owner of NFT");
require(s_listings[_tokenId].isActive == false, "NFT is already listed");
require(_price > 0, "Price must be greater than 0");

// @> Stores the listing using _tokenId as the key
s_listings[_tokenId] =
Listing({seller: msg.sender, price: _price, nft: address(this), tokenId: _tokenId, isActive: true});

// @> Emits listingsCounter instead of the actual storage key (_tokenId)
emit NFT_Dealers_Listed(msg.sender, listingsCounter);
}

Risk

Likelihood:

High. This issue is guaranteed to happen in production for any NFT listed where its tokenId does not perfectly match the current listingsCounter.

mpact:

High/Critical. It fundamentally breaks the core logic of the marketplace. Users and off-chain indexers (frontends) will use the wrong IDs. Buyers will be unable to purchase NFTs (transactions will revert), and sellers will be unable to cancel or update their listings, permanently locking their assets in a broken state.

Proof of Concept

Add the following test to test/NFTDealersPoC.t.sol to prove the collision. It demonstrates that if a user relies on the emitted listingsCounter ID, the transaction reverts.

Solidity
function test_PoC_ListingIdCollision() public {
vm.startPrank(attacker);
dealers.mintNft(); // Mints Token ID 1
dealers.mintNft(); // Mints Token ID 2

Recommended Mitigation

Standardize the state variable used for mapping. It is better to use listingsCounter as the unique _listingId for the s_listings mapping to align with the rest of the contract's logic. (Note: You may need to add a secondary check to prevent double-listing the same token, since we are changing the mapping key).

Diff
function list(uint256 _tokenId, uint32 _price) external onlyWhitelisted {
require(_price >= MIN_PRICE, "Price must be at least 1 USDC");
require(ownerOf(_tokenId) == msg.sender, "Not owner of NFT");

• 
  
  require(s_listings[_tokenId].isActive == false, "NFT is already listed");

  require(_price > 0, "Price must be greater than 0");

  ​

  listingsCounter++;

  activeListingsCounter++;
• 
  
  s_listings[_tokenId] =

• 
  
  s_listings[listingsCounter] =

  Listing({seller: msg.sender, price: _price, nft: address(this), tokenId: _tokenId, isActive: true});

  emit NFT_Dealers_Listed(msg.sender, listingsCounter);

  }