|
| 1 | +<!-- START doctoc generated TOC please keep comment here to allow auto update --> |
| 2 | +<!-- DON'T EDIT THIS SECTION, INSTEAD RE-RUN doctoc TO UPDATE --> |
| 3 | +**Table of Contents** *generated with [DocToc](https://github.com/thlorenz/doctoc)* |
| 4 | + |
| 5 | +- [OPCM SuperchainConfig Upgrade Fix: Design Doc](#opcm-superchainconfig-upgrade-fix-design-doc) |
| 6 | + - [Summary](#summary) |
| 7 | + - [Problem Statement + Context](#problem-statement--context) |
| 8 | + - [Customer Requirements and Expected Behavior](#customer-requirements-and-expected-behavior) |
| 9 | + - [OPContractsManager upgradeSuperchainConfig function:](#opcontractsmanager-upgradesuperchainconfig-function) |
| 10 | + - [OPContractsManager upgrade function:](#opcontractsmanager-upgrade-function) |
| 11 | + - [Proposed Solution](#proposed-solution) |
| 12 | + - [Proposal 1: Move the SuperchainConfig upgrade functionality to it's own function:](#proposal-1-move-the-superchainconfig-upgrade-functionality-to-its-own-function) |
| 13 | + - [Proposal 2: SuperchainConfig Upgrade Check Fix](#proposal-2-superchainconfig-upgrade-check-fix) |
| 14 | + - [Proposal 3: Support for different superchainConfigs i.e different Superchains](#proposal-3-support-for-different-superchainconfigs-ie-different-superchains) |
| 15 | + - [Failure Mode Analysis](#failure-mode-analysis) |
| 16 | + |
| 17 | +<!-- END doctoc generated TOC please keep comment here to allow auto update --> |
| 18 | + |
| 19 | +# OPCM SuperchainConfig Upgrade Fix: Design Doc |
| 20 | + |
| 21 | +| | | |
| 22 | +| ------------------ | -------------------------------------------------- | |
| 23 | +| Author | Michael Amadi | |
| 24 | +| Created at | _2025-07-29_ | |
| 25 | +| Initial Reviewers | Matt Solomon, John Mardlin | |
| 26 | +| Need Approval From | _To be assigned_ | |
| 27 | +| Status | _Draft_ | |
| 28 | + |
| 29 | +## Summary |
| 30 | + |
| 31 | +Currently, the OPCM's `upgrade()` function optionally upgrades the superchainConfigProxy based on the following checks: |
| 32 | + |
| 33 | +For [op-contracts/v2.0.0](https://github.com/ethereum-optimism/optimism/blob/8d0dd96e494b2ba154587877351e87788336a4ec/packages/contracts-bedrock/src/L1/OPContractsManager.sol#L477) |
| 34 | +```solidity |
| 35 | + // If the SuperchainConfig is not already upgraded, upgrade it. |
| 36 | + if (superchainProxyAdmin.getProxyImplementation(address(superchainConfig)) != impls.superchainConfigImpl) { |
| 37 | + // Attempt to upgrade. If the ProxyAdmin is not the SuperchainConfig's admin, this will revert. |
| 38 | + upgradeTo(superchainProxyAdmin, address(superchainConfig), impls.superchainConfigImpl); |
| 39 | + } |
| 40 | +``` |
| 41 | + |
| 42 | +and [op-contracts/v4.0.0](https://github.com/ethereum-optimism/optimism/blob/54c19f6acb7a6d3505f884bae601733d3d54a3a6/packages/contracts-bedrock/src/L1/OPContractsManager.sol#L615) |
| 43 | +```solidity |
| 44 | + // If the SuperchainConfig is not already upgraded, upgrade it. NOTE that this type of |
| 45 | + // upgrade means that chains can ONLY be upgraded via this OPCM contract if they use the |
| 46 | + // same SuperchainConfig contract. We will assert this later. |
| 47 | + if (_superchainProxyAdmin.getProxyImplementation(address(_superchainConfig)) != impls.superchainConfigImpl) { |
| 48 | + // Attempt to upgrade. If the ProxyAdmin is not the SuperchainConfig's admin, this will revert. |
| 49 | + upgradeToAndCall( |
| 50 | + _superchainProxyAdmin, |
| 51 | + address(_superchainConfig), |
| 52 | + impls.superchainConfigImpl, |
| 53 | + abi.encodeCall(ISuperchainConfig.upgrade, ()) |
| 54 | + ); |
| 55 | +``` |
| 56 | + |
| 57 | +The check basically says that, if the implementation of the superchainConfig is not the same as the implementation the OPCM has stored, then upgrade it. |
| 58 | + |
| 59 | +To upgrade the superchainConfig, the superchainProxyAdminOwner will need to call the OPCM's `upgrade()` function and optionally pass in values to upgrade any other chain's contracts as long as it controls that chain's proxyAdmin. |
| 60 | + |
| 61 | +## Problem Statement + Context |
| 62 | + |
| 63 | +This works for the most part but has a flaw. Assume the following: |
| 64 | +- There are 2 chains in the Superchain, ChainA and ChainB |
| 65 | +- ChainA's proxyAdmin is also the SuperchainConfig's ProxyAdmin |
| 66 | +- The SuperchainConfig's implementation is Impl0 |
| 67 | +- The OPCM has stored Impl1 as the implementation to upgrade to |
| 68 | + |
| 69 | +If ChainA's proxyAdminOwner (also the SuperchainConfig's ProxyAdminOwner) calls the OPCM's `upgrade()` function, the check above (if (superchainProxyAdmin.getProxyImplementation(address(superchainConfig)) != impls.superchainConfigImpl)) will be true and it will upgrade the SuperchainConfig to Impl1 and also upgrade ChainA's L1 contracts. |
| 70 | + |
| 71 | +If ChainB's `proxyAdminOwner` calls the OPCM's `upgrade()` function, the check above (if (`superchainProxyAdmin.getProxyImplementation(address(superchainConfig)) != impls.superchainConfigImpl)`) will be false and so it will not enter the if block (if it did it will revert since ChainB's `proxyAdminOwner` is not the SuperchainConfig's `ProxyAdminOwner`). So it will skip this and go ahead to upgrade ChainB's L1 contracts. |
| 72 | + |
| 73 | +Now the a problem arises here: |
| 74 | +- The SuperchainConfig's implementation is Impl1 |
| 75 | +- A new OPCM is created with an expected implementation of Impl2 |
| 76 | +- The OPCM's `upgrade()` function is called and the SuperchainConfig's implementation is upgraded to Impl2 |
| 77 | +- A new chain, ChainC comes in or is behind and has to use the old OPCM first |
| 78 | + |
| 79 | +When it calls the OPCM's `upgrade()` function, the check above (if (`superchainProxyAdmin.getProxyImplementation(address(superchainConfig)) != impls.superchainConfigImpl)`) will be true and it will attempt to upgrade the SuperchainConfig which: |
| 80 | +- If the ProxyAdmin is not the SuperchainConfig's ProxyAdmin, it will revert. This essentially means that all upgrades from that OPCM is not possible any longer. Of course unless the SuperchainProxyAdminOwner calls upgrade which would upgrade the SuperchainConfig to Impl1 (effectively a downgrade) and make it possible for other chains to upgrade but for obvious reasons downgrading the SuperchainConfig is not the best course of action. |
| 81 | +- If however it's ProxyAdmin is the SuperchainConfig's ProxyAdmin, it will upgrade the SuperchainConfig to impl1 (old implementation) and then upgrade ChainC's L1 contracts. |
| 82 | + |
| 83 | +## Customer Requirements and Expected Behavior |
| 84 | + |
| 85 | +### OPContractsManager upgradeSuperchainConfig function: |
| 86 | +- The SuperchainConfig has not been upgraded by the OPCM being called. |
| 87 | +- The caller must be the SuperchainConfig's ProxyAdminOwner. |
| 88 | + |
| 89 | +### OPContractsManager upgrade function: |
| 90 | +- ChainA's SuperchainConfig has been upgraded by the OPCM being called. |
| 91 | +- The caller must be ChainA's ProxyAdminOwner. |
| 92 | + |
| 93 | +## Proposed Solution |
| 94 | + |
| 95 | +### Proposal 1: Move the SuperchainConfig upgrade functionality to it's own function: |
| 96 | +This will help simplify the `upgrade()` function and make the checks easier to reason about and implement. As part of this the `opcm-upgrade-checks` script will also need to be updated to support this too. |
| 97 | + |
| 98 | + |
| 99 | +### Proposal 2: SuperchainConfig Upgrade Check Fix |
| 100 | +A proposed solution for this is to change the check to version comparisons. We can hardcode an expected previous and target versions for the SuperchainConfig and compare it to the actual version when performing upgrades. E.g |
| 101 | +- For `The SuperchainConfig has not been upgraded by the OPCM being called` in `OPCM.upgradeSuperchainConfig()`, we check if the SuperchainConfig's actual version is less than the target version. If it is, we can upgrade the SuperchainConfig. Otherwise we revert. We do this check to ensure that SuperchainConfigs are not accidentally downgraded. |
| 102 | +- For `ChainA's SuperchainConfig has been upgraded by the OPCM being called` in `OPCM.upgrade()`, we check if the SuperchainConfig's actual version is greater than or equal to the expected target version. If it is, we can proceed to upgrade the OP Chains L1 contracts. Otherwise we revert. We do not make a strict check here so that OP Chains that are more than 2 SuperchainConfig versions behind can still be upgraded. |
| 103 | + |
| 104 | + |
| 105 | +### Proposal 3: Support for different superchainConfigs i.e different Superchains |
| 106 | +While at it, it is proposed to also add support for different superchainConfigs i.e different Superchains. We can easily achieve this by getting the superchainConfig of each OPChain from its optimismPortal (gotten from the systemConfig in the OPChainConfig struct) rather than using the hardcoded SuperchainConfig address in the OPCM. Note this means that it is possible for OPChains of different superchains to be upgraded in one call to `OPCM.upgrade()` as long as they share the same proxyAdminOwner. This means that we must check that each OPChain's superchainConfig is at the correct version before proceeding to upgrade it, otherwise we revert the transaction. |
| 107 | + |
| 108 | + |
| 109 | +## Failure Mode Analysis |
| 110 | + |
| 111 | +- **A chain might be upgraded when it's superchainConfig is not upgraded:** |
| 112 | + - Mitigation: We can implement a simple loop to assert that the versions of the superchainConfig of all chains in the array is greater than or equal to the expected version. |
0 commit comments