# Mitigations for deposit front-running vulnerability

## Abstract

On Tuesday, Oct 5, the vulnerability allowing the malicious Node Operator to intercept the user funds on deposits to the Beacon chain in the Lido protocol was reported to Immunefi. On Wednesday, Oct 6, [the short-term fix was implemented](https://blog.lido.fi/vulnerability-response-update/). **Currently, no user funds are at risk, but the deposits to the Beacon chain are paused**.

This document outlines the long-term mitigation allowing deposits in the Lido protocol.

The vulnerability could only be exploited by the Node Operator front-running the `Lido.depositBufferedEther` transaction with direct deposit to the [DepositContract](https://etherscan.io/address/0x00000000219ab540356cbb839cbe05303d7705fa) of no less than 1 ETH with the same validator public key & withdrawal credentials different from the Lido's ones, effectively getting control over 32 ETH from Lido.

To mitigate the vulnerability, Lido contracts should be able to check that Node Operators' keys hadn't been used for malicious pre-deposits. DepositContract provides the Merkle root encoding of all the keys used in staking deposits. We propose to establish the **Deposit Security Committee** checking all deposits made and approving the current `deposit_data_root` for Lido deposits.

The idea was outlined on [research forum post](https://research.lido.fi/t/mitigations-for-deposit-front-running-vulnerability/1239#d-approving-deposit-contract-merkle-root-7) as the option `d)`.

The Merkle root data should be checked & signed by the guardian committee off-chain to save gas costs. Anyone with the signed `depositRoot` can call `depositBufferedEther` sending the ether from the Lido buffer to the DepositContract. In case any deposit happens before the `depositBufferedEther` transaction, the Merkle root on the DepositContract is changed and the Lido deposit transaction is reverted.

function depositBufferedEther(
    bytes32 depositRoot,
    Signature[] memory signatures
) external {
    bytes32 onchainDepositRoot = IDepositContract(depositContract).get_deposit_root();
    require(depositRoot == onchainDepositRoot, "deposit root changed");
    
    require(_verifySignatures(depositRoot, signatures), "invalid signatures");
    
    _depositBufferedEther();
}

The outlined approach requires relatively small changes of the protocol, preserving the current operations flow between the protocol, Node Operators and the Lido DAO.

To implement the proposed mitigation, the DAO would have to:
1) implement the upgrade for the protocol smart contracts;
2) establish a committee;
3) write & check the software for monitoring deposits, approving the Merkle roots & exchanging and publishing signed approval messages.

As all things require a significant amount of work & time, we propose to start with a committee of one operated by the Lido dev team to unblock deposits in the protocol as soon as possible.

## NodeOperatorsRegistry keyOpIndex

To reliably check that the next deposit is safe for the protocol, it is necessary to check that there is no intersection between the set of all historical public keys used for deposits in the DepositContract and the set of all ready to use public keys in [NodeOperatorRegistry](https://github.com/lidofinance/lido-dao/blob/master/contracts/0.4.24/nos/NodeOperatorsRegistry.sol). This means that it is not enough to keep track of only the `deposit_data_root`, but it is also necessary to keep track of pubkey set in the NodeOperatorRegistry.

One could build the same Merkle-tree solution for the NodeOperatorRegistry, but this would require a major smart contract rewrite. Therefore, we propose to introduce a Lamport-style counter, which we will increment for every modification of the depositable pubkey set.

The pair of `deposit_root` from DepositContract and `key_op_index` from NodeOperatorRegistry provides all the data required for the guardian committee to sign & the deposit contract to check before sending the deposits.

function depositBufferedEther(
    bytes32 depositRoot,
    uint256 keysOpIndex,
    Signature[] memory signatures
) external {
    bytes32 onchainDepositRoot = IDepositContract(depositContract).get_deposit_root();
    require(depositRoot == onchainDepositRoot, "deposit root changed");
    
    uint256 onchainKeysOpIndex = INodeOperatorsRegistry(nodeOperatorsRegistry).getKeysOpIndex();
    require(keysOpIndex == onchainKeysOpIndex, "keys op index changed");
    
    require(_verifySignatures(depositRoot, signatures), "invalid signatures");
    
    _depositBufferedEther();
}

Indeed, in the case when the committee members sign the pair of `depositRoot` and `keysOpIndex`, then in the case when one of the sets changes, the off-chain proof expires and deposit transaction reverts.

## Deposit Security Committee

As stated above, we propose to establish the **Deposit Security Committee** dedicated to ensuring the safety of deposits on the Beacon chain:
1) monitoring the history of deposits and the set of Lido keys available for the deposit, signing and disseminating messages allowing deposits;
2) signing the special message allowing anyone to pause deposits once the malicious Node Operator pre-deposits are detected.

To make a deposit, we propose to collect a quorum of 2/3 of the signatures of the committee members. Members of the committee can collude with node operators and steal money by signing bad data that contains malicious pre-deposits. To mitigate this we propose to allow single committee member to stop deposits and also enforce space deposits in time (e.g. no more than 150 deposits with 150 blocks in between them).

This design ensures the protocol robustness even with single honest committee member. The impact is limited to about, say 4800 ETH at most (150 keys allowed within a time window, 32 ETH deposited to every key). False positive pause would stop the deposits for couple days. Positive stop reveals the rogue committee member, which is good for the protocol. Overall, DoS seems not to be a big problem, and if it would be, the DAO can change the mitigation.

Each member must generate an EOA address to sign the pair `(depositRoot, keysOpIndex)` with their private key. The addresses of the committee members will be added to the smart contract.

Below is the pseudocode of the `depositBufferedEther` function, updated following the above considerations.

function depositBufferedEther(
    uint256 maxDeposits,
    bytes32 depositRoot,
    uint256 keysOpIndex,
    Signature[] memory sortedGuardianSignatures
) external {
    bytes32 onchainDepositRoot = IDepositContract(DEPOSIT_CONTRACT).get_deposit_root();
    require(depositRoot == onchainDepositRoot, "deposit root changed");

    require(!paused, "deposits are paused");
    require(quorum > 0 && sortedGuardianSignatures.length >= quorum, "no guardian quorum");

    require(maxDeposits <= maxDepositsPerBlock, "too many deposits");
    require(block.number - lastDepositBlock >= minDepositBlockDistance, "too frequent deposits");

    uint256 onchainKeysOpIndex = INodeOperatorsRegistry(nodeOperatorsRegistry).getKeysOpIndex();
    require(keysOpIndex == onchainKeysOpIndex, "keys op index changed");

    _verifySignatures(depositRoot, keysOpIndex, sortedGuardianSignatures);

    ILido(LIDO).depositBufferedEther(maxDeposits);
    lastDepositBlock = block.number;
}

## On-Chain Deposit Flow

Instead of adding the code directly to the Lido contract, we propose to extract the described functionality to a separate non-upgradable contract – `DepositSecurityModule`.

This approach will reduce the gas price of an unsuccessful deposit transaction (e.g. due to deposit data root change) and reduce the number of modifications to the Lido contract - it would be necessary only to wrap `depositBufferedEther` with ACL role and allow `DepositSecurityModule` contract to call the function.

`DepositSecurityModule` implements `Ownable` interface, so only the owner could change configuration parameters such as guardian addresses list and security parameters(e.g minDepositBlockDistance, maxDeposits, ...). After deploying and setting up the contract, the ownership would be transferred to the [DAO Agent](https://etherscan.io/address/0x3e40D73EB977Dc6a537aF587D48316feE66E9C8c).

contract DepositSecurityModule is Ownable {
    function setMinDepositBlockDistance(uint256 newValue) external onlyOwner;
    function setMaxDeposits(uint256 newValue) external onlyOwner;
    function setPauseIntentValidityPeriodBlocks(uint256 newValue) external onlyOwner;

    function addGuardians(address[] memory addresses, uint256 newQuorum) external onlyOwner;
    function addGuardian(address addr, uint256 newQuorum) external onlyOwner;
    function removeGuardian(address addr, uint256 newQuorum) external onlyOwner;
    function setGuardianQuorum(uint256 newValue) external onlyOwner 
    
    /**
     * The function might be called by the guardian directly   
     * OR by passing a valid guardian's signature.
     *
     * blockHeight is required to prevent reply attack.
     */
    function pauseDeposits(uint256 blockHeight, Signature memory sig) external;
    function unpauseDeposits() external onlyOwner;
    
    /**
     * Calls Lido.depositBufferedEther(maxDeposits),
     * which is not callable in any other way.
     */
    function depositBufferedEther(
        uint256 maxDeposits,
        bytes32 depositRoot,
        uint256 keysOpIndex,
        Signature[] memory sortedGuardianSignatures
    ) external;
}

## Off-chain part

### DSC Daemon
Each committee member has to run DSC Daemon that monitors the validators' public keys in the `DepositContract` and `NodeOperatorRegistry`. The daemon must have access to the committee member's private key to be able to perform ECDSA signing.

The daemon constantly watches all updates in `DepositContract` and `NodeOperatorRegistry`:
* If the state is correct, it signes the current `to_sign` struct and emits it to an off-chain message queue.
* If the state has malicious pre-deposits, it signs the "something's wrong" message at the current block and attempts to send `pauseDeposits()` tx.

### Depositor Bot

The proposal makes the deposits in Lido permissioned: one must gather the quorum of signatures and pass several on-chain checks to make deposit. Therefore, this process must be automated.

Lido dev team will run a deposit bot that will deposit buffered ether. The bot will listen to an off-chain message queue and:
* if there is a “something's wrong” message, try to pause a protocol with high gas
* else if there’s a quorum on the fresh deposit data root, there’s money in the buffer and the gas is low enough, try to call `depositBufferedEther`

## Minor additional changes not related to vuln

1. Mitigation for [A Node Operator can circumvent DAO validator key approval](https://github.com/lidofinance/lido-dao/issues/141).
2. Set DEFAULT_MAX_DEPOSITS_PER_CALL to 150 in [Lido.sol#59](https://github.com/lidofinance/lido-dao/blob/master/contracts/0.4.24/Lido.sol#L59).
3. Prevent creating node operators with [a non-zero staking key limit](https://github.com/lidofinance/lido-dao/blob/master/contracts/0.4.24/nos/NodeOperatorsRegistry.sol#L112).
4. Add batch key removal methods [removeSigningKey](https://github.com/lidofinance/lido-dao/blob/master/contracts/0.4.24/nos/NodeOperatorsRegistry.sol#L260) and [removeSigningKeyOperatorBH](https://github.com/lidofinance/lido-dao/blob/master/contracts/0.4.24/nos/NodeOperatorsRegistry.sol#L272).
5. Add missing event for [trimUnusedKeys](https://github.com/lidofinance/lido-dao/blob/master/contracts/0.4.24/nos/NodeOperatorsRegistry.sol#L207) method.

## Action Plan

It is very important to unpause the deposits to the Beacon chain, so we assume the deployment in several stages:

1) single committee member, simple daemon, full-fledged smart contract;
2) multiple committee members on a centralized mq, complex-ish daemon, complex-ish deposit bot, monitoring;
3) multiple committee members on a p2p network.

### Smart contract deploy plan

- [ ] Deploy Lido.sol implementation (no init)
- [ ] Deploy NodeOperatorsRegistry.sol implementation (no init)
- [ ] Deploy DepositSecurityModule.sol (constructor)
- [ ] Initialize DepositSecurityModule (add 1 guardian and set quorum to 1)
- [ ] Transfer ownership to the DAO Agent
- [ ] Create voting with several items:
1. Publishing new implementation of Lido to the APM repo
2. Updating implementaion of Lido app with a new one
3. Publishing new implementation of NodeOperatorsRegistry to the APM repo
4. Updating implementaion of NodeOperatorsRegistry app with a new one
5. Granting new permission DEPOSIT_ROLE for DepositSecurityModule
6. Increase limits for Node Operators to the current available key numbers.

## Links

- Pull request with all smart contracts changes (WIP): https://github.com/lidofinance/lido-dao/pull/357
- DepositSecurityModule.sol: https://github.com/lidofinance/lido-dao/blob/feature/deposit-frontrun-protection-upgrade/contracts/0.8.9/DepositSecurityModule.sol
- Mitigations research forum post: https://research.lido.fi/t/mitigations-for-deposit-front-running-vulnerability/1239
- Blog post with events log: https://blog.lido.fi/vulnerability-response-update/