ci(security): Safe-route main Diamond upgrades + add CODEOWNERS (CPL-291)

[GTC6244]

Closes the CRITICAL CPL-291 path where manual_contract-upgrade.yml let any workflow_dispatch caller sign Base-mainnet Diamond facet upgrades directly with PHALA_DSTACKAPP_PRIVATE_KEY. The main branch now deploys facets and proposes the diamondCut to the Safe multisig (vars.SAFE_ADDRESS_CHIPOTLE_MAIN, signed with SAFE_PROPOSER_PRIVATE_KEY — the direct mainnet key is no longer exposed to that job), gated by a new production-base environment; the next testing Diamond keeps its direct upgrade behind a light staging-base gate. A new manual_contract-upgrade-main-2-verify.yml verifies on-chain facet bytecode/selectors after Safe execution, mirroring the existing prod flow. .github/CODEOWNERS adds required-review coverage for lit-api-server/blockchain/, NodeConfig.*.toml, and .github/.

Operational prerequisites (controls are inert without these): create the production-base (required reviewers) and staging-base environments, set vars.SAFE_ADDRESS_CHIPOTLE_MAIN, transfer the main Diamond's ownership to that Safe if still EOA-owned, and enable "Require review from Code Owners" branch protection. CODEOWNERS uses @GTC6244 pending a proper team slug.

🤖 Generated with Claude Code

[Copilot]

Pull request overview

Hardens the manual Diamond upgrade process on Base mainnet by removing the ability for a workflow_dispatch caller to perform a direct diamondCut signed with a privileged mainnet key, instead routing main upgrades through a Safe proposal flow and adding CODEOWNERS coverage for security-critical paths.

Changes:

• Split manual_contract-upgrade.yml into a Safe-proposal-only flow for main (gated by production-base) and a direct upgrade flow for next (gated by staging-base).
• Add a new “phase 2” verification workflow for main to confirm Safe execution and on-chain facets/selectors match compiled artifacts.
• Add .github/CODEOWNERS entries to require review for .github/, lit-api-server/blockchain/, and NodeConfig.*.toml.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.

File	Description
.github/workflows/manual_contract-upgrade.yml	Reworks manual upgrades so main produces a Safe proposal (no direct upgrades) while next remains a direct upgrade behind an environment gate.
.github/workflows/manual_contract-upgrade-main-2-verify.yml	Adds post-execution verification for main upgrades (Safe execution + facet bytecode/selector validation).
.github/CODEOWNERS	Adds required-review coverage for workflow/contract/config surfaces tied to upgrade security.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[clawdbot-glitch003]

🤖 Code review (Claude)

Summary: This PR closes the CPL-291 hole where manual_contract-upgrade.yml let any workflow_dispatch caller upgrade the Base-mainnet main Diamond directly with PHALA_DSTACKAPP_PRIVATE_KEY. It splits the workflow into propose-main (deploy facets with SAFE_PROPOSER_PRIVATE_KEY, propose the diamondCut to vars.SAFE_ADDRESS_CHIPOTLE_MAIN, gated by a production-base environment) and upgrade-next (direct upgrade of the testing Diamond, gated by staging-base), adds a manual_contract-upgrade-main-2-verify.yml that mirrors the existing prod verify flow, and adds .github/CODEOWNERS. The overall design is sound — the proposer EOA genuinely can't upgrade the Diamond once ownership sits with the Safe, and the new propose-update deployer action and propose-diamond-cut/execute-safe-tx/verify-diamond-facets hardhat tasks it reuses all exist and match the established prod flow.

Findings (most severe first)

1. HIGH — This branch regresses the CPL-293 shell-injection fix (#485). Rebase required.
The zurich branch is ~75 commits behind main. Commit 4a8ff643 ("fix(ci): close NodeConfig-derived shell injection in manual_contract-upgrade (CPL-293)") landed on main after this branch forked, and this PR rewrites the same job without it:

• manual_contract-upgrade.yml (PR head) propose-main "Read config" (lines 70–94) only checks NETWORK == "base" and never validates ADDRESS against ^0x[a-fA-F0-9]{40}$; upgrade-next (lines 172–187) validates nothing. Current main has an allowlist case for the network and a regex check for the address precisely because these values come from a checked-out file a PR can mutate.
• Both new jobs interpolate the NodeConfig-derived values and a secret directly into run: scripts — --network=${{ steps.config.outputs.network }}, --address=${{ steps.config.outputs.address }}, --rpc-url=${{ secrets.BASE_CHAIN_RPC }} (PR head lines 126–133 and 215–221). Current main deliberately passes these via env: and quotes them ("$NETWORK", "$ADDRESS", "$RPC_URL"). A crafted contract_address in NodeConfig.next.toml on the next branch (which is not the CODEOWNERS-protected default branch) becomes shell injection in a job that holds PHALA_DSTACKAPP_PRIVATE_KEY and BASE_CHAIN_RPC.

Fix: rebase onto current main and carry the #485 validation + env-var quoting into both new jobs. Whether git surfaces this as a merge conflict or a silent overwrite depends on merge strategy — don't let it land as-is.

2. MEDIUM — Environment gates protect the job, not the secrets; the gate is bypassable until secrets are environment-scoped.
SAFE_PROPOSER_PRIVATE_KEY, PHALA_DSTACKAPP_PRIVATE_KEY, and BASE_CHAIN_RPC are evidently repo-level secrets (they're consumed by workflows with no environment:, e.g. manual_contract-upgrade-prod-1-propose.yml:77). Anyone with write access can push a branch containing a workflow_dispatch workflow with no environment: and read them — production-base/staging-base required reviewers never enter the picture, and CODEOWNERS only gates merges to the default branch, not branch pushes or dispatches. For main this is mitigated only after the Diamond's ownership actually moves to the Safe (listed as a prerequisite, not done in this PR) — until then PHALA_DSTACKAPP_PRIVATE_KEY presumably still owns the main Diamond and the CRITICAL path is still open. Recommend: (a) move these secrets into the production-base/staging-base environments so the reviewer gate also gates secret access, and (b) treat the ownership transfer as a blocker for closing CPL-291, not a follow-up.

3. MEDIUM — Verify workflow never binds the Safe tx to the expected Safe.
manual_contract-upgrade-main-2-verify.yml (Verify step, lines 55–69) calls execute-safe-tx without --safe. In tasks/execute-safe-tx.ts:21 the Safe is resolved from the transaction itself (taskArgs.safe || safeTransaction.safe), and even when --safe is passed the task never asserts it matches safeTransaction.safe. So an executed tx hash from any Safe on Base passes the "Safe transaction was executed" gate and its hash lands in the summary as the on-chain upgrade tx. The subsequent verify-diamond-facets step independently checks the Diamond's bytecode, so end-state verification still holds, but the "Safe TX executed" signal is attestation theater as written. Pass --safe "${{ vars.SAFE_ADDRESS_CHIPOTLE_MAIN }}" and add an equality assertion in execute-safe-tx.ts. (Same latent issue exists in the prod verify flow — worth fixing in the task once for both.)

4. LOW — inputs.safe_tx_hash interpolated into a run: script.
manual_contract-upgrade-main-2-verify.yml:62 uses --safe-tx-hash "${{ inputs.safe_tx_hash }}" — dispatcher-controlled input expanded before the shell runs, i.e. script injection in a job holding BASE_CHAIN_RPC. Low impact (dispatch already needs write access; job is read-only), and it mirrors the existing manual_contract-upgrade-prod-2-verify.yml, but new files shouldn't copy the antipattern — pass it via env:. A cheap belt-and-braces option is validating it as ^0x[a-fA-F0-9]{64}$ first.

5. LOW — upgrade-next lacks the empty-key guard that propose-main has.
propose-main refuses to run when SAFE_PROPOSER_PRIVATE_KEY is unset because contract_deployer falls back to a built-in default key (lines 122–125). upgrade-next (lines 210–221) has no such guard, and its DEPLOYER_KEY ternary can even evaluate to the literal string false when the network is non-base and CONTRACT_DEPLOYER_SECRET_SEPOLIA is unset. Pre-existing behavior, but since the job is being rewritten anyway, copy the guard.

Nits

• .github/CODEOWNERS: /.github/ already covers /.github/workflows/ and /.github/CODEOWNERS; the two extra lines are redundant (harmless, and arguably self-documenting).
• CODEOWNERS + "Require review from Code Owners" only protects the default branch; the next branch that upgrade-next deploys from gets no CODEOWNERS coverage unless next also gets branch protection. Acceptable for staging, but worth stating in the file's header comment.
• If "Propose diamondCut" fails after "Deploy facets" succeeds, freshly deployed facets are orphaned and a re-run redeploys them. Matches the existing prod flow; just an operational note.
• Concurrency looks right: propose-main (group contract-upgrade-main) and the new verify workflow share the same group, so they serialize.
• The execute-safe-tx.ts error-message generalization is correct — the task is now shared beyond addComposeHash.

Verdict

Needs changes. The Safe-routing design is correct and consistent with the existing prod flow, but finding 1 (regressing the already-landed CPL-293 injection fix) must be resolved via rebase + re-applying validation before merge, and finding 2 means the PR doesn't actually close CPL-291 until the secrets are environment-scoped and Diamond ownership moves to the Safe.

[GTC6244]

Response to Claude code review — addressed in 38411faf

Thanks for the review. Point-by-point:

1. HIGH — regresses the CPL-293 shell-injection fix (#485). ✅ Fixed in-branch. Rather than a 75-commit rebase (which risks silently dropping the fix depending on merge strategy), I carried the #485 hardening directly into both new jobs so it's present regardless of how the merge resolves:

• propose-main: added the 0x+40 hex regex check on ADDRESS (network is already pinned to base); NETWORK/ADDRESS/RPC_URL/SAFE_ADDRESS now flow through env: and are referenced quoted ("$ADDRESS", etc.) instead of ${{ }}-interpolated into run:.
• upgrade-next: added the network allowlist case (anvil|yellowstone|base-sepolia|base) and the address regex — it previously validated nothing — plus the same env:/quoting treatment.

⚠️ Maintainer note: please still confirm the merge doesn't overwrite main's copy; the two versions are now equivalent on the injection surface, but a rebase remains the clean long-term path.

2. MEDIUM — environment gates protect the job, not the secrets. ⏸️ Not fixed in code — can't be. Moving SAFE_PROPOSER_PRIVATE_KEY / PHALA_DSTACKAPP_PRIVATE_KEY / BASE_CHAIN_RPC into the production-base/staging-base environments is a GitHub Settings → Environments action, and the Diamond-ownership transfer to the Safe is an on-chain operational step. Both are prerequisites for actually closing CPL-291 and are out of scope for a workflow-file change. Flagging for the maintainer as a merge blocker for CPL-291, not a follow-up.

3. MEDIUM — verify never binds the Safe tx to the expected Safe. ✅ Fixed. The verify workflow now passes --safe "${{ vars.SAFE_ADDRESS_CHIPOTLE_MAIN }}", and execute-safe-tx.ts asserts safeTransaction.safe equals the expected Safe (case-insensitive), exiting non-zero on mismatch. The assertion only fires when --safe is supplied, so the existing prod verify flow is unchanged but can adopt --safe for free.

4. LOW — inputs.safe_tx_hash interpolated into run:. ✅ Fixed. Now passed via env: SAFE_TX_HASH and validated as ^0x[a-fA-F0-9]{64}$ before use.

5. LOW — upgrade-next lacks the empty-key guard. ✅ Fixed. Added a guard rejecting both "" and the literal "false" the ternary produces when a non-base network's key is unset.

Nits:

• CODEOWNERS default-branch-only caveat → ✅ added to the header comment.
• Redundant /.github/workflows/ + /.github/CODEOWNERS lines → left as-is (harmless / self-documenting, per your note).
• Orphaned-facets-on-partial-failure and concurrency notes → acknowledged, no change (matches the existing prod flow).