Lack of calling the ExitQueue#`push()` in the VaultEnterExit#`enterExitQueue()`, which lead to that a staker would never claim their unstaked-ETH from the Vault even if the exit queue is over

[hats-bug-reporter[bot]]

Github username: -- Submission hash (on-chain): 0xdad4f6e1599bc6b715fae42166a3596a9824491cfcee43acd27b01a3c3e2bbef Severity: high

Description: Title\ Lack of calling the ExitQueue#push() in the VaultEnterExit#enterExitQueue(), which lead to that a staker would never claim their unstaked-ETH from the Vault even if the exit queue is over

Severity\ High

Description\ Within the ExitQueue library, the Checkpoint struct would be defined to store the totalTickets and the exitedAssets like this: https://github.com/stakewise/v3-core/blob/5996ae760a7e4a24d42029e64c56f3df087053cd/contracts/libraries/ExitQueue.sol#L20-L23

  /**
   * @notice A struct containing checkpoint data
   * @param totalTickets The cumulative number of tickets (shares) exited
   * @param exitedAssets The number of assets that exited in this checkpoint
   */
  struct Checkpoint {
    uint160 totalTickets;
    uint96 exitedAssets;
  }

Within the ExitQueue library, the checkpoints array storage would be defined as a property of the History struct like this: https://github.com/stakewise/v3-core/blob/5996ae760a7e4a24d42029e64c56f3df087053cd/contracts/libraries/ExitQueue.sol#L30

  /**
   * @notice A struct containing the history of checkpoints data
   * @param checkpoints An array of checkpoints
   */
  struct History {
    Checkpoint[] checkpoints;
  }

Within the ExitQueue#push(), a new checkpoint would be added to the checkpoints array storage like this: https://github.com/stakewise/v3-core/blob/5996ae760a7e4a24d42029e64c56f3df087053cd/contracts/libraries/ExitQueue.sol#L161

  /**
   * @notice Pushes a new checkpoint onto a History
   * @param self An array containing checkpoints
   * @param shares The number of shares to add to the latest checkpoint
   * @param assets The number of assets that were exited for this checkpoint
   */
  function push(History storage self, uint256 shares, uint256 assets) internal {
    if (shares == 0 || assets == 0) revert Errors.InvalidCheckpointValue();
    Checkpoint memory checkpoint = Checkpoint({
      totalTickets: SafeCast.toUint160(getLatestTotalTickets(self) + shares),
      exitedAssets: SafeCast.toUint96(assets)
    });
    self.checkpoints.push(checkpoint); /// @audit
  }

When getting the exit queue index to claim exited-assets, the VaultEnterExit#getExitQueueIndex() would be called. Within the VaultEnterExit#getExitQueueIndex(), the ExitQueue#getCheckpointIndex() would be called like this: https://github.com/stakewise/v3-core/blob/5996ae760a7e4a24d42029e64c56f3df087053cd/contracts/vaults/modules/VaultEnterExit.sol#L25

  /// @inheritdoc IVaultEnterExit
  function getExitQueueIndex(uint256 positionTicket) external view override returns (int256) {
    uint256 checkpointIdx = _exitQueue.getCheckpointIndex(positionTicket); /// @audit 
    return checkpointIdx < _exitQueue.checkpoints.length ? int256(checkpointIdx) : -1;
  }

Within the ExitQueue#getCheckpointIndex(), the checkpoint index would be returned based on the length of the checkpoints array storage like this: https://github.com/stakewise/v3-core/blob/5996ae760a7e4a24d42029e64c56f3df087053cd/contracts/libraries/ExitQueue.sol#L56

  /**
   * @notice Get checkpoint index for the burned shares
   * @param self An array containing checkpoints
   * @param positionTicket The position ticket to search the closest checkpoint for
   * @return The checkpoint index or the length of checkpoints array in case there is no such
   */
  function getCheckpointIndex(
    History storage self,
    uint256 positionTicket
  ) internal view returns (uint256) {
    uint256 high = self.checkpoints.length; /// @audit
    uint256 low;
    while (low < high) {
      uint256 mid = Math.average(low, high);
      if (_unsafeAccess(self.checkpoints, mid).totalTickets > positionTicket) {
        high = mid;
      } else {
        low = mid + 1;
      }
    }
    return high;
  }

According to the "Unstaking and exit queue" in the documentation, a user send an unstaking request (an exit queue request) when the user request to unstake ETH from a particular Vault this:

Unstaking and exit queue Whenever users request to unstake ETH from a particular Vault, the unbonded ETH is used first to process an unstaking request. If there isn't enough unbonded ETH in the Vault, a sufficient number of Vault validators will be automatically exited to provide enough ETH for unstaking. Since exiting validators from the Beacon Chain takes time, users who requested to unstake their ETH are placed in the exit queue. Once the exit queue is over, users can claim their unstaked ETH whenever they want.

Based on the documentation above, every single time the VaultEnterExit#enterExitQueue() would be called to lock the shares to the exit queue, the ExitQueue#push() is supposed to be called in order to add a new exit queue request (a new unstaking request) in the form of the Checkpoint struct data to the checkpoints array storage.

However, within the VaultEnterExit#enterExitQueue(), there is no line where the ExitQueue#push() would internally be called like this: https://github.com/stakewise/v3-core/blob/5996ae760a7e4a24d42029e64c56f3df087053cd/contracts/vaults/modules/VaultEnterExit.sol#L57-L83

  /// @inheritdoc IVaultEnterExit
  function enterExitQueue(
    uint256 shares,
    address receiver
  ) public virtual override returns (uint256 positionTicket) {
    _checkCollateralized();
    if (shares == 0) revert Errors.InvalidShares();
    if (receiver == address(0)) revert Errors.ZeroAddress();

    // SLOAD to memory
    uint256 _queuedShares = queuedShares;

    // calculate position ticket
    positionTicket = _exitQueue.getLatestTotalTickets() + _queuedShares;

    // add to the exit requests
    _exitRequests[keccak256(abi.encode(receiver, positionTicket))] = shares;

    // reverts if owner does not have enough shares
    _balances[msg.sender] -= shares;

    unchecked {
      // cannot overflow as it is capped with _totalShares
      queuedShares = SafeCast.toUint96(_queuedShares + shares);
    }

    emit ExitQueueEntered(msg.sender, receiver, positionTicket, shares);
  }

In addition to that, the VaultEnterExit#enterExitQueue() would internally called via the following functions:

• EthVault#enterExitQueue() (EthVault.sol#L111)
• EthErc20Vault#enterExitQueue() (EthErc20Vault.sol#L127)
• VaultOsToken#enterExitQueue() (VaultOsToken.sol#L148)

However, within the following functions above, there is also no line where the ExitQueue#push() would internally be called.

As a result, the checkpoints.length would never be increased. And therefore the the checkpoints.length would always be 0, meaning that the returned-value of the ExitQueue#getCheckpointIndex() would always be 0.

Due to that, the number of the exit queues (checkpoints.length) to be queued would always be 0. And therefore, a staker would never claim their unstaked-ETH from the Vault even if the exit queue is over.

Recommendation:\ Within the VaultEnterExit#enterExitQueue(), consider adding the ExitQueue#push() like this:

  /// @inheritdoc IVaultEnterExit
  function enterExitQueue(
    uint256 shares,
    address receiver
  ) public virtual override returns (uint256 positionTicket) {
    ...
    // SLOAD to memory
    uint256 _queuedShares = queuedShares;

    // calculate position ticket
    positionTicket = _exitQueue.getLatestTotalTickets() + _queuedShares;

    // add to the exit requests
    _exitRequests[keccak256(abi.encode(receiver, positionTicket))] = shares;

+   // calculate the amount of assets that can be exited
+   uint256 unclaimedAssets = _unclaimedAssets;
+   uint256 exitedAssets = Math.min(
+     _vaultAssets() - unclaimedAssets,
+     convertToAssets(_queuedShares)
+   );
+   if (exitedAssets == 0) return 0;

+   // calculate the amount of shares that can be burned
+   burnedShares = convertToShares(exitedAssets);
+   if (burnedShares == 0) return 0;

+   queuedShares = SafeCast.toUint96(_queuedShares - burnedShares);
+   _unclaimedAssets = SafeCast.toUint96(unclaimedAssets + exitedAssets);

+   // push checkpoint so that exited assets could be claimed
+   _exitQueue.push(burnedShares, exitedAssets);
     ...
  }

[tsudmi]

The exit queue is processed outside the enterExitQueue call, it must be processed during the state update.