Root + Impact
Description
-
Normal behavior: While bidding, contracts should emit bid-phase events only (BidPlaced, AuctionExtended). A single settlement event must be emitted exactly once when the auction is finalized (after NFT transfer and payment distribution).
Issue: placeBid() emits AuctionSettled before any finalization and even before updating bids[tokenId]. Off-chain consumers treating AuctionSettled as authoritative will record false settlements, trigger premature automations, and end up with duplicate/conflicting settlement records once _executeSale() emits the real AuctionSettled.
function placeBid(uint256 tokenId) external payable isListed(tokenId) {
Listing storage listing = listings[tokenId];
address previousBidder = bids[tokenId].bidder;
uint256 previousBidAmount = bids[tokenId].amount;
require(listing.seller != msg.sender, "Seller cannot bid");
require(listing.auctionEnd == 0 || block.timestamp < listing.auctionEnd, "Auction ended");
emit AuctionSettled(tokenId, msg.sender, listing.seller, msg.value);
}
Risk
Likelihood:
Indexers/bots that subscribe to AuctionSettled will process false positives consistently.
Impact:
Operational/UX breakage: Premature payouts, wrong ownership displays, data reconciliation headaches for integrators.
Proof of Concept
What this proves
-
A normal bid triggers AuctionSettled even though no settlement happened.
-
Later, when the auction actually finalizes, _executeSale() emits another AuctionSettled, causing duplicate/conflicting settlement logs
### Test 1 — “Settlement” emitted during a regular bid (should never happen)
pragma solidity 0.8.20;
import {Test, console} from "forge-std/Test.sol";
import {BidBeastsNFTMarket} from "../src/BidBeastsNFTMarketPlace.sol";
import {BidBeasts} from "../src/BidBeasts_NFT_ERC721.sol";
contract MisleadingSettlementEvent_BidPhase_PoC is Test {
BidBeastsNFTMarket market;
BidBeasts nft;
address OWNER = address(0xA1);
address SELLER = address(0xB2);
address BIDDER = address(0xC3);
uint256 constant TOKEN_ID = 0;
uint256 constant MIN_PRICE = 1 ether;
event AuctionSettled(uint256 tokenId, address winner, address seller, uint256 price);
function setUp() public {
vm.startPrank(OWNER);
nft = new BidBeasts();
market = new BidBeastsNFTMarket(address(nft));
vm.stopPrank();
vm.deal(SELLER, 100 ether);
vm.deal(BIDDER, 100 ether);
vm.prank(OWNER);
nft.mint(SELLER);
vm.startPrank(SELLER);
nft.approve(address(market), TOKEN_ID);
market.listNFT(TOKEN_ID, MIN_PRICE, 0);
vm.stopPrank();
}
function test_SettlementEventShouldNotFireDuringPlaceBid_preFix() public {
vm.prank(BIDDER);
vm.expectEmit(true, true, true, true);
emit AuctionSettled(TOKEN_ID, BIDDER, SELLER, MIN_PRICE);
market.placeBid{value: MIN_PRICE}(TOKEN_ID);
}
}
Test 2 — Duplicate/conflicting settlement logs (one fake from bidding, one real at settlement)
pragma solidity 0.8.20;
import {Test} from "forge-std/Test.sol";
import {BidBeastsNFTMarket} from "../src/BidBeastsNFTMarketPlace.sol";
import {BidBeasts} from "../src/BidBeasts_NFT_ERC721.sol";
contract MisleadingSettlementEvent_DuplicateLogs_PoC is Test {
BidBeastsNFTMarket market;
BidBeasts nft;
address OWNER = address(0x11);
address SELLER = address(0x22);
address B1 = address(0x33);
address B2 = address(0x44);
uint256 constant TOKEN_ID = 0;
uint256 constant MIN_PRICE = 1 ether;
event AuctionSettled(uint256 tokenId, address winner, address seller, uint256 price);
function setUp() public {
vm.startPrank(OWNER);
nft = new BidBeasts();
market = new BidBeastsNFTMarket(address(nft));
vm.stopPrank();
vm.deal(SELLER, 100 ether);
vm.deal(B1, 100 ether);
vm.deal(B2, 100 ether);
vm.prank(OWNER);
nft.mint(SELLER);
vm.startPrank(SELLER);
nft.approve(address(market), TOKEN_ID);
market.listNFT(TOKEN_ID, MIN_PRICE, 0);
vm.stopPrank();
}
function test_DuplicateSettlementLogs_preFix() public {
vm.prank(B1);
vm.expectEmit(true, true, true, true);
emit AuctionSettled(TOKEN_ID, B1, SELLER, MIN_PRICE);
market.placeBid{value: MIN_PRICE}(TOKEN_ID);
uint256 incBid = (MIN_PRICE * 105) / 100;
vm.prank(B2);
vm.expectEmit(true, true, true, true);
emit AuctionSettled(TOKEN_ID, B2, SELLER, incBid);
market.placeBid{value: incBid}(TOKEN_ID);
(, , , uint256 end, ) = market.getListing(TOKEN_ID);
vm.warp(end + 1);
vm.expectEmit(true, true, true, true);
emit AuctionSettled(TOKEN_ID, B2, SELLER, incBid);
market.settleAuction(TOKEN_ID);
}
}
Recommended Mitigation
Why this is wrong
Event semantics: AuctionSettled should represent the finalization of an auction (NFT transferred, payouts computed). Emitting it in placeBid() lies to indexers, bots, and UIs that treat this event as canonical.
Operational risk: Off-chain automations might prematurely pay out, close positions, or update ownership due to a false settlement.
Goals of the fix
Never emit AuctionSettled during bidding.
Emit exactly one AuctionSettled, at the true settlement point inside _executeSale().
Keep bid-phase events (BidPlaced, AuctionExtended) for real-time UX updates.
@@ function placeBid(uint256 tokenId) external payable isListed(tokenId) {
- // Misleading: emits settlement during bidding
- emit AuctionSettled(tokenId, msg.sender, listing.seller, msg.value);
+ // Do NOT emit settlement here; this is still the bidding phase.
// --- Buy Now Logic ---
if (listing.buyNowPrice > 0 && msg.value >= listing.buyNowPrice) {
uint256 salePrice = listing.buyNowPrice;
uint256 overpay = msg.value - salePrice;
// EFFECT: set winner bid to exact sale price (keep consistent)
bids[tokenId] = Bid(msg.sender, salePrice);
listing.listed = false;
if (previousBidder != address(0)) {
_payout(previousBidder, previousBidAmount);
}
// Finalize here — this is the correct place that emits AuctionSettled
_executeSale(tokenId);
if (overpay > 0) {
_payout(msg.sender, overpay);
}
return;
}
// ... rest of bidding logic ...
bids[tokenId] = Bid(msg.sender, msg.value);
if (previousBidder != address(0)) {
_payout(previousBidder, previousBidAmount);
}
- // keep bid-phase telemetry here
+ // Bid-phase telemetry only
emit BidPlaced(tokenId, msg.sender, msg.value);
}
@@ function _executeSale(uint256 tokenId) internal {
- BBERC721.transferFrom(address(this), bid.bidder, tokenId);
+ // (Optional hardening) Consider safeTransferFrom; left as transferFrom per current design
+ 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);
// Single authoritative settlement signal
emit AuctionSettled(tokenId, bid.bidder, listing.seller, bid.amount);
}
