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

NFTDealers::list() emits listingsCounter as listingId instead of tokenId, causing buyers to be permanently unable to purchase relisted NFTs

Author Revealed upon completion

Root + Impact

Description

  • The list function stores listing data in s_listings[tokenId] and uses tokenId as the key for all subsequent operations (buy, cancelListing, updatePrice). Off-chain services and buyers are expected to rely on the NFT_Dealers_Listed event to retrieve the correct listingId and interact with the contract.

  • However, the NFT_Dealers_Listed event emits listingsCounter as the listingId instead of tokenId. This counter increments on every new listing regardless of the associated tokenId, causing the two values to permanently diverge as soon as any NFT is cancelled and relisted. Any buyer or off-chain service relying on the emitted listingId to call buy will reference a non-existent slot in s_listings, causing the transaction to revert with ListingNotActive.

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");
// @audit-info check inutile
require(_price > 0, "Price must be greater than 0");
​
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);
}

Risk

Likelihood:

  • A seller cancels and relists their NFT, which is a standard and expected marketplace operation, causing the divergence to occur permanently from that point forward

Any off-chain interface or indexer consuming NFT_Dealers_Listed events to populate listings will display incorrect listingId values, leading every buyer interacting through that interface to fail

Impact:

  • Relisted NFTs become permanently unpurchasable by any buyer relying on event data, effectively locking legitimate listings out of the marketplace

The protocol's public event interface becomes unreliable, undermining trust in any front-end or off-chain integration built on top of it

Proof of Concept

This test uses the same setup as the NFTDealersTest file, extended with both userWithEvenMoreCash and userWithCash acting as sellers, and maliciousUser as the buyer.

The vulnerability: The list function stores listings under s_listings[tokenId] but emits listingsCounter as the listingId in the NFT_Dealers_Listed event. On the first listing of each NFT the two values coincide, making the bug invisible. They permanently diverge as soon as any NFT is cancelled and relisted, since listingsCounter keeps incrementing while the mapping key stays fixed to the tokenId.

The divergence: Both users mint and list their NFTs — in both cases listingId from the event matches tokenId by coincidence. When userWithEvenMoreCash cancels and relists tokenId=1, the listing is correctly stored at s_listings[1], but listingsCounter has reached 3, so the event emits listingId=3. From this point the two identifiers are permanently misaligned.

Final assertions:

  • buy(3) reverts with ListingNotActive because s_listings[3] was never created

  • The real listing is confirmed active at s_listings[1] with the correct price, proving it is permanently inaccessible to any buyer relying on event data

function testListingCounterOnEventIsDifferentFromTokenId() public revealed {
// Setup: whitelist both users
vm.startPrank(owner);
nftDealers.whitelistWallet(userWithEvenMoreCash);
nftDealers.whitelistWallet(userWithCash);
vm.stopPrank();
​
uint256 collateralForMinting = nftDealers.lockAmount();
​
// userWithEvenMoreCash mint tokenId=1 and list it at 500 USDC
vm.startPrank(userWithEvenMoreCash);
usdc.approve(address(nftDealers), collateralForMinting);
nftDealers.mintNft();
uint256 tokenIdFirstMint = nftDealers.totalMinted(); // tokenId = 1
vm.recordLogs();
nftDealers.list(tokenIdFirstMint, uint32(500e6)); // s_listings[1] = Listing{price: 500 USDC}
Vm.Log[] memory firstListingLogs = vm.getRecordedLogs();
vm.stopPrank();
uint256 firstListingIdFromEvent = abi.decode(
firstListingLogs[0].data,
(uint256)
);
​
assertEq(firstListingIdFromEvent, tokenIdFirstMint);
assertEq(firstListingIdFromEvent, nftDealers.totalListings());
// in this case tokenId == listingsCounter, but it is just a coincidence
​
// userWithCash mint tokenId=2 and list it at 100 USDC
vm.startPrank(userWithCash);
usdc.approve(address(nftDealers), collateralForMinting);
nftDealers.mintNft();
​
uint256 tokenIdSecondMint = nftDealers.totalMinted(); // tokenId = 2
vm.recordLogs();
nftDealers.list(tokenIdSecondMint, uint32(100e6)); // s_listings[2] = Listing{price: 100 USDC}
Vm.Log[] memory secondListingLogs = vm.getRecordedLogs();
vm.stopPrank();
uint256 secondListingIdFromEvent = abi.decode(
secondListingLogs[0].data,
(uint256)
);
​
assertEq(secondListingIdFromEvent, tokenIdSecondMint);
assertEq(secondListingIdFromEvent, nftDealers.totalListings());
// also in this case tokenId == listingsCounter, but it is just a coincidence
​
// Now userWithEvenMoreCash cancels their listing and creates a new one
vm.startPrank(userWithEvenMoreCash);
nftDealers.cancelListing(1); // cancel s_listings[1]
vm.recordLogs();
nftDealers.list(1, uint32(999e6)); // s_listings[1] = Listing{price: 999 USDC}
Vm.Log[] memory relistingLogs = vm.getRecordedLogs();
vm.stopPrank();
uint256 relistingIdFromEvent = abi.decode(
relistingLogs[0].data,
(uint256)
);
assertNotEq(relistingIdFromEvent, tokenIdFirstMint);
assertEq(relistingIdFromEvent, nftDealers.totalListings());
// event emits: NFT_Dealers_Listed(seller, listingsCounter=3) NOW THEY DIVERGE
​
// A buyer sees the event with listingId=3 and tries to buy
uint256 listingIdFromEvent = 3;
bool wrongListingIsActive;
(, , , , wrongListingIsActive) = nftDealers.s_listings(
listingIdFromEvent
);
​
// s_listings[3] does not exist so isActive = false
assertFalse(wrongListingIsActive);
​
// buy(3) will revert with ListingNotActive
usdc.mint(maliciusUser, 999e6);
vm.startPrank(maliciusUser);
usdc.approve(address(nftDealers), 999e6);
vm.expectRevert(
abi.encodeWithSelector(
NFTDealers.ListingNotActive.selector,
listingIdFromEvent
)
);
nftDealers.buy(listingIdFromEvent);
vm.stopPrank();
​
// The listing actually exists but under key tokenId=1, not listingId=3
uint32 correctListingPrice;
bool correctListingIsActive;
(, correctListingPrice, , , correctListingIsActive) = nftDealers
.s_listings(1);
assertTrue(correctListingIsActive);
assertEq(correctListingPrice, 999e6);
}

Recommended Mitigation

Emit _tokenId instead of listingsCounter in the NFT_Dealers_Listed event, so that the listingId in the event always matches the key used in s_listings. This ensures that any buyer or off-chain service relying on the event can directly use the emitted value to call buy, cancelListing, or updatePrice without any mismatch.

- event NFT_Dealers_ListingCanceled(uint256 listingId);
+ event NFT_Dealers_ListingCanceled(uint256 tokenId);
​
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] =
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);
}

Support

FAQs

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

Give us feedback!