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

Inconsistent mapping key between listing stored (mint) and get function causes all listings to be permanently unreachable

Author Revealed upon completion

Inconsistent mapping key between NFTDealers::list() and NFTDealers::buy(), NFTDealers::cancelListing() and any other function that need to get nft data causes all listings to be permanently unreachable

Description

In NFTDealers::list(), listings are stored in s_listings using _tokenId as the mapping key, but the NFT_Dealers_Listed event emits listingsCounter as the listing identifier. When a buyer calls NFTDealers::buy() using the listingId obtained from the event, s_listings[_listingId] returns an empty struct since the actual data is stored under _tokenId. The same inconsistency propagates to cancelListing() and collectUsdcFromSelling().

// stores with tokenId as key
s_listings[_tokenId] = Listing({...});
​
// but emits listingsCounter as identifier
emit NFT_Dealers_Listed(msg.sender, listingsCounter); // @>
​
​
// buy() lookups with listingId from event → always empty
Listing memory listing = s_listings[_listingId]; // @>
if (!listing.isActive) revert ListingNotActive(_listingId); // always reverts

Risk

Any buyer relying on the emitted listingId to call NFCDealers::buy() will always receive an empty listing causing the transaction to always revert with ListingNotActive. This effectively **breaks the core marketplace functionality, **no NFT can be purchased through the intended flow.

Likelihood: HIGH

  • occur when minting NFT

Impact: HIGH

  • when user call function that need to get data of nft such as buy(), user assume get with listingId, however in the NFTDealers::mintNft() contract store with _tokenId

Proof of Concept

1. User 1 mint 2 nft, but only list 1

2. User 2 mint 1 nft and list em

3. Form user 2 list call successfull, event emited that listingId is 2

4. Buyer want to buy NFT that listed by user 2

5. Because inconsistency of mapping on storing in list and get, log from other func, become s_lisitngs[2] is on default state

function test_inconsistentMappingKey() public whitelisted richWhitelisted revealed {
// Approve
vm.prank(userWithCash);
usdc.approve(address(nftDealers), type(uint256).max);
​
vm.prank(userWithEvenMoreCash);
usdc.approve(address(nftDealers), type(uint256).max);
​
// userWithEvenMoreCash mint 2 nft, list only 1
vm.startPrank(userWithEvenMoreCash);
nftDealers.mintNft(); // tokenId -> 1
nftDealers.mintNft(); // tokenId -> 2
nftDealers.list(1, 100e6); // -> s_listings 1
// emited NFT_Dealers_Listed(msg.sender, listingsCounter -> 1);
vm.stopPrank();
​
// userWithCash mint and list
vm.startPrank(userWithCash);
nftDealers.mintNft(); // tokenId -> 3
nftDealers.list(3, 100e6); // -> s_listings 3
// emited NFT_Dealers_Listed(msg.sender, listingsCounter -> 2);
vm.stopPrank();
​
// user that want to buy NFT that listed by userWithCash wich logged as 2 (listingCounter), expect to get buy(uint256 _listingId) or any other that have _lisitngId params, with 2
​
/**
* but the reality when
* Listing memory listing = s_listings[_listingId];
* the s_lisitng[2] is nothing, becauce here is the state that we currently on
* nftDealers.list(1, 100e6); // -> s_listings 1
* nftDealers.list(3, 100e6); // -> s_listings 3
*/
​
address buyer = makeAddr("buyer");
​
usdc.mint(buyer, 100e6);
​
vm.prank(buyer);
vm.expectRevert();
nftDealers.buy(2);
}

Recommended Mitigation

Use listingsCounter as the consistent mapping key across all functions:

function list(uint256 _tokenId, uint32 _price) external onlyWhitelisted {
listingsCounter++;
activeListingsCounter++;
​
- s_listings[_tokenId] = Listing({
+ 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!