-
Notifications
You must be signed in to change notification settings - Fork 27
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
feat(proofs): FaultDisputeGame refund mode #85
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,101 @@ | ||
# FaultDisputeGame Refund Mode | ||
|
||
## Context | ||
|
||
Dealing with `FaultDisputeGame` bonds appropriately is an important part of the current OP Stack | ||
incident response process. `FaultDisputeGame` stores player bonds inside of the `DelayedWETH` | ||
contract which imposes a 7 day delay on bond withdrawals. During this delay period, the 2/2 Safe | ||
managed jointly by the Security Council and the Optimism Foundation can hold bonds from specific | ||
games or from all games at the same time. Held bonds are transferred to this 2/2 Safe where they | ||
can be manually disbursed to the correct recipients. | ||
|
||
## Problem Statement | ||
|
||
Although `DelayedWETH` is an effective tool at preventing a buggy or malicious `FaultDisputeGame` | ||
from distributing bonds to a potential attacker, the actual process of returning those bonds back | ||
to the correct players is underspecified. Underspecified proceses can introduce significant stress | ||
to already stressful situations where executive decisions around fallbacks must be made quickly and | ||
confidently. We therefore want to introduce alternative low-stress options for bond recovery. | ||
|
||
## Proposed Solution | ||
|
||
We propose modifying the `FaultDisputeGame` to enter a "refund mode" if the game is blacklisted. | ||
When the `FaultDisputeGame` is in this refund mode it will distribute bonds back to the users who | ||
originally deposited those bonds. | ||
|
||
### Preventing Complex States | ||
|
||
`FaultDisputeGame` currently allows users to begin withdrawing bonds before the game has fully | ||
resolved. It would likely be necessary to modify the `FaultDisputeGame` to block withdrawals until | ||
full resolution so that we can more easily reason about the security properties of this change. It | ||
may otherwise be possible to be caught in strange states. | ||
|
||
For instance, `FaultDisputeGame` instances are usually expected to be blacklisted only after they | ||
resolve incorrectly, so it may be possible for some users to begin withdrawing *before* a game is | ||
blacklisted, then the game becomes blacklisted and those same users can withdraw their original | ||
bonds. We can avoid this sort of double-counting and generally simplify the security model of the | ||
contract by preventing bond withdrawals until the game has resolved. | ||
Comment on lines
+33
to
+37
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like the idea of simplifying when bonds can be claimed. Having it match when the dispute game could be used withdrawals (ie 3.5 days after the game finalizes) makes a lot of sense and is much easier to reason about. |
||
|
||
We also likely need to prevent `FaultDisputeGame` contracts from entering refund mode if any | ||
withdrawals have already started to avoid dealing with complex state. If any user is able to | ||
withdraw before refund mode is activated then we assume that all withdrawals could've been | ||
completed and there's no reason to activate refund mode for those contracts. | ||
|
||
### Enabling Blanket Refunds | ||
|
||
Existing blacklisting functionality targets specific `FaultDisputeGame` instances and cannot easily | ||
handle cases where many games need to be blacklisted. If we were to adopt DD#76 alongside | ||
[DD#77](https://github.com/ethereum-optimism/design-docs/pull/77) then we could use the proposed | ||
`gameCreationValidFrom` timestamp to act as an implicit blacklist of all games created before this | ||
timestamp. The `AnchorStateRegistry.isValidGame(...)` function would return `false` if the game is | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add note for flag to not go into refund mode if withdrawals have already started There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Edit: actually, handle this in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we're not handling this in |
||
explicitly blacklisted OR if the game was created before this validity timestamp. All games created | ||
before `gameCreationValidFrom` would then enter refund mode. | ||
|
||
### FaultDisputeGame Bond Unlock Modification | ||
|
||
The `FaultDisputeGame` contract currently calls `DelayedWETH.unlock(...)` when it credits a user | ||
with a bond. However, `DelayedWETH.unlock(...)` is responsible for (1) starting the 7 day | ||
withdrawal period and (2) restricting how `FaultDisputeGame` can withdraw out of `DelayedWETH`. We | ||
would need to move this unlock to only be possible after the `FaultDisputeGame` has resolved. | ||
Additionally, this unlock would need to be re-triggered if refund mode is activated. | ||
|
||
We need to make sure that there is never a case where a `FaultDisputeGame` can distribute bonds | ||
under two different distribution modes. If the `FaultDisputeGame` has already started to distribute | ||
bonds under the default distribution mode, it must not be able to switch into the refund mode. We | ||
can do this with some sort of flag that tracks if the game has already started distributing bonds | ||
normally. | ||
|
||
## Risks & Uncertainties | ||
|
||
### Security | ||
|
||
`FaultDisputeGame` is, obviously, an important part of the security of the current OP Stack. Any | ||
modifications to the `FaultDisputeGame` contract will likely need to be audited. | ||
|
||
### AnchorStateRegistry modifications | ||
|
||
[Design Doc #76](https://github.com/ethereum-optimism/design-docs/pull/76) proposes to shift the | ||
state validation logic currently found within the `OptimismPortal` contract into the | ||
`AnchorStateRegistry`. Adoption of DD#76 would significantly simplify the specification for this | ||
proposal. One potential risk here is that this proposal effectively becomes a dependency of | ||
DD#76 as it would likely be too messy if `FaultDisputeGame` needed to reference `OptimismPortal`. | ||
|
||
### DelayedWETH Interactions | ||
|
||
We believe that the changes described here would not impact the continued ability to use | ||
`DelayedWETH` to act as a more serious fallback in the case that refund mode would not properly | ||
refund users for some reason. We should double check to confirm that we would not see any | ||
unexpected interactions between the two systems. | ||
|
||
### Challenger and Dispute Monitor Changes | ||
|
||
Changes that need to be made to the `FaultDisputeGame` bond unlock logic may require some small | ||
modifications to `op-challenger` and `op-dispute-mon`. We may be able to avoid changes to | ||
`op-challenger` if we move the `unlock` call into `claimCredit` and simply fail if the unlock | ||
has already been made. Need to confirm here that `op-challenger` will just retry this claim | ||
function over and over, but if that's the case then this works just fine. | ||
Comment on lines
+92
to
+96
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Challenger should just keep retrying already. But I'm not concerned about the need to modify challenger - it has really good support for handling multiple different contract versions already so can tolerate behaviour changing. |
||
|
||
`op-dispute-mon` would need to be modified slightly to account for the fact that (1) the credits | ||
inside of `DelayedWETH` will only match *after* the game has resolved and (2) the unlocks may | ||
become mismatched if refund mode is activated. Both situations can be managed but do require some | ||
changes to `op-dispute-mon`. | ||
Comment on lines
+98
to
+101
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's quite a few alerts in dispute-mon that will go wonky after manual intervention. We generally accept that because manual intervention is unusual and is only done when the system is in an unexpected state. So if you start from an unexpected state and do unusual things monitoring has very little chance of being able to make sense of it all. We can add dispute games to an ignore list in dispute-mon for that case when we know they're in a weird state and have been dealt with manually. There's a fair bit of monitoring around withdrawal credits that would need to be modified but that's quite doable. dispute-mon hasn't had to deal with multiple versions as much but it has some support and is generally pretty flexible and easy to work with so I don't expect the modifications to be difficult. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The major downside of this is that it makes attacks which were previously extremely expensive because you lose your bonds very cheap because everyone gets a refund of bonds. That's a very unfortunate change in incentives which makes griefing much more likely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah so this is an optional mechanism that acts as a default.
DelayedWETH
can still be used to remove bonds just like before, we're just optionally adding this extra mechanism that can be used in the case where removing bonds wouldn't be worth the effort.So if someone is executing an extremely expensive attack then
DelayedWETH
could still be used to hold those bonds and would therefore disincentivize such an attack just as before.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, it just sounds like when we blacklist a game it will go into refund mode immediately which we wouldn't want. If would could build a refund script offline without it being too complex I'd be tempted to do that, but it may be easier to have it built into the contracts.
An offline script just gives us more flexibility - for example we might want to create a list of honest and dishonest actor addresses and run a script to pay out bonds appropriately or only refund actors on the honest list etc. We should write those scripts ahead of time for anything we're likely to need though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could modify this so that refund mode begins after the airgap window -- that would solve this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah if it only kicked in after the air gap if manual intervention hadn't happened before then that would make sense.