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

H02. Listing stored by `tokenId` but event emits `listingsCounter`, breaking `buy()` for out-of-order and re-listed NFTs

Author Revealed upon completion

Root + Impact

Description

  • list() is expected to create a listing that buyers can reference by the ID emitted in the NFT_Dealers_Listed event.

  • list() stores the listing at s_listings[_tokenId], keyed by the NFT token ID. However, the event emits listingsCounter as the listing identifier. All consumer functions (buy, cancelListing, updatePrice, collectUsdcFromSelling) accept a _listingId parameter and look up s_listings[_listingId]. When a user passes the emitted counter value, they read a different — possibly empty — storage slot, causing the call to revert with ListingNotActive.

function list(uint256 _tokenId, uint32 _price) external onlyWhitelisted {
// ...
listingsCounter++;
activeListingsCounter++;
​
// @> listing stored at s_listings[_tokenId]
s_listings[_tokenId] = Listing({
seller: msg.sender, price: _price, nft: address(this), tokenId: _tokenId, isActive: true
});
// @> but event announces listingsCounter, not _tokenId
emit NFT_Dealers_Listed(msg.sender, listingsCounter);
}
​
function buy(uint256 _listingId) external payable {
// @> reads s_listings[_listingId]; if _listingId == listingsCounter (from event) != tokenId, returns empty struct
Listing memory listing = s_listings[_listingId];
if (!listing.isActive) revert ListingNotActive(_listingId);
// ...
}

Risk

Likelihood: High

  • Triggered by any of these routine actions: a seller cancels and re-lists the same NFT (counter increments past tokenId permanently), or multiple NFTs are listed in any order other than strict sequential minting order. Both are normal user behaviours.

  • Any off-chain integration or marketplace UI built on the emitted listingId — the standard expectation for event-driven contracts — will produce incorrect call arguments.

Impact: High

  • Buyers who use the emitted listing ID cannot purchase the NFT; their transaction always reverts.

  • The listing is effectively invisible to any external marketplace UI or indexer following the contract's own events.

  • Sellers cannot cancel or update listings via the event-announced ID, creating permanently stuck listings from an off-chain perspective.

Proof of Concept

Two scenarios are demonstrated. The first shows out-of-order listing. The second shows cancel-and-relist, a routine action that permanently desynchronises the counter from the tokenId for every future listing of that NFT.

Scenario 1 — Out-of-order listing:

function test_listingIdMismatch_outOfOrder() public {
// buyer mints tokenId=2 (seller already has tokenId=1 from setUp)
vm.prank(owner);
nftDealers.whitelistWallet(buyer);
​
vm.startPrank(buyer);
usdc.approve(address(nftDealers), LOCK_AMOUNT);
nftDealers.mintNft(); // tokenId=2
vm.stopPrank();
​
// list tokenId=2 first: listingsCounter becomes 1, stored at s_listings[2]
vm.prank(buyer);
nftDealers.list(2, uint32(500e6));
// event emits NFT_Dealers_Listed(buyer, 1) — listingId=1
​
// Buyer tries to purchase using the emitted listingId=1: reverts
vm.startPrank(seller);
usdc.approve(address(nftDealers), 500e6);
vm.expectRevert(abi.encodeWithSelector(NFTDealers.ListingNotActive.selector, 1));
nftDealers.buy(1); // reads s_listings[1] which is empty
vm.stopPrank();
​
// Must use tokenId=2 instead — not what the event advertised
vm.startPrank(seller);
nftDealers.buy(2);
vm.stopPrank();
}

Scenario 2 — Cancel and relist (routine user action):

function test_listingIdMismatch_cancelAndRelist() public {
// tokenId=1, listingsCounter=1 -> s_listings[1]: works first time (tokenId == counter)
vm.prank(seller);
nftDealers.list(1, uint32(500e6));
​
// Seller cancels (e.g., wants to change price)
vm.prank(seller);
nftDealers.cancelListing(1);
​
// Re-lists the same NFT: listingsCounter=2, still stored at s_listings[1]
vm.prank(seller);
nftDealers.list(1, uint32(600e6));
// event emits NFT_Dealers_Listed(seller, 2) — listingId=2
​
// Buyer uses the emitted listingId=2: gets ListingNotActive
vm.startPrank(buyer);
usdc.approve(address(nftDealers), 600e6);
vm.expectRevert(abi.encodeWithSelector(NFTDealers.ListingNotActive.selector, 2));
nftDealers.buy(2); // s_listings[2] is empty
vm.stopPrank();
​
// After this one cancel-and-relist, event IDs are permanently off by one.
// Every further relist widens the gap.
}

Recommended Mitigation

Emit _tokenId instead of listingsCounter in the NFT_Dealers_Listed event so that off-chain callers always receive the correct key for all consumer functions.

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, redesign the storage key to use listingsCounter and maintain a listingIdToTokenId mapping so that counter-based IDs in events match storage keys.

Support

FAQs

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

Give us feedback!