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

Listings are resolved by `tokenId` instead of `listingId`

Author Revealed upon completion

Root + Impact

the marketplace interface expects users to interact through listingId while listings are actually stored and resolved by tokenId, marketplace operations can execute against a different order than the one identified by the provided listing ID.

Description

  • The intended behavior is that each new listing should receive a unique listingId, and that this identifier should be the one used by buy(), cancelListing(), updatePrice(), and collectUsdcFromSelling() to operate on that specific order.

  • The issue is that list() increments listingsCounter but does not use that value as the mapping key. Instead, it stores the listing in s_listings[_tokenId]. As a result, the protocol appears to work with listingId, but in practice it resolves orders by tokenId. This allows a purchase using _listingId to point to a different listing than the one the user believes they are executing.

function list(uint256 _tokenId, uint32 _price) external onlyWhitelisted {
require(_price >= MIN_PRICE, "Price must be at least 1 USDC");
​
...
​
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);
}
​
function buy(uint256 _listingId) external payable {
@> Listing memory listing = s_listings[_listingId];
if (!listing.isActive) revert ListingNotActive(_listingId);
...
}

Risk

Likelihood: High

  • The issue is structural and follows directly from how listings are stored and resolved.

  • It manifests whenever the order of listing creation does not match the tokenId values used as the internal storage keys.

Impact: Medium

  • Purchase and listing-management operations may no longer correspond to the listingId exposed by the protocol to users and integrators.

  • This can cause failed purchases or execution against a different listing than the one the user expected.

Proof of Concept

The following test shows that the first listing created is for tokenId 2, but buy(1) does not resolve that first listing. Instead, it resolves s_listings[1], which contains the listing for tokenId 1.

function test_Buy_UsesTokenIdInsteadOfListingId() public revealed whitelisted {
uint256 priceListing1 = 2_000e6;
uint256 priceListing2 = 2_500e6;
​
usdc.mint(seller, LOCK_AMOUNT * 2);
vm.startPrank(seller);
usdc.approve(address(nftDealers), LOCK_AMOUNT * 2);
nftDealers.mintNft(); // tokenId 1
nftDealers.mintNft(); // tokenId 2
​
// The first listing created (listingId 1) is for tokenId 2.
nftDealers.list(2, uint32(priceListing1));
​
// The second listing created (listingId 2) is for tokenId 1.
nftDealers.list(1, uint32(priceListing2));
vm.stopPrank();
​
// The protocol reports that two listings have been created.
assertEq(nftDealers.totalListings(), 2);
​
// The first created listing should correspond to tokenId 2,
// but s_listings[1] actually returns the listing for tokenId 1.
(, uint32 storedPrice1, , uint256 storedTokenId1, ) = nftDealers.s_listings(1);
assertEq(storedTokenId1, 1);
assertEq(storedPrice1, priceListing2);
​
// s_listings[2] contains the listing for tokenId 2,
// even though that was the first listing created.
(, uint32 storedPrice2, , uint256 storedTokenId2, ) = nftDealers.s_listings(2);
assertEq(storedTokenId2, 2);
assertEq(storedPrice2, priceListing1);
​
// The buyer expects buy(1) to execute the first created listing,
// which was priced at 2,000 USDC.
usdc.mint(buyer, priceListing1);
vm.startPrank(buyer);
usdc.approve(address(nftDealers), priceListing1);
​
// Instead, buy(1) resolves s_listings[1], i.e. the listing for tokenId 1,
// which is priced at 2,500 USDC, so the purchase reverts.
vm.expectRevert();
nftDealers.buy(1);
​
vm.stopPrank();
}

Recommended Mitigation

Listings should be stored using listingsCounter as the actual mapping key, and all marketplace functions should operate on that identifier. If the protocol intends to allow only one active listing per NFT, that restriction should be enforced separately without conflating tokenId with listingId.

function list(uint256 _tokenId, uint32 _price) external onlyWhitelisted {
...
​
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);
}

Support

FAQs

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

Give us feedback!