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

Listing event id does not match the storage id used by marketplace functions

Author Revealed upon completion

list() stores listings under tokenId but emits listingsCounter as the listing identifier. Offchain consumers that rely on the emitted id will interact with incorrect storage slots, leading to failed trades, broken cancellations, or incorrect price updates.

Description

  • Normal behavior: the identifier emitted in listing events should correspond to the canonical storage key used by trading functions such as buy, cancelListing, updatePrice, and collectUsdcFromSelling.

  • Issue: list() stores listings under _tokenId while emitting listingsCounter as the listingId. As a result, external consumers (indexers, bots, UIs) that rely on the emitted identifier will interact with the wrong slot in s_listings.

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);
}

Marketplace entry points expect the storage key (which is _tokenId), not the emitted counter:

function buy(uint256 _listingId) external payable {
@> Listing memory listing = s_listings[_listingId];
if (!listing.isActive) revert ListingNotActive(_listingId);
...
}
function cancelListing(uint256 _listingId) external {
@> Listing memory listing = s_listings[_listingId];
if (!listing.isActive) revert ListingNotActive(_listingId);
...
}
function updatePrice(uint256 _listingId, uint32 _newPrice) external onlySeller(_listingId) {
@> Listing memory listing = s_listings[_listingId];
if (!listing.isActive) revert ListingNotActive(_listingId);
...
}

Because the event id and storage key diverge, the protocol exposes inconsistent listing identity semantics.

Risk

Likelihood:

  • Offchain consumers typically rely on emitted identifiers to construct subsequent transactions such as buys or cancellations.

  • Listing counters and token ids naturally diverge in non-sequential minting or multi-listing scenarios.

Impact:

  • Buyers can unknowingly call buy() on an inactive or empty slot, causing failed trades and degraded market reliability.

  • Automated agents and UIs may cancel or update the wrong listing or become desynchronized from actual marketplace state.

Proof of Concept

The PoC lists token #2 as the first marketplace listing so that listingsCounter == 1 while the active storage slot is s_listings[2].
An offchain consumer that trusts the emitted listingId will call marketplace functions with 1 and interact with the wrong slot.

function test_EmittedListingId_DoesNotMatchStorageKey() public revealed {
uint256 listedTokenId = 2;
uint32 nftPrice = 1000e6;
​
vm.prank(owner);
nftDealers.whitelistWallet(userWithCash);
​
// setUp() minted only 20e6 to userWithCash, so seed one more mint worth of USDC
usdc.mint(userWithCash, 20e6);
​
vm.startPrank(userWithCash);
usdc.approve(address(nftDealers), 40e6);
nftDealers.mintNft(); // tokenId = 1
nftDealers.mintNft(); // tokenId = 2
​
vm.recordLogs();
nftDealers.list(listedTokenId, nftPrice);
vm.stopPrank();
​
Vm.Log[] memory entries = vm.getRecordedLogs();
assertEq(entries.length, 1, "expected exactly one log from list()");
​
bytes32 expectedTopic0 = keccak256("NFT_Dealers_Listed(address,uint256)");
assertEq(entries[0].topics[0], expectedTopic0, "unexpected event emitted");
​
uint256 emittedListingId = abi.decode(entries[0].data, (uint256));
​
(,,,, bool emittedSlotActive) = nftDealers.s_listings(emittedListingId);
(, uint32 storedPrice,, uint256 storedTokenId, bool actualSlotActive) = nftDealers.s_listings(listedTokenId);
​
assertEq(emittedListingId, 1, "event emits listingsCounter");
assertEq(storedTokenId, listedTokenId, "listing is actually stored under tokenId");
assertEq(storedPrice, nftPrice, "stored listing price mismatch");
assertFalse(emittedSlotActive, "emitted id should point to inactive slot");
assertTrue(actualSlotActive, "tokenId slot should hold the active listing");
​
// A consumer using the emitted id will interact with the wrong slot and revert
vm.startPrank(userWithEvenMoreCash);
usdc.approve(address(nftDealers), nftPrice);
​
vm.expectRevert(abi.encodeWithSelector(NFTDealers.ListingNotActive.selector, emittedListingId));
nftDealers.buy(emittedListingId);
​
// Using the real storage key (tokenId) succeeds
nftDealers.buy(listedTokenId);
vm.stopPrank();
​
assertEq(nftDealers.ownerOf(listedTokenId), userWithEvenMoreCash, "correct listing key should complete the sale");
}

Recommended Mitigation

Settlement and trading entry points should use the same canonical identifier that is emitted in events.

The simplest fix is to emit _tokenId instead of listingsCounter.

function list(uint256 _tokenId, uint32 _price) external onlyWhitelisted {
...
- emit NFT_Dealers_Listed(msg.sender, listingsCounter);
+ emit NFT_Dealers_Listed(msg.sender, _tokenId);
}

Alternatively, listings could be stored under listingsCounter if sequential listing identifiers are desired.

Mitigation rationale:
Ensuring identifier consistency prevents offchain desynchronization and guarantees that emitted marketplace state can be safely consumed by integrators.

Support

FAQs

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

Give us feedback!