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

Permanent loss of seller funds due to overwritten listings

Author Revealed upon completion

Description

  • Under the intended marketplace flow, when a sale completes via buy(), the original seller should still be able to call collectUsdcFromSelling() later and receive their locked collateral plus the buyer’s payment minus fees, even if the NFT has already changed ownership. The right to collect sale proceeds should therefore remain tied to the completed sale record, not to whatever happens to be the NFT’s latest active listing state.

  • The issue is that the contract stores listings in s_listings[_tokenId], so the mapping key is the NFT’s tokenId rather than a unique listing ID. After a buyer purchases an NFT, the sale record remains stored under that token ID with isActive = false. If the new owner immediately lists the same NFT again, list() completely overwrites s_listings[_tokenId] with the new seller and new price. As a result, the original seller’s sale record is destroyed before they collect, and onlySeller(_listingId) will permanently reject them because the stored seller address now belongs to the new owner.

mapping(uint256 => Listing) public s_listings;
​
modifier onlySeller(uint256 _listingId) {
require(s_listings[_listingId].seller == msg.sender, "Only seller can call this function");
_;
}
​
function list(uint256 _tokenId, uint32 _price) external onlyWhitelisted {
...
// @> storage key is tokenId, not a unique listingId
s_listings[_tokenId] =
Listing({seller: msg.sender, price: _price, nft: address(this), tokenId: _tokenId, isActive: true});
...
}
​
function buy(uint256 _listingId) external payable {
Listing memory listing = s_listings[_listingId];
...
_safeTransfer(listing.seller, msg.sender, listing.tokenId, "");
s_listings[_listingId].isActive = false; // @> old sale data still lives under tokenId
...
}
​
function collectUsdcFromSelling(uint256 _listingId) external onlySeller(_listingId) {
Listing memory listing = s_listings[_listingId];
...
}

Risk

Likelihood: High

  • This occurs whenever a seller completes a sale but does not call collectUsdcFromSelling() before the buyer relists the same NFT, because the new list() call overwrites the old storage entry at the same tokenId key.

  • This occurs during normal marketplace usage because immediate relisting by the new owner is a standard secondary-market action, while seller collection is intentionally deferred into a separate transaction.

Impact: High

  • The original seller can permanently lose access to the proceeds of a completed sale, because the only authorization check for collectUsdcFromSelling() now points to the new seller’s address. [contracts | Txt]

  • Sale accounting becomes unsafe because one token’s future listing activity can destroy the metadata required to settle an earlier completed sale.

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 testPermanentLossOfSellerFundsDueToOverwrittenListings -vv --via-ir.

function testPermanentLossOfSellerFundsDueToOverwrittenListings() public revealed whitelisted {
uint256 tokenId = 1;
uint32 firstSalePrice = 1000e6;
uint32 relistPrice = 1500e6;
​
uint256 feeOnFirstSale = nftDealers.calculateFees(firstSalePrice);
uint256 originalSellerExpectedPayout = uint256(firstSalePrice) + nftDealers.lockAmount() - feeOnFirstSale;
​
// ------------------------------------------------------------
// Phase 1: original seller mints and lists token #1
// ------------------------------------------------------------
mintAndListNFTForTesting(tokenId, firstSalePrice);
​
console2.log("Original seller:", userWithCash);
console2.log("Initial buyer / future relister:", userWithEvenMoreCash);
console2.log("Original sale price:", uint256(firstSalePrice));
console2.log("Expected original seller payout:", originalSellerExpectedPayout);
​
// Whitelist the buyer so the current implementation allows them to relist later.
// This is test setup only and not part of the overwritten-listing bug itself.
vm.prank(owner);
nftDealers.whitelistWallet(userWithEvenMoreCash);
​
// ------------------------------------------------------------
// Phase 2: buyer purchases token #1
// ------------------------------------------------------------
vm.startPrank(userWithEvenMoreCash);
usdc.approve(address(nftDealers), firstSalePrice);
nftDealers.buy(tokenId);
vm.stopPrank();
​
(
address sellerAfterFirstSale,
uint32 priceAfterFirstSale,,
uint256 storedTokenIdAfterFirstSale,
bool isActiveAfterFirstSale
) = nftDealers.s_listings(tokenId);
​
console2.log("Owner after first sale:", nftDealers.ownerOf(tokenId));
console2.log("Stored seller after first sale:", sellerAfterFirstSale);
console2.log("Stored price after first sale:", uint256(priceAfterFirstSale));
console2.log("Stored tokenId after first sale:", storedTokenIdAfterFirstSale);
console2.log("Stored active flag after first sale:", isActiveAfterFirstSale ? uint256(1) : uint256(0));
console2.log("Contract USDC balance after first sale:", usdc.balanceOf(address(nftDealers)));
console2.log("Original seller USDC balance after first sale:", usdc.balanceOf(userWithCash));
​
assertEq(nftDealers.ownerOf(tokenId), userWithEvenMoreCash, "sanity: buyer now owns token");
assertEq(
sellerAfterFirstSale, userWithCash, "sanity: old listing still points to original seller before overwrite"
);
assertEq(priceAfterFirstSale, firstSalePrice, "sanity: old sale price still present before overwrite");
assertFalse(isActiveAfterFirstSale, "sanity: original listing becomes inactive after buy");
assertEq(usdc.balanceOf(userWithCash), 0, "sanity: original seller has not collected yet");
​
// ------------------------------------------------------------
// Phase 3: new owner immediately relists the same token
// This overwrites s_listings[tokenId] entirely.
// ------------------------------------------------------------
vm.prank(userWithEvenMoreCash);
nftDealers.list(tokenId, relistPrice);
​
(
address sellerAfterOverwrite,
uint32 priceAfterOverwrite,,
uint256 storedTokenIdAfterOverwrite,
bool isActiveAfterOverwrite
) = nftDealers.s_listings(tokenId);
​
console2.log("Owner after relist:", nftDealers.ownerOf(tokenId));
console2.log("Stored seller after overwrite:", sellerAfterOverwrite);
console2.log("Stored price after overwrite:", uint256(priceAfterOverwrite));
console2.log("Stored tokenId after overwrite:", storedTokenIdAfterOverwrite);
console2.log("Stored active flag after overwrite:", isActiveAfterOverwrite ? uint256(1) : uint256(0));
​
// Core bug signal:
// the completed sale record for the original seller has been overwritten
assertEq(sellerAfterOverwrite, userWithEvenMoreCash, "BUG: original seller address was overwritten");
assertEq(priceAfterOverwrite, relistPrice, "BUG: original sale price was overwritten");
assertEq(storedTokenIdAfterOverwrite, tokenId, "storage slot is still keyed only by tokenId");
assertTrue(isActiveAfterOverwrite, "new owner's listing is now the only record at this key");
​
// ------------------------------------------------------------
// Phase 4: original seller can no longer collect proceeds from the completed sale
// ------------------------------------------------------------
vm.startPrank(userWithCash);
vm.expectRevert("Only seller can call this function");
nftDealers.collectUsdcFromSelling(tokenId);
vm.stopPrank();
​
uint256 originalSellerBalanceAfterFailure = usdc.balanceOf(userWithCash);
uint256 contractBalanceAfterFailure = usdc.balanceOf(address(nftDealers));
​
console2.log("Original seller balance after failed collect:", originalSellerBalanceAfterFailure);
console2.log("Contract balance after failed collect:", contractBalanceAfterFailure);
console2.log("Expected payout now locked away from original seller:", originalSellerExpectedPayout);
​
// Strong end-to-end signal:
// the completed sale generated claimable funds, but the original seller permanently lost authorization.
assertEq(
originalSellerBalanceAfterFailure,
0,
"BUG: original seller still cannot access proceeds from their completed sale"
);
assertEq(
contractBalanceAfterFailure,
nftDealers.lockAmount() + uint256(firstSalePrice),
"contract still holds the original sale funds while the rightful seller is locked out"
);
}

Output:

[⠊] Compiling...
No files changed, compilation skipped
​
Ran 1 test for test/NFTDealersTest.t.sol:NFTDealersTest
[PASS] testPermanentLossOfSellerFundsDueToOverwrittenListings() (gas: 538290)
Logs:
Original seller: 0x22CdC71E987473D657FCe79C9C0C0B1A62148056
Initial buyer / future relister: 0x533575789af8F38A73C7747E36C17C1835FDF44a
Original sale price: 1000000000
Expected original seller payout: 1010000000
Owner after first sale: 0x533575789af8F38A73C7747E36C17C1835FDF44a
Stored seller after first sale: 0x22CdC71E987473D657FCe79C9C0C0B1A62148056
Stored price after first sale: 1000000000
Stored tokenId after first sale: 1
Stored active flag after first sale: 0
Contract USDC balance after first sale: 1020000000
Original seller USDC balance after first sale: 0
Owner after relist: 0x533575789af8F38A73C7747E36C17C1835FDF44a
Stored seller after overwrite: 0x533575789af8F38A73C7747E36C17C1835FDF44a
Stored price after overwrite: 1500000000
Stored tokenId after overwrite: 1
Stored active flag after overwrite: 1
Original seller balance after failed collect: 0
Contract balance after failed collect: 1020000000
Expected payout now locked away from original seller: 1010000000
​
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 4.39ms (2.09ms CPU time)
​
Ran 1 test suite in 16.55ms (4.39ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

Recommended Mitigation

  • The contract should stop using tokenId as the canonical key for seller settlement records.

  • Each listing should have a unique listing ID, and completed sale data should remain immutable until the rightful seller has claimed their proceeds.

- 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;
+ bool wasSold;
+ bool proceedsClaimed;
}
​
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);
require(listing.seller != msg.sender, "Seller cannot buy their own NFT");
​
activeListingsCounter--;
bool success = usdc.transferFrom(msg.sender, address(this), listing.price);
require(success, "USDC transfer failed");
​
_safeTransfer(listing.seller, msg.sender, listing.tokenId, "");
s_listings[_listingId].isActive = false;
+ s_listings[_listingId].wasSold = true;
+ activeListingIdByTokenId[listing.tokenId] = 0;
​
emit NFT_Dealers_Sold(msg.sender, listing.price);
}
​
function collectUsdcFromSelling(uint256 _listingId) external onlySeller(_listingId) {
Listing memory listing = s_listings[_listingId];
require(!listing.isActive, "Listing must be inactive to collect USDC");
+ require(listing.wasSold, "Listing was not sold");
+ require(!listing.proceedsClaimed, "Proceeds already claimed");
​
+ s_listings[_listingId].proceedsClaimed = true;
...
}

Support

FAQs

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

Give us feedback!