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

Event emits `listingsCounter` but storage uses `tokenId` as key, causing buyers to purchase the wrong NFT at the wrong price

Author Revealed upon completion

Description

When a seller lists an NFT, the NFT_Dealers_Listed event should emit the correct identifier that a buyer needs to pass to buy(). Frontends and indexers rely on this event to show available listings and route purchases.

list() stores the listing at s_listings[_tokenId] but emits listingsCounter in the event. buy() uses its _listingId parameter to index s_listings. Whenever listingsCounter diverges from tokenId — which happens as soon as any minted NFT is not immediately listed — a buyer using the event ID calls buy() with the wrong key and silently purchases a different listing at a different price.

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");
​
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);
// Storage key: _tokenId. Event ID: listingsCounter. These diverge.
}
​
function buy(uint256 _listingId) external payable {
@> Listing memory listing = s_listings[_listingId];
// _listingId indexes s_listings — buyer must pass tokenId, not listingsCounter

Risk

Likelihood:

  • Every time a user mints an NFT without immediately listing it, tokenIdCounter advances but listingsCounter does not. From that point on, every event ID is offset from the actual storage key.

  • This is normal marketplace usage — users mint and list at different times. The divergence grows with every unlisted mint.

Impact:

  • A buyer who uses the event ID to call buy() silently purchases a different NFT at a different price. With a broad USDC approval, the buyer pays up to 50x more than expected with no revert or warning.

  • The buyer receives the wrong NFT. The intended seller's listing remains unsold. Both parties are harmed — the buyer overpays for an unwanted asset, and the intended seller misses the sale.

Proof of Concept

Alice mints tokenId 1 but doesn't list. Bob mints tokenId 2 and lists at 500 USDC — the event emits listingsCounter = 1. Charlie mints tokenId 3 and lists at 10 USDC — the event emits listingsCounter = 2. A buyer sees Charlie's event (ID 2, 10 USDC) and calls buy(2) expecting the cheap NFT. But s_listings[2] is Bob's 500 USDC listing. The buyer pays 500 USDC instead of 10 (50x overpayment) and receives Bob's tokenId 2 instead of Charlie's tokenId 3. No revert, no warning — a silent wrong purchase.

function test_poc_wrongPurchaseDueToIdMismatch() public {
// Alice mints tokenId 1 but does NOT list
vm.startPrank(alice);
usdc.approve(address(nftDealers), LOCK_AMOUNT);
nftDealers.mintNft(); // tokenId 1
vm.stopPrank();
​
// Bob mints tokenId 2, lists at 500 USDC
// s_listings[2] = Bob's listing, event emits listingsCounter = 1
vm.startPrank(bob);
usdc.approve(address(nftDealers), LOCK_AMOUNT);
nftDealers.mintNft();
nftDealers.list(2, BOB_PRICE);
vm.stopPrank();
​
// Charlie mints tokenId 3, lists at 10 USDC
// s_listings[3] = Charlie's listing, event emits listingsCounter = 2
vm.startPrank(charlie);
usdc.approve(address(nftDealers), LOCK_AMOUNT);
nftDealers.mintNft();
nftDealers.list(3, CHARLIE_PRICE);
vm.stopPrank();
​
// Buyer sees Charlie's event (ID 2, 10 USDC), calls buy(2)
// But s_listings[2] is Bob's 500 USDC listing
vm.startPrank(buyer);
usdc.approve(address(nftDealers), 10_000e6);
nftDealers.buy(2);
vm.stopPrank();
​
// Buyer paid 500 USDC instead of 10 — got Bob's NFT instead of Charlie's
uint256 buyerPaid = 10_000e6 - usdc.balanceOf(buyer);
assertEq(buyerPaid, BOB_PRICE); // 500 USDC
assertEq(nftDealers.ownerOf(2), buyer); // wrong NFT
assertEq(nftDealers.ownerOf(3), charlie); // intended NFT still unsold
}

Recommended Mitigation

Use listingsCounter as the actual mapping key instead of tokenId. This makes the storage key match the emitted event ID, and each listing gets a unique key that doesn't collide when tokens are re-listed (also fixes F-003). This requires updating buy(), cancelListing(), collectUsdcFromSelling(), and updatePrice() to use the same key — but it's the correct structural fix.

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

Alternatively, as a minimal fix, emit _tokenId instead of listingsCounter in the event so the emitted ID matches the current storage key:

- 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!