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

[M-2] State is Not Cleared on Unlisting, Leading to Stale Data

cosminmarian53

State is Not Cleared on Unlisting, Leading to Stale Data

Description

  • When a seller unlists an NFT, the contract state should be cleanly reset for that tokenId.

  • The unlistNFT function sets the listed flag to false but does not clear the rest of the Listing struct data (seller, prices, etc.). This leaves stale data on-chain.

// src/BidBeastsNFTMarketPlace.sol
​
function unlistNFT(uint256 tokenId) external isListed(tokenId) isSeller(tokenId, msg.sender) {
require(bids[tokenId].bidder == address(0), "Cannot unlist, a bid has been placed");
​
Listing storage listing = listings[tokenId];
listing.listed = false; // @> Other struct members are not cleared
BBERC721.transferFrom(address(this), msg.sender, tokenId);
emit NftUnlisted(tokenId);
}

Risk

Likelihood: High

  • This occurs every time an NFT is successfully unlisted.

Impact: Low

  • Funds are not at risk.

  • The contract maintains unnecessary, stale state, which can increase gas costs for future interactions with that storage slot and potentially lead to bugs if future logic incorrectly reads the stale data.

Proof of Concept

Simple PoC to show that only one field is changed, while the others are kept the same.

function test_unlistNFT_leaves_stale_listing_data() public {
// Setup: Mint and list NFT
_mintNFT();
_listNFT();
​
// Unlist the NFT
vm.prank(SELLER);
market.unlistNFT(TOKEN_ID);
​
// Fetch the listing struct after unlisting
BidBeastsNFTMarket.Listing memory listing = market.getListing(TOKEN_ID);
​
// Assert: listed is false, but other fields are not reset
assertEq(listing.listed, false, "Listed flag should be false");
assertEq(listing.seller, SELLER, "Seller field should still be set (stale)");
assertEq(listing.minPrice, MIN_PRICE, "Min price should still be set (stale)");
assertEq(listing.buyNowPrice, BUY_NOW_PRICE, "Buy now price should still be set (stale)");
assertEq(listing.auctionEnd, 0, "Auction end should still be set (stale)");
}

Recommended Mitigation

Long term, this could lead to expensive storage overrides if the token owner ever decides to re-list their NFT. So, instead, consider removing the entry entirely, to clear state:

// src/BidBeastsNFTMarketPlace.sol
Listing storage listing = listings[tokenId];
listing.listed = false;
+ delete listings[tokenId];
BBERC721.transferFrom(address(this), msg.sender, tokenId);
emit NftUnlisted(tokenId);
Updates

Lead Judging Commences

cryptoghost Lead Judge 2 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

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

Give us feedback!