Description

In the LidoVault, the variable-length array fixedOngoingWithdrawalUsers is used to store a list of fixed-side users that requested a withdrawal while the vault has not ended.

During the vault-finalization process, the internal function claimOngoingFixedWithdrawals() is called to finish all of the ongoing fixed-side deposits so that fees can be properly accounted for even if the fixed-side user never completes their withdrawal themselves.

This function loops through the fixedOngoingWithdrawalUsers array:

function claimOngoingFixedWithdrawals() internal {
  uint256 arrayLength = fixedOngoingWithdrawalUsers.length;
  for (uint i = 0; i < arrayLength; i++) {
    address fixedUser = fixedOngoingWithdrawalUsers[i];
    fixedToPendingWithdrawalAmount[fixedUser] = claimFixedVaultOngoingWithdrawal(fixedUser);
  }
}

The function that it calls inside the loop, claimFixedVaultOngoingWithdrawal, however, also loops through the array to set the user in question to zero:

function claimFixedVaultOngoingWithdrawal(address user) internal returns (uint256) {
  // [...]

  uint256 arrayLength = fixedOngoingWithdrawalUsers.length;
  for (uint i = 0; i < arrayLength; i++) {
    if (fixedOngoingWithdrawalUsers[i] == user) {
      delete fixedOngoingWithdrawalUsers[i];
    }
  }

  // [...]
}

This means the overall vault finalization process runs in $O(n^2)$ time, if $n$ is the number of users. Note that the delete statement does not reduce the size of the array, it sets the array entry at index i to zero.

Impact

The only limit on the number of fixed-side depositors is the fixed-side capacity divided by the minimum deposit. If this implicit limit is too large, then it is possible that a vault with many fixed-side depositors cannot be finalized because it would take too much gas.

Recommendations

We recommend reworking the withdrawal logic to not require traversing a variable-length array.

Alternatively, adding an optional parameter to claimFixedVaultOngoingWithdrawal that provides a hint for the index of the user to be removed will make the overall logic linear-time in all cases — either the caller or the callee takes $O(n)$ to find the index.

Alternatively, if the suggestion in Discussion ref↗ is implemented, then an ongoing fixed withdrawal would be settled immediately in stETH (with optional use of the withdrawal queue happening in a different contract), and so this would not be an issue.

Remediation

This issue has been acknowledged by Saffron, and a fix was implemented in commit a75cf521↗.