feat: update withdrawal command for output roots in super proposals #18733
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #18733 +/- ##
==========================================
+ Coverage 76.7% 77.4% +0.7%
==========================================
Files 571 516 -55
Lines 53806 49763 -4043
==========================================
- Hits 41289 38547 -2742
+ Misses 12371 11216 -1155
+ Partials 146 0 -146
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Add TestSuperRootWithdrawalFromGameExtraData that proves withdrawals by reading super root proof directly from SuperFaultDisputeGame.extraData() instead of requiring supervisor-rpc. Also adds DSL support via ProveFromGameExtraData() method.
stevennevins
force-pushed
the
feat/prove-output-root-withdrawals
branch
from
7316c37 to
90b9a2a
Compare
AdvanceTime() in shared orchestrator tests leaves L2 sequencer's l1Origin stuck, causing subsequent tests to fail with "L2 Deposit never found". Fix by: - Moving TestProveFromGameExtraData first (Go runs tests in file order) - Simplifying it to only test prove step (no AdvanceTime needed) - Full withdrawal flow still tested by TestSuperRootWithdrawal
![almanax-ai[bot]](https://avatars.githubusercontent.com/in/934833?s=60&v=4){kind=small}
| // Find output root index for target chain in the super root proof | ||
| var outputRootIndex *big.Int | ||
| for i, outputRoot := range superRootProof.OutputRoots { | ||
| if outputRoot.ChainID.Uint64() == targetChainID { | ||
| outputRootIndex = big.NewInt(int64(i)) | ||
| break | ||
| } | ||
| } |
There was a problem hiding this comment.
Input Validation: Truncated chain ID match in txDataForSuperRootProofFromGame
outputRoot.ChainID.Uint64() is compared to a uint64 chain ID, which silently truncates uint256 chain IDs and can select the wrong OutputRootIndex if a malformed/hostile dispute game returns a large chain ID in extraData. On mainnet L1 this would most likely cause the prove transaction to revert, wasting gas for the operator using the CLI.
Compare chain IDs as *big.Int (use Cmp against SetUint64(targetChainID)) to avoid uint64 truncation.
Don't like this finding? Reply "dismiss" and it won't appear again in future scans.
If it's acknowledged or addressed, reply "resolve" to mark it resolved.
| l2SequenceNumber, ok := unpackedSeq[0].(*big.Int) | ||
| if !ok { | ||
| return nil, errors.New("l2SequenceNumber result is not *big.Int") | ||
| } | ||
|
|
||
| // Convert sequence number (timestamp) to L2 block number using rollup config | ||
| l2BlockNumber, err := rollupCfg.TargetBlockNumber(l2SequenceNumber.Uint64()) | ||
| if err != nil { |
There was a problem hiding this comment.
Input Validation: Sequence number truncation risk in txDataForSuperRootProofFromGame
l2SequenceNumber.Uint64() truncates a potentially large *big.Int without bounds checks before converting to an L2 block number. A malformed/hostile dispute game could return a very large l2SequenceNumber, causing the CLI to build proofs against an unintended L2 block (likely reverting on L1 and burning gas).
Reject l2SequenceNumber values that do not fit in uint64 before calling Uint64() (e.g., BitLen() > 64).
Don't like this finding? Reply "dismiss" and it won't appear again in future scans.
If it's acknowledged or addressed, reply "resolve" to mark it resolved.
| numRoots := len(remaining) / 64 | ||
| outputRoots := make([]SuperRootProofOutputRoot, numRoots) | ||
| for i := 0; i < numRoots; i++ { |
There was a problem hiding this comment.
Denial of Service (DoS): Unbounded allocation in DecodeSuperRootProof
DecodeSuperRootProof allocates outputRoots := make(..., numRoots) directly from attacker-influenced extraData length. If a user is tricked into pointing --dispute-game at a contract returning very large extraData (via eth_call), the CLI/test harness can be forced into excessive memory allocation and potentially crash.
Add a reasonable upper bound for numRoots (expected to be small in practice) and error if exceeded.
Don't like this finding? Reply "dismiss" and it won't appear again in future scans.
If it's acknowledged or addressed, reply "resolve" to mark it resolved.
--dispute-game is now required for super root withdrawals.
Replace two proveWithdrawalTransaction overloads (6-param for super roots, 4-param for output roots) with a single unified 4-param function. When superRootsActive=true, the contract now calls game.rootClaimByChainId(chainId) internally instead of requiring callers to pass SuperRootProof. Changes: - Remove 6-param proveWithdrawalTransaction and related errors - Update _proveWithdrawalTransaction to use rootClaimByChainId - Remove OptimismPortal_WrongProofMethod, InvalidSuperRootProof, InvalidOutputRootIndex, InvalidOutputRootChainId errors - Update tests to use unified signature with mocked rootClaimByChainId BREAKING: Callers using 6-param signature must update to 4-param.
|
|
||
| // Validate the provided Output Root and/or Super Root proof depending on proof method. | ||
| // Validate the output root proof depending on proof method. | ||
| if (superRootsActive) { |
There was a problem hiding this comment.
Logic & Business Rules: chain binding bypass risk in _proveWithdrawalTransaction
When superRootsActive is true, OptimismPortalInterop._proveWithdrawalTransaction trusts _disputeGameProxy.rootClaimByChainId(systemConfig.l2ChainId()) to enforce chain-specific output roots. Some respected game implementations (e.g., OptimisticZkGame.rootClaimByChainId(uint256) returns rootClaim() for any chain ID) do not enforce chain ID, so a prover could select such a game index and satisfy Hashing.hashOutputRootProof with an arbitrary root, weakening cross-chain separation on L1 if those game types remain wasRespectedGameTypeWhenCreated == true under the same factory/registry.
In superRootsActive mode, additionally enforce that the dispute game is a super-root-capable type (or that rootClaimByChainId reverts for non-matching chain IDs), e.g., require _disputeGameProxy.gameType() matches the expected super-root game type, or compare against IFaultDisputeGame(_disputeGameProxy).l2ChainId() when available.
Don't like this finding? Reply "dismiss" and it won't appear again in future scans.
If it's acknowledged or addressed, reply "resolve" to mark it resolved.
Description
Update the op-chain-ops/cmd/withdrawal script to have new flags that allow proving a withdrawal using a specific output root in the super proposal. Removes the need for the supervisor-rpc to prove withdrawals
Tests
TBD
Additional context
N/A
Metadata
Completes #18615