NFT Dealers
First Flight #58
Beginner FriendlyFoundry
100 EXP
View results
Previous Next
Submission Details
Severity: medium
Valid

list() stores by tokenId but emits listingsCounter, causing buy() to target wrong NFTs

oxeugenio

Root + Impact

Description

  • list() uses _tokenId as the mapping key for s_listings but emits the auto-incremented listingsCounter in the NFT_Dealers_Listed event.

  • All other functions (buy, cancelListing, collectUsdcFromSelling, updatePrice) take a _listingId parameter and look up s_listings[_listingId]. When tokenId != listingsCounter, the listing ID from the event does not match the actual storage key.

function list(uint256 _tokenId, uint32 _price) external onlyWhitelisted {
listingsCounter++;
activeListingsCounter++;
s_listings[_tokenId] = Listing({...}); // @> stored at tokenId
emit NFT_Dealers_Listed(msg.sender, listingsCounter); // @> event says listingsCounter
}
​
function buy(uint256 _listingId) external payable {
Listing memory listing = s_listings[_listingId]; // @> reads by _listingId
if (!listing.isActive) revert ListingNotActive(_listingId);
}

Risk

Likelihood:

  • This occurs whenever NFTs are not listed in sequential tokenId order starting from 1 — for example if tokenId 5 is listed first, listingsCounter=1 but data is at s_listings[5]

  • Any frontend or integration reading the emitted event will get the wrong ID

Impact:

  • Buyers calling buy(listingId) using the event data will read an empty or wrong mapping slot, either reverting or purchasing a completely different NFT

  • cancelListing and updatePrice also fail for the same reason, locking seller's collateral

  • The marketplace is fundamentally broken for any non-sequential listing order

Proof of Concept

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.34;
​
import {Test, console2} from "forge-std/Test.sol";
import {NFTDealers} from "../src/NFTDealers.sol";
import {ERC20} from "@openzeppelin/contracts/token/ERC20/ERC20.sol";
​
contract MockUSDC is ERC20 {
constructor() ERC20("USD Coin", "USDC") {}
function mint(address to, uint256 amount) external { _mint(to, amount); }
function decimals() public pure override returns (uint8) { return 6; }
}
​
contract ListingIdMismatchTest is Test {
NFTDealers dealers;
MockUSDC usdc;
address owner = makeAddr("owner");
address bob = makeAddr("bob");
address buyer = makeAddr("buyer");
uint256 constant LOCK = 20e6;
​
function setUp() public {
usdc = new MockUSDC();
dealers = new NFTDealers(owner, address(usdc), "Dealers", "DEAL", "ipfs://", LOCK);
vm.startPrank(owner);
dealers.revealCollection();
dealers.whitelistWallet(bob);
vm.stopPrank();
​
usdc.mint(bob, 500e6);
vm.prank(bob);
usdc.approve(address(dealers), type(uint256).max);
usdc.mint(buyer, 500e6);
vm.prank(buyer);
usdc.approve(address(dealers), type(uint256).max);
​
// Mint tokenId 1 (not listed) so Bob gets tokenId 2
vm.prank(owner);
dealers.whitelistWallet(makeAddr("filler"));
usdc.mint(makeAddr("filler"), 100e6);
vm.prank(makeAddr("filler"));
usdc.approve(address(dealers), type(uint256).max);
vm.prank(makeAddr("filler"));
dealers.mintNft(); // tokenId 1
}
​
function test_listingIdMismatchBreaksBuy() public {
// Bob mints tokenId 2, lists it. listingsCounter becomes 1.
// Event emits listingId=1, but data stored at s_listings[2]
vm.startPrank(bob);
dealers.mintNft(); // tokenId 2
dealers.list(2, 50e6);
vm.stopPrank();
​
// Buyer reads event "listingId=1" and calls buy(1) — REVERTS
vm.prank(buyer);
vm.expectRevert(abi.encodeWithSelector(NFTDealers.ListingNotActive.selector, 1));
dealers.buy(1);
​
// Must use tokenId=2 directly to buy (bypassing event system)
vm.prank(buyer);
dealers.buy(2);
assertEq(dealers.ownerOf(2), buyer);
}
}

Recommended Mitigation

Use listingsCounter consistently as the mapping key:

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] =
+ s_listings[listingsCounter] =
Listing({seller: msg.sender, price: _price, nft: address(this), tokenId: _tokenId, isActive: true});
emit NFT_Dealers_Listed(msg.sender, listingsCounter);
}
Updates

Lead Judging Commences

rubik0n Lead Judge 16 days ago
Submission Judgement Published
Validated
Assigned finding tags:

listing-id-mismatch

Support

FAQs

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

Give us feedback!