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

`s_listings` keyed by `tokenId` but events emit `listingsCounter`, breaking off-chain indexing

Author Revealed upon completion

Description

The s_listings mapping uses tokenId as its key, and all on-chain functions — buy(), cancelListing(), updatePrice(), and collectUsdcFromSelling() — accept a _listingId parameter that resolves as a tokenId against this mapping.

The NFT_Dealers_Listed event emits listingsCounter as the listingId. After the first resale and relisting of any token, listingsCounter increments while s_listings[tokenId] is simply overwritten in place. Any off-chain consumer — front-end, indexer, or bot — that stores the emitted listingId and later calls buy(_listingId) will pass the wrong key, receiving a ListingNotActive revert or interacting with an entirely different listing.

function list(uint256 _tokenId, uint32 _price) external onlyWhitelisted {
...
listingsCounter++;
activeListingsCounter++;
// @> Stored using tokenId as the mapping key
s_listings[_tokenId] = Listing({ seller: msg.sender, price: _price, nft: address(this), tokenId: _tokenId, isActive: true });
// @> Event emits listingsCounter — a different value from _tokenId after first listing
emit NFT_Dealers_Listed(msg.sender, listingsCounter);
}
​
// All consumer functions resolve _listingId as a tokenId — not the emitted counter value
function buy(uint256 _listingId) external payable {
// @> s_listings[_listingId] resolves by tokenId, not the emitted listingsCounter
Listing memory listing = s_listings[_listingId];
if (!listing.isActive) revert ListingNotActive(_listingId);
...
}

Root Cause

File: src/NFTDealers.sol

The root cause is that s_listings was designed with two conflicting identifiers: the mapping key is tokenId (an NFT property), while the emitted listingId in NFT_Dealers_Listed is listingsCounter (a monotone counter). These two values only coincide by accident when tokenId == listingsCounter, which breaks permanently after any token is relisted. All read functions (buy, cancelListing, updatePrice, collectUsdcFromSelling) were built against the tokenId key but the public-facing event advertises the counter, creating an irreconcilable mismatch for any off-chain consumer.

Risk

Likelihood:

  • Any front-end or indexer consuming the NFT_Dealers_Listed event and using the emitted listingId to call buy() or cancelListing() passes a wrong key from the second listing of any token onward.

  • listingsCounter and tokenId diverge permanently after the first resale and relisting cycle — there is no on-chain mechanism to reconcile them.

Impact:

  • Off-chain consumers calling buy() or cancelListing() with the event-emitted listingId receive ListingNotActive errors or silently interact with a different listing than intended.

  • listingsCounter (exposed via totalListings()) is a misleading metric that does not correspond to any retrievable on-chain state, corrupting protocol analytics and front-end displays.

Proof of Concept

function test_listingIdEventMismatch() public revealed {
vm.prank(owner);
nftDealers.whitelistWallet(alice);
vm.prank(owner);
nftDealers.whitelistWallet(bob);
​
usdc.mint(alice, 20e6);
vm.startPrank(alice);
usdc.approve(address(nftDealers), 20e6);
nftDealers.mintNft(); // tokenId = 1
nftDealers.list(1, 500e6); // listingsCounter = 1, s_listings[1] set
vm.stopPrank(); // event emits listingId = 1 (matches here only)
​
usdc.mint(bob, 500e6);
vm.startPrank(bob);
usdc.approve(address(nftDealers), 500e6);
nftDealers.buy(1);
nftDealers.list(1, 600e6); // listingsCounter = 2, s_listings[1] overwritten
vm.stopPrank(); // event emits listingId = 2 — WRONG KEY
​
// Front-end reads emitted listingId = 2 and calls buy(2)
// s_listings[2] is empty — reverts with ListingNotActive
usdc.mint(address(this), 600e6);
usdc.approve(address(nftDealers), 600e6);
vm.expectRevert(abi.encodeWithSelector(NFTDealers.ListingNotActive.selector, 2));
nftDealers.buy(2); // correct call would be buy(1)
}

Recommended Mitigation

function list(uint256 _tokenId, uint32 _price) external onlyWhitelisted {
...
listingsCounter++;
activeListingsCounter++;
s_listings[_tokenId] = Listing({ seller: msg.sender, price: _price, nft: address(this), tokenId: _tokenId, isActive: true });
- emit NFT_Dealers_Listed(msg.sender, listingsCounter);
+ emit NFT_Dealers_Listed(msg.sender, _tokenId);
}

Alternatively, refactor s_listings to use listingsCounter as the mapping key and store tokenId inside the struct, ensuring the emitted ID and the on-chain lookup key are always identical. All functions accepting _listingId would then resolve correctly against the counter-keyed mapping.

Support

FAQs

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

Give us feedback!