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.

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

Conversation

smartcontracts
Copy link
Contributor

Introduces a proposal to improve some of the incident response capabilities present in the DelayedWETH contract.

ajsutton
ajsutton previously approved these changes Sep 16, 2024
Copy link
Contributor

@ajsutton ajsutton left a comment

Choose a reason for hiding this comment

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

This sounds good to me. If we operating DelayedWETH is easier I think we'd be fine to go back to having a single DelayedWETH per chain. Sharing across multiple chains is another level again that we'd probably want the ability to pause per chain still and I'm not too sure how to do that.

Copy link
Member

@clabby clabby left a comment

Choose a reason for hiding this comment

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

This looks good in general, not against these changes.

But at the same time, this still doesn't address the issue with "poisoned state" I don't think. There's still a decent risk of state corruption upon recovery here. The recover function is still in a really weird spot, as it just transfers ETH straight out of the contract without actually subtracting balance or allowance.

We should figure out a way to ensure that when funds are recovered, the contract's state isn't entirely corrupted imo (it sounds like just removing recover and using the hold + transferFrom route is the right way to go.)

Another pretty annoying thing about DelayedWETH is the manual reimbursements by the owner if funds are rescued. This used to be a lot easier to reason about prior to DelayedWETH, because the ETH was stored within the FDG, and we could make decisions about reimbursements on-chain based off of the state within the FDG. Not sure if we want to solve that here, but the "recover -> manually send ETH out to accounts" path feels like it would really suck in a high-stress scenario.

@smartcontracts
Copy link
Contributor Author

Yeah I fully agree that this response mechanism has the potential to be a huge nuisance. I think these changes are strictly improvements over the status quo but I would like to explore a more automated system that minimizes the amount of effort here.

Ethnical
Ethnical previously approved these changes Sep 20, 2024
Copy link
Contributor

@Ethnical Ethnical left a comment

Choose a reason for hiding this comment

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

Overall, really nice improvement to avoid to widely pause the Superchain. In case a of a targeted action into DelayedWETH contract.
I have let comments/suggestions but looks promising to me.
As some people already suggested the recover() can lead to a brick of the contract DelayedWETH as the balanceOf[] is not updated when the ETH are transfered, but seems to out of scope for this current design.

Also the function recover() is sending native ETH to the msg.sender that would be the owner() the proxyAdminOwner, I don't know if in the future we would like to control the receiver as sometimes safe like proxyAdminOwner are tidious to move the funds to send them to a Multisig dedicated for this effect.

Introduces a proposal to improve some of the incident response
capabilities present in the DelayedWETH contract.
@smartcontracts smartcontracts dismissed stale reviews from Ethnical and ajsutton October 19, 2024 04:57

Need fresh approvals

Copy link
Contributor

@wildmolasses wildmolasses left a comment

Choose a reason for hiding this comment

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

lgtm

Comment on lines +40 to +44
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).
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

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

@smartcontracts smartcontracts merged commit 15364a5 into main Oct 28, 2024
@smartcontracts smartcontracts deleted the sc/proofs-dw-2 branch October 28, 2024 08:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants