Change DeputyGuardianModule to use a Proxy to be DeputyGuardianModuleProxy
#89
Conversation
Ethnical
changed the title
DeputyGuardianModule a Proxy DeputyGuardianModule to use a Proxy and be DeputyGuardianModuleProxy
Ethnical
changed the title
DeputyGuardianModule to use a Proxy and be DeputyGuardianModuleProxyDeputyGuardianModule to use a Proxy to be DeputyGuardianModuleProxy
Ethnical
added
documentation
There was a problem hiding this comment.
This makes a lot of sense to me - we should be able to modify the deputy guardian module without invalidating PSPs. Ensuring only the security council can perform that upgrade preserves the security properties of the system so the deputy guardian is still restricted in the actions they can perform and the security council is in control.
|
|  | ||
|
|
||
| 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`. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| 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`. | ||
|
|
||
| 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`. |
There was a problem hiding this comment.
We should think a bit more about whether the proxy owner should be:
- The L1 ProxyAdmin: This has the benefit of an upgrade path that is most like the other contracts.
- 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.
- 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
ModuleManagerfunctionality.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| 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). |
There was a problem hiding this comment.
What do you mean by "previous call that can run out of gas"?
There was a problem hiding this comment.
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.
|
I have merged the change for the gap PSPs: |
Introduce a proposal that will prevent to regenerate the PSPs and make some unnecessary changes to the guardian contract each time we update the
DeputyGuardianModulecode.As discussed here on discord.