Description
The _executeSale function transfers the NFT to the winning bidder before sending the payment to the seller, which could allow a malicious bidder to execute a reentrancy attack.
Expected Behavior
State changes should be completed before external calls to prevent reentrancy attacks.
Actual Behavior
The NFT is transferred to the bidder before the ETH is sent to the seller, allowing a malicious bidder to potentially reenter the contract and manipulate state.
Root Cause
The function performs an external call (BBERC721.transferFrom) before updating critical state and making another external call (_payout):
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);
}
Risk Assessment
Impact
If exploited, a malicious bidder could potentially manipulate the contract state during the reentrancy, leading to theft of funds or other unexpected behavior.
Likelihood
The likelihood is medium because it requires the bidder to be a malicious contract capable of executing reentrancy attacks, and the attack vector depends on the specific implementation of the NFT contract.
Proof of Concept
pragma solidity ^0.8.0;
import "forge-std/Test.sol";
import "../src/BidBeastsNFTMarketPlace.sol";
import "../src/BidBeasts_NFT_ERC721.sol";
contract MaliciousBidder {
BidBeastsNFTMarket market;
uint256 tokenId;
constructor(address _market, uint256 _tokenId) {
market = BidBeastsNFTMarket(_market);
tokenId = _tokenId;
}
function bid() external payable {
market.placeBid{value: msg.value}(tokenId);
}
function onERC721Received(address, address, uint256, bytes calldata) external returns (bytes4) {
return this.onERC721Received.selector;
}
receive() external payable {}
}
contract ExploitTest is Test {
BidBeastsNFTMarket market;
BidBeasts nft;
address seller = address(0x1);
MaliciousBidder attacker;
uint256 tokenId = 0;
function setUp() public {
nft = new BidBeasts();
market = new BidBeastsNFTMarket(address(nft));
vm.prank(address(nft.owner()));
nft.mint(seller);
vm.startPrank(seller);
nft.approve(address(market), tokenId);
market.listNFT(tokenId, 1 ether, 5 ether);
vm.stopPrank();
attacker = new MaliciousBidder(address(market), tokenId);
vm.deal(address(attacker), 2 ether);
}
function testExploit() public {
attacker.bid{value: 1.1 ether}();
vm.warp(block.timestamp + market.S_AUCTION_EXTENSION_DURATION() + 1);
market.settleAuction(tokenId);
}
}
Recommended Mitigation
Implement the checks-effects-interactions pattern by updating all state before making external calls:
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);
}
function _executeSale(uint256 tokenId) internal {
Listing storage listing = listings[tokenId];
Bid memory bid = bids[tokenId];
address bidder = bid.bidder;
address seller = listing.seller;
uint256 amount = bid.amount;
listing.listed = false;
delete bids[tokenId];
uint256 fee = (amount * S_FEE_PERCENTAGE) / 100;
s_totalFee += fee;
uint256 sellerProceeds = amount - fee;
BBERC721.transferFrom(address(this), bidder, tokenId);
_payout(seller, sellerProceeds);
emit AuctionSettled(tokenId, bidder, seller, amount);
}
Explanation
The fixed implementation follows the checks-effects-interactions pattern by updating all state variables before making any external calls, which prevents reentrancy attacks.
