feat(deployer): add verify-proposal and auto-generate upgrade proposals #4618
Conversation
- Add `deploy verify-proposal --contract <kind> --input schedule.json --input execute.json`: independently verify a timelock (schedule+execute) upgrade proposal against on-chain state and shipped bytecode, without trusting Etherscan or the JSON description - Bytecode check: compare the on-chain impl to the contract binding's DEPLOYED_BYTECODE, normalizing trailing solc CBOR metadata and the UUPS __self immutable; classify FullMatch / CodeMatchMetaDiffers / Mismatch - Governance checks: proxy owner()/currentAdmin() == mapped timelock, delay >= getMinDelay, value/predecessor zero, init selector, version prerequisite; assert schedule and execute share inner payload, salt, value, predecessor - Compute Safe EIP-712 domain/message/tx hashes so signers can match their Ledger - deploy writes proposals into contracts/deployments/proposals/<network>/<date>-<slug>/ (network from chain id; --network/--proposal-slug/--proposals-root overrides): schedule.json, execute.json, a contract kind file, and a templated README - Auto-fill the README Safe hashes: resolve the signing Safe from the deployment-info toml and read its nonce on-chain; skip with a warning when the signer set is ambiguous or lookup fails - Add a CI job to verify committed proposals under contracts/deployments/proposals/**
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. |
|
Claude finished @sveitser's task in 3m 37s —— View job Code Review:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| esac | ||
| echo "Verifying $dir (contract=$contract, network=$network) ..." | ||
| # Build --input flags for each JSON file in the dir. | ||
| input_flags=$(ls "$dir"/*.json | sort | sed 's/^/--input /' | tr '\n' ' ') |
There was a problem hiding this comment.
The wildcard glob "$dir"/*.json will pick up any .json file in the proposal directory, not just schedule.json/execute.json. If someone adds a metadata.json or notes.json, the verification fails with a confusing "expected 2 input files" error rather than running correctly.
Consider enumerating the expected files explicitly:
| input_flags=$(ls "$dir"/*.json | sort | sed 's/^/--input /' | tr '\n' ' ') | |
| input_flags="--input $dir/schedule.json --input $dir/execute.json" |
Or at least document in the contracts/deployments/proposals/README.md that proposal directories must contain exactly schedule.json and execute.json (no other .json files).
| None => pass( | ||
| "init-selector", | ||
| if init_data.is_empty() { | ||
| "empty (expected for patch/no-reinitializer)".to_owned() | ||
| } else { | ||
| format!( | ||
| "non-empty init data but no selector expected; selector=0x{}", | ||
| alloy::hex::encode(&init_data[..4.min(init_data.len())]) | ||
| ) | ||
| }, | ||
| ), |
There was a problem hiding this comment.
When expected_init_selector is None (e.g. FeeContract, RewardClaim) but the proposal contains non-empty init data, this silently passes. For a trust-minimizing verifier, unexpected init data in a contract that isn't supposed to have a reinitializer could indicate a malformed or malicious proposal.
Consider emitting a WARN-level row (still passing, but with a prominent detail) or even failing when init data is present but no selector is expected — matching the defense-in-depth tone of the rest of the checks.
| None => pass( | |
| "init-selector", | |
| if init_data.is_empty() { | |
| "empty (expected for patch/no-reinitializer)".to_owned() | |
| } else { | |
| format!( | |
| "non-empty init data but no selector expected; selector=0x{}", | |
| alloy::hex::encode(&init_data[..4.min(init_data.len())]) | |
| ) | |
| }, | |
| ), | |
| None => { | |
| if init_data.is_empty() { | |
| pass( | |
| "init-selector", | |
| "empty (expected for patch/no-reinitializer)".to_owned(), | |
| ) | |
| } else { | |
| fail( | |
| "init-selector", | |
| format!( | |
| "unexpected non-empty init data for contract with no expected selector; selector=0x{}", | |
| alloy::hex::encode(&init_data[..4.min(init_data.len())]) | |
| ), | |
| ) | |
| } | |
| }, |
| if value.contains('/') || value.contains('\\') { | ||
| anyhow::bail!("{label} must not contain '/' or '\\': {value:?}"); | ||
| } | ||
| if value == ".." || value.starts_with("../") || value.ends_with("/..") { | ||
| anyhow::bail!("{label} must not be '..': {value:?}"); | ||
| } | ||
| Ok(()) |
There was a problem hiding this comment.
Nit: the starts_with("../") and ends_with("/..") checks on lines 40-41 are unreachable — any string containing / is already rejected by the check on line 37. The only reachable case is value == "..".
Not a bug (it's defense-in-depth), just noting it could be simplified to if value == ".." without changing behavior.
- Replace the two --input files with a positional proposal directory; read schedule.json, execute.json, and the contract kind from <dir>/contract (--contract now optional, cross-checked against the file). Fixes the broken num_args=2 usage that rejected two separate --input flags - Simplify the CI job to verify-proposal "$dir"; update README template (PROPOSAL_DIR token), docs, and the helper script comment to the dir form - Remove unused test imports (OpsTimelock, ext::AnvilApi) that failed clippy --all-targets -D warnings; apply nightly rustfmt import grouping
Nextest failures (2) in this run
See the step summary for flaky tests and slowest tests. |
…al README - Proposal PRs add only data: schedule.json, execute.json, and proposal.toml (contract kind, per-phase safe/nonce, all six Safe hashes, and metadata). The author is untrusted; correctness is established by verify-proposal, not by text - verify-proposal recomputes proxy/impl/salt/delay/predecessor/hashes from the JSONs + on-chain + shipped bytecode and asserts they match proposal.toml; the per-phase safe is cross-checked against deployment-info; nonce drift warns - Drop --safe/--nonce from verify (read from the toml); deploy writes the toml (auto-resolves signers, --safe override for ambiguous networks) - Remove per-proposal README generation and the contract file; the only README is the static, trusted contracts/deployments/proposals/README.md describing the flow
- verify-proposal reads chain_id from proposal.toml and defaults --rpc-url to the network's public node (1 -> mainnet, 11155111 -> sepolia publicnode); --rpc-url overrides - Run verify read-only via its own provider, dispatched before the wallet provider is built, so it needs no mnemonic and no localhost node
- build with rustup/mold/protoc like build.yml instead of the nix dev shell - run ./target/debug/deploy; --profile test outputs to target/debug, so the old ./target/test/deploy path failed every run - fetch-depth: 0 so git diff against the base ref resolves - verify a proposal dir when any file in it changes; the .json-only grep skipped proposal.toml tampering and broke under pipefail on no match - drop the per-network RPC case; the binary resolves the public node itself - add Swatinem/rust-cache and raise the timeout to 30 minutes
- require exactly one transaction per Safe batch; extra txs escaped checks - fail non-empty init data for kinds without an expected init selector - embed deployment-info TOMLs via include_str!; the CWD-relative path let checks silently skip outside the repo root - always emit timelock-addr-match and proxy-addr-match from deployment-info; previously skipped without contract-address env vars, letting a look-alike proxy owned by an attacker timelock pass - hard-fail Safe proposer/executor checks on unknown networks and cover the SafeExit timelock instead of skipping it - pin network name to chain id (mainnet/decaf/hoodi); the default RPC was chosen from toml.chain_id and compared against itself - honor ESPRESSO_L1_PROVIDER on the verify-proposal subcommand - strip CBOR metadata only for a bounded tail ending in the canonical solc entry; the old scan never matched real bindings and allowed oversized attacker tails - mask immutables at per-kind artifact offsets instead of a substring scan; offsets kept in sync with bindings by tests over all five contract kinds
- capture each proposal dir's verify output and PASS/FAIL status into a markdown file, posted via marocchino/sticky-pull-request-comment like test.yml - four-backtick fences since proposal descriptions are untrusted - stale comment is deleted when nothing was verified - job gets pull-requests: write; contents stays read-only
- build or setup failures previously posted nothing and deleted the previous sticky comment - write a fallback FAIL comment with the run link when the job fails without producing pr-comment.md
|
@claude review again |
|
Claude encountered an error after 1m 34s —— View job Code Review:
|
- compare bytecode by explaining every difference: accept only 20-byte windows holding the impl address over reference zeros (UUPS __self slots), require the exact per-contract window count, and report the offset plus both values on mismatch - delete the per-contract immutable offset tables; pin each binding's zero runs to the expected slot count in tests instead - accept empty init data only when the proxy already reached the reinitializer's target major version; fail closed if the version query fails
2 participants
deploy verify-proposal --contract <kind> --input schedule.json --input execute.json: independently verify a timelock (schedule+execute) upgradeproposal against on-chain state and shipped bytecode, without trusting
Etherscan or the JSON description
DEPLOYED_BYTECODE, normalizing trailing solc CBOR metadata and the UUPS
__self immutable; classify FullMatch / CodeMatchMetaDiffers / Mismatch
getMinDelay, value/predecessor zero, init selector, version prerequisite;
assert schedule and execute share inner payload, salt, value, predecessor
(network from chain id; --network/--proposal-slug/--proposals-root overrides):
schedule.json, execute.json, a contract kind file, and a templated README
deployment-info toml and read its nonce on-chain; skip with a warning when the
signer set is ambiguous or lookup fails