Submission Details
Impact:
Likelihood:
Active listings are not escrowed: sellers can make listings stale
Author Revealed upon completion
Description
-
Under a normal marketplace design, creating a listing should either escrow the NFT into the marketplace contract or otherwise enforce that the listed asset cannot be transferred away while the listing remains active. That way, when a buyer calls
buy(), the marketplace can reliably deliver the NFT associated with the active listing. -
The issue is that
list()only records listing metadata ins_listings[_tokenId], but it does not escrow the NFT and does not add any transfer restriction while the listing is active. Because the seller continues to own the token directly through the underlying ERC721 implementation, they can transfer it away after listing. Once that happens, the listing still appears active on-chain, butbuy()later reverts because it tries to_safeTransfer()the token from the original seller address stored in the listing rather than from the token’s current 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");
require(_price > 0, "Price must be greater than 0");
listingsCounter++;
activeListingsCounter++;
// @> only marketplace metadata is stored
// @> NFT remains in seller custody and is not escrowed
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);
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");
// @> assumes listing.seller still owns the token
_safeTransfer(listing.seller, msg.sender, listing.tokenId, "");
s_listings[_listingId].isActive = false;
emit NFT_Dealers_Sold(msg.sender, listing.price);
}
Risk
Likelihood: High
-
This occurs whenever a seller lists an NFT and then uses the normal ERC721
transferFrom/safeTransferFrompath to move the token elsewhere, because nothing inlist()prevents further transfers by the current owner. -
This occurs during normal marketplace usage because listings are designed to remain in seller custody, so the stale-listing condition can be created without any unusual privileges beyond being the seller.
Impact: High
-
Buyers can encounter active listings that always revert at purchase time, causing denial of service, wasted gas, and broken marketplace UX.
-
The marketplace’s active listing count and on-chain listing state become unreliable because listings can remain active even though the marketplace no longer has a deliverable token associated with them.
Proof of Concept
-
Add
import {console2} from "forge-std/console2.sol";at the top ofNFTDealersTest.t.sol. -
Copy the code below to
NFTDealersTestcontract. -
Run command
forge test --mt testActiveListingsAreNotEscrowedAndCanBecomeStale -vv --via-ir.
function testActiveListingsAreNotEscrowedAndCanBecomeStale() public revealed whitelisted {
uint256 tokenId = 1;
uint32 listPrice = 1000e6;
address staleHolder = makeAddr("staleHolder");
// ------------------------------------------------------------
// Phase 1: seller mints and lists token #1
// ------------------------------------------------------------
mintAndListNFTForTesting(tokenId, listPrice);
(
address sellerBeforeTransfer,
uint32 storedPriceBeforeTransfer,
address nftBeforeTransfer,
uint256 storedTokenIdBeforeTransfer,
bool isActiveBeforeTransfer
) = nftDealers.s_listings(tokenId);
console2.log("Owner before external transfer:", nftDealers.ownerOf(tokenId));
console2.log("Stored seller before external transfer:", sellerBeforeTransfer);
console2.log("Stored price before external transfer:", uint256(storedPriceBeforeTransfer));
console2.log("Stored nft before external transfer:", nftBeforeTransfer);
console2.log("Stored tokenId before external transfer:", storedTokenIdBeforeTransfer);
console2.log("Stored active flag before external transfer:", isActiveBeforeTransfer ? uint256(1) : uint256(0));
console2.log("totalActiveListings before external transfer:", nftDealers.totalActiveListings());
assertEq(nftDealers.ownerOf(tokenId), userWithCash, "sanity: seller owns token before external transfer");
assertTrue(isActiveBeforeTransfer, "sanity: listing should be active before external transfer");
assertEq(nftDealers.totalActiveListings(), 1, "sanity: one active listing before external transfer");
// ------------------------------------------------------------
// Phase 2: seller transfers the listed NFT away using plain ERC721 transfer
// ------------------------------------------------------------
vm.prank(userWithCash);
nftDealers.transferFrom(userWithCash, staleHolder, tokenId);
(
address sellerAfterTransfer,
uint32 storedPriceAfterTransfer,
address nftAfterTransfer,
uint256 storedTokenIdAfterTransfer,
bool isActiveAfterTransfer
) = nftDealers.s_listings(tokenId);
console2.log("Owner after external transfer:", nftDealers.ownerOf(tokenId));
console2.log("Stored seller after external transfer:", sellerAfterTransfer);
console2.log("Stored price after external transfer:", uint256(storedPriceAfterTransfer));
console2.log("Stored nft after external transfer:", nftAfterTransfer);
console2.log("Stored tokenId after external transfer:", storedTokenIdAfterTransfer);
console2.log("Stored active flag after external transfer:", isActiveAfterTransfer ? uint256(1) : uint256(0));
console2.log("totalActiveListings after external transfer:", nftDealers.totalActiveListings());
// Core bug signal:
// token ownership moved, but the listing is still active and still points to the old seller
assertEq(nftDealers.ownerOf(tokenId), staleHolder, "token ownership changed outside the marketplace");
assertEq(sellerAfterTransfer, userWithCash, "listing still points to original seller");
assertTrue(isActiveAfterTransfer, "BUG: listing remains active after token left seller custody");
assertEq(nftDealers.totalActiveListings(), 1, "BUG: active listing count still includes stale listing");
// ------------------------------------------------------------
// Phase 3: buyer attempts to purchase the stale listing
// ------------------------------------------------------------
vm.startPrank(userWithEvenMoreCash);
usdc.approve(address(nftDealers), listPrice);
// buy() will attempt _safeTransfer(listing.seller, buyer, tokenId, "")
// but listing.seller no longer owns the token, so the purchase reverts.
// Revert with the error ERC721: transfer from incorrect owner,
// which confirms the root cause is the stale listing pointing to the wrong seller.
vm.expectRevert();
nftDealers.buy(tokenId);
vm.stopPrank();
(
address sellerAfterFailedBuy,
uint32 storedPriceAfterFailedBuy,
address nftAfterFailedBuy,
uint256 storedTokenIdAfterFailedBuy,
bool isActiveAfterFailedBuy
) = nftDealers.s_listings(tokenId);
console2.log("Owner after failed buy:", nftDealers.ownerOf(tokenId));
console2.log("Stored seller after failed buy:", sellerAfterFailedBuy);
console2.log("Stored price after failed buy:", uint256(storedPriceAfterFailedBuy));
console2.log("Stored nft after failed buy:", nftAfterFailedBuy);
console2.log("Stored tokenId after failed buy:", storedTokenIdAfterFailedBuy);
console2.log("Stored active flag after failed buy:", isActiveAfterFailedBuy ? uint256(1) : uint256(0));
console2.log("totalActiveListings after failed buy:", nftDealers.totalActiveListings());
// The listing is still stale and still active after the failed purchase.
assertEq(nftDealers.ownerOf(tokenId), staleHolder, "stale holder still owns the token after failed buy");
assertTrue(isActiveAfterFailedBuy, "BUG: failed buy does not clean up stale active listing");
assertEq(nftDealers.totalActiveListings(), 1, "BUG: stale listing still counted as active");
}
Output:
[⠊] Compiling...
No files changed, compilation skipped
Ran 1 test for test/NFTDealersTest.t.sol:NFTDealersTest
[PASS] testActiveListingsAreNotEscrowedAndCanBecomeStale() (gas: 513003)
Logs:
Owner before external transfer: 0x22CdC71E987473D657FCe79C9C0C0B1A62148056
Stored seller before external transfer: 0x22CdC71E987473D657FCe79C9C0C0B1A62148056
Stored price before external transfer: 1000000000
Stored nft before external transfer: 0x2e234DAe75C793f67A35089C9d99245E1C58470b
Stored tokenId before external transfer: 1
Stored active flag before external transfer: 1
totalActiveListings before external transfer: 1
Owner after external transfer: 0x928f16eb8A5F541De90d9aaF9893cEff4F8C0E8e
Stored seller after external transfer: 0x22CdC71E987473D657FCe79C9C0C0B1A62148056
Stored price after external transfer: 1000000000
Stored nft after external transfer: 0x2e234DAe75C793f67A35089C9d99245E1C58470b
Stored tokenId after external transfer: 1
Stored active flag after external transfer: 1
totalActiveListings after external transfer: 1
Owner after failed buy: 0x928f16eb8A5F541De90d9aaF9893cEff4F8C0E8e
Stored seller after failed buy: 0x22CdC71E987473D657FCe79C9C0C0B1A62148056
Stored price after failed buy: 1000000000
Stored nft after failed buy: 0x2e234DAe75C793f67A35089C9d99245E1C58470b
Stored tokenId after failed buy: 1
Stored active flag after failed buy: 1
totalActiveListings after failed buy: 1
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 3.72ms (1.12ms CPU time)
Ran 1 test suite in 11.94ms (3.72ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)
Recommended Mitigation
Escrow the NFT into the marketplace contract at listing time and transfer it out only on cancel or successful purchase.
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");
require(_price > 0, "Price must be greater than 0");
+ // Escrow the NFT into the marketplace contract
+ _safeTransfer(msg.sender, address(this), _tokenId, "");
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);
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, "");
+ _safeTransfer(address(this), msg.sender, listing.tokenId, "");
s_listings[_listingId].isActive = false;
emit NFT_Dealers_Sold(msg.sender, listing.price);
}
function cancelListing(uint256 _listingId) external {
Listing memory listing = s_listings[_listingId];
if (!listing.isActive) revert ListingNotActive(_listingId);
require(listing.seller == msg.sender, "Only seller can cancel listing");
s_listings[_listingId].isActive = false;
activeListingsCounter--;
+ // Return the NFT from escrow to the seller
+ _safeTransfer(address(this), listing.seller, listing.tokenId, "");
+
usdc.safeTransfer(listing.seller, collateralForMinting[listing.tokenId]);
collateralForMinting[listing.tokenId] = 0;
emit NFT_Dealers_ListingCanceled(_listingId);
}
Rewards breakdown
nSLOC
253This contest has a flat reward structure with the following payouts:
100 EXP
20 EXP
2 EXP
Live
The contest is live. Earn rewards by submitting a finding.
Appeals
This is your time to appeal against judgements on your submissions.
Appeals Review
Appeals are being carefully reviewed by our judges.
Rewards Distribution
The contest is complete and the rewards are being distributed.