---
tags: research, ethereum, governance, dsm
title: DepositSecurityModule upgrade options
---

## DepositSecurityModule upgrade options

### Context

We are planning to upgrade the `DepositSecurityModule` (DSM) contract to deliver the changes implemented with the latest audit from MixBytes. There are no [significant modifications](https://github.com/lidofinance/lido-dao/pull/426/files#diff-75928d071fec4c088f8b179878941ea6332b9acad2df8b96ce57a281c8cb9528) and zero new features: mostly additional fixes, chores and sanity checks.

### About `DepositSecutiryModule`

The contract was developed to mitigate [deposits front-running](https://github.com/lidofinance/lido-improvement-proposals/blob/develop/LIPS/lip-5.md) vulnerability.

The contract is non-upgradable, and has permissioned methods (current owner is the Lido DAO Agent contract).

### Problem

It's required to replicate the state between old and new DSM instances during the upgrade procedure.

The only state part that has dynamic behavior and can't be initialized during the genesis new instance deployment, is the internal `lastDepositBlock` counter.

If the counter is set to the some irrelevant too old value (i.e. `0` for simplicity), then during the transition from old to new instance there is an opportunity to deposit buffered ether twice (while normally it's prohibited to provide a reaction time for council daemons: it is at least `25` blocks required between adjacent deposits, and no more than `150` deposits per block).

We have added the `setLastDepositBlock` setter to the new DSM contract. There are aditional percularities to use the setter: since the call is wrapped into the regular Aragon voting then it's not possible to pass pre-calculated value for the block number of the voting enactment tx. The contract's code uses unsigned numbers and the following expression of the `canDeposit()` and `depositBufferEther()` methods would revert for the integer underflow cases (since the contract use solidity 0.8 version):

block.number - lastDepositBlock >= minDepositBlockDistance;

So, there is a discussion arised about possible ways to make the upgrade as smooth as it possible.

### Possible options

Below the possible options to solve the problem. They are ordered by increasing general intuitive difficulty (additional operations, development and communications overheads).

*UPD(28-05-2022): Options list updated.*
#### Option 1. Set the block number in the future

The `canDeposit()` and `depositBufferEther()` methods will revert before the actual moment in the future will happen since solidity 0.8 is used.

##### Pro
* The voting goes as is
##### Contra
* It's still a design flaw to have the throwable `canDeposit` view method.
* Need little tuning from the tooling side to deal with reverts
* If old contract is paused during the voting, then new contract become unpaused inititially (could be mitigated with daemons upgrade, though)

#### Option 2. Relax the invariant only once

We may leave the `lastDepositBlock` number as `0` (i.e. don't call the `setLastDepositBlock` setter at all) but this one allows to send two deposits in a row at the moment of upgrade (breaking the security invariants by doubling maximum funds amount at risk, though, only once).

##### Pro
* Upgrade without additional calls and turns.
##### Contra
* There is an invariant violation, even it's for short-term only (in the worst case: sending `150` deposits with two adjacent blocks)
* If the deposits put on pause, then we unpause them unconditionally by inserting new DSM implementation

#### Option 3. Put the deposits on pause before upgrade

Another option is to call `setLastDepositBlock` with some block value in between of the ongoing Aragon vote (start time + 48h, for example) and put the deposits on pause just before the vote will have been started.

##### Pro
* Ensures all security params and concerns.
##### Contra
* If voting doesn't reach quorum, the deposits would be paused for a relatively long period.
* Pausing requires sending ad-hoc transaction and may raise additional concerns on the purpose of pausing and possibilities of misusing pausing mechanics in the future

#### Option 4. Use voting enactor guard

Put the additional call into the voting items: `SecurityModuleReplaceGuard(addr).enforceDSMIsReplaceable()`.

It would allow to revert the voting enact transaction if:
- deposts had been put on pause in the middle of voting;
- last deposit was too close.
Voting could be enated somewhat later when conditions are met.

The contract code is relatively simple and doesn't worth to run an audit.
It could be written with Vyper and pre-loaded on etherscan.

interface IDepositSecurityModule {
    function canDeposit() external view returns (bool);
}

contract SecurityModuleReplaceGuard {
    address constant public OLD_DSM_ADDRESS = 0xDb149235B6F40dC08810AA69869783Be101790e7; 

    function enforceDSMIsReplaceable() external view {
        assert(IDepositSecurityModule(OLD_DSM_ADDRESS).canDeposit());
    }
}

NB: here we intentionally mark the method as `view` to underline zero state changes despite possible `revert`.

##### Pro
* The solution enforces all security invariants, and the code is super-simple and immutable with respect to chain state
##### Contra
* There are concerns about the hanging unexecutable vote (see below)

#### Option 5. Use additional state replicator contract

Put the additional calls into the voting items:

1) transfer ownership for the new DSM contract to the `SecurityModuleReplicator` instance.
2) `SecurityModuleReplicator(addr).replicateDSMState()`.

interface IDepositSecurityModule {
    function setLastDepositBlock(uint256 _blockNumber) external;
    function getLastDepositBlock() external view returns (uint256);
    function isPaused() external view returns(bool);
    function pauseDeposits() external;
}

contract SecurityModuleReplicator {
    address constant public OLD_DSM_ADDRESS = 0xDb149235B6F40dC08810AA69869783Be101790e7; 
    address constant public NEW_DSM_ADDRESS = 0x710B3303fB508a84F10793c1106e32bE873C24cd;
    address constant public DAO_AGENT_ADDRESS = 0x3e40D73EB977Dc6a537aF587D48316feE66E9C8c;

    function replicateDSMState() external {
        IDepositSecurityModule(NEW_DSM_ADDRESS).setLastDepositBlock(
            IDepositSecurityModule(OLD_DSM_ADDRESS).getLastDepositBlock()
        );
        
        if(IDepositSecurityModule(OLD_DSM_ADDRESS).isPaused()) {
            IDepositSecurityModule(NEW_DSM_ADDRESS).pauseDeposits();
        }
        
        IDepositSecurityModule(NEW_DSM_ADDRESS).setOwner(DAO_AGENT_ADDRESS);
    }
}

The call doesn't induce additional explicit revert reasons, and provides safe state transition preserving the current `isPaused` state.

##### Pro
* The solution enforces all security invariants, and code is relatively simple
##### Contra
* There are concerns about the non-audited contract which have to be temporary (only within the same transaction) set as the new DSM owner.

### Proposed decision

*UPD(28-05-2022): Options list updated.*

It looks like the following preference order might exist:
- **Option 1. Set the block number in the future**
- Option 4. Use voting enactor guard
- Option 3. Put the deposits on pause

### Edge cases and mitigations

#### Option 1. Set the block number in the future

We allow `canDeposit()` revert before the actual time happen. If old DSM has been put on pause during the voting, then new DSM doesn't replicate its state initially. Mitigation could be introduced by the daemons upgrade.

#### Option 4. Use voting enactor guard

If deposits are put on pause during the voting process (or before enactment), than the upgrade voting becomes blocked in the 'unexecutable state' till the deposits will be `unpaused` using the old DSM instance with another voting.

If can happen if the deposits were put on pause since `canDeposit` code is straightforward:

function canDeposit() external view returns (bool) {
    return !paused && quorum > 0 && block.number - lastDepositBlock >= minDepositBlockDistance;
}

So, the voting could be enacted by running another voting on top to unpause deposits for the old contract, and enact the first one after the second will be executed.

To completetely block and abandon the 'unexecutable' voting there are exist following measures:
- revoke 'DEPOSIT_ROLE' from the old DSM with another voting
- put old DSM on pause (there is no way to unpause without voting rights)

#### Option 3. Put the deposits on pause

If voting doesn't get quorum, then the deposits will be paused till the next quorum-reached voting. It can harden the ops and induce additional incident-covering comms.