contracts-bedrock: Bind the SystemConfig chain ID in the standard validator - #22366
Merged
Merged
Conversation
…idator The standard validator accepted the expected L2 chain ID as caller-supplied input but never compared it with SystemConfig.l2ChainId(). The only predicate that used the input compared it with the dispute game args, and super game implementations deliberately store zero there, so the super-permissioned branch returned an empty success string for a SystemConfig pointing at another chain. OptimismPortal2.proveWithdrawalTransaction selects the member output root with rootClaimByChainId(systemConfig.l2ChainId()). Without the comparison, validating chain X can succeed while the Portal settles withdrawals against chain Y. Check the chain ID as SYSCON-140 in the SystemConfig assertions, where it binds both game modes, and keep the game args predicate as an additional check.
…tor tests The super mode and ZK mode fixtures took the expected L2 chain ID from the local deploy config and from the super game args. On fork tests neither matches the forked chain, so SYSCON-140 fired on every super mode fork test. Use the L2ChainId artifact that ForkL1Live saves from the superchain registry, matching the convention the non-super fixture already follows.
ajsutton
marked this pull request as ready for review
August 11, 2026 00:18
mds1
approved these changes
Aug 12, 2026
Contributor
Author
|
@OptimismBot Please review this PR |
OptimismBot
approved these changes
Aug 12, 2026
OptimismBot
left a comment
Collaborator
There was a problem hiding this comment.
Approve. The fix is correct, minimal, and closes a real gap. I verified the premise rather than taking it from the description:
StandardValidatorUtils.sol:436setsexpectedL2ChainId = isSuperGame(gameType) ? 0 : _args.l2ChainID, so in super mode the-60game-args predicate was the only consumer of_input.l2ChainIDand it deliberately ignores it. Nothing else bound the caller's expected chain ID toSystemConfig.OptimismPortal2.sol:427resolves the member output root withrootClaimByChainId(systemConfig.l2ChainId()), so the unbound input is exactly the value that decides which chain's withdrawals settle. AddingSYSCON-140inassertValidSystemConfigis the right placement — it's on the unconditional path invalidateWithOverrides, so it binds both super and non-super modes.- In non-super mode the
PDDG-60/CKDG-60game-args check already caught a mismatchedSystemConfig, so this is a strict tightening, not a behavior change for standard chains.
On the two things that looked suspicious and turned out fine:
- Dropping the
SYSCON-140test fromOPContractsManagerStandardValidator_SystemConfig_Testis legitimate.Config.devFeatureSuperRootGamesMigration()is hardcodedreturn true(scripts/libraries/Config.sol:341), soOPContractsManagerStandardValidator_TestInit.setUp()always hitsvm.skip(true, ...)and that whole suite is dead. The surviving test inOPContractsManagerStandardValidator_SuperModeCoreValidation_Testis the one that actually runs, and its suite never skips. - The fixture change to
uint256(uint160(address(artifacts.mustGetAddress("L2ChainId"))))is safe: that artifact is only saved bytest/setup/ForkL1Live.s.sol:133from the registry's.chain_id, and both new reads sit insideisL1ForkTest()branches. The ZK fixture previously took the chain ID from super game args, which are pinned to0, so it was guaranteed wrong onceSYSCON-140exists.
Non-blocking observation, no action needed here: validateMigratedChain gets no equivalent binding — MigrationValidationInput carries no expected chain IDs, so a caller checking a shared super-root DGF still can't assert "this set includes chain X". OPContractsManagerMigrator already rejects zero and duplicate l2ChainIds at migration time, so the loss-of-funds shape is covered; it's just a weaker guarantee for that entrypoint.
OPContractsManagerV2.sol is untouched, no storage layout snapshots change, ABI snapshot correctly unchanged (the modified function is internal), and all 77 checks are green including the op-mainnet / ink-mainnet / unichain-mainnet upgrade fork jobs and the ZK and interop variants.
ClaytonNorthey92
pushed a commit
to hemilabs/optimism
that referenced
this pull request
Sep 19, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The standard validator took the expected L2 chain ID as caller-supplied input but never compared it with
SystemConfig.l2ChainId(). Check it asSYSCON-140.Without it, validation of chain X returns success for a SystemConfig configured for chain Y. The Portal then settles X's withdrawals against Y.
The second commit fixes the fork test fixtures, which took the expected chain ID from the local deploy config rather than the forked chain. They now read the
L2ChainIdartifact from the superchain registry, as the non-super fixture already does.Test plan
SystemConfig.l2ChainId()to a different value. It fails on the unmodified validator with an empty success string.just test-devandjust prpass.test/{L1,dispute,cannon}/**pass for op-mainnet, ink-mainnet, unichain-mainnet, and the ZK_DISPUTE_GAME and OPTIMISM_PORTAL_INTEROP variants.