Bid Beasts
First Flight #49
Beginner FriendlyFoundrySolidityNFT
100 EXP
View results
Previous Next
Submission Details
Impact: medium
Likelihood: low
Invalid

Missing Check for Already Listed NFTs in listNFT()

objectplayer

Root + Impact

The listNFT function doesn't verify whether an NFT is already listed before creating a new listing, which can lead to state inconsistencies and lost bids.
Already listed NFTs can be re-listed, potentially erasing existing auction data, disrupting active auctions, and causing users to lose track of their bids.

Description

  • The normal behavior in auction marketplaces is to prevent NFTs that are already in an active auction from being re-listed until they are properly unlisted or the auction concludes.

  • The specific issue is that the listNFT function overwrites any existing listing data without checking if the NFT is already part of an active listing, potentially disrupting ongoing auctions and existing bids.

function listNFT(uint256 tokenId, uint256 _minPrice, uint256 _buyNowPrice) external {
require(BBERC721.ownerOf(tokenId) == msg.sender, "Not the owner");
require(_minPrice >= S_MIN_NFT_PRICE, "Min price too low");
if (_buyNowPrice > 0) {
require(_minPrice <= _buyNowPrice, "Min price cannot exceed buy now price");
}
​
BBERC721.transferFrom(msg.sender, address(this), tokenId);
​
listings[tokenId] = Listing({ @> // Overwrites existing listing without checking
seller: msg.sender,
minPrice: _minPrice,
buyNowPrice: _buyNowPrice,
auctionEnd: 0, // Timer starts only after the first valid bid.
listed: true
});
​
emit NftListed(tokenId, msg.sender, _minPrice, _buyNowPrice);
}

Risk

Likelihood: Low

  • This would occur if a token is transferred back to its original owner (or to a new owner) after being listed, and then listed again.

  • The scenario requires multiple transactions, but is entirely possible within the normal operation of the contract.

Impact: Medium to High

  • Existing bids on the NFT would be lost as the listings mapping is overwritten.

  • Bidders would lose track of their active bids and have no way to claim their NFT even if they were previously the highest bidder.

  • The auction timing would be reset, potentially allowing the seller to manipulate the auction to their advantage.

  • This could lead to auction participants losing confidence in the marketplace.

Proof of Concept

async function demonstrateRelistingIssue() {
// Setup: Deploy contracts
const nft = await BidBeasts.deploy();
const marketplace = await BidBeastsNFTMarket.deploy(nft.address);
// Accounts
const owner = accounts[0];
const bidder = accounts[1];
// Mint an NFT
const tokenId = await nft.mint(owner.address);
await nft.approve(marketplace.address, tokenId);
// List the NFT
await marketplace.listNFT(tokenId, ethers.utils.parseEther("1.0"), ethers.utils.parseEther("2.0"));
console.log("NFT Listed");
// Check the listing
const listing1 = await marketplace.getListing(tokenId);
console.log("Original Listing:", {
seller: listing1.seller,
minPrice: ethers.utils.formatEther(listing1.minPrice),
listed: listing1.listed
});
// Place a bid
await marketplace.connect(bidder).placeBid(tokenId, {
value: ethers.utils.parseEther("1.1")
});
console.log("Bid placed by bidder");
// Check the bid
const bid = await marketplace.getHighestBid(tokenId);
console.log("Highest Bid:", {
bidder: bid.bidder,
amount: ethers.utils.formatEther(bid.amount)
});
// In a real scenario, this would be complex, but for demonstration:
// 1. Seller could unlist (but can't due to active bid)
// 2. Let's simulate seller somehow getting the NFT back and relisting
// First, trigger a pretend bug or admin function to get the NFT back to owner
// (In a real scenario, this might be through a vulnerability, admin function, or other means)
// For testing purposes only - not a real attack vector:
await mockResetOwnership(nft, marketplace, tokenId, owner.address);
// Now, owner has the NFT back but it's still technically "listed" in the contract
// Owner can still call listNFT again
await nft.approve(marketplace.address, tokenId);
await marketplace.listNFT(tokenId, ethers.utils.parseEther("1.5"), ethers.utils.parseEther("3.0"));
console.log("NFT Re-listed with different prices");
// Check the new listing
const listing2 = await marketplace.getListing(tokenId);
console.log("New Listing:", {
seller: listing2.seller,
minPrice: ethers.utils.formatEther(listing2.minPrice),
listed: listing2.listed
});
// Check what happened to the previous bid
const newBid = await marketplace.getHighestBid(tokenId);
console.log("Highest Bid after re-listing:", {
bidder: newBid.bidder,
amount: newBid.bidder === ethers.constants.AddressZero ? "0" : ethers.utils.formatEther(newBid.amount)
});
// The previous bid is still in the mapping, but the listing has been overwritten
// This creates an inconsistent state where the bid exists but the auction parameters have changed
// Helper function to simulate NFT returning to owner (for testing only)
async function mockResetOwnership(nft, marketplace, tokenId, ownerAddress) {
// In a real scenario, this would be through a vulnerability or admin function
// This is just to simulate the NFT returning to the owner
console.log("Simulating NFT returning to owner through some mechanism");
}
}
​
// Expected output:
// NFT Listed
// Original Listing: { seller: '0x...', minPrice: '1.0', listed: true }
// Bid placed by bidder
// Highest Bid: { bidder: '0x...', amount: '1.1' }
// Simulating NFT returning to owner through some mechanism
// NFT Re-listed with different prices
// New Listing: { seller: '0x...', minPrice: '1.5', listed: true }
// Highest Bid after re-listing: { bidder: '0x...', amount: '1.1' }

This proof of concept demonstrates that:

  1. An NFT can be listed with initial auction parameters

  2. A bidder can place a bid on this NFT

  3. If the original owner somehow regains possession of the NFT, they can re-list it

  4. Re-listing overwrites the original listing parameters but doesn't clear the existing bid

  5. This creates an inconsistent state where bids are associated with a listing that has changed parameters

Recommended Mitigation

function listNFT(uint256 tokenId, uint256 _minPrice, uint256 _buyNowPrice) external {
require(BBERC721.ownerOf(tokenId) == msg.sender, "Not the owner");
+ require(!listings[tokenId].listed, "NFT is already listed");
require(_minPrice >= S_MIN_NFT_PRICE, "Min price too low");
if (_buyNowPrice > 0) {
require(_minPrice <= _buyNowPrice, "Min price cannot exceed buy now price");
}
​
BBERC721.transferFrom(msg.sender, address(this), tokenId);
​
listings[tokenId] = Listing({
seller: msg.sender,
minPrice: _minPrice,
buyNowPrice: _buyNowPrice,
auctionEnd: 0, // Timer starts only after the first valid bid.
listed: true
});
​
emit NftListed(tokenId, msg.sender, _minPrice, _buyNowPrice);
}

This change:

  1. Prevents Re-listing: Adds a check to ensure NFTs that are already listed cannot be re-listed.

  2. Maintains Auction Integrity: Ensures that ongoing auctions cannot be disrupted by re-listing the same NFT.

  3. Protects Bidders: Prevents scenarios where bidders might lose track of their bids due to re-listing.

  4. Consistent State: Maintains a consistent state where an NFT can only be in one active listing at a time.

This simple addition significantly improves the security and reliability of the marketplace, protecting both sellers and bidders from potential issues caused by re-listing.

Updates

Lead Judging Commences

cryptoghost Lead Judge 2 months ago
Submission Judgement Published
Invalidated
Reason: Design choice

Appeal created

objectplayer Submitter
2 months ago
cryptoghost Lead Judge
2 months ago
cryptoghost Lead Judge 2 months ago
Submission Judgement Published
Invalidated
Reason: Design choice

Support

FAQs

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

Give us feedback!