-
Notifications
You must be signed in to change notification settings - Fork 46
Add bytecode verification doc #291
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,138 @@ | ||||||
# OPCM Bytecode Verification: Design Doc | ||||||
|
||||||
| | | | ||||||
| ------------------ | ---------- | | ||||||
| Author | Maurelian | | ||||||
| Created at | 2025-06-04 | | ||||||
| Initial Reviewers | TBD | | ||||||
| Need Approval From | TBD | | ||||||
| Status | Draft | | ||||||
|
||||||
## Purpose | ||||||
|
||||||
Ensure that the contracts referenced in `OPContractsManager` (OPCM) are verifiably built from | ||||||
trusted source code by introducing a bytecode verification step into the release process. | ||||||
|
||||||
## Summary | ||||||
|
||||||
We propose integrating the `VerifyOPCM.s.sol` Foundry script into the release process via | ||||||
`op-deployer` to validate that deployed contract bytecode matches locally built artifacts. This | ||||||
prevents accidental or malicious mismatches and establishes trust in deployments originating from a | ||||||
known commit. The step will be automated in CI, and become part of the documented SDLC. | ||||||
|
||||||
## Problem Statement + Context | ||||||
|
||||||
Currently, there is no enforced mechanism to ensure that the OPCM used in an upgrade is built from a | ||||||
trusted commit, which should be one labelled as an `op-contracts/vX.Y.Z` tag, and approved by | ||||||
governance. | ||||||
|
||||||
This creates a risk of human error or maliciousness leading to an upgrade performed by an incorrect | ||||||
version of the OPCM (or the implementation contracts it sets). It also presents a risk of a failed | ||||||
upgrade resulting from a misconfigured OPCM (ie. if any [constructor | ||||||
vars](https://github.com/ethereum-optimism/optimism/blob/a10fd5259a3af9a465955b035e16f516327d51d5/packages/contracts-bedrock/src/L1/OPContractsManager.sol#L266-L269) | ||||||
are set incorrectly). | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need a strategy for ensuring that VerifyOPCM remains complete, so that if a new contract is added to the system, that will be identified. |
||||||
|
||||||
We want to eliminate this risk by automating the [Contract's Release | ||||||
Verification](https://www.notion.so/oplabs/Contracts-Release-Checklist-1f8f153ee162805e8236f022ebb8c868?source=copy_linkhttps:/) | ||||||
process, making it easy to demonstrate that an OPCM at a given address corresponds to a trusted | ||||||
commit. | ||||||
|
||||||
The solution should be: | ||||||
|
||||||
- contained within the release commit in the monorepo | ||||||
- easily runnable locally with a single command accepting only an RPC URL and the address of the | ||||||
OPCM | ||||||
- runnable in CI in the `superchain-registry` repo | ||||||
- incorporated into the upgrade process in a way that ensures it is run by multiple people | ||||||
|
||||||
## Proposed Solution | ||||||
|
||||||
### Bytecode verification against the local source | ||||||
|
||||||
A new command should be added to `op-deployer`. | ||||||
|
||||||
``` | ||||||
op-deployer verify-bytecode <opcm-address> | ||||||
|
||||||
``` | ||||||
|
||||||
This command will invoke the `VerifyOPCM.s.sol` script's [default | ||||||
entrypoint](https://github.com/ethereum-optimism/optimism/blob/158e990b76a85acbb018577bd4079190b2d97281/packages/contracts-bedrock/scripts/deploy/VerifyOPCM.s.sol#L126-L129) | ||||||
to verify the OPCM. | ||||||
|
||||||
``` | ||||||
op-deployer verify-bytecode --single-contract <contract-name> <contract-address> | ||||||
``` | ||||||
|
||||||
This command will invoke the `VerifyOPCM.s.sol` script's [runSingle | ||||||
entrypoint](https://github.com/ethereum-optimism/optimism/blob/158e990b76a85acbb018577bd4079190b2d97281/packages/contracts-bedrock/scripts/deploy/VerifyOPCM.s.sol#L135) | ||||||
in order to verify any contracts involved in the upgrade which are not included in the OPCM (ie. the | ||||||
new `DeputyPauseModule` introduced in upgrade 16). | ||||||
|
||||||
### Artifacts source | ||||||
|
||||||
By default, `op-deployer verify-bytecode` will use locally build forge-artifacts to check bytecode. | ||||||
maurelian marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
In order to facilitate quickly running in CI, without having to checkout and rebuild different | ||||||
commits, the command will also accept a tag locator, with the following invocation: | ||||||
|
||||||
``` | ||||||
op-deployer verify-bytecode --dangerously-use-remote-artifacts --artifacts-locator tag://op-contracts/vX.Y.Z | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit, could combine these two args?
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I somewhat prefer it as is because |
||||||
|
||||||
``` | ||||||
|
||||||
The flag `--dangerously-use-remote-artifacts` is intended to discourge the use of remote artifacts | ||||||
when running locally, while still enabling a fast mechanism to run in CI. | ||||||
Comment on lines
+83
to
+84
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ideally we want to be ensuring parity between:
We should consider parallelization so that we can have that property in CI. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. worst case, have the local process be the more complete of the two. |
||||||
|
||||||
### OPCM config verification | ||||||
|
||||||
`op-deployer verify-bytecode` will require one of the following groups of arguments to confirm the | ||||||
configuration of the OPCM. | ||||||
maurelian marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
|
||||||
Address flags: | ||||||
|
||||||
``` | ||||||
--upgrade-controller 0x... \ | ||||||
--superchain-config 0x... \ | ||||||
--protocol-versions 0x... \ | ||||||
--superchain-proxy-admin 0x... | ||||||
``` | ||||||
|
||||||
Superchain-registry flag: | ||||||
|
||||||
``` | ||||||
--superchain-registry <path/to/registry/contracts/superchain-registry.json> | ||||||
maurelian marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
``` | ||||||
|
||||||
The argument groups should be mutually exclusive, either all of the address flags or the | ||||||
superchain-registry flag should be provided, but not both. | ||||||
|
||||||
### Integration points | ||||||
|
||||||
It is important to ensure that the bytecode verification process is run by multiple people and on | ||||||
multiple different machines. | ||||||
Comment on lines
+111
to
+112
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is it important for multiple people to run this? What are the risks if only one person runs the verification? |
||||||
|
||||||
The specific points that we should include the process are: | ||||||
|
||||||
1. Automated in superchain-registry CI (as discussed above). | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this create a circular dependency, in that the monorepo depends on the superchain-registry and now the superchain-registry depends on the monorepo for op-deployer? i.e. how do we know this will work in practice? More broadly, what value does the op-deployer wrapper add? I can instead imagine the superchain-registry CI just cloning the monorepo at the required commit/tag, then running the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good questions which clarify the reason for the op-deployer wrapper:
Although there is a sense in which rebuilding is safer than relying on a download, in both cases the build is occurring on cloud infrastructure, so I don't think we get a ton of benefit from the rebuild. That's why it is important to have it run in at least step 2 here. |
||||||
2. Included in the [contracts release checklist](https://www.notion.so/oplabs/Contracts-Release-Checklist-1f8f153ee162805e8236f022ebb8c868?source=copy_link#1f8f153ee16280998c6bfa1140a5854d) process. | ||||||
3. Additionally for consideration: run by signers during the upgrade process. | ||||||
|
||||||
### Resource Usage | ||||||
|
||||||
N/A | ||||||
|
||||||
## Single Point of Failure and Multi Client Considerations | ||||||
|
||||||
N/A | ||||||
|
||||||
## Impact on Developer Experience | ||||||
|
||||||
- No impact on app developers. | ||||||
- Protocol developers and release managers will have one additional verification step that is | ||||||
largely automated. | ||||||
|
||||||
## Risks & Uncertainties | ||||||
|
||||||
- Reliance on Etherscan APIs. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. in order to remove this reliance, we need to get the initcode another way. |
||||||
- We exclude from this discussion the possibility that the code in the repo is malicious, and are | ||||||
only concerned with verifying the bytecode against a monorepo commit and a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
general note to add codehash verification, with regard to this comment.