Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change DeputyGuardianModule to use a Proxy to be DeputyGuardianModuleProxy #89

Merged
merged 4 commits into from
Oct 22, 2024
Merged

Change DeputyGuardianModule to use a Proxy to be DeputyGuardianModuleProxy #89

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6ed60ec
docs: add the `dgm` proxy upgrade design-doc
Ethnical Sep 23, 2024
e81e861
Update DeputyGuardianModule-Proxy.md
Ethnical Sep 23, 2024
6385ef8
Update DeputyGuardianModule-Proxy.md
Ethnical Sep 24, 2024
8a65e70
Update DeputyGuardianModule-Proxy.md
Ethnical Sep 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 63 additions & 0 deletions security/DeputyGuardianModule-Proxy.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
# Proxify the `DeputyGuardianModule`

## Context

![image](https://github.com/user-attachments/assets/466c7d28-f4d1-425d-82ec-eb1326287689)




The `DeputyGuardianModule` is an important component for the Superchain security model. \
The `DeputyGuardianModule`, also known as _DGM_, is a smart contract deployed on L1 (currently [here](https://etherscan.io/address/0xc6901f65369fc59fc1b4d6d6be7a2318ff38db5b)) that has, for example, the ability to widely pause the Superchain. \
The `DGM` is owned by the _Foundation Operation Safe_ so only operation run from the _Foundation Operation Safe_ can be executed into the `DGM`. \
To make sure we can widely pause the Superchain rapidly enough in case of an emergency, we need to presign the pause transaction from the signers of the _FoS_. \
These transactions are also known as `PSP` for PreSigned Pause transactions. \
This process requires a ceremony and this is slow and tedious to put in place.

The list of available actions that the `DeputyGuardianModule` can execute are:
| Function Name | Description |
| ---------------------- | ---------------------------------------------------------- |
| setRespectedGameType() | Change the current `gameType` used by the `OptimismPortal` |
| blacklistDisputeGame() | Blacklist a disputeGame into the `OptimismPortal` |
| unpause() | Unpause the withdrawal of the Superchain |
| pause() | Pause the withdrawal of the Superchain |
| setAnchorState()| Set a state into `AnchorStateRegistry` |


## Problem Statement

The existing `DeputyGuardianModule` is not _proxified_. \
This is inconvenient each time we want to upgrade the `DeputyGuardianModule` to add a new feature or to fix a potential bug. \
As this is not a proxy this require to redeploy the `DeputyGuardianModule` on L1. And also, to update the `Guardian` contract to add the new `DeputyGuardianModule` address as authorized module. \
Moreover, changing the `DeputyGuardianModule` will break the current PSPs setup as these pause transactions are presigned with the **previous** `DeputyGuardianModule`. \
Thus, we need to generate new PSPs through the tedious process of a ceremony to add the new `DeputyGuardianModule` for making the PSPs valid again. \
We are clearly seeing that this is not a sustainable solution for the long term.

Additionally, we have to simulate the new PSPs and share them with other member of the Superchain.

## Proposed Solution

![image](https://github.com/user-attachments/assets/099258f4-19fb-44b8-84e9-670b78a2f868)

Here we propose some modification of the architecture with a proxy contract on top of the `DeputyGuardianModule`, this will allow us to upgrade the `DeputyGuardianModule` and keeping the same address.
Thus, this will not invalidate the PSPs and we will not need to regenerate new PSPs each time we upgrade the `DeputyGuardianModule`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will invalidate the PSPs when we replace the DGM with the proxy though right? Perhaps we should wait until we need to upgrade the DGM for another reason, and then move to a proxy at the same time.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will invalidate the PSPs when we replace the DGM with the proxy though right?

This is correct.

Perhaps we should wait until we need to upgrade the DGM for another reason

Yes this is the idea! With Incident Response Improvements We will do the upgrade during the improvements of our incident response SC on L1.
I should write this down somewhere.


The owner of the proxy contract `DeputyGuardianModuleProxy` would be the `Security Council` as the `DeputyGuardianModule` is doing action into the `Guardian` contract that is owned by the `Security Council`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should think a bit more about whether the proxy owner should be:

  1. The L1 ProxyAdmin: This has the benefit of an upgrade path that is most like the other contracts.
  2. The 2 of 2: This has the same upgrade auth as other contracts, but a different path as it skips the L1 ProxyAdmin. I don't like it.
  3. The Security Council: This would makes sense as the Security Council should have full control over the DGM, but it can already disable/replace the DGM via the Safe's ModuleManager functionality.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it can already disable/replace the DGM via the Safe's ModuleManager functionality.

I think we should discuss about it during the review, I am not sure to follow this point there.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Decision: we will make it the 2of2 (L1 ProxyAdmin owner), this prevents us from needing a separate upgrade transaction for the DGM, without weakening the Security Council's ability to remove/replace the module.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I missed this design review so not sure if this was discussed—Do we need to run this decision by @sammcingvale or L2Beat to verify there's no reason it might cause a change in stage 1 status? Intuitively it feels like the SC is the right owner, since it's the SC's module, and the rationale is we're choosing the 2 of 2 to simplify operations by reducing the number of playbooks?

Copy link
Contributor Author

@Ethnical Ethnical Oct 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point @mds1!
I didn't thought about the L2Beat during the meeting!
We should definitely make sure we are not going in the correct decision will reach @sammcingvale in the meanwhile.

This will means that only the security council would be able to upgrade the implementation of the `DeputyGuardianModuleProxy`.

In this solution, we propose to follow the current proxy standard used by the rest of the contracts.

## Alternatives Considered

No other alternatives were considered for now.

## Risks & Uncertainties

### Security Model

We don't see any particular risk with this current design for the long term as this is a common pattern to use a proxy contract for upgrading a contract.

A potential issue can occur with the PSPs coverage.
During the upgrade the PSPs are going to be invalid so we need to make sure to presign new PSPs with the new `DeputyGuardianModuleProxy` address and simulate the PSPs accordingly.

Now, we are using a proxy executing the same actions than before will cost a more gas. We should ensure there no previous call that can run out of gas (really unlikely).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by "previous call that can run out of gas"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

previous call that can run out of gas?

First, this should not happen (this is extremely unlikely).
But, as I don't know the full components used with the DGM, it is possible that some hardcoded call are made with a certain amount of gas to the DGM (with automated code for switching automatically to certain game type or automatically call pause() or unpause() like Automated Pause or automated game switcher).
Since the delegatecall will now be replacing the previous normal call into the DGM as this is a proxy this will be more expensive and can lead to DoS if there is hardcoded gas limit in some code that called the DGM methods automatically.