feat: only allow challenger to play permissioned game #138
Conversation
Proposes a change to the PermissionedDisputeGame such that the Proposer will no longer be allowed to play the game. Instead, only the Challenger will be allowed to play the game. This more clearly mirrors the behavior of the L2OutputOracle.
There was a problem hiding this comment.
Makes sense to me. Worth noting that the spec currently indicates that only the challenger can call attack, defend, step, resolve, resolveClaim and addLocalData. As part of this we should update the spec to indicate that attack, defend and step are restricted to the CHALLENGER and resolve, resolveClaim remains permissionless (important so that op-challenger can call it and resolve games created by op-proposer). Not sure if addLocalData should be permissioned on not, it's currently unrestricted which is probably fine since it only matters if you call step anyway.
| behavior of the original `L2OutputOracle` where a challenge by the `Challenger` role has ultimate | ||
| authority. |
There was a problem hiding this comment.
What I find confusing is that the Permissioned game still has the full FDG interface and logic, just with different auth.
If I understand correctly, the challenger still needs to play out the full game, which would be quite difficult for a multisig. Should we also enable the Challenger to invalidate the game with a single call, which was the case with the L2OutputOracle?
There was a problem hiding this comment.
OK, looking again, assuming that we change onlyAuthorized to accept calls from only the CHALLENGER, then the PROPOSER cannot respond to a move() from the CHALLENGER, and the game can never resolve, effectively making it invalid. Is that right?
There was a problem hiding this comment.
Challenger can execute literally any attack on the root claim and proposer would not be able to respond, so claim would resolve in favor of the challenger.
There was a problem hiding this comment.
Got it. It still feels confusing that there is so much unnecessary logic in here. It requires a lot of knowledge of how the entire system works to recognize that anny move() by the CHALLENGER immediately invalidates the game.
IMO the PermissionedGame would be much easier to reason about if we simply override to move() function to immediately set the state to CHALLENGER_WINS.
There was a problem hiding this comment.
I think if we're going to change the game semantics like that we should just replace the PermissionedGame with something completely custom. It could probably auto-resolve itself after 3.5 days without a challenge as well (just change what it returns from the status() method based on timestamp rather than needing an additional transaction).
That said, it's important to note that to understand any fault dispute game you need to realise that it doesn't have to be played out to the max depth. Every proposal depends on that because every proposal creates a game with a single root claim and if it goes unchallenged it wins. It's not actually expected to ever play a game out to max depth because the incentives are set to discourage making invalid claims.
There was a problem hiding this comment.
Yeah like @ajsutton said, the main benefit here is that we don't need to add a bunch of custom code. We're reusing the fault dispute game under the hood. I'd really prefer not to add a completely custom dispute game (yet).
There was a problem hiding this comment.
I'd really prefer not to add a completely custom dispute game (yet).
Can you expand on the "yet" here? How much bigger is the lift to trim down the permissioned game instead of having it inherit from the permissionless game? The permissioned game sounds like it would be pretty simple, so I'm wondering about the tradeoff here. What makes us lean in favor of this stopgap solution instead of the (seemingly) preferred custom solution?
There was a problem hiding this comment.
Basically any new contract is going to need a design doc, spec, implementation, probably an audit, etc etc. If we just make a tiny tweak to the existing contract we can get the exact same behavior immediately without doing any of that work and it ends up being a 1 line change in an audit. Just trying to reduce the scope of all of this work because we already have a lot of stuff involved in the incident response improvements project.
| Given the above developments, this design doc proposes to remove the ability for the `Proposer` to | ||
| submit moves. This would have the effect of aligning the `PermissionedDisputeGame` with the |
There was a problem hiding this comment.
So the proposer can still step, and the diff is to remove the onlyAuthorized modifier from move and replace it with if (msg.sender != CHALLENGER)) revert BadAuth()? Or is the diff to change the onlyAuthorized modifier so the auth change applies to step also? Whichever is the answer, what is the rationale?
There was a problem hiding this comment.
Both step and move should be restricted to the CHALLENGER. The proposer should be able to create games but not participate in them. step and move are essentially the same concept of "counter this claim" just with implementation differences based on whether we need to actually execute the VM on chain to determine who's right or if we let the game continue playing out.
So only the proposer can create games, only the challenger can counter claims.
Proposes a change to the PermissionedDisputeGame such that the Proposer will no longer be allowed to play the game. Instead, only the Challenger will be allowed to play the game. This more clearly mirrors the behavior of the L2OutputOracle.