-
Notifications
You must be signed in to change notification settings - Fork 49
feat: TimelockGuard #324
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.
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
Merged
Merged
feat: TimelockGuard #324
Changes from all commits
Commits
Show all changes
16 commits
Select commit
Hold shift + click to select a range
d63c2d2
feat: TimelockGuard
smartcontracts 8b523b9
Update safe-modules/timelock-guard.md
alcueca c647764
Resolved comments, added cancellation feature, removed dual delay
669aec6
Incorporated other notes from the design review
2b835bb
Final edits with regards to cancellation threshold and cancellation f…
0476fa7
Added possibility of dynamic cancellation threshold
bae1493
Included comment on single contract for liveness module and timelock …
5e77da4
removed .DS_Store
0457e38
addressed multiple comments from @wildmolasses and @smartcontracts
8cf63d6
Edited for clarity after adressing the comments.
3d1a4e6
Details around instant execution
6ca237b
Update safe-modules/timelock-guard.md
alcueca 3cd5265
Removing L1PAO from scope
alcueca c1e6d75
Allowing for a disabled timelock to be installed
alcueca 2b4e987
We can just not enable the guard
alcueca 91660d0
Making it clear that the timelock guard and the liveness module are a…
alcueca File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,169 @@ | ||
| # TimelockGuard | ||
|
|
||
| ## Context and Problem Statement | ||
|
|
||
| <!-- The Context and Problem Statement section is one of the most critical parts of the design | ||
| document process. Use this section to clearly highlight the background context for the problem, | ||
| the specific issues being faced by customers, and any constraints on a solution as defined either | ||
| by customers or by technical limitations. Context and Problem Statement is an opportunity to tell | ||
| the story that helps to motivate the rest of the design document. --> | ||
|
|
||
| [Safe Accounts](https://safe.global) are configured as a set of owner addresses where some | ||
| threshold of addresses can sign and ultimately execute a transaction through the account. One | ||
| common concern with Safe accounts is that, by default, transactions execute immediately once signed | ||
| without any type of delay. | ||
|
|
||
| A mandatory transaction delay is a significant improvement in the security model for Safe accounts | ||
| because it provides ample time for review and entirely mitigates the impact of compromised signing | ||
| machines. It would be immensely valuable to OP Labs and Optimism Foundation multisig operations if | ||
| all multisigs managed by these entities were to require mandatory delays. We want to build a | ||
| component that we can fit into our existing multisigs that provides this delay with minimal | ||
| configuration and minimal overhead for both signers and multisig leads. | ||
|
|
||
| ## Customer Description | ||
|
|
||
| <!-- Provide a brief summary of the customers for this design document. --> | ||
|
|
||
| The customers for this design doc are any participants in the multisig accounts that would utilize | ||
| this guard, as well as any 3rd parties who rely on the security properties of these accounts. | ||
|
|
||
| ### Customer Reviewers | ||
|
|
||
| <!-- Identify at least one customer who should be involved in the review of this document. --> | ||
|
|
||
| - Optimism Foundation Finance | ||
| - OP Labs Finance | ||
|
|
||
| ## Requirements and Constraints | ||
|
|
||
| <!-- Identify the solution requirements and any additional design constraints from the Context and | ||
| Problem Statement section in a bulleted list. --> | ||
|
|
||
| - Mitigate the impact of a malicious majority of owners capable to execute a single transaction. | ||
alcueca marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| - Mitigate the impact of an honest majority approving a malicious or erroneous transaction. | ||
| - Keep code as minimal as possible to avoid complex logic in Safe modules/guards. | ||
| - Limit mental overhead for Safe owners and multisig leads, don't break workflows if possible. | ||
| - Apply cleanly for all of the major multisigs. | ||
alcueca marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| - Support nonceless execution, which we are planning to implement for operational reasons. | ||
| - The designed solution should be aplicable to single and arbitrarily nested Safe configurations. | ||
|
|
||
| ## Proposed Solution | ||
|
|
||
| <!-- Explain the solution that you believe best addresses the problem described above. --> | ||
|
|
||
| We propose the introduction of a new Safe guard, the `TimelockGuard`. The `TimelockGuard` would | ||
| take advantage of functionality in the latest version of Safe accounts to enforce a mandatory | ||
| timelock with minimal effort on the part of signers and a mostly unchanged workflow for multisig | ||
| leads. | ||
|
|
||
| ### Singleton | ||
|
|
||
| `TimelockGuard` is a singleton that exists at a known address on all chains and has no constructor | ||
| parameters. Any Safe on any network can choose to configure the Guard and then enable the Guard | ||
| within the Safe. | ||
|
|
||
| The timelock guard will be merged with the liveness module contract that we install so that we can | ||
| install the exact same contract address on any safe and just configure it during installation. | ||
|
|
||
| ### Configuration | ||
|
|
||
| A transaction from the Safe configures the Guard by executing a transaction that sets | ||
| a delay period. | ||
|
|
||
| ### Transaction Execution Flow | ||
|
|
||
| 1. Users sign the transaction normally, either via the UI or via `superchain-ops`. | ||
| 2. Anyone can collect the signatures and send the transaction calldata along with the signatures | ||
| to `TimelockGuard.schedule`, instead of executing via `Safe.execTransaction`. This function has | ||
| the exact same inputs as `execTransaction` and is therefore compatible with both the standard | ||
| Safe and the proposed nonceless execution module. | ||
| 3. `TimelockGuard.schedule` verifies the signatures attached to the transaction and starts a timer | ||
alcueca marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| for execution of the transaction. | ||
| 4. After the specified time, anyone can then call `Safe.execTransaction` as normal and execution | ||
alcueca marked this conversation as resolved.
Show resolved
Hide resolved
alcueca marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| will complete as expected. | ||
maurelian marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| ### Replayability Of Transactions | ||
| Each scheduled transaction would be identified by a hash derived from the transaction parameters, | ||
| which already include a nonce for replayability protection. Cancelled transactions will be marked | ||
| as cancelled so that they can't be scheduled again with the same signatures. | ||
|
|
||
| ### Storage Of Scheduled Transactions | ||
| Scheduled transactions will be stored with as many of its parameters as possible, limited by the | ||
| gas required to execute the most complex expected transactions withi projected block gas limits. | ||
| Rich transaction data helps with identifying the effects of a transaction, in particular to | ||
| external operators such as bug bounty hunters that might lack the tools or the context from | ||
| internal operators. | ||
|
|
||
| ### Transaction Cancellation Flow | ||
| To cancel a scheduled transaction, a `cancellation_threshold` number of owners would need to | ||
| signal that they agree with the cancellation. Cancelled transactions would need to be signed again | ||
| to be scheduled again. | ||
|
|
||
| In nested safe setups, the owners of a child multisig would need to signal the rejection of the | ||
| transaction in numbers no less than the `cancellation_threshold` of the child multisig, for the | ||
| child multisig to signal rejection of the transaction to the parent multisig. | ||
|
|
||
| #### Dynamic `cancellation_threshold` | ||
| A `cancellation_threshold` of 1 is convenient for usability as it requires the least | ||
| number of owners to signal their agreement to cancel a transaction in an emergency situation. | ||
|
|
||
| However, a `cancellation_threshold` that is set too low can also be abused by malicious owners to | ||
| permanently block the multisig from executing transactions, inducing a liveness failure. | ||
|
|
||
| A dynamic `cancellation_threshold` is chosen as a best compromise between security and usability. | ||
| The `cancellation_threshold` would start at 1 and increase by 1 with each successful consecutive | ||
| cancellation. The `cancellation_threshold` would reset to 1 with the successful execution of an | ||
| scheduled transaction. | ||
|
|
||
| The upper boundary to the `cancellation_threshold` would be the "blocking minority", defined as | ||
| the minimum number of `owners` that can stop the multisig from approving transactions, which is | ||
| `min(quorum, total_owners - quorum + 1)`. | ||
|
|
||
| ### Interaction with the LivenessModule | ||
| The LivenessModule will execute an ownership change through the safe if a challenge is successful, | ||
| this ownership challenge should not be cancellable, so it shouldn't be subject to the scheduling | ||
| flow. | ||
|
|
||
| When using a LivenessModule, the liveness challenges are responded with a regular transaction which | ||
| is subject to the timelock delay, and as such the LivenessModule needs to have a challenge response | ||
| time that allows for both assembling a quorum of signers and executed a delayed transaction. An | ||
| integrated LivenessModule and TimelockGuard is a viable option. | ||
|
|
||
| ## Alternatives Considered | ||
| <!-- Describe any alternatives that were considered during the development of this design. Explain | ||
| why the alternative designs were ultimately not chosen and where they failed to meet the product | ||
| requirements. --> | ||
|
|
||
| Other potential designs that were considered make trade-offs for security or usability that are | ||
| suboptimal. Examples of this are: | ||
|
|
||
| - Allowing any user to reveal a transaction for later execution, which creates more considerations | ||
| for monitoring/offchain infrastructure and changes the transaction flow. | ||
| - Having multisigs queue transaction via the timelock directly and then allowing anyone to execute | ||
| the transaction via a module, which changes the signing flow significantly and means transactions | ||
| are challenging to simulate properly. | ||
| - Configurable `cancellation_thresholds` were considered, but the risk of misconfiguration over | ||
| time was expected to be considerable. | ||
| - A fixed `cancellation_threshold` of 1 was considered, but the risk of operational disruption by | ||
| malicious actors was considered unacceptable. | ||
|
|
||
| We could not find any alternative design with the simplicity and security of the one that was | ||
| ultimately presented in this document. | ||
|
|
||
| ## Risks and Uncertainties | ||
alcueca marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| <!-- Explain any risks and uncertainties that this design includes. Highlight aspects of the design | ||
| that remain unclear and any potential issues we may face down the line. --> | ||
|
|
||
| ### Timelock Overhead | ||
| Any type of mandatory timelock introduces overhead for multisigs that want to execute transactions | ||
| in a timely fashion. Multisig leads will have to plan ahead of time in a way that they were not | ||
| previously required to do. | ||
|
|
||
| ### Time Critical Transactions | ||
| Some multisigs may face scenarios where they need to be able to execute transactions very quickly | ||
alcueca marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| in a worst-case scenario. We should do our best to avoid these situations as much as possible and | ||
| use other protocol features (like the Pause) to mitigate them entirely. | ||
|
|
||
| Instant execution is not required for finance multisigs. The delay will however be no longer than | ||
| the time required for a transaction review by trained finance staff. | ||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.