NFT Dealers

First Flight #58

Beginner FriendlyFoundry

100 EXP

View all submissions

Previous Next

Submission Details

Impact: medium

Likelihood: high

Listing IDs are inconsistent and can break integrations/accounting

Author Revealed upon completion

Description

Under a normal marketplace design, every newly created listing should have a single, consistent identifier that is used everywhere: in emitted events, in on-chain storage, in user-facing APIs like buy() / cancelListing() / updatePrice(), and in off-chain indexers that track listing history. That consistency is especially important when the same NFT is listed multiple times over its lifetime, because each listing instance is a separate accounting event.
The issue is that this contract mixes two different notions of “listing ID”. It increments listingsCounter and emits that value in NFT_Dealers_Listed, but it actually stores the listing in s_listings[_tokenId] and all listing-management functions (buy, cancelListing, collectUsdcFromSelling, updatePrice) operate on that mapping key, which is effectively the token ID, not the emitted/listing-counter ID. As a result, the second time the same NFT is listed, totalListings() becomes 2 and the event reports listing ID 2, but the actual active listing still lives at storage key 1 if the token ID is 1. This breaks integration assumptions and historical accounting.

uint32 listingsCounter = 0;

mapping(uint256 => Listing) public s_listings;

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++;

// @> listing is stored by tokenId

s_listings[_tokenId] =

Listing({seller: msg.sender, price: _price, nft: address(this), tokenId: _tokenId, isActive: true});

// @> but event emits listingsCounter as "listingId"

emit NFT_Dealers_Listed(msg.sender, listingsCounter);

}

function buy(uint256 _listingId) external payable {

// @> _listingId is treated as a key into s_listings

Listing memory listing = s_listings[_listingId];

if (!listing.isActive) revert ListingNotActive(_listingId);

...

}

Risk

Likelihood: High

This occurs whenever a token is listed, canceled, and then listed again, because listingsCounter increments to a new value while the storage slot remains the same s_listings[tokenId].
This occurs for any off-chain UI, indexer, or accounting system that trusts NFT_Dealers_Listed(..., listingId) or totalListings() as the canonical identifier for later calls like buy(listingId) or cancelListing(listingId).

Impact: Medium

Users and integrations can attempt to buy or manage a listing ID that appears valid from events/counters but does not actually exist in storage, causing failed transactions and broken UX.
Historical listing/accounting data becomes unreliable because multiple logically distinct listings of the same token are collapsed into one storage slot, while totalListings() suggests they are separate records.

Proof of Concept

Add import {console2} from "forge-std/console2.sol"; at the top of NFTDealersTest.t.sol.
Copy the code below to NFTDealersTest contract.
Run command forge test --mt testListingIdsAreInconsistentAndBreakIntegrationsAccounting -vv --via-ir.

function testListingIdsAreInconsistentAndBreakIntegrationsAccounting() public revealed whitelisted {

uint256 tokenId = 1;

uint32 firstPrice = 1000e6;

uint32 secondPrice = 1500e6;

// Mint token #1

mintNFTForTesting();

// ------------------------------------------------------------

// First listing of token #1

// ------------------------------------------------------------

vm.startPrank(userWithCash);

nftDealers.list(tokenId, firstPrice);

console2.log("After first list - totalListings:", nftDealers.totalListings());

console2.log("After first list - totalActiveListings:", nftDealers.totalActiveListings());

(

address sellerAfterFirstList,

uint32 storedPriceAfterFirstList,

address nftAfterFirstList,

uint256 storedTokenIdAfterFirstList,

bool isActiveAfterFirstList

) = nftDealers.s_listings(tokenId);

console2.log("Storage key used for active listing after first list:", tokenId);

console2.log("Stored seller after first list:", sellerAfterFirstList);

console2.log("Stored price after first list:", uint256(storedPriceAfterFirstList));

console2.log("Stored nft after first list:", nftAfterFirstList);

console2.log("Stored tokenId after first list:", storedTokenIdAfterFirstList);

console2.log("Stored active flag after first list:", isActiveAfterFirstList ? uint256(1) : uint256(0));

assertEq(nftDealers.totalListings(), 1);

assertEq(nftDealers.totalActiveListings(), 1);

assertEq(storedTokenIdAfterFirstList, tokenId);

assertEq(storedPriceAfterFirstList, firstPrice);

assertTrue(isActiveAfterFirstList);

// Cancel the first listing

nftDealers.cancelListing(tokenId);

console2.log("After cancel - totalListings:", nftDealers.totalListings());

console2.log("After cancel - totalActiveListings:", nftDealers.totalActiveListings());

(, uint32 storedPriceAfterCancel,, uint256 storedTokenIdAfterCancel, bool isActiveAfterCancel) =

nftDealers.s_listings(tokenId);

console2.log("Stored price after cancel:", uint256(storedPriceAfterCancel));

console2.log("Stored tokenId after cancel:", storedTokenIdAfterCancel);

console2.log("Stored active flag after cancel:", isActiveAfterCancel ? uint256(1) : uint256(0));

assertEq(nftDealers.totalListings(), 1, "historical listing count remains 1 after cancel");

assertEq(nftDealers.totalActiveListings(), 0, "no active listings after cancel");

assertFalse(isActiveAfterCancel, "listing at storage key tokenId should now be inactive");

// ------------------------------------------------------------

// Second listing of the SAME token #1

// ------------------------------------------------------------

nftDealers.list(tokenId, secondPrice);

vm.stopPrank();

console2.log("After second list - totalListings:", nftDealers.totalListings());

console2.log("After second list - totalActiveListings:", nftDealers.totalActiveListings());

(

address sellerAfterSecondList,

uint32 storedPriceAfterSecondList,

address nftAfterSecondList,

uint256 storedTokenIdAfterSecondList,

bool isActiveAfterSecondList

) = nftDealers.s_listings(tokenId);

console2.log("Active listing still stored at key:", tokenId);

console2.log("Stored seller after second list:", sellerAfterSecondList);

console2.log("Stored price after second list:", uint256(storedPriceAfterSecondList));

console2.log("Stored nft after second list:", nftAfterSecondList);

console2.log("Stored tokenId after second list:", storedTokenIdAfterSecondList);

console2.log("Stored active flag after second list:", isActiveAfterSecondList ? uint256(1) : uint256(0));

// There is no distinct storage entry for "listingId = 2"

(address sellerAtKey2, uint32 priceAtKey2, address nftAtKey2, uint256 tokenIdAtKey2, bool isActiveAtKey2) =

nftDealers.s_listings(2);

console2.log("Storage entry at key 2 - seller:", sellerAtKey2);

console2.log("Storage entry at key 2 - price:", uint256(priceAtKey2));

console2.log("Storage entry at key 2 - nft:", nftAtKey2);

console2.log("Storage entry at key 2 - tokenId:", tokenIdAtKey2);

console2.log("Storage entry at key 2 - active flag:", isActiveAtKey2 ? uint256(1) : uint256(0));

// Core inconsistency:

// totalListings() == 2 suggests there are two distinct listing IDs,

// but the active listing still lives at storage key 1, and key 2 is empty.

assertEq(nftDealers.totalListings(), 2, "contract reports two listings have been created");

assertEq(nftDealers.totalActiveListings(), 1, "there should be one currently active listing");

assertEq(storedTokenIdAfterSecondList, tokenId, "active listing is still keyed by tokenId");

assertEq(storedPriceAfterSecondList, secondPrice, "second listing overwrote the same storage slot");

assertTrue(isActiveAfterSecondList, "tokenId key remains the active listing slot");

assertEq(sellerAtKey2, address(0), "no actual listing record exists at key 2");

assertEq(priceAtKey2, 0, "no actual listing record exists at key 2");

assertEq(tokenIdAtKey2, 0, "no actual listing record exists at key 2");

assertFalse(isActiveAtKey2, "listingId 2 is not a valid active storage entry");

// ------------------------------------------------------------

// Integration failure:

// Off-chain systems that trust "listingId = 2" would call buy(2),

// but the contract actually expects tokenId/storage key 1.

// ------------------------------------------------------------

vm.startPrank(userWithEvenMoreCash);

usdc.approve(address(nftDealers), secondPrice);

vm.expectRevert(abi.encodeWithSelector(NFTDealers.ListingNotActive.selector, 2));

nftDealers.buy(2);

// Sanity: buying with storage key 1 succeeds.

nftDealers.buy(1);

vm.stopPrank();

console2.log("Owner of token after buy(1):", nftDealers.ownerOf(tokenId));

assertEq(

nftDealers.ownerOf(tokenId),

userWithEvenMoreCash,

"only buy(1) succeeds because the contract uses tokenId as the real listing key"

);

}

Output:

[⠊] Compiling...

No files changed, compilation skipped

Ran 1 test for test/NFTDealersTest.t.sol:NFTDealersTest

[PASS] testListingIdsAreInconsistentAndBreakIntegrationsAccounting() (gas: 537885)

Logs:

After first list - totalListings: 1

After first list - totalActiveListings: 1

Storage key used for active listing after first list: 1

Stored seller after first list: 0x22CdC71E987473D657FCe79C9C0C0B1A62148056

Stored price after first list: 1000000000

Stored nft after first list: 0x2e234DAe75C793f67A35089C9d99245E1C58470b

Stored tokenId after first list: 1

Stored active flag after first list: 1

After cancel - totalListings: 1

After cancel - totalActiveListings: 0

Stored price after cancel: 1000000000

Stored tokenId after cancel: 1

Stored active flag after cancel: 0

After second list - totalListings: 2

After second list - totalActiveListings: 1

Active listing still stored at key: 1

Stored seller after second list: 0x22CdC71E987473D657FCe79C9C0C0B1A62148056

Stored price after second list: 1500000000

Stored nft after second list: 0x2e234DAe75C793f67A35089C9d99245E1C58470b

Stored tokenId after second list: 1

Stored active flag after second list: 1

Storage entry at key 2 - seller: 0x0000000000000000000000000000000000000000

Storage entry at key 2 - price: 0

Storage entry at key 2 - nft: 0x0000000000000000000000000000000000000000

Storage entry at key 2 - tokenId: 0

Storage entry at key 2 - active flag: 0

Owner of token after buy(1): 0x533575789af8F38A73C7747E36C17C1835FDF44a

Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 4.23ms (1.87ms CPU time)

Ran 1 test suite in 15.24ms (4.23ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

Recommended Mitigation

The contract should adopt one canonical listing identifier model and use it consistently everywhere.
Use listingsCounter as the actual storage key and store tokenId as data inside the Listing struct.
All user-facing functions should take that true listingId and multiple listings of the same token can coexist historically without overwriting one another.

- mapping(uint256 => Listing) public s_listings;

+ mapping(uint256 => Listing) public s_listings;

+ mapping(uint256 => uint256) public activeListingIdByTokenId;

struct Listing {

address seller;

uint32 price;

address nft;

uint256 tokenId;

bool isActive;

}

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");

+ uint256 currentListingId = activeListingIdByTokenId[_tokenId];

+ require(!s_listings[currentListingId].isActive, "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});

+ activeListingIdByTokenId[_tokenId] = listingsCounter;

emit NFT_Dealers_Listed(msg.sender, listingsCounter);

}

function buy(uint256 _listingId) external payable {

Listing memory listing = s_listings[_listingId];

if (!listing.isActive) revert ListingNotActive(_listingId);

...

s_listings[_listingId].isActive = false;

+ activeListingIdByTokenId[listing.tokenId] = 0;

}

function cancelListing(uint256 _listingId) external {

Listing memory listing = s_listings[_listingId];

if (!listing.isActive) revert ListingNotActive(_listingId);

require(listing.seller == msg.sender, "Only seller can cancel listing");

s_listings[_listingId].isActive = false;

activeListingsCounter--;

+ activeListingIdByTokenId[listing.tokenId] = 0;

...

}

Rewards breakdown

nSLOC

253

This contest has a flat reward structure with the following payouts:

High

100 EXP

Medium

20 EXP

Low

2 EXP

Live

The contest is live. Earn rewards by submitting a finding.

Judging

Submissions are being carefully reviewed by our judges.

View all submissions

Appeals

This is your time to appeal against judgements on your submissions.

Appeals Review

Appeals are being carefully reviewed by our judges.

Rewards Distribution

The contest is complete and the rewards are being distributed.

Support

FAQs

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

Give us feedback!