feat(authoriser): retire the Fireblocks service signer — revoke bundle on all three chains - #298
Merged
Merged
Conversation
Contributor
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
📝 WalkthroughWalkthroughThe PR adds a Foundry script that revokes the retired Fireblocks signer’s canonical roles on Base, Ethereum, and HyperEVM. It validates authoriser and Safe state, emits and verifies Safe transaction artifacts, and adds unit, library, and production-fork coverage. ChangesFireblocks signer revocation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant RevokeFireblocksServiceSigner
participant V4Authoriser
participant Safe
participant SafeTxArtifact
RevokeFireblocksServiceSigner->>V4Authoriser: validate deployment and grants
RevokeFireblocksServiceSigner->>Safe: build revokeRole transactions
Safe->>V4Authoriser: execute revocation calls
RevokeFireblocksServiceSigner->>V4Authoriser: verify post-revocation invariants
RevokeFireblocksServiceSigner->>SafeTxArtifact: emit chain-specific artifact
RevokeFireblocksServiceSigner->>SafeTxArtifact: verify expected transaction bundle
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…e on all three chains The original service signer (GRANTEE_SERVICE_1C66, Fireblocks-custodied) is retiring in favour of the additional signer the 20260723 bundle provisioned (GRANTEE_SERVICE_3D0C). This authors the second half of the rotation: a Safe Tx Builder bundle per chain revoking the retired signer's DEPOSIT / WITHDRAW / CERTIFY on the V4 authoriser, dispatched via run-script for base, ethereum and hyperevm. The drift guard doubles as a rotation gate: every non-retired row of the canonical map — the replacement signer's three rows included — must hold before the revoke is authored, so an under-provisioned chain can never be left without an active signer. The canonical map keeps the retired rows until each chain's bundle executes (it pins what production IS); execution flips the prod pin here to FireblocksSignerAlreadyRevoked and the map's 1C66 rows to ExpectedGrantMissing, forcing the post-execution pin PR that removes the rows and pins their absence. Artifacts are chain-suffixed (-<chainid>.json): three chains dispatch in one operational window, and Ethereum + HyperEVM share both the Safe and authoriser addresses, so only the chain id distinguishes their bundles. run-script.yaml additionally gains the hyperevm network option (RPC secret forwarded), which the registry lacked until now. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LUJhyNa4VzJ19qMQTnqPY4
hardyjosh
force-pushed
the
ops/revoke-fireblocks-service-signer
branch
from
August 11, 2026 09:52
404868a to
00f83ac
Compare
# Conflicts: # .github/workflows/run-script.yaml
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/run-script.yaml (1)
46-48: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winScope the HyperEVM option or add HyperEVM support to provisioning.
The workflow maps
hyperevmcorrectly, and the revocation script selects chain ID999and writes a chain-suffixed artifact. However, the workflow exposeshyperevmfor every registered script, while20260723-provision-additional-service-signer.s.solstill revertsUnsupportedChainForProvisioning(999). Add the HyperEVM authoriser branch or reject unsupported script/network combinations.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/run-script.yaml around lines 46 - 48, Update the workflow’s network selection for hyperevm so it is not offered to provisioning scripts until supported, or add the HyperEVM authorizer branch in 20260723-provision-additional-service-signer.s.sol for chain ID 999. Ensure unsupported script/network combinations are rejected instead of reaching the existing UnsupportedChainForProvisioning revert.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In @.github/workflows/run-script.yaml:
- Around line 46-48: Update the workflow’s network selection for hyperevm so it
is not offered to provisioning scripts until supported, or add the HyperEVM
authorizer branch in 20260723-provision-additional-service-signer.s.sol for
chain ID 999. Ensure unsupported script/network combinations are rejected
instead of reaching the existing UnsupportedChainForProvisioning revert.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ea74bb3f-5394-443e-bdf0-d46e6c747c56
📒 Files selected for processing (2)
.github/workflows/run-script.yamlsrc/lib/LibAuthoriserInvariants.sol
Two signer-side verify implementations had already drifted (one skipped the operation field). Extract the comparison into LibSafeOps.assertParsedTxsMatch with a shared TxBuilderArtifactMismatch error; the migration script's verify keeps only its bundle re-derivation and hash logging. The comparison is exactly the part of a verify that must not vary per script. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…fy(string) The script logged the FIRST-TX SafeTxHash, but the Safe UI batches the bundle into one MultiSend and displays the MultiSend SafeTxHash — signers were given a hash their UI never shows. Compute and log the canonical MultiSend hash at the captured nonce, and add the signer-side verify(string) entrypoint (shared comparison, re-derived bundle, live hash + nonce logged), proven by an accept + tamper-reject fork test. The prod test also now re-asserts post-run that every canonical row NOT belonging to the retired signer survives — independently of the script's own post-state, so the test fails even if those checks rot — and pins the EMITTED remainder artifact for a partial re-dispatch (CERTIFY already revoked -> exactly the DEPOSIT+WITHDRAW bundle), via a harness writing a dedicated path because its content differs from the parallel full-bundle tests sharing the canonical file. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…= tampered assertParsedTxsMatch is the single integrity gate for two signing ceremonies but only its data arm was ever exercised — a mutant deleting the chainId, txCount, firstTarget, to or value comparison survived the suite. Walk each field with every other intact so deleting any single comparison fails. operation is not walkable: the JSON layer cannot represent a non-CALL, so the comparison guards non-JSON callers only. verify's staleness path gets its own fork test distinct from tamper: author while all pairs hold, revoke one on the live chain, and the previously-valid artifact refuses with txCount — the authoring-to-signing drift window closing on actual state drift. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
rainix-sol-single-contract reds a .sol declaring two contracts; the partial-re-dispatch harness moves next to the existing selection harness. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@script/20260810-revoke-fireblocks-service-signer.s.sol`:
- Around line 349-358: The documentation around the local integrity-check
function must not claim that verify rejects nonce drift. Either persist the
authoring Safe nonce in emitTxBuilderJson and compare it during verify alongside
the existing assertParsedTxsMatch checks, or remove the nonce-drift claim while
retaining only the guarantees actually enforced.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5c1d21da-8f94-4327-b43f-9eb697adaf1b
📒 Files selected for processing (8)
script/20260729-migrate-governance-to-timelock.s.solscript/20260810-revoke-fireblocks-service-signer.s.solsrc/lib/LibSafeOps.soltest/script/20260729-migrate-governance-to-timelock.t.soltest/script/20260810-revoke-fireblocks-service-signer.prod.t.soltest/script/RevokeFireblocksServiceSignerRemainderHarness.soltest/src/lib/LibSafeOps.t.soltest/src/lib/ParseHarness.sol
Comment on lines
+349
to
+358
| /// @notice Signer-side integrity check for a CI-authored revocation | ||
| /// artifact, run LOCALLY against a live fork before signing: re-runs | ||
| /// the pre-flight and grant-map drift guard, re-derives the revoke | ||
| /// bundle from CURRENT live chain state, asserts the artifact at | ||
| /// `jsonPath` matches it byte-exactly (shared | ||
| /// `LibSafeOps.assertParsedTxsMatch`), and logs the canonical MultiSend | ||
| /// `SafeTxHash` at the live nonce for the Safe-UI cross-check. Any | ||
| /// drift between authoring and signing — a nonce bump, a pair already | ||
| /// revoked, a stale or tampered artifact — surfaces as a typed | ||
| /// mismatch before anyone signs. Deliberately NOT in the run-script |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not state that verify rejects nonce drift.
run() logs the nonce, but emitTxBuilderJson does not store it. assertParsedTxsMatch compares only the chain ID and transaction fields. An unrelated Safe transaction can change safe.nonce() and verify will still accept the artifact. Persist and compare the authoring nonce, or remove the nonce-drift claim.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@script/20260810-revoke-fireblocks-service-signer.s.sol` around lines 349 -
358, The documentation around the local integrity-check function must not claim
that verify rejects nonce drift. Either persist the authoring Safe nonce in
emitTxBuilderJson and compare it during verify alongside the existing
assertParsedTxsMatch checks, or remove the nonce-drift claim while retaining
only the guarantees actually enforced.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
|
Reviewed 4433bf3: KEEP — the revoke bundle is authored from the canonical map with the rotation gate (under-provisioned replacement refuses), self-scopes to still-held pairs, and is proven on live forks of all three chains end to end: full pre-flight, byte-exact artifact round-trip, retired signer stripped while every other canonical row is independently re-asserted to survive, n+1 reversal. Signers now get the canonical MultiSend SafeTxHash (the hash the Safe UI displays for the batched bundle — previously the first-tx hash, which the UI never shows) plus a verify(string) entrypoint proven against accept, tamper (exact field named), and genuine staleness (state drift after authoring refuses with txCount). The shared LibSafeOps.assertParsedTxsMatch is per-field mutation-covered. The sole red check is build-artifact: a confirmed upstream 503 from GitHub's release CDN on the graph-cli tarball in the workflow's nix closure, twice at the same asset — this identical tree passed that job before the empty retrigger commits, and every code-bearing job is green. Rulings-conformance: checked against the repo CLAUDE.md rulings and the rulings stated for this work —
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Authors the second half of the service-signer rotation: a Safe Tx Builder bundle per chain that revokes the retired Fireblocks-custodied signer's (
GRANTEE_SERVICE_1C66) three canonical grants —DEPOSIT/WITHDRAW/CERTIFY— on the V4 authoriser. Its duties moved to the additional service signer (GRANTEE_SERVICE_3D0C) provisioned by the20260723bundle, which is live on all three chains.PENDING — no artifact has been authored or signed yet.
How to dispatch
Actions → run-scriptwithscript = 20260810-revoke-fireblocks-service-signer,sig = run(), once pernetwork∈ {base,ethereum,hyperevm}; each run uploads that chain's Tx Builder JSON for the Safe UI. Artifacts are chain-suffixed (-<chainid>.json) — Ethereum and HyperEVM share both the Safe and authoriser addresses, so only the chain id distinguishes their bundles.run-script.yamlgains thehyperevmnetwork option (RPC secret forwarded), which the registry lacked until now.Before signing, each signer runs the local integrity check against a live fork —
forge script script/20260810-revoke-fireblocks-service-signer.s.sol --sig 'verify(string)' <downloaded.json> --rpc-url <network>— which re-derives the bundle from current chain state, asserts the artifact matches byte-exactly, and prints the canonical MultiSendSafeTxHashat the live nonce: the hash the Safe UI actually displays for the batched bundle (run()logs the same hash at authoring).Safety
expectedGrants()to hold — the replacement signer's three rows included — so a chain where the replacement isn't fully provisioned refuses to author the revoke. The service can never be left without an active signer.FireblocksSignerAlreadyRevoked.Forcing function
The canonical map keeps the
1C66rows for now — it pins what production IS. Executing a chain's bundle flips that chain's rows toExpectedGrantMissingin every live invariant and this PR's prod pin toFireblocksSignerAlreadyRevoked, forcing the post-execution pin PR that removes the rows, pins their absence, and retires the spent prod fixture.Testing
20260810-….t.sol): selection logic on a real local authoriser clone — full/partial/empty bundles, drift guard, under-provisioned-replacement gate.20260810-….prod.t.sol): real forks of Base, Ethereum and HyperEVM — the signer still holds all three pairs on each chain,run()authors the full bundle end to end, the artifact round-trips, every non-retired canonical row is independently re-asserted to survive,verify(string)accepts the authored artifact on a fresh fork and pinpoints a tampered field, and a partial re-dispatch (CERTIFY pre-revoked) emits exactly the two-tx remainder artifact.rainix-sol-test837/837,rainix-sol-staticclean,rainix-sol-legalclean.🤖 Generated with Claude Code
https://claude.ai/code/session_01LUJhyNa4VzJ19qMQTnqPY4
Summary by CodeRabbit
New Features
Tests
Documentation