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

`s_listings` keyed by `tokenId` causes re-listing to overwrite seller, permanently locking sale proceeds

Author Revealed upon completion

Description

When a buyer purchases an NFT, the original seller should be able to call collectUsdcFromSelling to receive their sale proceeds and collateral. The onlySeller modifier checks that s_listings[listingId].seller == msg.sender.

s_listings is keyed by tokenId, not by a unique listing ID. When the new owner re-lists the same token, list() overwrites the entire Listing struct — including the seller field. The original seller's collectUsdcFromSelling call permanently reverts because onlySeller now points to the new owner.

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

Risk

Likelihood:

  • Every time a buyer re-lists the same tokenId before the original seller collects, the seller field is overwritten. This is normal marketplace usage — no malicious intent required.

  • The list() function only checks isActive == false, which is already the case after buy() sets it. There is no guard preventing overwrite of an uncollected listing.

Impact:

  • The original seller's sale proceeds and minting collateral are permanently locked in the contract. On a 100 USDC sale with 20 USDC collateral, the seller loses 119 USDC (proceeds + collateral - fee) with no recovery path.

  • There is no admin function to rescue locked funds. The seller cannot call collectUsdcFromSelling because onlySeller will always revert for them on that listing.

Proof of Concept

Alice mints an NFT (20 USDC collateral) and lists it at 100 USDC. Bob buys it — 100 USDC enters the contract. Before Alice calls collectUsdcFromSelling, Bob re-lists the same tokenId at 150 USDC. The s_listings[tokenId] struct is overwritten with Bob as the seller. Alice calls collectUsdcFromSelling and it reverts with "Only seller can call this function". Alice ends with 0 USDC — her 100 USDC proceeds and 20 USDC collateral are permanently locked.

function test_poc_relistOverwritesSeller() public {
uint256 tokenId = 1;
​
// Alice mints and lists at 100 USDC
vm.startPrank(alice);
usdc.approve(address(nftDealers), LOCK_AMOUNT);
nftDealers.mintNft();
nftDealers.list(tokenId, ALICE_PRICE);
vm.stopPrank();
​
// Bob buys Alice's NFT — 100 USDC enters the contract
vm.startPrank(bob);
usdc.approve(address(nftDealers), ALICE_PRICE);
nftDealers.buy(tokenId);
vm.stopPrank();
​
// Bob re-lists the same tokenId before Alice collects
vm.prank(bob);
nftDealers.list(tokenId, BOB_PRICE);
​
// Seller field is overwritten — Alice is gone
(address sellerAfter,,,,) = nftDealers.s_listings(tokenId);
assertEq(sellerAfter, bob);
​
// Alice tries to collect — reverts permanently
vm.prank(alice);
vm.expectRevert("Only seller can call this function");
nftDealers.collectUsdcFromSelling(tokenId);
​
// Alice ends with 0 USDC — proceeds + collateral locked forever
assertEq(usdc.balanceOf(alice), 0);
}

Recommended Mitigation

Use listingsCounter as the mapping key instead of tokenId. Each listing gets a unique ID, so re-listing the same token creates a new entry without overwriting the previous one. The original seller can still collect from their listing ID.

- mapping(uint256 => Listing) public s_listings;
+ mapping(uint256 => Listing) public s_listings; // key: listingsCounter, not tokenId
​
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] =
+ 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!