Lido V2 Contest Report

The contest audit report and MixBytes make no statements or warranties about the utility of the code, the safety of the code, suitability of the business model, investment advice, endorsement of the platform or its products, regulatory regime for the business model, or any other statements about the fitness of the contracts to purpose, or their bug-free status. The contest documentation is for discussion and information purposes only.

Mixbytes services offered are limited to contest hosting and contest conduct assistance on the MixBytes Camp platform and within the MixBytes Camp community. Mixbytes does not provide audit services within this contest. MixBytes Camp community members are not employees, partners, agents, contractors, or subcontractors of Mixbytes. MixBytes in no way shall be held liable or responsible for MixBytes Camp community members. MixBytes in no way controls or distributes the bounty fund. Mixbytes does not conduct any KYC verification on its platform.

The audit contest takes place in order to identify possible security vulnerabilities and flaws by the independent security researchers who are team members of the MixBytes Camp community.

The result of the contest is a unified report containing a list of all triaged vulnerabilities found during the contest, ranked according to the approved severity level.

Within the scope of the audit contest, there is no stage of re-audit after bug fixing on the Project’s side.

All vulnerabilities discovered during the audit contest are classified based on their potential severity and ranked in the following way:

The Lido Ethereum Liquid Staking Protocol built on Ethereum allowes their users to earn staking rewards on the Beacon chain without locking Ether or maintaining staking infrastructure.

The second version of the protocol (Lido V2) has a withdrawal feature. This is a complex task due to the difficulty of synchronizing access to data between the Consensus and Execution Layers, as well as the async nature of the Consensus Layer validator exit and slashing mechanics.

Lido V2 handles withdrawal requests in an asynchronous manner using a FIFO queue. The mechanics are hugely affected by the asynchronous nature of the Ethereum withdrawal operations.

Lido V2 enables various staking modules to be introduced with the Staking Route.

The Staking Router contract is responsible for appropriately balancing stake distribution, accrued rewards and operation of the plugged-in staking modules.

The audit covered the following files:

In the pauseDeposits function a user pauses deposits for the staking module if the sig param is a valid signature by the guardian.
The pauseIntentValidityPeriodBlocks param is set in constructor or setPauseIntentValidityPeriodBlocks and is more than 0 but this param has no limits and can have a big value.

Thus, the sig message will be valid and can be used again during a pause intent.

A possible attack flow:

1. A user with the sig param which is a valid signature pauses deposits for the staking module.
2. The owner calls unpauseDeposits with stakingModuleId.
3. During a pause intent another user calls pauseDeposits with this sig and pauses deposits for the staking module again.

https://github.com/lidofinance/lido-dao/blob/e57517730c3e11a41e9cbc32ce018726722335b7/contracts/0.8.9/DepositSecurityModule.sol#L336
https://github.com/lidofinance/lido-dao/blob/e57517730c3e11a41e9cbc32ce018726722335b7/contracts/0.8.9/DepositSecurityModule.sol#L356
https://github.com/lidofinance/lido-dao/blob/e57517730c3e11a41e9cbc32ce018726722335b7/contracts/0.8.9/DepositSecurityModule.sol#L361

It’s recommended to add limits for pauseIntentValidityPeriodBlocks and add a message to reuse the check.

Fixed in commit:
https://github.com/lidofinance/lido-dao/commit/29efd7f69df2ef983ca07c43e7e855d05c3ca8c3.

This bug was found with the Echidna fuzzer.
We have revert for the type(int256).min value in the abs function because the range for int256 is – 2²⁵⁵… 2²⁵⁵ – 1.
https://github.com/lidofinance/lido-dao/blob/e57517730c3e11a41e9cbc32ce018726722335b7/contracts/common/lib/Math256.sol#L13

It’s recommended to add a check for this case.

Acknowledged

The depositBufferedEther() function allows to pass some depositCalldata for some users.
A user can frontrun a transaction with valid input data and replace it with depositCalldata.

https://github.com/lidofinance/lido-dao/blob/e57517730c3e11a41e9cbc32ce018726722335b7/contracts/0.8.9/DepositSecurityModule.sol#L413
https://github.com/lidofinance/lido-dao/blob/e57517730c3e11a41e9cbc32ce018726722335b7/contracts/0.8.9/DepositSecurityModule.sol#L419

It’s recommended to add depositCalldata to msgHash in the _verifySignatures function.

Acknowledged

The range for variable _delay is not set:
https://github.com/lidofinance/lido-dao/blob/e57517730c3e11a41e9cbc32ce018726722335b7/contracts/0.4.24/nos/NodeOperatorsRegistry.sol#L1303.

A possible flow:

1. A user with MANAGE_NODE_OPERATOR_ROLE can set a big value for _delay accidentally.
2. A user with STAKING_ROUTER_ROLE calls updateRefundedValidatorsCount or updateStuckValidatorsCount functions and sets penalty in stuckPenaltyStats if needed:
3. https://github.com/lidofinance/lido-dao/blob/e57517730c3e11a41e9cbc32ce018726722335b7/contracts/0.4.24/nos/NodeOperatorsRegistry.sol#L622
4. https://github.com/lidofinance/lido-dao/blob/e57517730c3e11a41e9cbc32ce018726722335b7/contracts/0.4.24/nos/NodeOperatorsRegistry.sol#L644.
5. A user with MANAGE_NODE_OPERATOR_ROLE undoes _delay to the normal value but cannot undo stuckPenaltyStats.

It’s recommended to add an allowable range for variable _delay.

Fixed in commit:
https://github.com/lidofinance/lido-dao/commit/57b675212a68db308e690e6daacc37ae99ca2f99.

Unused functions in library UnstructuredRefStorage:
https://github.com/lidofinance/lido-dao/blob/e57517730c3e11a41e9cbc32ce018726722335b7/contracts/0.8.9/lib/UnstructuredRefStorage.sol#L7
https://github.com/lidofinance/lido-dao/blob/e57517730c3e11a41e9cbc32ce018726722335b7/contracts/0.8.9/lib/UnstructuredRefStorage.sol#L13.

It’s recommended to remove unused functions.

Fixed in commit:
https://github.com/lidofinance/lido-dao/commit/0a9dbcbb1c035fff0c97128aee3a0f8530ece586.

There are lost type bytes in constructor:
https://github.com/lidofinance/lido-dao/blob/e57517730c3e11a41e9cbc32ce018726722335b7/contracts/0.8.9/EIP712StETH.sol#L56.

It’s recommended to add a bytes type to exclude the implicit type conversion

Acknowledged

In hashes:
https://github.com/lidofinance/lido-dao/blob/e57517730c3e11a41e9cbc32ce018726722335b7/contracts/0.8.9/utils/access/AccessControlEnumerable.sol#L22
https://github.com/lidofinance/lido-dao/blob/e57517730c3e11a41e9cbc32ce018726722335b7/contracts/0.8.9/utils/access/AccessControl.sol#L60
https://github.com/lidofinance/lido-dao/blob/e57517730c3e11a41e9cbc32ce018726722335b7/contracts/0.8.9/utils/Versioned.sol#L24
https://github.com/lidofinance/lido-dao/blob/e57517730c3e11a41e9cbc32ce018726722335b7/contracts/0.8.9/utils/PausableUntil.sol#L12.

It’s recommended to improve the security of the constants by decreasing it by -1 just like it is described in reference implementation in EIP-1967.

Fixed in commits:
https://github.com/lidofinance/lido-dao/commit/7e2a32f8b60e400a091d7a119e7095bb04a6e14b,
https://github.com/lidofinance/lido-dao/commit/ddf756779a07076460272a24b4491ef5542cd5e5.

admin:
https://github.com/lidofinance/lido-dao/blob/e57517730c3e11a41e9cbc32ce018726722335b7/contracts/0.8.9/sanity_checks/OracleReportSanityChecker.sol#L149
lido:
https://github.com/lidofinance/lido-dao/blob/e57517730c3e11a41e9cbc32ce018726722335b7/contracts/0.8.9/oracle/AccountingOracle.sol#L157

It’s recommended to add null data checks.

Fixed in commit:
https://github.com/lidofinance/lido-dao/commit/da4ac3420f8dc7cd4ada08560926d0b183405751.

It is expected that two arrays would have the same length as they contain corresponding data. But their lengths aren’t compared.
_stakingModuleIds == _totalShares:
https://github.com/lidofinance/lido-dao/blob/e57517730c3e11a41e9cbc32ce018726722335b7/contracts/0.8.9/StakingRouter.sol#L260
_stakingModuleIds == _exitedValidatorsCounts:
https://github.com/lidofinance/lido-dao/blob/e57517730c3e11a41e9cbc32ce018726722335b7/contracts/0.8.9/StakingRouter.sol#L271

It's recommended to check that the arrays lengths are the same.

Fixed in commit:
https://github.com/lidofinance/lido-dao/commit/25a53c093f530e6315a87d1b5c95911ee6d081fe.

Typos in texts below:

1. panlty => penalty: https://github.com/lidofinance/lido-dao/blob/e57517730c3e11a41e9cbc32ce018726722335b7/contracts/0.4.24/nos/NodeOperatorsRegistry.sol#L111
2. timastamp => timestamp: https://github.com/lidofinance/lido-dao/blob/e57517730c3e11a41e9cbc32ce018726722335b7/contracts/0.4.24/nos/NodeOperatorsRegistry.sol#L113
3. validoator => validator: https://github.com/lidofinance/lido-dao/blob/e57517730c3e11a41e9cbc32ce018726722335b7/contracts/0.4.24/nos/NodeOperatorsRegistry.sol#L656
4. reawards => reawards: https://github.com/lidofinance/lido-dao/blob/e57517730c3e11a41e9cbc32ce018726722335b7/contracts/0.4.24/nos/NodeOperatorsRegistry.sol#L115
5. memebrs => members: https://github.com/lidofinance/lido-dao/blob/e57517730c3e11a41e9cbc32ce018726722335b7/contracts/0.8.9/oracle/HashConsensus.sol#L308

It’s recommended to correct them.

Fixed in commit:
https://github.com/lidofinance/lido-dao/commit/59b0476fef2c89a09e3b7cca264923eb6f9635b9.

We have a comment in code:
“if staking was paused or unlimited previously, or the new limit is lower than previous, then reset the prev stake limit to the new max stake limit”
https://github.com/lidofinance/lido-dao/blob/e57517730c3e11a41e9cbc32ce018726722335b7/contracts/0.4.24/lib/StakeLimitUtils.sol#L148.

But conditions don’t check the case when staking was paused:
https://github.com/lidofinance/lido-dao/blob/e57517730c3e11a41e9cbc32ce018726722335b7/contracts/0.4.24/lib/StakeLimitUtils.sol#L151
Thus, we can add a condition: _data.prevStakeBlockNumber == 0.

It’s recommended to add a check.

Acknowledged

Use magic number 1e9:
https://github.com/lidofinance/lido-dao/blob/e57517730c3e11a41e9cbc32ce018726722335b7/contracts/0.8.9/oracle/AccountingOracle.sol#L585
https://github.com/lidofinance/lido-dao/blob/e57517730c3e11a41e9cbc32ce018726722335b7/contracts/0.8.9/oracle/AccountingOracle.sol#L610.

It’s recommended to replace magic numbers with the constant.

Acknowledged

The _setMaxDeposits function can override the same value and emit an extra event:
https://github.com/lidofinance/lido-dao/blob/e57517730c3e11a41e9cbc32ce018726722335b7/contracts/0.8.9/DepositSecurityModule.sol#L189.

It’s recommended to add a check for the previous value.

Acknowledged

Return values and balances are not checked:
https://github.com/lidofinance/lido-dao/blob/e57517730c3e11a41e9cbc32ce018726722335b7/contracts/0.6.12/WstETH.sol#L57
https://github.com/lidofinance/lido-dao/blob/e57517730c3e11a41e9cbc32ce018726722335b7/contracts/0.8.9/Burner.sol#L166
https://github.com/lidofinance/lido-dao/blob/e57517730c3e11a41e9cbc32ce018726722335b7/contracts/0.8.9/Burner.sol#L199
https://github.com/lidofinance/lido-dao/blob/e57517730c3e11a41e9cbc32ce018726722335b7/contracts/0.8.9/WithdrawalQueue.sol#L380
https://github.com/lidofinance/lido-dao/blob/e57517730c3e11a41e9cbc32ce018726722335b7/contracts/0.8.9/WithdrawalQueue.sol#L390.

It’s recommended to add checks for return values and balances.

Acknowledged

It’s recommended to add conditions for the input values:

1. _maxTimestamp < getQueue()[startId].timestamp to: https://github.com/lidofinance/lido-dao/blob/e57517730c3e11a41e9cbc32ce018726722335b7/contracts/0.8.9/WithdrawalQueueBase.sol#L157.
2. Calc requiredEth for startId and add a _ethBudget < requiredEth check to: https://github.com/lidofinance/lido-dao/blob/e57517730c3e11a41e9cbc32ce018726722335b7/contracts/0.8.9/WithdrawalQueueBase.sol#L198.

Fixed in commits:
https://github.com/lidofinance/lido-dao/commit/2032f0dc57521437457a934daebe16d98e40b2bc,
https://github.com/lidofinance/lido-dao/commit/d1facbd5f3f39cfffe31201f2142f1eea9e1e1ce.

1. Double check
Function resume has a whenPaused modifier:
https://github.com/lidofinance/lido-dao/blob/e57517730c3e11a41e9cbc32ce018726722335b7/contracts/0.8.9/WithdrawalQueue.sol#L101
and function _resume has a whenPaused modifier:
https://github.com/lidofinance/lido-dao/blob/e57517730c3e11a41e9cbc32ce018726722335b7/contracts/0.8.9/utils/PausableUntil.sol#L54
It's recommended to remove the double check.

2. An unnecessary slot in the staking module name
In contract StakingRouter it’s possible to create a staking module name with length == 32,
in this case it will be allocated in two slots: the first one will store only the size, the second one will store the name.

It’s recommended to fix them.

Acknowledged

We have the receiver function in:
https://github.com/lidofinance/lido-dao/blob/e57517730c3e11a41e9cbc32ce018726722335b7/contracts/0.8.9/LidoExecutionLayerRewardsVault.sol#L75.
This accepts eth from anyone and emits an event but this method doesn’t check that msg.value > 0. An attacker can call a contract with a null msg value in loop what will cause a large amount of ETHReceived events with a null argument what can DoS the off-chain logic.

It’s recommended to add a check for the min value.