vizay9652 - Vulnerability in `LidoVault::_claimWithdrawals` function: `unlockReceive` and `address(this).balance` leads to Potential Overpayments

[sherlock-admin3]

vizay9652

High

Vulnerability in LidoVault::_claimWithdrawals function: unlockReceive and address(this).balance leads to Potential Overpayments

Summary

The LidoVault::_claimWithdrawals function allows users to claim withdrawals, but it does in a manner that exposes users to potential overpayments due to unlockReceive flag and `address(this).balance.

Vulnerability Detail

The LidoVault::_claimWithdrawals function sets unlockReceive to true at the beginning of the function and resets it to false at the end. However, this flag is not properly protected, allowing arbitrary users to send ETH to the contract during the withdrawal process. As a result, the withdrawnAmount calculation becomes inflated, which leads to unintended overpayments.

Impact

Financial Losses: Users may receive more ETH during withdrawal process, which leads to financial losses to contract owners or other users.

Code Snippet

https://github.com/sherlock-audit/2024-08-saffron-finance/blob/main/lido-fiv/contracts/LidoVault.sol#L1168-L1185

  function _claimWithdrawals(address user, uint256[] memory requestIds) internal returns (uint256) {
// @audit-issue: contract receives ETH from any user when true, which makes address(this).balance gets inflated
@>    unlockReceive = true; 
@>    uint256 beforeBalance = address(this).balance;

    // Claim Ether for the burned stETH positions
    // this will fail if the request is not finalized
    for (uint i = 0; i < requestIds.length; i++) {
      lidoWithdrawalQueue.claimWithdrawal(requestIds[i]); // amount gets transferred from `lido` to this contract.
    }
// @audit-issue: contract balance inflated(claimed balance + arbitrary user's ETh deposit)
@>    uint256 withdrawnAmount = address(this).balance - beforeBalance; // user receives more amount than intended.
    require(withdrawnAmount > 0, "IWA");

    emit WithdrawalClaimed(withdrawnAmount, requestIds, user);

    unlockReceive = false;
    return withdrawnAmount;
  }

Proof of Concept-1

1. An user requested for withdrawal which triggered LidoVault::_claimWithdrawals function.
2. Now unlockReceive sets to true which allows LidoVault::receive function to receive ETH.
3. Assume the uint256 beforeBalance = address(this).balance; is 1000 Ether.
4. Arbitrary user sends 10 ether to this contract, because unlockReceive = true.
5. Now this line lidoWithdrawalQueue.claimWithdrawal(requestIds[i]); triggered, where lidoWithdrawalQueue transfers the amount to this contract.
6. Assume lidoWithdrawalQueue sends 100 ether to this contract.
7. So, 100 ether is intended withdrawal that should be received by the user.
8. But this line uint256 withdrawnAmount = address(this).balance - beforeBalance; makes withdrawn Amount = (1000 + 10 +100) - 1000 ether = 110 ether .

9. So, user receives more ether than intended.

   Proof of Concept-2

   1. user1 requested for withdrawal which triggered LidoVault::_claimWithdrawals function, which sets unlockReceive = true.
   2. Now while _claimWithdrawals::for loop executing, user2 initiated his withdrawal request and his transaction is executed first which sets unlockReceive = false.
   3. since unlockReceive = false, the contract won't receive any incoming ether.
   4. So, user1's transaction gets reverted.

      Tool used

   Manual Review

   Recommendation

   Keep track of withdrawn amounts instead of relying on address(this).balance.