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

[M-01] Emitted listingId Does Not Match Actual Listing Storage Key

Author Revealed upon completion

Root Cause

In list(), listings are stored using _tokenId as the mapping key, but listingsCounter is incremented and emitted as the listingId in the event. These two values are never in sync.

Impact: Any off-chain system or frontend using the emitted listingId to look up a listing will retrieve an empty or incorrect struct.

Description

When a user lists an NFT, the listing is stored at s_listings[_tokenId]. However the event emits listingsCounter as the listingId. These two values diverge as soon as the first NFT listed does not have a tokenId matching the current listingsCounter.

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++; // incremented as listingId
@> s_listings[_tokenId] = Listing({...}); // stored at tokenId key
@> emit NFT_Dealers_Listed(msg.sender, listingsCounter); // emits wrong key
}

Risk

Likelihood: High

  • This occurs every time an NFT with a tokenId that does not match the current listingsCounter is listed

  • Any user who mints multiple NFTs and lists them out of order will trigger this immediately

Impact: Medium

  • Off-chain systems, frontends, and indexers relying on the emitted listingId to look up listings will retrieve empty structs

  • buyers relying on the emitted listingId to call buy() will pass the wrong key and the transaction will revert with ListingNotActive

Proof of Concept

// PoC: We prove the emitted listingId is out of sync with the actual mapping key

// by minting 5 NFTs and only listing token ID 5, showing the event emits listingId=1

// while the real listing is stored at s_listings[5] thus making s_listings[1] empty

​
function test_POC_ListingCounterOutOfSync() public revealed {
vm.prank(owner);
nftDealers.whitelistWallet(userWithCash);
​
// Mint 5 NFTs
usdc.mint(userWithCash, 100e6);
vm.startPrank(userWithCash);
usdc.approve(address(nftDealers), 100e6);
nftDealers.mintNft(); // tokenId = 1
nftDealers.mintNft(); // tokenId = 2
nftDealers.mintNft(); // tokenId = 3
nftDealers.mintNft(); // tokenId = 4
nftDealers.mintNft(); // tokenId = 5
​
// Only list token ID 5
nftDealers.list(5, 1000e6);
vm.stopPrank();
​
// listingsCounter = 1, event emitted listingId = 1
assertEq(nftDealers.totalListings(), 1);
​
// Listing stored at key 5 (tokenId) not 1 (listingsCounter)
(,,,, bool isActive1) = nftDealers.s_listings(1);
assertEq(isActive1, false); // nothing at key 1 — empty struct!
​
(address seller5,,,, bool isActive5) = nftDealers.s_listings(5);
assertEq(seller5, userWithCash); // real listing is here at key 5
assertTrue(isActive5);
}

Recommended Mitigation

  • // remove emitting listingsCounter which is out of sync with the mapping key and we replace it with

  • // emit tokenId so off-chain systems can correctly look up the listing

​
- emit NFT_Dealers_Listed(msg.sender, listingsCounter);
​
+ emit NFT_Dealers_Listed(msg.sender, _tokenId);

Support

FAQs

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

Give us feedback!