feat: Add new FMA Shared Lockbox #193
Conversation
|
This looks pretty thorough to me |
Co-authored-by: Matt Solomon <[email protected]>
| - **Detection:** Verify deployed bytecode against expected hash values before proxy to target the implementation contract. | ||
| - **Recovery Path(s):** Pause the system if a Solidity compiler bug is identified. | ||
|
|
||
| ### FM5: Buggy upgrade over the `SharedLockbox` |
There was a problem hiding this comment.
We have a detailed list of FMA failure modes for bad upgrades in https://github.com/ethereum-optimism/design-docs/blob/main/security/fma-generic-contracts.md. I would suggest having a failure mode that links to that doc, and explains any lockbox-specific mitigations for each item
Edit: Below I see the box checked for "Check this box to confirm that these items have been considered and updated if necessary" but it's not clear to me the relation between that checkbox, this failure mode, and any lockbox-specific mitigations for that failure mode
security/fma-shared-lockbox.md
Outdated
| - **Detection:** Monitor `DependencyAdded` events to catch unexpected additions. Do periodical checks to `dependencySet` to review the existing dependency set recorded in the chain. | ||
| - **Recovery Path(s):** Because removing a chain from the dependency set is currently irreversible, a hard fork will be the only way to revert an unauthorized addition. Patch any potential issue around `DEPOSITOR_ACCOUNT`. | ||
|
|
||
| ### FM7: Unable to add new Chains by Reaching the `uint8` limit in `addDependency` |
There was a problem hiding this comment.
Why limit it to uint8? Can we just make it a uint64 or uint256 and remove this failure mode?
There was a problem hiding this comment.
The original spec passed the dep set via L1 attributes, so it was limited to u8 to keep it small enough to pass on every block. The max size should actually be smaller, given it should not be possible to create an unprovable cluster
security/fma-shared-lockbox.md
Outdated
| - **Detection:** A simple periodic check of `dependencySetSize` in both `DependencyManager` and `SuperchainConfigInterop` should be sufficient to ensure the limit is not approaching. Reverts on calls to `addDependency` would indicate this issue. | ||
| - **Recovery Path(s):** An upgrade over `SuperchainConfigInterop` and `DependencyManager` is needed to support a larger capacity. | ||
|
|
||
| ### FM8: `ETHMigrated` flag remains `false` in the `OptimismPortalInterop` after migration |
There was a problem hiding this comment.
Is the migration plan fleshed out more? Migration is a high-risk action and it would be good to see a concrete sequence of steps we plan to take to perform and validate ETH migration for all portals.
For example one way to mitigate issues here is to atomically set portal implementations to a migrator contract, and then once migration is complete, set it to a portal implementation with no migration features. This removes the need to carry around all migration code and if (!s.migrated) checks, and removes the need to remember to remove those in the future
(This may have all been discussed in the design doc phase, in which case feel free to link to those docs. I feel like we discussed this at some point in the past but don't recall where we left off)
There was a problem hiding this comment.
As we discussed on the TT meeting, we will maintain the current migration path
| ## Action Items | ||
|
|
||
| - [ ] Resolve all the comments. | ||
| - [ ] FM1: Provide tests. |
There was a problem hiding this comment.
may need to tweak some of these a bit based on the above, e.g. adding info about where to verify things in scripts, etc
Co-authored-by: Matt Solomon <[email protected]>
Co-authored-by: Matt Solomon <[email protected]>
Co-authored-by: Matt Solomon <[email protected]>
| - Likelihood: Low. Access controls are specified in `if (!superchainConfig().authorizedPortals(msg.sender)) revert Unauthorized()`, but errors or misconfigurations remain possible. | ||
| - **Mitigation:** There should be tests for `lockETH` and `unlockETH` checks. Access control must be strictly enforced, permitting only approved `OptimismPortal` contracts via the `authorizedPortals` mapping in `SuperchainConfigInterop`. | ||
| - **Detection:** Monitor `ETHUnlocked` events to verify consistency with authorized portal addresses. Set up alerts for suspected unauthorized activity from non-approved addresses. | ||
| - **Recovery Path(s):** Pause the system through `SuperchainConfig` to halt the `SharedLockbox` upon detection of unauthorized access. |
There was a problem hiding this comment.
Is this possible with the current guardian?
|
After reviewing the latest Shared Lockbox design, the decision has been made to remove the feature that allowed adding a new chain via an upgrade initiated from L2. This change has been reflected in the code, and I will update this doc as well. |
Description
This PR introduces the FMA for the new Shared Lockbox design.
Additional context
Below are the references for this project:
It replaces the previous FMA, which can be found here for reference.