Root + Impact
Description
-
Normal behavior: A “Buy-Now” price should either (a) disable itself once competitive bidding exceeds it, or (b) require the buyer to pay at least the greater of buyNowPrice and the current highest bid (± increment). Otherwise, the auction’s price discovery is violated.
Issue: In placeBid, when msg.value >= buyNowPrice, the contract:
force-sets bids[tokenId] = (msg.sender, buyNowPrice),
refunds the previous highest bidder,
executes the sale at buyNowPrice,
even if previousBidAmount is already higher than buyNowPrice.
Result: the seller is paid less than the current highest bid, and the current highest bidder loses unfairly despite having bid more.
if (listing.buyNowPrice > 0 && msg.value >= listing.buyNowPrice) {
uint256 salePrice = listing.buyNowPrice;
...
bids[tokenId] = Bid(msg.sender, salePrice);
listing.listed = false;
if (previousBidder != address(0)) {
_payout(previousBidder, previousBidAmount);
}
_executeSale(tokenId);
...
return;
}
Risk
Likelihood:
Impact:
Bidder unfairness: Highest bidder is involuntarily outbid by a lower buy-now taker and only refunded, breaking auction fairness.
Proof of Concept
A bidder has already offered 6 ETH while buyNowPrice is 5 ETH. Another user pays 5 ETH (≥ buy-now), and the auction settles at 5 ETH, refunding the 6-ETH bidder. This violates price discovery and harms the seller.
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 BuyNowUndercutsHighestBid_PoC is Test {
BidBeastsNFTMarket market;
BidBeasts nft;
address OWNER = address(0xA1);
address SELLER = address(0xB2);
address BIDDER1 = address(0xC3);
address BUYER = address(0xD4);
uint256 constant TOKEN_ID = 0;
uint256 constant MIN_PRICE = 1 ether;
uint256 constant BUY_NOW_PRICE = 5 ether;
uint256 constant FEE_BPCT = 5;
function setUp() public {
vm.startPrank(OWNER);
nft = new BidBeasts();
market = new BidBeastsNFTMarket(address(nft));
vm.stopPrank();
vm.deal(SELLER, 100 ether);
vm.deal(BIDDER1, 100 ether);
vm.deal(BUYER, 100 ether);
vm.prank(OWNER);
nft.mint(SELLER);
vm.startPrank(SELLER);
nft.approve(address(market), TOKEN_ID);
market.listNFT(TOKEN_ID, MIN_PRICE, BUY_NOW_PRICE);
vm.stopPrank();
}
function test_BuyNowUndercutsHigherStandingBid_preFix() public {
vm.prank(BIDDER1);
market.placeBid{value: 6 ether}(TOKEN_ID);
(address hb, uint256 amt) = (market.getHighestBid(TOKEN_ID).bidder, market.getHighestBid(TOKEN_ID).amount);
assertEq(hb, BIDDER1);
assertEq(amt, 6 ether);
uint256 sellerBefore = SELLER.balance;
vm.prank(BUYER);
market.placeBid{value: 5 ether}(TOKEN_ID);
uint256 expectedProceeds = BUY_NOW_PRICE - (BUY_NOW_PRICE * FEE_BPCT) / 100;
assertEq(SELLER.balance, sellerBefore + expectedProceeds, "seller was paid below standing market price");
vm.expectRevert();
market.getHighestBid(TOKEN_ID);
assertEq(nft.ownerOf(TOKEN_ID), BUYER, "buyer got NFT at lower price than standing bid");
}
}
Explanation:
In placeBid, the buy-now path overwrites the standing highest bid with buyNowPrice and finalizes the sale, even if the standing previousBidAmount is higher. As seen above, the seller receives proceeds based on 5 ETH, not 6 ETH, and the 6-ETH bidder loses unfairly.
Recommended Mitigation
A buy-now taker must not clear below the current fair price already discovered by bidding. Enforce a floor of max(buyNowPrice, previousBidAmount) (optionally also apply your min-increment over the standing bid).
Why this works
Sets a non-undercutting floor: buy-now can only clear at least the standing price (or your chosen incremented floor).
Preserves UX: buyers can still press buy-now; they just pay a fair amount (≥ discovered price).
Keeps overpay refunds consistent with the current design.
Optional hardening: If you want buy-now to disable itself once bidding surpasses it, simply set listing.buyNowPrice = 0 as soon as a bid exceeds it (and remove the buy-now branch thereafter)
@@ function placeBid(uint256 tokenId) external payable isListed(tokenId) {
- 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);
- }
-
- _executeSale(tokenId);
-
- // Refund overpay (if any) to buyer
- if (overpay > 0) {
- _payout(msg.sender, overpay);
- }
- return;
- }
+ if (listing.buyNowPrice > 0 && msg.value >= listing.buyNowPrice) {
+ // Do not undercut the discovered market price.
+ // Floor = max(buyNowPrice, current highest bid)
+ uint256 floor = listing.buyNowPrice;
+ if (previousBidAmount > floor) {
+ // Optional: enforce min increment over standing bid for consistency:
+ // uint256 inc = S_MIN_BID_INCREMENT_PERCENTAGE;
+ // uint256 numerator = previousBidAmount * (100 + inc);
+ // uint256 minWithInc = (numerator + 99) / 100; // ceil((prev*(100+inc))/100)
+ // floor = minWithInc;
+ floor = previousBidAmount;
+ }
+ require(msg.value >= floor, "buy-now below standing price");
+
+ uint256 salePrice = floor;
+ uint256 overpay = msg.value - salePrice;
+
+ // Record final winner & delist
+ bids[tokenId] = Bid(msg.sender, salePrice);
+ listing.listed = false;
+
+ // Refund prior highest bidder (fairly outbid at >= floor)
+ if (previousBidder != address(0)) {
+ _payout(previousBidder, previousBidAmount);
+ }
+
+ // Finalize at the (non-undercutting) salePrice
+ _executeSale(tokenId);
+
+ // Return any excess sent by buyer
+ if (overpay > 0) {
+ _payout(msg.sender, overpay);
+ }
+ return;
+ }
