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

Buyer re-listing an NFT overwrites `s_listings[tokenId]`, permanently blocking the original seller from collecting sale proceeds and collateral

Author Revealed upon completion

Root + Impact

Description

The s_listings mapping (L50) is keyed by tokenId, meaning only one listing record can exist per token at any time. After buy() (L141-155) sets s_listings[_listingId].isActive = false at L152 but leaves the seller field intact, the new owner (buyer) can call list() (L127-139) which overwrites the entire s_listings[tokenId] struct at L136-137 -- replacing the original seller address with the buyer's address. The original seller can then never call collectUsdcFromSelling() (L171-183) because the onlySeller modifier (L72-75) checks s_listings[_listingId].seller == msg.sender, which now points to the buyer.

The root cause is architectural: buy() defers seller payment to a separate collectUsdcFromSelling() call, but the listing data that collectUsdcFromSelling depends on is stored in a mapping keyed by tokenId -- a key that gets reused when the token is re-listed. The listingsCounter (L46) is incremented at L133 but is never used as a mapping key, so each new listing for the same token silently destroys the previous listing's data.

This is not merely a front-running concern. Re-listing a purchased NFT is normal marketplace behavior -- a buyer who wants to flip the NFT will naturally call list() after buying, unknowingly and irreversibly blocking the original seller from collecting.

The vulnerable flow in list():

function list(uint256 _tokenId, uint32 _price) external onlyWhitelisted { // @audit L127
require(_price >= MIN_PRICE, "Price must be at least 1 USDC"); // @audit L128
require(ownerOf(_tokenId) == msg.sender, "Not owner of NFT"); // @audit L129: buyer is now owner after buy()
require(s_listings[_tokenId].isActive == false, "NFT is already listed"); // @audit L130: true after buy() sets isActive=false
require(_price > 0, "Price must be greater than 0"); // @audit L131
​
listingsCounter++; // @audit L133: incremented but never used as key
activeListingsCounter++;
​
s_listings[_tokenId] = // @audit L136-137: OVERWRITES entire struct
Listing({seller: msg.sender, price: _price, nft: address(this), tokenId: _tokenId, isActive: true});
emit NFT_Dealers_Listed(msg.sender, listingsCounter);
}

And buy() which only sets isActive = false without finalizing payment:

function buy(uint256 _listingId) external payable { // @audit L141
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; // @audit L152: only sets isActive, seller field preserved
// but vulnerable to overwrite by list()
emit NFT_Dealers_Sold(msg.sender, listing.price);
}

Compare with cancelListing() (L157-169), which pays the seller immediately and zeros collateralForMinting at L165-166, leaving no deferred state that could be overwritten.

Risk

Likelihood:

  • Re-listing a purchased NFT is standard marketplace behavior -- the overwrite occurs as part of a normal user flow, not just via a deliberate attack

  • The buyer only needs to be whitelisted (list() requires onlyWhitelisted at L127), which is a primary actor type in the protocol

  • The window between buy() and collectUsdcFromSelling() is unbounded -- the seller may not collect for hours or days, during which the buyer can re-list at any time

  • A malicious buyer can also front-run the seller's collectUsdcFromSelling transaction to guarantee the overwrite

Impact:

  • Original seller permanently loses sale proceeds (listing.price - fees) and minting collateral (collateralForMinting[tokenId]) -- both remain locked in the contract with no recovery mechanism

  • For a 100 USDC sale with 20 USDC collateral, the seller loses 119 USDC (proceeds + collateral minus fees)

  • The collateralForMinting[tokenId] (L51) is never zeroed by either buy() or the overwriting list(), so it persists as a ghost entry with no claimant

  • Every secondary sale in the marketplace is vulnerable -- this is not an edge case but a flaw in the core trading flow

Proof of Concept

Attack steps:

  1. Seller mints an NFT via mintNft() (locks 20 USDC collateral) and lists it for 100 USDC via list()

  2. Buyer purchases the NFT via buy(), which sets isActive = false at L152

  3. Buyer re-lists the same tokenId via list(), which overwrites s_listings[tokenId] at L136-137 with seller = buyer

  4. Original seller calls collectUsdcFromSelling(tokenId) -- reverts at onlySeller (L72-75) because s_listings[tokenId].seller is now the buyer

  5. Seller's 119 USDC (sale proceeds + collateral) is permanently locked in the contract

The following Foundry test demonstrates the full attack. Run with forge test --match-test test_relistOverwriteBlocksSellerCollection -vv:

// SPDX-License-Identifier: UNLICENSED
pragma solidity 0.8.34;
​
import {Test, console} from "forge-std/Test.sol";
import {NFTDealers} from "../src/NFTDealers.sol";
import {MockUSDC} from "../src/MockUSDC.sol";
​
contract H02PoCTest is Test {
NFTDealers public nftDealers;
MockUSDC public usdc;
​
address public owner = makeAddr("owner");
address public seller = makeAddr("seller");
address public buyer = makeAddr("buyer");
​
uint256 constant LOCK_AMOUNT = 20e6; // 20 USDC
​
function setUp() public {
usdc = new MockUSDC();
nftDealers = new NFTDealers(owner, address(usdc), "NFTDealers", "NFTD", "ipfs://test/", LOCK_AMOUNT);
​
vm.startPrank(owner);
nftDealers.revealCollection();
nftDealers.whitelistWallet(seller);
nftDealers.whitelistWallet(buyer);
vm.stopPrank();
​
usdc.mint(seller, 1000e6);
usdc.mint(buyer, 1000e6);
​
vm.prank(seller);
usdc.approve(address(nftDealers), type(uint256).max);
vm.prank(buyer);
usdc.approve(address(nftDealers), type(uint256).max);
}
​
function test_relistOverwriteBlocksSellerCollection() public {
// --- Step 1: Seller mints and lists tokenId 1 for 100 USDC ---
vm.prank(seller);
nftDealers.mintNft(); // tokenId 1, locks 20 USDC collateral
vm.prank(seller);
nftDealers.list(1, uint32(100e6));
​
// Confirm seller is recorded in the listing
(address listedSeller,,,, bool isActive) = nftDealers.s_listings(1);
assertEq(listedSeller, seller);
assertTrue(isActive);
​
// --- Step 2: Buyer purchases the NFT ---
vm.prank(buyer);
nftDealers.buy(1); // pays 100 USDC, receives NFT, isActive set to false
​
// After buy(): listing is inactive, but seller field still points to original seller
(address sellerAfterBuy,,,, bool isActiveAfterBuy) = nftDealers.s_listings(1);
assertEq(sellerAfterBuy, seller);
assertFalse(isActiveAfterBuy);
​
uint256 sellerBalBefore = usdc.balanceOf(seller);
​
// --- Step 3: Buyer re-lists the NFT (normal marketplace behavior) ---
// list() checks: ownerOf(1) == buyer (true), isActive == false (true) -> overwrites s_listings[1]
vm.prank(buyer);
nftDealers.list(1, uint32(200e6));
​
// s_listings[1].seller is now the buyer, original seller is erased
(address newSeller,,,,) = nftDealers.s_listings(1);
assertEq(newSeller, buyer);
​
// --- Step 4: Original seller tries to collect sale proceeds -> REVERTS ---
vm.prank(seller);
vm.expectRevert("Only seller can call this function");
nftDealers.collectUsdcFromSelling(1);
​
// Seller's balance unchanged - sale proceeds + collateral permanently locked
assertEq(usdc.balanceOf(seller), sellerBalBefore);
​
// --- Quantify the loss ---
// Seller paid 20 USDC collateral to mint, sold for 100 USDC, should receive:
// 100 USDC - 1% fee (1 USDC) + 20 USDC collateral = 119 USDC
// Instead: receives 0 USDC. Net loss = 119 USDC (proceeds) + 20 USDC (collateral from mint) = permanent loss
uint256 fees = nftDealers.calculateFees(100e6);
uint256 expectedPayout = 100e6 - fees + LOCK_AMOUNT;
console.log("Seller expected payout (USDC):", expectedPayout / 1e6);
console.log("Seller actual payout (USDC): 0");
console.log("Permanent loss (USDC): ", expectedPayout / 1e6);
}
}

Recommended Mitigation

Finalize seller payment atomically inside buy(), eliminating the deferred collection step and the window for overwrite. This mirrors how cancelListing() (L157-169) already handles collateral returns immediately.

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;
+ // Finalize payment atomically -- no deferred state that can be overwritten
+ uint256 fees = _calculateFees(listing.price);
+ uint256 amountToSeller = listing.price - fees + collateralForMinting[listing.tokenId];
+ totalFeesCollected += fees;
+ collateralForMinting[listing.tokenId] = 0;
+ delete s_listings[_listingId];
+ usdc.safeTransfer(listing.seller, amountToSeller);
​
emit NFT_Dealers_Sold(msg.sender, listing.price);
}

This fix:

  1. Pays the seller (proceeds + collateral) in the same transaction as the sale -- no window for overwrite

  2. Deletes the listing via delete s_listings[_listingId], so subsequent list() calls create a clean entry

  3. Zeros collateralForMinting[listing.tokenId], preventing ghost collateral entries

  4. Also eliminates the need for a separate collectUsdcFromSelling() call for the buy flow, which additionally prevents the repeated-collection vulnerability (H-01) for sold listings

Support

FAQs

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

Give us feedback!