feat(audit): hot-key custody rule — the deploy key deploys and holds NOTHING (#81) - #82
Conversation
…NOTHING Domain rule with three flags: transient hot-key authority (initialAdmin/ owner = deployer, grant-then-renounce ceremonies) severity by what the authority gates; handoffs as per-chain claims needing per-chain fork verification; invariant surfaces asserting the NEGATIVE (broadcast and retired keys hold no role, not DEFAULT_ADMIN_ROLE). Live shape and reference implementation: S01-Issuer/st0x.deploy#250. Closes #81 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughThe audit skill moves to version ChangesAudit skill rules
Estimated code review effort: 2 (Simple) | ~5 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@skills/audit/SKILL.md`:
- Line 228: Update the “Hot-key custody” guidance to clarify that governance may
only be embedded as constants in fixed init bytecode, or supplied through an
atomic deterministic deployment path; do not imply that arbitrary constructor
arguments or later initializer calls are acceptable. Align the wording with
“Deterministic production deployments” and preserve the requirement that the
deploy key never holds authority.
- Line 233: Revise the “Non-findings” guidance to remove CREATE/CREATE2
deployment transactions as sufficient evidence that no authorization check is
needed. Require reviewers to verify that constructor or initializer roles such
as initialAdmin or owner are explicit constants or inputs, not defaults derived
from the broadcaster; retain the separate treatment of Zoltu deployment address
independence.
🪄 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: 5395c3fd-26aa-4f90-88bb-42456e87c595
📒 Files selected for processing (1)
skills/audit/SKILL.md
…ustody rule Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…findings exclude broadcaster-seated roles Hot-key custody now states that governance reaches the artifact as an initializer argument to the deterministic clone for an instance / per-token proxy, and as a hardcoded constant for an implementation (singleton, beacon, deployer), which per Deterministic production deployments takes NO constructor arguments and bakes its config into fixed init bytecode. The non-findings cover a deploy key whose deploy transactions seat no role on the broadcaster — every constructor/initializer role value a constant or explicitly passed in, never derived from msg.sender / tx.origin — and a single-key governance design whose key is not the broadcaster, naming the broadcasting key itself the standing owner as this rule at its strongest. Sender-independence of the ADDRESS is not sender-independence of the AUTHORITY: a Zoltu CREATE2 address is a pure function of its init code and says nothing about what the constructor or initializer writes into storage. Co-Authored-By: Claude Opus 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 `@skills/audit/SKILL.md`:
- Line 231: Expand the negative invariant guidance around
LibAuthoriserInvariants so each live contract enumerates every applicable
authority surface, not only AccessControl roles and DEFAULT_ADMIN_ROLE. Require
deploy and retired keys to be absent from owner/admin fields, proxy admins, Safe
ownership and thresholds, timelocks, and custom authority slots, and compose
these checks into the same assertAll used by production fork tests.
🪄 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: 3f1c0f67-9cdc-4836-9485-8f535dc7e1ac
📒 Files selected for processing (3)
.claude-plugin/marketplace.json.claude-plugin/plugin.jsonskills/audit/SKILL.md
…tract exposes Hot-key custody's flag 3 requires the deploy key and every retired key to hold none of the authority the contract exposes, with the slot set read off the contract's own surface rather than a fixed list — AccessControl roles and DEFAULT_ADMIN_ROLE, an Ownable owner/pendingOwner, a proxy admin slot, a Safe's owners and threshold, a timelock's proposer/executor/canceller and any bespoke onlyX address are examples of it, not its definition. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… is authority The rule prescribes an initializer argument for instances, so it carries the condition that makes that pattern safe: an instance still initializable in a later transaction is front-runnable into seating an attacker. Nothing else in the document states initialization atomicity, so it is stated here, tied to the "from block one" the rule already requires. The custody boundary the rule states is "holds NOTHING", and its verification clause reads the slot set off each contract's own surface, so the opening and the first non-finding name authority rather than roles: an Ownable owner or a proxy admin is authority the broadcaster must not hold, and appears in no role graph. Co-Authored-By: Claude Opus 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 `@skills/audit/SKILL.md`:
- Line 233: Tighten the non-finding in SKILL.md so it only covers clone/deploy
flows where initialization is consumed atomically with creation and the
authority stored in the initializer is bound to the intended governance
principal. Update the non-finding text around the deploy-key and CREATE2
examples to exclude cases where a separate initialize call can be front-run or
where an explicitly passed authority value is not the intended owner; keep the
scoped exemption only for inert deploy metadata and truly atomic,
governance-bound initialization.
🪄 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: 2c0fc60d-5403-4ab5-874d-d2cb13207659
📒 Files selected for processing (1)
skills/audit/SKILL.md
…empts Non-finding 1 restated the requirements rather than referencing them, so each change to them left it behind: it exempted a deploy whose authority values read as constants while saying nothing about when they are seated, passing a clone initialized in a later transaction that the requirements above forbid. It now states the outcome those requirements exist to produce — the key never holds any authority the deployed contracts expose, and creation leaves no window for anyone to seat some — so a requirement added above cannot fall out of it. The single-key governance exemption covers WHO governance is, not how it was seated, and says so: a declared EOA owner is a Security question, and the seating requirements still apply to it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The opening became normative on atomicity, and its violation is the rule with a stranger as principal — nobody's key compromised, no flag matched. "Three flags, all shapes of the one defect" invited a reader to treat the flag list as the checklist, so the list would have had to grow with every requirement the opening gains. It now says what it is: everything above is normative on its own, and the flags are the shapes the hot-key defect takes. A fourth flag was the other option and is the restatement hazard again — it would carry a second copy of the atomicity requirement and its fix, which an edit to the opening could leave behind, and its attacker is a stranger where all three flags key on the broadcaster. The rule's principal is the broadcaster, so flag 1 names the seating by that property rather than by one token: authority taken from the calling context instead of an explicit governance argument. msg.sender is the broadcaster only where the broadcaster calls directly; the rule's own atomic-seating fix routes through a factory, and there msg.sender is the factory while tx.origin is the hot key. Both tokens are now greppable and neither is the definition. The inert-record non-finding turned on "structurally inert" while the rule's boundary is what a contract exposes, and flag 3 counts any bespoke onlyX address the contract reads as authority. It now says recorded but not exposed, so the exemption cannot be read wider than the rule. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Reviewed 3935673: ready Verified before merge: version-hygiene SUCCESS, CodeRabbit SUCCESS reading The rule carries the human's ruling — "the deployer should generally be considered hot wallet, so it shouldn't be in the middle of auth dances, it just needs to deploy and that's it" — as three flags plus non-findings, grounded throughout in the live instance: the ~18 days TEN defects were fixed on this branch, all one class: a concrete clause narrower or looser than the rule it serves. Two found by CodeRabbit, one by me, seven by sweeping for more of the same. In order: "passed as an init argument" citing a sibling rule that FORBIDS constructor args; the CREATE/CREATE2 non-finding exempting the rule's own primary example; the single-key-governance non-finding not excluding the BROADCASTING key itself; the negative assertion enumerating AccessControl roles only; its diagnostic still saying "roles" after the requirement widened; the opening and a worked example still saying "role graph"; the first non-finding RESTATING the seating requirements instead of deferring to them; Two of those are worth calling out because they changed the shape rather than the wording. The seventh was fixed at the CAUSE: the non-finding now DEFERS ("deploy transactions meeting the seating requirements above") instead of re-listing, so a requirement added upstream cannot fall out of it — there is no list left to fall behind. And the ninth was fixed by PROPERTY rather than token: The introduction now states what the structure actually is — "Everything above is normative on its own; the three flags below are the shapes the hot-key defect takes, not a checklist that exhausts the rule — an instance a stranger can still initialize trips it with no key compromised and no flag matched." A fourth flag was declined three times, finally on the strongest ground: it would carry a SECOND COPY of the atomicity requirement and its fix, reintroducing by construction the restatement hazard the seventh fix had just cured. A requirement-by-clause matrix (governance principal / from-block-one / atomic-with-creation / holds-no-authority-exposed, against all three flags and all three non-findings) was run over the final text; the one intentionally partial cell is flag 1's atomicity coverage, which is exactly what the softened introduction exists to disclose. Cross-rule coherence checked rather than assumed: no contradiction with Deterministic production deployments (the instance/implementation partition is that rule's own), Deploy-pin completeness & release historicals (flag 2's per-chain fork assertion is its item-(b) shape, and it borrows its network-set convention verbatim), or State-changing scripts carry a pranked fork test (flag 1 states precedence explicitly). Length balance holds and the opening is not the longest element. Still +9/-3 after ten fixes — the rule absorbed all of them without growing. |
Closes #81
One composite Domain rule, Hot-key custody: the deploy key deploys and holds NOTHING, inserted in the deploy-rule cluster of
skills/audit/SKILL.md(immediately after State-changing scripts carry a pranked fork test), carrying the issue's three flags in the document's idiom:initialAdmin/owner= deployer, msg.sender-defaulting owners, Ownable-then-transferOwnership, grant-then-renounce ceremonies. The window is the finding even when the handoff later completed; severity by what the authority gates (HIGH for value/minting/upgrades/permissions); fix is governance-from-block-one, never a better ceremony. Cross-filed with Hazard cat. 7 and composed with the pranked-fork rule's n+1 rehearsal where a ceremony nonetheless ships.DEFAULT_ADMIN_ROLE, alongside the positive grants, per chain.Live worked example and reference implementation cited throughout: S01-Issuer/st0x.deploy#250 (the 18-day
initialAdmin = deploy keywindow, the decoded 3×grantRole/7×renounceRole batch, the deployer-holds-nothingLibAuthoriserInvariantsassertion). Non-findings stated (sender-independent Zoltu broadcasts, inert protocol metadata, documented single-key governance designs).Skill version bumped 0.33.0 → 0.34.0.
QA
skills/audit/SKILL.md); the repo has no executable test surface for skill prose.0x26519d1c…, creator = the CI deploy key via the nonce factory), the 3×grantRole/7×renounceRolebatch decoded from that key's tx history (2026-07-15), and directhasRolereads on the Base and Ethereum clones confirming the end-state.🤖 Generated with Claude Code
Summary by CodeRabbit