ethereum-optimism / ethereum-optimism/optimism

contracts-bedrock: MIG-SASR-RGT accepts super game types the migration validator never validates

Open
#22,731 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Go
Stars
6.5k
Forks
4k
Avg merge
2d 18h
Merged PRs (30d)
134

Description

**Claude:** Filed on behalf of @ajsutton, from review feedback by @wwared on ethereum-optimism/optimism#22471.

## Problem

`OPContractsManagerMigrationValidator.MIG-SASR-RGT` accepts any game type that satisfies `GameTypes.isSuperGame`:

https://github.com/ethereum-optimism/optimism/blob/develop/packages/contracts-bedrock/src/L1/opcm/OPContractsManagerMigrationValidator.sol#L128-L132

`isSuperGame` returns true for `SUPER_CANNON`, `SUPER_PERMISSIONED`, `SUPER_ASTERISC_KONA`, `SUPER_CANNON_KONA` and `ZK_DISPUTE_GAME`, but the migration validator only requires and validates `SUPER_PERMISSIONED` and `SUPER_CANNON_KONA` (`MIG-DGF-10` / `MIG-DGF-20`), and it requires `SUPER_CANNON` to be unregistered (`MIG-DGF-60`).

So a migrated chain can respect a game type that has no registered implementation and still pass migration validation. The respected game type gates withdrawal finalization, so the result is a chain that validates clean while withdrawals cannot finalize.

`SUPER_CANNON` is the clearest case: `MIG-DGF-60` asserts it is *not* registered, yet `MIG-SASR-RGT` accepts it as the respected type.

## Why it is filed separately

ethereum-optimism/optimism#22471 added the equivalent assertion to the standard validator (`ASR-RGT`), which now restricts the respected type to the set the selected branch validates. That PR deliberately did not touch the migration validator: the two validators now disagree, and closing the gap needs a design decision plus its own semver bump and tests.

## The naive fix is not correct

Restricting the check to exactly `SUPER_PERMISSIONED` and `SUPER_CANNON_KONA` would reject a ZK-respecting migrated chain. `OPContractsManagerMigrationValidator.sol` has no `ZK_DISPUTE_GAME` references at all, and nothing in the migration path asserts ZK is absent, so a migrated chain can legitimately register and respect a ZK game. That is the mirror of the bug @wwared caught in `ASR-RGT` during review.

Pick one:

1. Allow ZK under the same two conditions the standard validator uses — the `ZK_DISPUTE_GAME` dev feature is enabled *and* the factory registers an implementation. See `isRespectedGameTypeValidated` in `OPContractsManagerStandardValidator.sol` for the reference implementation.
2. Decide ZK is out of scope post-migration and additionally assert `gameImpls(ZK_DISPUTE_GAME) == 0`, alongside the existing `MIG-DGF-60` treatment of `SUPER_CANNON`.

Option 1 keeps the two validators consistent and is what I would expect to want.

## Work required

- Tighten the `MIG-SASR-RGT` condition per the decision above.
- Bump the `OPContractsManagerMigrationValidator` semver and regenerate `snapshots/semver-lock.json`.
- Add coverage for the `SUPER_CANNON` case. Current coverage at `packages/contracts-bedrock/test/L1/opcm/OPContractsManagerMigrationValidator.t.sol:753` only exercises a non-super type, so the gap is untested today.

## Related

- ethereum-optimism/optimism#22471 — added `ASR-RGT` to the standard validator; review thread that raised this.
- ethereum-optimism/optimism#21529 — ZK dev feature removal, relevant to whichever option is chosen.

Contributor guide

Open the contributing guide

Research direction

Start with MIG-SASR-RGT and MIG-DGF-10, MIG-DGF-20, and MIG-DGF-60 in packages/contracts-bedrock/src/L1/opcm/OPContractsManagerMigrationValidator.sol, then compare isRespectedGameTypeValidated in OPContractsManagerStandardValidator.sol. Review the test around packages/contracts-bedrock/test/L1/opcm/OPContractsManagerMigrationValidator.t.sol:753 and choose the documented ZK behavior. Done includes the semver bump, regenerated snapshots/semver-lock.json, and coverage for SUPER_CANNON.

Written by the indexing model from the issue text.

Assessment

Tech stack
solidity
Domain
blockchain
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.