Bid Beasts

Description: The placeBid function in BidBeastsNFTMarketPlace.sol is vulnerable to reentrancy attacks due to external calls that send Ether or transfer tokens before completing state updates. Specifically, the function makes external calls via _payout (lines 132, 140) and _executeSale (line 136), which include Ether transfers (address(recipient).call{value: amount}) and an NFT transfer (BBERC721.transferFrom). These calls occur before or alongside updates to critical state variables such as bids, listings, and failedTransferCredits. Slither identifies that failedTransferCredits (line 235) is updated after failed Ether transfers, enabling potential cross-function reentrancy with withdrawAllFailedCredits (lines 243-252). Additionally, the transferFrom call in _executeSale (line 217) could trigger a callback (e.g., onERC721Received) in a malicious contract, allowing re-entry.

Risk:

Impact: High. A successful reentrancy attack could lead to significant financial and operational consequences, including:

• Manipulation of Multiple Auctions

• Fund Theft via Cross-Function Reentrancy

• Denial-of-Service

• NFT Transfer Risks

Likelihood: Medium. The likelihood is tempered by certain protections in the contract, but the risk remains significant:

• The protocol already utilizes the isListed modifier and the listing.seller != msg.sender.

• An attacker needs a malicious contract to act as a bidder or seller, which requires some setup (e.g., deploying a contract with a malicious fallback or onERC721Received function). The attacker must also interact with an active auction, which may limit opportunities to specific scenarios.

• The buy-now path is particularly vulnerable due to multiple external calls (_payout and _executeSale). Cross-function reentrancy via failedTransferCredits is feasible if an attacker can trigger failed transfers (e.g., by rejecting Ether). The prevalence of malicious contracts in NFT marketplaces makes this a plausible threat.

Proof of Concept:

1. A malicious contract, MaliciousBidder, is deployed and funded with Ether.

2. An NFT with tokenId=1 is listed with a minPrice of 1 ETH and a buyNowPrice of 5 ETH.

3. MaliciousBidder places a bid on tokenId=1 with msg.value=5 ETH, triggering the buy-now path in placeBid.

4. In the buy-now path:

   1. bids[tokenId] and listing.listed are updated (lines 130-131).

   2. _executeSale(tokenId) is called (line 136), which:

      1. Sets listing.listed = false and deletes bids[tokenId] (lines 214-215).

      2. Calls BBERC721.transferFrom (line 217), triggering MaliciousBidder’s onERC721Received.

      3. In onERC721Received, MaliciousBidder re-enters placeBid for a different tokenId=2 (assuming it’s listed), placing a new bid and potentially manipulating its auction state.

   3. Alternatively, _payout to the seller (line 222) fails (e.g., seller is a contract that rejects Ether), updating failedTransferCredits[seller] (line 235).

5. MaliciousBidder calls withdrawAllFailedCredits during re-entry, draining credited funds before the original transaction completes.

6. This could lead to incorrect auction states, stolen funds, or disrupted auctions for tokenId=2.

Recommended Mitigation:

1. Apply ReentrancyGuard: Use OpenZeppelin’s ReentrancyGuard to protect placeBid, settleAuction, takeHighestBid, and withdrawAllFailedCredits. Add the nonReentrant modifier to prevent reentrant calls.

2. Follow Checks-Effects-Interactions: Restructure placeBid and _executeSale to perform all state updates (e.g., bids[tokenId], listing.listed, failedTransferCredits) before external calls (_payout, transferFrom).

3. Use Pull-over-Push Payments: Modify _payout to always credit failedTransferCredits instead of attempting direct Ether transfers. Require users to call withdrawAllFailedCredits to retrieve funds, eliminating external calls during critical operations.