Owner can drain funds reserved for user withdrawals

[hats-bug-reporter[bot]]

Github username: @4gontuk Twitter username: 4gontuk Submission hash (on-chain): 0x6f44bcb01bd213530d45974dc2123ad3b45dc109fd0ec15296c7f3e5233c2f95 Severity: medium

Description:

Description:

The NativeMinterWithdrawal and ERC20MinterWithdrawal contracts have a critical issue in the interaction between the withdraw() and claimWithdrawal() functions. These contracts are designed to handle user withdrawals and owner withdrawals separately, but a logical flaw allows the owner to withdraw funds that should be reserved for user withdrawals.

Relevant Components:

1. balanceAvailable() Function:

   • Calculates the available balance for withdrawal by subtracting totalUnclaimedWithdrawals from the contract's balance.

   • Code from NativeMinterWithdrawal:

     function balanceAvailable() public view virtual returns (uint256) {
      uint256 availableBalance = address(this).balance;
      uint256 balance;
     
      if (availableBalance < totalUnclaimedWithdrawals) {
          balance = 0;
      } else {
          balance = availableBalance - totalUnclaimedWithdrawals;
      }
     
      return balance;
     }

2. withdraw() Function:

   • Allows the owner to withdraw the calculated available balance.
   • Code from NativeMinterWithdrawal:

     function withdraw(address receiver) public virtual onlyOwner override {
      uint256 balance = balanceAvailable();
      require(balance > 0, "BalanceNotEnough");
      SafeTransferLib.safeTransferETH(receiver, balance);
      emit Withdraw(address(msg.sender), receiver, balance);
     }

3. claimWithdrawal() Function:

   • Allows users to claim their processed withdrawals.
   • Code from NativeMinterWithdrawal:

     function claimWithdrawal(uint256 withdrawalId, address receiver) public virtual nonReentrant {
      require(ownerOf(withdrawalId) == msg.sender, "NotOwner");
      WithdrawalRequest storage request = _withdrawalRequests[withdrawalId];
      require(request.processed, "NotProcessedYet");
      _burn(withdrawalId);
      request.claimed = true;
      totalUnclaimedWithdrawals = totalUnclaimedWithdrawals - request.amount;
      SafeTransferLib.safeTransferETH(receiver, request.amount);
      emit ClaimWithdrawal(address(msg.sender), receiver, request.amount, withdrawalId);
     }

Issue:

The withdraw() function can drain all the funds from the contract, leaving nothing for users to claim, even though their withdrawals have been processed and are marked as claimable. This violates the principle that processed withdrawals should always be claimable.

Highest Impact Scenario:

1. Users request withdrawals, increasing totalUnclaimedWithdrawals.
2. The owner processes these withdrawals, marking them as claimable.
3. New deposits are made, increasing the contract's balance.
4. The owner calls withdraw(), which transfers all funds exceeding totalUnclaimedWithdrawals.
5. Users try to claim their withdrawals, but the transaction fails due to insufficient funds in the contract.

Impact

The owner can withdraw funds that should be reserved for user withdrawals, leading to a situation where users cannot claim their processed withdrawals. This undermines the integrity of the withdrawal system and can result in loss of user funds.

Attack Scenario:

1. Users request withdrawals, increasing totalUnclaimedWithdrawals.
2. The owner processes these withdrawals, marking them as claimable.
3. New deposits are made, increasing the contract's balance.
4. The owner calls withdraw(), which transfers all funds exceeding totalUnclaimedWithdrawals.
5. Users try to claim their withdrawals, but the transaction fails due to insufficient funds in the contract.

Proof of Concept

1. User Requests Withdrawal:

   • User calls requestWithdrawal(100, userAddress).
   • totalUnclaimedWithdrawals increases by 100.

2. Owner Processes Withdrawal:

   • Owner calls processWithdrawals([withdrawalId]).
   • Withdrawal is marked as processed and claimable.

3. New Deposits:

   • New deposits are made, increasing the contract's balance to 200.

4. Owner Withdraws Funds:

   • Owner calls withdraw(ownerAddress).
   • balanceAvailable() returns 100 (200 - 100).
   • Owner withdraws 100, leaving the contract with 100.

5. User Claims Withdrawal:

   • User calls claimWithdrawal(withdrawalId, userAddress).
   • Transaction fails due to insufficient funds in the contract.

This scenario demonstrates how the owner can drain funds reserved for user withdrawals, leading to failed user withdrawal claims.

[ilzheev]

Tokens are reserved for withdrawal only when withdrawal is being processed. When withdrawal is NOT processed, tokens can be withdrawn by multisig admin.