scammed - StakingModule::convertAndDeposit wrong buffered and unfinalized check can prevent some deposits

[sherlock-admin3]

scammed

Medium

StakingModule::convertAndDeposit wrong buffered and unfinalized check can prevent some deposits

Summary

The unnecessary check performed in StakingModule::convertAndDeposit will prevent new deposits in Lido when WithdrawalQueue::unfinalizedStETH is bigger than Lido::getBufferedEther.

Vulnerability Detail

In order to understand the issue with this superfluous check we should take a look at stETH::submit:

function _submit(address _referral) internal returns (uint256) {
    require(msg.value != 0, "ZERO_DEPOSIT");

    StakeLimitState.Data memory stakeLimitData = STAKING_STATE_POSITION.getStorageStakeLimitStruct();
    // There is an invariant that protocol pause also implies staking pause.
    // Thus, no need to check protocol pause explicitly.
    require(!stakeLimitData.isStakingPaused(), "STAKING_PAUSED");

    if (stakeLimitData.isStakingLimitSet()) {
        uint256 currentStakeLimit = stakeLimitData.calculateCurrentStakeLimit();

        require(msg.value <= currentStakeLimit, "STAKE_LIMIT");

        STAKING_STATE_POSITION.setStorageStakeLimitStruct(stakeLimitData.updatePrevStakeLimit(currentStakeLimit - msg.value));
    }

    uint256 sharesAmount = getSharesByPooledEth(msg.value);

    _mintShares(msg.sender, sharesAmount);

    _setBufferedEther(_getBufferedEther().add(msg.value));//@audit this updates the getBufferedEther
    emit Submitted(msg.sender, msg.value, _referral);

    _emitTransferAfterMintingShares(msg.sender, sharesAmount);
    return sharesAmount;
}

As we can see it increases the bufferedEther when new ETH is being staked.

Indeed the exact same check is performed inside depositSecurityModule::depositBufferedEther, as it ends up calling the following functions from stETH:

function deposit(uint256 _maxDepositsCount, uint256 _stakingModuleId, bytes _depositCalldata) external {
      ILidoLocator locator = getLidoLocator();

      require(msg.sender == locator.depositSecurityModule(), "APP_AUTH_DSM_FAILED");
      require(canDeposit(), "CAN_NOT_DEPOSIT");

      IStakingRouter stakingRouter = _stakingRouter();
      uint256 depositsCount = Math256.min(
          _maxDepositsCount,
          stakingRouter.getStakingModuleMaxDepositsCount(_stakingModuleId, getDepositableEther())//@audit check hereb 
      );
    ...MORE CODE
  }

/**
     * @dev Returns depositable ether amount.
     * Takes into account unfinalized stETH required by WithdrawalQueue
     */
    function getDepositableEther() public view returns (uint256) {
        uint256 bufferedEther = _getBufferedEther();
        uint256 withdrawalReserve = _withdrawalQueue().unfinalizedStETH();
        return bufferedEther > withdrawalReserve ? bufferedEther - withdrawalReserve : 0;
    }

Now, knowing that stETH::submit increases bufferedEther let’s consider the following scenario:

• bufferedEther = 95 ETH
• unfinalizedStETH = 100 ETH
• StakingModule::convertAndDeposit with 10 ETH:

function convertAndDeposit(
  uint256 amount,
  uint256 blockNumber,
  bytes32 blockHash,
  bytes32 depositRoot,
  uint256 nonce,
  bytes calldata depositCalldata,
  IDepositSecurityModule.Signature[] calldata sortedGuardianSignatures
) external onlyDelegateCall {
  if (IERC20(weth).balanceOf(address(this)) < amount)
      revert NotEnoughWeth();

  uint256 unfinalizedStETH = withdrawalQueue.unfinalizedStETH();
  uint256 bufferedEther = ISteth(steth).getBufferedEther();
  if (bufferedEther < unfinalizedStETH)
      revert InvalidWithdrawalQueueState();

  _wethToWSteth(amount);
  depositSecurityModule.depositBufferedEther(
      blockNumber,
      blockHash,
      depositRoot,
      stakingModuleId,
      nonce,
      depositCalldata,
      sortedGuardianSignatures
  );
}

function _wethToWSteth(uint256 amount) private {
  IWeth(weth).withdraw(amount);
  ISteth(steth).submit{value: amount}(address(0));
  IERC20(steth).safeIncreaseAllowance(address(wsteth), amount);
  IWSteth(wsteth).wrap(amount);
}

In this configuration all Lido functions should have passed successfully, since first ETH is submitted and then depositBufferedEther is called that will use the most recent bufferedEther which will become 105 ETH and 100 unfinalizedStETH.

Instead the tx will revert unfairly preventing Vault from depositing into Lido, as the bufferedEther will be before staking and will remain at 95 and the unfinalizedStETH = 100.

Impact

• superfluous checks will prevent StakingModule from depositing into Lido

Code Snippet

https://github.com/mellow-finance/mellow-lrt/blob/dev-symbiotic-deploy/src/modules/obol/StakingModule.sol#L48-L82

Tool used

Manual Review

Recommendation

Remove the check completely or perform it after _wethToWSteth is executed and the bufferedEther is being updated

[0x3b33]

Escalate

This issue is valid and clearly shows how in certain scenarios StakingModule will fail to deposit into Lido unfairly, while there will be still some buffer left. The example that we have given explains how the order of operations, first to check for the bufferedEther and unfinalizedStETH, and then submit the stETH to Lido, bufferedEther will not be able to be increased and will revert the transaction, while in reality depositing should be possible:

function convertAndDeposit(
        uint256 amount,
        uint256 blockNumber,
        bytes32 blockHash,
        bytes32 depositRoot,
        uint256 nonce,
        bytes calldata depositCalldata,
        IDepositSecurityModule.Signature[] calldata sortedGuardianSignatures
    ) external onlyDelegateCall {
        if (IERC20(weth).balanceOf(address(this)) < amount)
            revert NotEnoughWeth();

        uint256 unfinalizedStETH = withdrawalQueue.unfinalizedStETH();
        uint256 bufferedEther = ISteth(steth).getBufferedEther();
        if (bufferedEther < unfinalizedStETH)//1
            revert InvalidWithdrawalQueueState();

        _wethToWSteth(amount);//2 - this increases the bufferedEther
        depositSecurityModule.depositBufferedEther(
            blockNumber,
            blockHash,
            depositRoot,
            stakingModuleId,
            nonce,
            depositCalldata,
            sortedGuardianSignatures
        );
    }

    function _wethToWSteth(uint256 amount) private {
        IWeth(weth).withdraw(amount);
        ISteth(steth).submit{value: amount}(address(0));
        IERC20(steth).safeIncreaseAllowance(address(wsteth), amount);
        IWSteth(wsteth).wrap(amount);
    }

[sherlock-admin3]

Escalate

This issue is valid and clearly shows how in certain scenarios StakingModule will fail to deposit into Lido unfairly, while there will be still some buffer left. The example that we have given explains how the order of operations, first to check for the bufferedEther and unfinalizedStETH, and then submit the stETH to Lido, bufferedEther will not be able to be increased and will revert the transaction, while in reality depositing should be possible:

function convertAndDeposit(
        uint256 amount,
        uint256 blockNumber,
        bytes32 blockHash,
        bytes32 depositRoot,
        uint256 nonce,
        bytes calldata depositCalldata,
        IDepositSecurityModule.Signature[] calldata sortedGuardianSignatures
    ) external onlyDelegateCall {
        if (IERC20(weth).balanceOf(address(this)) < amount)
            revert NotEnoughWeth();

        uint256 unfinalizedStETH = withdrawalQueue.unfinalizedStETH();
        uint256 bufferedEther = ISteth(steth).getBufferedEther();
        if (bufferedEther < unfinalizedStETH)//1
            revert InvalidWithdrawalQueueState();

        _wethToWSteth(amount);//2 - this increases the bufferedEther
        depositSecurityModule.depositBufferedEther(
            blockNumber,
            blockHash,
            depositRoot,
            stakingModuleId,
            nonce,
            depositCalldata,
            sortedGuardianSignatures
        );
    }

    function _wethToWSteth(uint256 amount) private {
        IWeth(weth).withdraw(amount);
        ISteth(steth).submit{value: amount}(address(0));
        IERC20(steth).safeIncreaseAllowance(address(wsteth), amount);
        IWSteth(wsteth).wrap(amount);
    }

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[WangSecurity]

Could please elaborate on what is unfinalizedStETH and how it can become larger than buffered eth?

[blckhv]

unfinalizedStETH returns the queued stETH withdrawals from Lido's withdrawal queue. When more withdrawal requests are initiated cumulativeStETH in the queue is increased, or in simpler terms when there are more beacon chain withdrawals than deposits it becomes larger. The idea here is that this Strategy will stake stETH and thus increase the bufferedEth, but since the check is performed before _wethToWSteth makes it wrong.

[IlIlHunterlIlI]

if it increases stETH -> increasing bufferedEth
then this check will pass? or what am i missing here? if (bufferedEther < unfinalizedStETH)
from what i see we just need bufferedEth to be bigger, stacking makes it bigger, then check passes?

[10xhash]

The intention here is to not make a deposit just so that it will pass without any reverts in the lido side. The protocol wants to allocate their deposits specifically to Obol validators. If the currently buffered ETH is less than unfinalizedStETH, the deposits of mellow will be used to settle the unfinalizedStETH, hence why the check is implemented

[WangSecurity]

@10xhash as I understand from your comment, if the unfinalizedStETH is bigger than buffered ETH, then the deposit will not go to the Obol Validators. But the goal of the protocol (or idea) is to deposit to them, so they don't want deposits to go through if it doesn't go to Obol Validators.

I know it may look silly to clarify this part, but just want to confirm. Could you also share where you've got this info from?

btw, I've hidden one comment that was a bit aggressive and disrespectful.

[10xhash]

@10xhash as I understand from your comment, if the unfinalizedStETH is bigger than buffered ETH, then the deposit will not go to the Obol Validators. But the goal of the protocol (or idea) is to deposit to them, so they don't want deposits to go through if it doesn't go to Obol Validators.

I know it may look silly to clarify this part, but just want to confirm. Could you also share where you've got this info from?

btw, I've hidden one comment that was a bit aggressive and disrespectful.

Yes

https://github.com/lidofinance/lido-dao/blob/5fcedc6e9a9f3ec154e69cff47c2b9e25503a78a/contracts/0.4.24/Lido.sol#L682-L686

[WangSecurity]

Thank you, but I think there was misunderstanding, excuse me for poor question phrasing, I mean about the info that Mellow protocol wants to deposit only yo Obol validators? @10xhash

[10xhash]

The strategy is named SimpleDVTStakingStrategy, the module is named Obol, deposits are made with moduleId set to the SimpleDVTModule of lido, discord message and the above check itself

[blckhv]

Per my understanding of the Lido there can't be a scenario, which makes this Module's deposit to be allocated to another validator.
Yes, it is possible for the ETH that you've submitted to Lido to be used by the withdrawal queue, but your staked funds remains the same and hence you can't just lose your staked tokens. Lido is a representation of pooled staking and it is up to its decision what fund management will be applied to your stake, the only obligation that they have towards your funds is for you to receive the respective rewards based on the staked amount that you've provided.
https://docs.lido.fi/contracts/staking-router/#stake-allocation

[WangSecurity]

As I understand the problem is not that the funds will be lost. The problem is that the protocol wants to deposit the funds to Obol validators, but if a portion of funds is used to cover the withdrawals from another validator, then the withdrawer will receive that portion of ETH and the Mellow's funds will be partially allocated to another validator, not Obol. This partially answers my question at #260, @10xhash could I flag if this indeed what you meant?

[Slavchew]

The whole discussion here was about the same as in #260, not what is actually stated in the report. Please read the report again and express a judging opinion, based on it and this #213 (comment), because the escalation is going in the wrong direction and nothing about the issue was commented.

[10xhash]

As I understand the problem is not that the funds will be lost. The problem is that the protocol wants to deposit the funds to Obol validators, but if a portion of funds is used to cover the withdrawals from another validator, then the withdrawer will receive that portion of ETH and the Mellow's funds will be partially allocated to another validator, not Obol. This partially answers my question at #260, @10xhash could I flag if this indeed what you meant?

Yes, general idea is correct but you may be misunderstanding how withdrawals work. but if a portion of funds is used to cover the withdrawals from another validator, if unfinalized stETH is 100 (this has not been tied to any validator etc. A user has just requested to withdraw 100 steth) and mellow deposits 101 eth, then the first 100 eth will not be deposited to the beacon chain and will go towards settling the unfinalized steth. so no new deposits to beacon chain (and hence to obol) will be made here

[WangSecurity]

Thank you for clarification on that part.

Excuse me for the confusion and for reflecting more on the discussion rather than the report and the issue. Firstly, as we see it's intended the protocol wants to allocate their ETH specifically to Obol validators and doesn't want their ETH to be used for finalising withdrawals. Therefore, there's a special check in place to prevent such a scenario from happening, i.e. checking for buffered ETH being larger than unfinalised ETH or revert.

As I was informed, the defending side wants to get confirmation from the Lido team, that the scenario expressed in the report is viable. I agree the following scenario is possible and can prevent deposits:

Now, knowing that stETH::submit increases bufferedEther let’s consider the following scenario:
bufferedEther = 95 ETH
unfinalizedStETH = 100 ETH
StakingModule::convertAndDeposit with 10 ETH

Indeed, in that case, the deposit would revert. But it works as intended, in this case, 5 ETH from the deposit would be allocated to settling withdrawals, which is not the goal. Moreover, this DOS, and I believe it would be the denial of service on deposits, doesn't lead to loss of funds, lock of funds for >= 7 days or fails the time-sensitive functions.

Hence, I believe this report should remain invalid since it's a design decision and the impact doesn't qualify for M impact. If any of my assumptions are incorrect or you have additional information, please correct me and provide the info.

If not, then I'm planning to reject the escalation and leave the issue as it is.

[WangSecurity]

Result:
Invalid
Unique

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• 0x3b33: rejected