Lido

The main purpose of the function MpcManager::reportGeneratedKey is to verify that it has been called by every participant of the authorized MPC group for the key to be generated.

// MpcManager.sol:reportedGeneratedKey:219
if (confirmationCount == groupSize) {
  _keyToGroupIds[generatedPublicKey] = groupId;
  lastGenPubKey = generatedPublicKey;
  lastGenAddress = _calculateAddress(generatedPublicKey);
  emit KeyGenerated(groupId, generatedPublicKey);
  // Dedaub: The keygen request should be marked as COMPLETED now
  // Dedaub: The following lines should be added
  // lastKeygenRequest = KeygenStatusHelpers.
  //   makeKeygenRequest(groupId, uint8(KeygenStatus.COMPLETED));

Even though this functionality is implemented correctly, the status of the keygen request is not updated to COMPLETED when it is identified that all group members have reported, i.e., confirmationCount == groupSize. As a result, the next call to function requestKeygen will revert as it requires the status of the lastKeygenRequest to not be set to REQUESTED, which is as it is never updated by reportGeneratedKey. The only way to bypass this and allow the protocol to continue functioning would be to call cancelKeygen after every successful key generation to change the status of the lastKeygenRequest to CANCELED. Of course, this would be messy and just a compromise.

The function MpcManager::_isSortedAscendingOrder does not ensure that all keys are unique as it requires that pkey[i] >= pkey[i-1] or in other words that a key at position i of the public keys array to be greater or equal to that at position i-1.

// MpcManager.sol::_isSortedAscendingOrder:339
function _isSortedAscendingOrder(bytes[] calldata publicKeys)
    private pure returns (bool) {
  uint256 prev = 0;

  for (uint256 i = 0; i < publicKeys.length; i++) {
    uint256 curr = uint256(bytes32(publicKeys[i][:32]));
    if (curr < prev) {
      return false;
    }
    prev = curr;
  }
  return true;
}

An MPC group should strictly consist of unique public keys (members) as otherwise concepts like the MPC group threshold lose their value/purpose.

In the original audit, we reported that AvaLido::fillUnstakeRequests contains an unbounded loop that could allow an adversary to perform a DoS by filling a large number of unstake requests (original audit issue H1). This vulnerability has been mostly addressed by performing a partial execution, up to an unstakeLoopBound, so that the remaining requests will be filled in the next execution and progress is always guaranteed.

However, AvaLido::fillUnstakeRequests is written in a way that works even if the queue contains requests that are already filled (it’s not clear whether this can really happen, but it’s a good practice to not make unnecessary assumptions). Filled requests are simply ignored by a check in the code:

for (uint256 i = unfilledHead; i < unstakeRequests.length; i++) {
  if (remaining == 0) {
    return (false, 0);
  }

  // Return early to prevent unstake flooding
  if (numberFilled == unstakeLoopBound) {
  	return (false, remaining);
  }

  // Dedaub: the following 'if' ignores already filled requests
  if (unstakeRequests[i].amountFilled < unstakeRequests[i].amountRequested) {
    	...
      unfilledHead = i + 1;
      ...
  }

  remaining = inputAmount - amountFilled;
  numberFilled++;
}

However, in the context of the DoS vulnerability, unfilled requests might nullify the unstakeLoopBound protection! This is due to the fact that the unfilledHead is only modified inside the if branch, only for unfilled requests. If there is a large number of filled requests in the queue, we might reach unstakeLoopBound without ever entering the if branch, hence without ever modifying the queue head. The loop will terminate, but no actual progress will be made, the next execution will have to iterate the same exact requests starting from the same unfilledHead, so any future execution will also fail.

To ensure that progress is always made, we recommend that unfilledHead is always modified (outside the if branch).

Function Oracle::getLatestValidators checks if there is an ongoing update of node ids in order to not return an array of invalidated pointers back to the node ids list. Nevertheless, there exist other functions such as getAllValidatorsByEpochId and allValidatorNodeIds that do not implement such checks even though they might provide inaccurate data if called during an update. For example, if the id of the latest finalized epoch is provided to getAllValidatorsByEpochId, it should return the same result as getLatestValidators but it does not. The severity of the issue is mitigated by the check in getLatestValidators, which in combination with another check in Oracle::selectValidatorsForStake does not allow the contract execution to reach calls to the other view functions if an update is in progress.

The function MpcManager::createGroup checks that the provided publicKeys array parameter is sorted in ascending order by calling _isSortedAscendingOrder. However, the call happens inside a loop which goes over the publicKeys array, thus in every iteration of that loop it is checked that the immutable publicKeys array is sorted, which is clearly wasteful.

// MpcManager.sol::createGroup:141
function createGroup(bytes[] calldata publicKeys, uint8 threshold)
    external onlyRole(ROLE_MPC_MANAGER) {
  // ..

  bytes memory b;
  for (uint256 i = 0; i < groupSize; i++) {
    if (publicKeys[i].length != PUBKEY_LENGTH) revert InvalidPublicKey();
    if (!_isSortedAscendingOrder(publicKeys))  // Dedaub: wasteful op
      revert PublicKeysNotSorted();
    b = bytes.concat(b, publicKeys[i]);
  }

  // ..
}

The ordering check should be moved before the loop.

According to comments in the code the functions setPrincipalTreasuryAddress and setRewardTreasuryAddress of the AvaLido contract exist to be used just for initialization purposes. Contrary to what the comments describe, the two functions can be called multiple times as there is no check which would lead to the execution’s reversion when the principalTreasury and rewardTreasury storage variables have already been set.

Two recent changes have been made to AvaLido::claim.

First, to save gas, requests are only updated on partial claims:

// To save gas we only update the mapping on partial claims.
// We delete full claims at the end of the function.
if (!isFullyClaimed(request)) {
   unstakeRequests[requestIndex] = request;
}

Second, transfers are made with sender.call instead of the previously used sender.transfer (a good practice, to avoid hard-coded gas limits):

(bool success, ) = msg.sender.call{value: amountAVAX}("");
if (!success) revert TransferFailed();

if (isFullyClaimed(request)) {
  …
  delete unstakeRequests[requestIndex];
  …
}

Note also that the transfer is made before deleting the full claim.

So the effect of these two changes is that the adversary-controlled receive hook is now called with no gas restriction, and before updating the protocol’s state, which makes it susceptible to reentrancy attacks.

Of course, the function is marked as nonReentrant, so no reentrancy attack is really possible. Nevertheless, there is no need to introduce changes that weaken the protocol’s protection (it is enough to forget a reentrancy guard in any function in the future to introduce a vulnerability). So we recommend simply performing the transfer after all updates to the protocol’s state.

The function ValidatorSelector::getAvailableValidatorsWithCapacity could return early if the call to Oracle::getLatestValidators returns an empty array (e.g., during an update) in order to save on gas.

The function MpcManager::joinRequest makes two consecutive calls to ConfirmationHelpers::confirm while ignoring the result of the first one at line 245 of MpcManager.sol.

In the function ValidatorHelpers::getNodeIndex the comment “Take 12 bits from the middle which represents our index” is inaccurate after the latest changes to the code. It should be updated to “Take the first 14 bits which represent our index”.

The code is compiled with Solidity 0.8.10, which, at the time of writing, has some known bugs, which we do not believe to affect the correctness of the contracts.