Lido Report 3 and incidents

# Lido Report 3 and incidents ###### tags `Lido`, `Ethereum`, `Vlad` [TOC] ## Findings Report ### CRITICAL No critical issues were found. ### MAJOR No major issues were found. ### WARNING #### [NEW] Missing validations in `unsafeChangeDepositedValidators` in `Lido` ##### Description In the function [`unsafeChangeDepositedValidators`](https://github.com/lidofinance/lido-dao/blob/e45c4d6fb8120fd29426b8d969c19d8a798ca974/contracts/0.4.24/Lido.sol#L613) of `Lido` contract there is not enough checks of the `_newDepositedValidators` variable and validations when the function can be invoked or not. Even considering that the function is operated via `UNSAFE_CHANGE_DEPOSITED_VALIDATORS_ROLE` which belongs to Lido DAO, the risk of using it incorrectly is very high, the following risks are: * In the [`_getTransientBalance`](https://github.com/lidofinance/lido-dao/blob/e45c4d6fb8120fd29426b8d969c19d8a798ca974/contracts/0.4.24/Lido.sol#L1098) function of the Lido contract there is an `assert` that validates if `depositedValidators >= clValidators`, but due to the lack of validation of input variable in `unsafeChangeDepositedValidators` function the variable in `DEPOSITED_VALIDATORS_POSITION` can be changed so the `assert` will be met, while the `assert` should be used only for invariants, and the fail of the `assert` will lead to the failed report, which is critical point for Lido. Futhermore the incorrect setting for smaller value of the deposited validators with the `unsafeChangeDepositedValidators` function will revert `require` in [`_processClStateUpdate`](https://github.com/lidofinance/lido-dao/blob/e45c4d6fb8120fd29426b8d969c19d8a798ca974/contracts/0.4.24/Lido.sol#L810) function and lead to incorrect accounting. * If the [`deposit`](https://github.com/lidofinance/lido-dao/blob/e45c4d6fb8120fd29426b8d969c19d8a798ca974/contracts/0.4.24/Lido.sol#L694) function is executed right before the inokation of `unsafeChangeDepositedValidators` function, the `unsafeChangeDepositedValidators` will overwrite the value in `DEPOSITED_VALIDATORS_POSITION` which was set during the `deposit` function to the other one since the [`unsafeChangeDepositedValidators`](https://github.com/lidofinance/lido-dao/blob/e45c4d6fb8120fd29426b8d969c19d8a798ca974/contracts/0.4.24/Lido.sol#L616) is setting the new value isntead of adding value to the existing one. This leads to icorrect accounting of the deposited validators. * In the [`unsafeChangeDepositedValidators`](https://github.com/lidofinance/lido-dao/blob/e45c4d6fb8120fd29426b8d969c19d8a798ca974/contracts/0.4.24/Lido.sol#L613) function of the Lido contract the `canDeposit` `modifier` is missing, so the amount of the deposited validators can change during bunker mode, or when all staking modules are stopped or paused. * After the [`unsafeChangeDepositedValidators`](https://github.com/lidofinance/lido-dao/blob/e45c4d6fb8120fd29426b8d969c19d8a798ca974/contracts/0.4.24/Lido.sol#L613) function of the Lido contract the amount of deposited validators is changed, but these validators doesn't associate with any of the existing staking modules. Thus it is possible to deposit more then `_maxDepositsCount` of any staking module, there is no execution of the `obtainDepositData` hook of the staking module address, there is no update of the `lastDepositBlock` variable in DSM and `stakingModule.lastDepositAt`, `stakingModule.lastDepositBlock` variables in the StakingRouter contract. ##### Recommendation We recommend refactoring the `unsafeChangeDepositedValidators` function with more validations for bunker mode, introducing interactions with the stakingRouter contract, adding value to the existing amount of valitators instead of setting it. ### INFO #### [NEW] Out-of-gas validation in `StakingRouter` ##### Description In the [`reportRewardsMinted`](https://github.com/lidofinance/lido-dao/blob/e45c4d6fb8120fd29426b8d969c19d8a798ca974/contracts/0.8.9/StakingRouter.sol#L303) function of `StakingRouter` contract there is a `try/catch` block for validation of the `out-of-gas` error, but the error `UnrecoverableModuleError` is not used in the [`_estimate_gas`](https://github.com/lidofinance/lido-oracle/blob/3.0.0-rc.1/src/web3py/extensions/tx_utils.py#L90) function for recalculating of gas if the Ethereum nodes proposed incorrect amount of gas. ##### Recommendation We recommend recalculating neccessary amount of gas in `_estimate_gas` function if the `UnrecoverableModuleError` occurs. #### [NEW] No logic for reward distribution mannually in `StakingRouter` ##### Description In the [`getStakingRewardsDistribution`](https://github.com/lidofinance/lido-dao/blob/e45c4d6fb8120fd29426b8d969c19d8a798ca974/contracts/0.8.9/StakingRouter.sol#L1022-L1027) function of `StakingRouter` contract if the module is stopped, the reward for that module will go to the Lido treasury, but there is no logic for distributing the rewards to the staking module with the `onRewardsMinted` call. If the module is unstopped and Lido DAO decides to transfer this rewards to the staking module, the `onRewardsMinted` call of the staking module won't be executed, since it's very likely that this hook will be authorized from `StakingRouter` contract. ##### Recommendation We recommend adding a function for transfering rewards in this case with `onRewardsMinted` call on the `StakingRouter` contract. #### [NEW] Penalty timestamp not implemented in `StakingRouter` ##### Description In the `StakingRouter` contract the [`stuckPenaltyEndTimestamp`](https://github.com/lidofinance/lido-dao/blob/e45c4d6fb8120fd29426b8d969c19d8a798ca974/contracts/0.8.9/StakingRouter.sol#L684) variable is not used in the [`getStakingRewardsDistribution`](https://github.com/lidofinance/lido-dao/blob/e45c4d6fb8120fd29426b8d969c19d8a798ca974/contracts/0.8.9/StakingRouter.sol#L986) function, so there is no penalty for stuck validators to the staking module. ##### Recommendation We recommend adding `stuckPenaltyEndTimestamp` variable to the `getStakingRewardsDistribution` in order to keep track and penalize staking modules for stuck validators. ### INCIDENTS #### Frontrun `deposit_root` for pausing deposits to LIDO ##### Description According to the [EIP-6110](https://eips.ethereum.org/EIPS/eip-6110) there is a probability for DoS vector with the minimal deposit amount as 1 ETH, even though it's not related with the `depositRoot`, the risks related to the DoS of minimal deposit amount should be considered. In the function [`depositBufferedEther`](https://github.com/lidofinance/lido-dao/blob/e45c4d6fb8120fd29426b8d969c19d8a798ca974/contracts/0.8.9/DepositSecurityModule.sol#L433) of `DepositSecurityModule` contract there is a check that the `onchainDepositRoot` on the `deposit_contract` hasn't changed and equals to the `depositRoot` provided from the calldata. There is a risk of frontrunning every single transaction of the `depositBufferedEther` and creating deposit directly through `deposit_contract` with minimal stake amount in order to change the `deposit_root` and revert the execution of the `depositBufferedEther` function which will lead to the pausing of deposits to all staking modules. ##### Solution In total there is a `24 * 60 * 60 / 12 = 7200` blocks on ethereum network, according to the [beacon statistics](https://beaconscan.com/stat/deposits) the max day deposits is around 200.000 ETH per day, which can be up to 200.000 validators taking in consideration that minimum deposit amount is 1 ETH. Anyone can frontrun deposits signed by guardians in order to revert the function execution and prevent lido from staking. This attack can be executed with at least 7200 ETH in order to block Lido deposits for all day long, which is a very big amount of ETH, but at the same time this ETH can be withdrawn from the beacon chain after adding additional ETH up to 32 ETH. There is a probability, that this attack can take place, since the attacker doesn't lose any funds, but at the same time there is no immediate profit. This attack can be executed by other protocols that work with consensus layer staking during days, when there are a lot of people willing to make a deposit to the beacon chain and by pausing the deposits in Lido they will push users to try other liquid staking protocol. But this is still a very expensive attack and can be executed only for a very limited range of purposes, without making harm to the Lido protocol itself. Futhermore, the user can deposit himself and after that transfer the keys to LIDO and become transient vals. #### STETH/ETH stability during bunker mode ##### Description During the bunker mode withdrawal requests are not finalized, which can become a reason of moving the price in STETH/ETH pool from 1/1 to other values. ##### Solution During the bunker mode withdrawal requests are accepted and user can exit Lido staking in order to stop taking risks of futher slashings. Since a slashing is associated with the withdrawal request, after the the slashing is covered the withdrawal request can be finalized and user will receive his funds no matter how much slashings will occur later during the covering proccess. Taking into the consideration that before the Lido update there was no withdrawals and users continues to hold STETH tokens in the liquidity provider pools for collecting fees, the price stability in the pool more likely will remain as it is, even though the withdrawal proccess will take longer time. #### `getStakingRewardsDistribution` returns empty values for stopped modules ##### Description In the function `handleOracleReport` of [`Lido`](https://github.com/lidofinance/lido-dao/blob/e45c4d6fb8120fd29426b8d969c19d8a798ca974/contracts/0.4.24/Lido.sol#L572) contract during the [`_processRewards`](https://github.com/lidofinance/lido-dao/blob/e45c4d6fb8120fd29426b8d969c19d8a798ca974/contracts/0.4.24/Lido.sol#L895) function call the fee is distributed only on the profitable reports. If the report was profitable the Lido contract will call `StakingRouter` with the [`getStakingRewardsDistribution`](https://github.com/lidofinance/lido-dao/blob/e45c4d6fb8120fd29426b8d969c19d8a798ca974/contracts/0.8.9/StakingRouter.sol#L986) call for calculations. If the module is stopped, but it has active validators, the module will be present in the [return arrays](https://github.com/lidofinance/lido-dao/blob/e45c4d6fb8120fd29426b8d969c19d8a798ca974/contracts/0.8.9/StakingRouter.sol#L990-L992) of this function, but the [`stakingModuleFee`](https://github.com/lidofinance/lido-dao/blob/e45c4d6fb8120fd29426b8d969c19d8a798ca974/contracts/0.8.9/StakingRouter.sol#L1028) of this module will be zero. ##### Solution The module with empty fees will be present in all `for` loops in [`_transferModuleRewards`](https://github.com/lidofinance/lido-dao/blob/e45c4d6fb8120fd29426b8d969c19d8a798ca974/contracts/0.4.24/Lido.sol#L1063) function of Lido contract and [`reportRewardsMinted`](https://github.com/lidofinance/lido-dao/blob/e45c4d6fb8120fd29426b8d969c19d8a798ca974/contracts/0.8.9/StakingRouter.sol#L300) of `StakingRouter` contract, but the calculations won't be procceded with the module and there will be no [`onRewardsMinted`](https://github.com/lidofinance/lido-dao/blob/e45c4d6fb8120fd29426b8d969c19d8a798ca974/contracts/0.8.9/StakingRouter.sol#L303) call, which is correct logic so returning empty values from arrays doesn't create any problems, but it still cost gas for iterating over the loop for the stopped modules with actieve validators, which should be refactored. #### Length of `_depositCalldata` doesn't equal amount of deposits ##### Description In the [`deposit`](https://github.com/lidofinance/lido-dao/blob/e45c4d6fb8120fd29426b8d969c19d8a798ca974/contracts/0.8.9/StakingRouter.sol#L1093) function of the `StakingRouter` contract the `_depositCalldata` variables' length is not validated with the `_depositsCount`. For e.g. it's possible to pass 2 deposits with 1 key, which will lead to revert, or with 3 keys which will lead to 1 unused key, because the `_depositsCount` variable is passed to [`_makeBeaconChainDeposits32ETH`](https://github.com/lidofinance/lido-dao/blob/e45c4d6fb8120fd29426b8d969c19d8a798ca974/contracts/0.8.9/StakingRouter.sol#L1125) function. ##### Solution In the [`obtainDepositData`](https://github.com/lidofinance/lido-dao/blob/e45c4d6fb8120fd29426b8d969c19d8a798ca974/contracts/0.8.9/StakingRouter.sol#L1121) function call to the `stakingModuleAddress` contract returned `publicKeysBatch` and `signaturesBatch` must return the exact number of requested keys according to the [pull request](https://github.com/lidofinance/lido-dao/pull/626). Even if the staking module doesn't return correct amount of public keys and signatures, there is a check [`_makeBeaconChainDeposits32ETH`](https://github.com/lidofinance/lido-dao/blob/e45c4d6fb8120fd29426b8d969c19d8a798ca974/contracts/0.8.9/BeaconChainDepositor.sol#L47-L52) function preventing incorrect handling of these variables.