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

feat: improve DelayedWETH response capabilities #80

Merged
merged 1 commit into from
Oct 28, 2024
Merged

feat: improve DelayedWETH response capabilities #80

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
78 changes: 78 additions & 0 deletions protocol/proofs/delayed-weth-expansion.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
# Expanded DelayedWETH Capabilities

## Context

The `DelayedWETH` contract acts as a holding contract for the bonded ETH submitted by any
participant in a `DisputeGame` contract. As of the Granite upgrade, each implementation of the
`DisputeGame` uses its own `DelayedWETH` contract. Each `DelayedWETH` contract is `Ownable` and is
subject to a number of safety net actions by the owner address (holding balances from specific
smartcontracts marked this conversation as resolved.
Show resolved Hide resolved
addresses or removing all ETH held in the `DelayedWETH` contract). `DelayedWETH` is additionally
subject to the Superchain-wide pause mechanism within the `SuperchainConfig`. Bonds cannot be
retrieved by game participants if the Superchain pause is active.

## Problem Statement

Existing security mechanisms within the `DelayedWETH` contract are inconvenient and slow down the
incident response process. Specifically:

- The `DelayedWETH.hold(address target)` function creates an approval from the `target` address to
the `owner` address. Since the `target` can simply remove this approval, `hold` MUST be triggered
alongside a `transferFrom` in the same transaction. This adds unnecessary complexity to something
that could simply be a single transaction.
- The various security mechanisms inside of `DelayedWETH` can only be triggered by the System Owner
which has a slow SLA. The Deputy Guardian can act to trigger the Superchain-wide pause, but this
is highly disruptive to the entire Superchain ecosystem.

Additionally, it should be noted that all ETH inside a `DelayedWETH` contract is pooled together
regardless of which games are using the contract. We currently mitigate this by using one
`DelayedWETH` contract per game type, but this expands the number of contracts that the System
Owner and Guardian roles must manage by at least 2 per additional chain. However, the modifications
required to appropriately address this issue are sufficiently extensive that they are considered
out of scope for this proposal.

## Proposed Solution

Here we propose some modifications to `DelayedWETH` that help to resolve the two noted limitations:

1. We propose that `DelayedWETH.hold` executes a `transferFrom` automatically to reduce the number
smartcontracts marked this conversation as resolved.
Show resolved Hide resolved
of required transactions from 2 to 1 when holding ETH from a single dispute game. `hold` will
specifically transfer to the `owner` address.
2. We propose a new `pause` function on the `DelayedWETH` contract itself that pauses *just* the
`DelayedWETH` contract and supplements the Superchain-wide pause functionality. This `pause`
function would be executable by the Guardian role and could therefore be triggered quickly in
case the System Owner is unable to execute another security action in time (e.g., holding bonds
from a particular dispute game).
Comment on lines +40 to +44
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the use case for pausing DelayedWETH, e.g. what situation do we expect to do it? I ask to confirm if placing the pause at the DelayedWETH level what we want—the implication here is that if we need to pause DelayedWETH across the full superchain, it's n calls and we must ensure we don't miss a chain. Is the expected scenario that we only need to pause a single DelayedWETH for a single chain?

Copy link
Contributor Author

@smartcontracts smartcontracts Oct 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The main reason to pause DelayedWETH for a single chain is because our only other option is to pause the entire superchain and doing so is highly disruptive. We want the option to do both. If it's just a single game type being exploited then we want a way to quickly pause withdrawals out of a specific DelayedWETH (so that we can potentially recover bonds if needed) without pausing the entire superchain.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd also say that if we're in the situation where we need to pause DelayedWETH across every chain, there's a pretty good chance things are really messed up and we would just pause everything. But much more likely is that there's a bunch of games on one chain that have resolved incorrectly (e.g. because op-challenger for that chain was mis configured) and we just need more time to sort that out. We could invalidate all games on that chain with setRespectedGameType (with the option to invalidate since that will be optional in the future) and pause DelayedWETH to give us more time to work through how the bonds should be paid out.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you both! One follow up:

We could invalidate all games on that chain with setRespectedGameType

On this point, do we have a plan / is it desirable to make the respected game type part of the superchain config (or some other analogous solution that makes all chains follow the same game type)? It’s of scope for this design doc, but it seems eventually we’d want to enforce the same game being used for all interop chains (and maybe all standard chains too) to ensure consistent security properties

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do think this probably makes sense but I suspect it would probably come with a follow-up set of changes that allows multiple game types to be accepted. And possibly for the same contracts to be used everywhere with an anchor state keyed by chain ID.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense. I do want to make sure this is tracked somewhere, and will defer you on where to track


### Updated Pause Behavior

We propose slightly modifying the behavior of the `pause` functionality for the `DelayedWETH`
contract as follows:

- As previously stated, the `Guardian` role will also be able to trigger the `pause` functionality
for a particular `DelayedWETH` contract. The `DelayedWETH` contract will be in a paused state if
the `Guardian` triggers the pause OR if the Superchain-wide pause is triggered.
- When the `pause` is active, users will not be able to trigger the `withdraw` function (same as
before) EXCEPT the `owner` will still be able to trigger `withdraw` (may be useful in certain
situations).
- When the `pause` is active, users other than the `owner` will not be able to transfer any tokens
within the contract.

## Alternatives Considered

### Wider Rewrite

We considered the alternative to rewrite the `DelayedWETH` contract more broadly to be able to
easily manage multiple game types across multiple chains within a single `DelayedWETH` instance.
However, this would require significant modifications to the `DelayedWETH` contract and would act
as an iterative improvement to the status quo. We still believe that a larger rewrite should be
considered for the near future.

## Risks & Uncertainties

### Security Model

We see little risk with the modification to the `hold` function. Introducing a new `pause` function
smartcontracts marked this conversation as resolved.
Show resolved Hide resolved
slightly changes the existing security model (though not significantly). The Guardian role can
already pause the `DelayedWETH` contract by triggering the Superchain-wide pause. Allowing this
mds1 marked this conversation as resolved.
Show resolved Hide resolved
role to pause the `DelayedWETH` contract without pausing the rest of the system should not open up
capabilities that the Guardian does not already have. Comments on this subject are welcome.
smartcontracts marked this conversation as resolved.
Show resolved Hide resolved