design doc for the ocpm superchain upgrade fix #310

AmadiMichael · 2025-07-29T12:52:17Z

design doc for the ocpm superchain upgrade fix

protocol/opcm-superchain-upgrade-fix.md

maurelian · 2025-07-29T13:26:42Z

protocol/opcm-superchain-upgrade-fix.md

+
+This means all upgrades from that OPCM is not possible any longer.
+
+## Proposed Solution


Before we talk about a solution, we first need to outline the requirements. Please try to list the various situations we need to consider, and the correct behaviour in each situation.

can you re-review?

Towards @maurelian's point, can we move the "Requirements and Expected behaviour" above, so it comes before "Proposed Solution"?

Actually, I see now that the "Requirements and Expected behaviour" is really talking about the Requirements and Expected behaviour needed to successfully implement the proposed solution. I believe what @maurelian is asking for (which I agree with) is a section that comes before "Proposed Solution" to explain what customer and developer requirements are, i.e. for various situations what is the expected behavior when calling upgrade? In slack @maurelian gave some scenarios, here are a few to start with:

Scenario Number PAO == superchainPAO? Target version is latest? SuperchainConfig already on latest? Expected Output

1 Yes Yes Yes TODO

2 Yes Yes No TODO

3 No Yes Yes TODO

4 No Yes No TODO

5 Yes No Yes TODO

6 Yes No No TODO

7 No No Yes TODO

8 No No No TODO

What should happen in each of those scenarios? It would be great to include a table like this will all the TODOs populated.

Can you think through any other missing scenarios (i.e. headers), and add the completed version to a new "Customer Requirements and Expected Behavior" section before "Proposed Solution"? I think we can also delete the "Requirements and Expected behaviour" header and make that part of the "Proposed Solution" header.

I just really want to make sure we are clear on the "Customer Requirements and Expected Behavior" before agreeing on a solution, and we'll want to get approval from Product on this design doc as well

Alternatively, makes for a nice flowchart

graph TD A{PAO == superchainPAO?} A -- Yes --> B{Target version is latest?} A -- No --> E{Target version is latest?} B -- Yes --> C{SuperchainConfig already upgraded?} B -- No --> D{SuperchainConfig already upgraded?} E -- Yes --> F{SuperchainConfig already upgraded?} E -- No --> G{SuperchainConfig already upgraded?} C -- Yes --> S1["TODO"] C -- No --> S2["TODO"] D -- Yes --> S5["TODO"] D -- No --> S6["TODO"] F -- Yes --> S3["TODO"] F -- No --> S4["TODO"] G -- Yes --> S7["TODO"] G -- No --> S8["TODO"]

Loading

just pushed the table

The second column here doesn't make sense to me.
In any situation where the ChainAPAO != superchainPAO you will always get a revert if you are trying to upgrade contracts with a different PAO.

The more relevant consideration is ChainAProxyAdmin == superchainProxyAdmin?, because that determines whether or not the current interface enables us to access the superchainProxyAdmin.

Co-authored-by: Maurelian <[email protected]>

maurelian · 2025-07-29T16:19:59Z

protocol/opcm-superchain-upgrade-fix.md

+
+| Scenario | Description | Expected Behavior |
+|----------|-------------|------------------|
+| Dedicated SuperchainConfig ProxyAdmin | A SuperchainConfig has its own ProxyAdmin that is not shared with any of it's OP chains | When OPCM.upgrade() is called by this SuperchainProxyAdminOwner, it should check if the SuperchainConfig is already upgraded via a isSuperchainUpgraded mapping. If not upgraded, it should upgrade the SuperchainConfig and set isSuperchainUpgraded for that SuperchainConfig to true. No other contract upgrades would occur since this ProxyAdmin only controls the SuperchainConfig. |


When OPCM.upgrade() is called... If not upgraded, it should upgrade the SuperchainConfig...

How can we achieve that? AFAIK there is no way of getting the SuperchainProxyAdmin address from within the upgrade() function.

achieve what in particular?

How can we upgrade the superchainConfig, if the SuperchainConfigProxyAdmin is not shared with the OP Chain? I don't think we can unless we add an argument to upgrade()

if we call upgrade with the superchainPAO and pass in an empty opChainConfig array

protocol/opcm-superchain-upgrade-fix.md

maurelian · 2025-07-29T16:30:36Z

protocol/opcm-superchain-upgrade-fix.md

+## Failure Mode Analysis
+
+- **Protecting the `setSuperchainUpgraded(ISuperchainConfig superchainConfig)` function from being called directly by a malicious actor:**
+    - Explainer: The way the OPCM sets the `isSuperchainUpgraded` variable is by calling  `OPCMUpgrader.setSuperchainUpgraded(ISuperchainConfig superchainConfig)` function. This is safe and does not need checks because if a malicious actor tries doing this, it will fail when trying to actually upgrade the superchainConfig. However, A malicious actor could call the `setSuperchainUpgraded(ISuperchainConfig superchainConfig)` function directly (not via an OPCM upgrade call) to set the `isSuperchainUpgraded` variable to true for any superchainConfig while not being that Superchain's ProxyAdmin. 


This is safe and does not need checks because if a malicious actor tries doing this, it will fail when trying to actually upgrade the superchainConfig

This seems to imply that the upgrade itself happens within setSuperchainUpgraded() is that correct?

oh nope, i was talking about when calling the setSuperchainUpgraded from the upgrade function. it is safe since the upgrade will happen next and if the caller is not the superchainProxyAdmin, the tx will revert

maurelian · 2025-07-29T16:31:07Z

protocol/opcm-superchain-upgrade-fix.md

+
+- **Protecting the `setSuperchainUpgraded(ISuperchainConfig superchainConfig)` function from being called directly by a malicious actor:**
+    - Explainer: The way the OPCM sets the `isSuperchainUpgraded` variable is by calling  `OPCMUpgrader.setSuperchainUpgraded(ISuperchainConfig superchainConfig)` function. This is safe and does not need checks because if a malicious actor tries doing this, it will fail when trying to actually upgrade the superchainConfig. However, A malicious actor could call the `setSuperchainUpgraded(ISuperchainConfig superchainConfig)` function directly (not via an OPCM upgrade call) to set the `isSuperchainUpgraded` variable to true for any superchainConfig while not being that Superchain's ProxyAdmin. 
+    - Mitigation: The obvious way to do this is to check that the msg.sender is the Superchain's ProxyAdmin. This is possible for future OPCM versions because the proxyAdmin variable is accessible via the SuperchainConfig contract. For v2.0.0 and v4.0.0, this is not possible.


I'm a bit confused by this. If the mitigation is impossible, what can we actually do?

i am still looking into it currently

Co-authored-by: Maurelian <[email protected]>

mds1 · 2025-07-29T22:24:37Z

protocol/opcm-superchain-upgrade-fix.md

+- The SuperchainConfig's implementation is Impl0
+- The OPCM has stored Impl1 as the implementation to upgrade to
+
+If ChainA's proxyAdmin (also the SuperchainConfig's ProxyAdmin) 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.


We say ProxyAdmin a lot in this section, but is it correct that really it's the ProxyAdmin Owner that matter?

mds1 · 2025-07-29T22:25:57Z

protocol/opcm-superchain-upgrade-fix.md

+- The OPCM's `upgrade()` function is called and the SuperchainConfig's implementation is upgraded to Impl2
+- A new chain, ChainC comes in or is behind and has to use the old OPCM first
+
+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 will fail since it's ProxyAdmin will not be the SuperchainConfig's ProxyAdmin.


Nit, implicit here is that ChainC has a different ProxyAdmin Owner (PAO)—let's state that explicitly for clarity

We should also explain what happens when ChainC has the same PAO

mds1 · 2025-07-29T22:49:28Z

protocol/opcm-superchain-upgrade-fix.md

+A proposed solution for this is to change the check:
+- Create a variable `bool isSuperchainUpgraded` in the OPCMUpgrader contract.
+- When `OPCM.upgrade()` is called, it delegates call to `OPCMUpgrader.upgrade()` as usual.
+- `OPCMUpgrader.upgrade()` calls back into itself (to be able to access it's storage) and checks if the superchainConfig is already upgraded by checking the `isSuperchainUpgraded` variable.
+- If it is not upgraded, it calls back into itself once more to set the `isSuperchainUpgraded` variable to true and then upgrade the superchainConfig.
+- If it is already upgraded, it will skip the upgrade and continue execution.
+
+While at it, it is proposed to also add support for different superchainConfigs i.e different Superchains. We can easily do this by replacing the `isSuperchainUpgraded` variable with a mapping `mapping(ISuperchainConfig superchainConfig => bool isSuperchainUpgraded)`. This way we can check which superchainConfig is being upgraded and set the `isSuperchainUpgraded` variable to true for that superchainConfig while also preventing the same superchainConfig from being upgraded again.


Hmm the control flow here feels a bit confusing and error prone. Is there a solution where we add inputs so the caller can simply specify whether they want to try upgrading the superchainConfig, with the following logic:

Add bool shouldUpgradeSuperchainConfig as an input to the upgrade method`

If isRc == false, this input value MUST be true. Revert if this condition is violated

Otherwise, proceed to do the upgrade as normal and try upgrading superchainConfig when requested

Note: Downside here is that chains with their own superchainConfig must remember to manually pass in true for this new bool

Alternatively, can we just make the (superchainProxyAdmin.getProxyImplementation(address(superchainConfig)) != impls.superchainConfigImpl)) check smarter to do a greater than/less than semver comparison instead of strict inequality?

Yeah but we want to try not changing the interface at all

Got it, so would it be fair to say there's two solutions here:

Short term patches for the existing OPCM where we don't want to change the interface

Longer term solution (i.e. for all future OPCMs) where we are ok with breaking the interface to simplify things. In these future OPCMs, we can read the PAO from any contract, therefore we can keep the interfaces the same and just have smarter internal logic about when to upgrade?

yes exactly

maurelian · 2025-07-31T17:03:54Z

protocol/opcm-superchain-upgrade-fix.md

+}
+```
+
+## Requirements and Expected behaviour


We should remove this section since we added a similar one above.

protocol/opcm-superchain-upgrade-fix.md

design doc for the ocpm superchain upgrade fix

057bd86

AmadiMichael requested review from mds1 and maurelian July 29, 2025 12:52

maurelian reviewed Jul 29, 2025

View reviewed changes

protocol/opcm-superchain-upgrade-fix.md Outdated Show resolved Hide resolved

maurelian reviewed Jul 29, 2025

View reviewed changes

AmadiMichael and others added 3 commits July 29, 2025 15:59

add requirements and more FMAs

d4a34a3

Update protocol/opcm-superchain-upgrade-fix.md

7497f11

Co-authored-by: Maurelian <[email protected]>

fixes

ad1ce6a

maurelian reviewed Jul 29, 2025

View reviewed changes

AmadiMichael and others added 2 commits July 29, 2025 18:02

Update protocol/opcm-superchain-upgrade-fix.md

e129c34

Co-authored-by: Maurelian <[email protected]>

Update protocol/opcm-superchain-upgrade-fix.md

e0c1348

Co-authored-by: Maurelian <[email protected]>

mds1 reviewed Jul 29, 2025

View reviewed changes

AmadiMichael added 3 commits July 30, 2025 13:21

fix

1bfaadf

fix

cb02adc

add table of expected behavior

76f1ff8

maurelian reviewed Jul 31, 2025

View reviewed changes

protocol/opcm-superchain-upgrade-fix.md Show resolved Hide resolved


		This means all upgrades from that OPCM is not possible any longer.

		## Proposed Solution

Scenario Number	PAO == superchainPAO?	Target version is latest?	SuperchainConfig already on latest?	Expected Output
1	Yes	Yes	Yes	TODO
2	Yes	Yes	No	TODO
3	No	Yes	Yes	TODO
4	No	Yes	No	TODO
5	Yes	No	Yes	TODO
6	Yes	No	No	TODO
7	No	No	Yes	TODO
8	No	No	No	TODO

design doc for the ocpm superchain upgrade fix #310

Are you sure you want to change the base?

design doc for the ocpm superchain upgrade fix #310

Uh oh!

Conversation

AmadiMichael commented Jul 29, 2025

Uh oh!

Uh oh!

maurelian Jul 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mds1 Jul 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mds1 Jul 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

maurelian Jul 29, 2025 •

edited

Loading

mds1 Jul 29, 2025 •

edited

Loading

mds1 Jul 30, 2025 •

edited

Loading