feat: propose DeputyGuardianModule improvements #167
Conversation
Proposes a number of improvements to the DeputyGuardianModule. These improvements would supercede the previous proposal to introduce a DeputyPauseModule into the Foundation Safe and would simplify the OP Stack security model.
| The `DeputyGuardianModule` would **no longer** be permitted to: | ||
|
|
||
| 1. Undo the Superchain-wide pause ("unpause"). | ||
| 1. Change the respected game type. |
There was a problem hiding this comment.
This creates a serious gap in our ability to respond to issues with the fault dispute system. We would need to escalate any issue affecting a significant number of games to a superchain wide pause which we had previously explicitly designed to avoid.
It's important to note that changing the respected game type can only switch to a game implementation previously approved by governance and deployed by the security council. I can see the argument that it changes the security model but I think there's an equally valid view that it doesn't because the permissioned game is always a part of the system and thus part of its security model.
There was a problem hiding this comment.
Does your opinion here change if we add the invalidateExistingGames() function to the AnchorStateRegistry and allow the deputy guardian to call that function? That would allow the deputy guardian to respond to a large number of invalid games without triggering the pause.
There was a problem hiding this comment.
Not really. I guess we could call that regularly until the security council changes the game type but it doesn't seem like a good incident response process. The case I'm thinking about is where there's a bug in the fault dispute game that someone is using to continuously cause invalid output roots to resolve as valid.
I guess given timelines we could call invalidateExistingGames() once and depend on the security council signing to switch respected game type before more games resolve as valid and we'd bundle another call to invalidateExistingGames() in that task. With a 3.5 day air gap and a 3 day SLA for security council that would work but it doesn't feel good to leave a major issue unmitigated for 3 days. I think we'd need to carefully review our incident response again and see what it looks like without the ability to fallback to permissioned games and confirm we're ok with it. I think it increases the likelihood that we would need to pause the superchain and we've generally tried to restrict that action to only the most critical of situations.
There was a problem hiding this comment.
I think I could be convinced of allowing the deputy guardian to switch the respected game type, need to think about it a bit more. My primary concern is that if we're making the deputy guardian more permissive then giving it the ability to change the respected game type becomes a bit more dangerous. A malicious/leaked deputy key would be able to swap the respected game type back to the permissionless game which would probably force you to trigger the pause to avoid an endless back-and-forth.
That said I think the likelihood of this happening is relatively low. My short-term proposal would then be that we give the deputy guardian access to everything except for unpause.
There was a problem hiding this comment.
I was going to come up with a broader proposal but I think the reality is that with Stage 1.4 + interop it's possible that the landscape around security responses looks very different in 6 months, so I'd be happy with a simplified/permissive deputy guardian for now, even if it's not ideal.
There was a problem hiding this comment.
We could also potentially allow the deputy guardian module to switch the respected game type to permissioned but not allow it to switch back to permissionless.
There was a problem hiding this comment.
That's actually a pretty good compromise. When kona is deployed we would typically want to switch to it instead of permissioned if we can so need to think through if that should be allowed by the deputy guardian or if we're ok with requiring the security council for that. e.g. we could allow the deputy guardian to change the respected game arbitrarily unless it is already the permissioned game.
There was a problem hiding this comment.
Alright going to update this proposal with what we've talked about so far.
There was a problem hiding this comment.
We could also potentially allow the deputy guardian module to switch the respected game type to permissioned
Isn't that exactly the issue that we're trying to fix? ie. allowing the guardian to change the state transition is a safety, not liveness, consideration.
There was a problem hiding this comment.
Although I agree with this in principle, I'm trying to find a middle ground that improves on the status quo right now without needing to make a bunch of contract changes
Updates the proposed capabilities so that the DeputyGuardianModule can carry out additional actions with certain restraints.
| 1. Change the respected game type to the `PermissionedDisputeGame`. | ||
| 1. Trigger the `setAnchorState` function in the `AnchorStateRegistry`. |
There was a problem hiding this comment.
IMO allowing the Guardian to trigger the "fallback" to the permissioned game goes beyond liveness considerations and is what muddies the waters. Would prefer to have only the first two items in this list available.
There was a problem hiding this comment.
I generally agree with you, this was my original proposal but see conversation with Adrian above for why I changed it
| whenever an upgrade touches the `DeputyGuardianModule`. Gripes with the pre-signed pause system | ||
| could fill a whole role of toilet paper and are not just limited to the issues noted above. | ||
|
|
||
| ### Unclear Responsibilities |
There was a problem hiding this comment.
I'm worried about changing what the DGM can do, but not also changing what that Guardian role is capable of doing. IMO it is much cleaner to maintain the property that "Anything the Guardian can do, the DGM can do", so I'd prefer to see us move any safety impacting actions (ie. unpausing, fallback activation) to the Upgrade Controller (2of2 Safe).
There was a problem hiding this comment.
This would violate the condition that L2Beat wants that the Security Council alone can unpause the system. I fully agree with you but that's the constraint I was operating under.
There was a problem hiding this comment.
Generally speaking, my goal with this proposal is to solve a lot of problems in the short-term and then come back to this problem in ~6 months when the landscape has changed a bit with Stage 1.4
There was a problem hiding this comment.
This would violate the condition that L2Beat wants that the Security Council alone can unpause the system.
Actually, what I'm advocating or is to also update the auth in the system.
It should not be too much of a lift once the Isthmus contract changes are in, as they introduce a new upgrader role that is stored in the SuperchainConfig, so all you'd need to do is update the auth check in unpause() and setRespectedGameType().
There was a problem hiding this comment.
Yes I agree with this in principle, my goal with this proposal is to be able to ship something into prod ASAP and then work on these more in-depth changes later
|
Sigh as much as I want to see this proposal be approved, I am sufficiently concerned that this allows a minority of the security council to brick everything that I will likely retract this proposal until we have a liveness guard in place on the security council. |
| The `DeputyGuardianModule` would be accessible to: | ||
|
|
||
| 1. Any member of the Security Council Safe. | ||
| 1. An EOA operated by the Optimism Foundation. |
There was a problem hiding this comment.
Can we make this scalable to N EOAs, so that ability to pause can be delegated to other security-facing orgs?
| live third-party infrastructure in the hot path for critical security actions. It therefore seems | ||
| prudent that the deputy be able to act via signature instead of directly via `msg.sender` check. | ||
|
|
||
| If we allow the deputy to act via signature then we must also make sure that the signature includes |
There was a problem hiding this comment.
How different is this from just using 1/1 SAFEs instead of raw EOA addresses?
There was a problem hiding this comment.
If 1/1 safe is compromised then owner can be changed and you lose access to pause. Using raw EOA makes this impossible.
@smartcontracts Is the design review meeting scheduled for today going to be looking at the design doc as is? |
Proposes a number of improvements to the DeputyGuardianModule. These improvements would supercede the previous proposal to introduce a DeputyPauseModule into the Foundation Safe and would simplify the OP Stack security model.