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

Premature deletion of bid data in _executeSale can cause inconsistent state

_okchev

Root + Impact

Description

The intended behavior is that when an auction is finalized, the contract should:

  1. Transfer the NFT to the winning bidder.

  2. Deduct the marketplace fee.

  3. Pay the seller their proceeds.

  4. Only after successful settlement, clear out the stored bid data.

However, in the current implementation, the contract deletes the bids[tokenId] entry before performing the NFT transfer and payouts. This means that if the transfer reverts (for example, when the bidder is a contract that does not accept ERC721 tokens), the auction cannot be finalized and bid data is already lost. This leads to an inconsistent state where the highest bid has been erased but the NFT is not delivered.

function _executeSale(uint256 tokenId) internal {
Listing storage listing = listings[tokenId];
Bid memory bid = bids[tokenId];
​
listing.listed = false;
@> delete bids[tokenId]; // bid removed too early
​
BBERC721.transferFrom(address(this), bid.bidder, tokenId);
​
uint256 fee = (bid.amount * S_FEE_PERCENTAGE) / 100;
s_totalFee += fee;
uint256 sellerProceeds = bid.amount - fee;
_payout(listing.seller, sellerProceeds);
​
emit AuctionSettled(tokenId, bid.bidder, listing.seller, bid.amount);
}

Risk

Likelihood:

  • A bidder can be a smart contract that does not implement onERC721Received correctly or deliberately reverts on NFT transfers.

  • ERC721’s transferFrom will revert if address(this) is not the token owner (e.g., due to unexpected state changes).

Impact:

  • The auction enters an inconsistent state: the NFT remains locked in the marketplace, but the bid record is already deleted.

  • The auction cannot be correctly finalized, potentially leading to locked assets and disputes.

Proof of Concept

The test deploys a malicious bidder contract (RevertingReceiver) that always reverts on NFT transfers. When this contract places the highest bid and the auction ends, _executeSale deletes the bid record before attempting the NFT transfer. The transfer fails, leaving the auction in an inconsistent state: the NFT remains locked in the marketplace, but the bid data is already erased.

contract RevertingReceiver {
function onERC721Received(address, address, uint256, bytes calldata) external pure returns (bytes4) {
revert("Receiver does not accept NFTs");
}
}
​
function test_executeSaleDeleteTooEarly() public {
RevertingReceiver badBidder = new RevertingReceiver();
​
_mintNFT();
_listNFT();
​
// badBidder places a winning bid
vm.prank(address(badBidder));
market.placeBid{value: MIN_PRICE + 1}(TOKEN_ID);
​
// Fast forward auction
vm.warp(block.timestamp + 3 days);
​
// Auction finalization will revert due to transfer failure,
// but bids[tokenId] is already deleted.
vm.expectRevert("Receiver does not accept NFTs");
market.endAuction(TOKEN_ID);
​
// Inconsistent state: NFT is stuck, highest bid erased
}

Recommended Mitigation

Move the deletion of bids[tokenId] to the end of the _executeSale function to ensure it only happens after a successful transfer and payouts.

function _executeSale(uint256 tokenId) internal {
Listing storage listing = listings[tokenId];
Bid memory bid = bids[tokenId];
​
listing.listed = false;
- delete bids[tokenId];
​
BBERC721.transferFrom(address(this), bid.bidder, tokenId);
​
uint256 fee = (bid.amount * S_FEE_PERCENTAGE) / 100;
s_totalFee += fee;
uint256 sellerProceeds = bid.amount - fee;
_payout(listing.seller, sellerProceeds);
​
emit AuctionSettled(tokenId, bid.bidder, listing.seller, bid.amount);
​
+ delete bids[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!