Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .claude-plugin/marketplace.json
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
"name": "audit",
"source": "./",
"description": "Multi-pass codebase audit: a strictly-sequential pipeline of file-scoped review passes (process, security, test coverage, documentation, code quality, correctness/intent, hazard surface) plus triage, with findings reported (not fixed) and tracked as GitHub issues.",
"version": "0.26.0",
"version": "0.27.0",
"author": { "name": "Rain Open Source Software Ltd" },
"keywords": ["audit", "security", "code-review", "test-coverage", "correctness", "hazard", "solidity"]
}
Expand Down
2 changes: 1 addition & 1 deletion .claude-plugin/plugin.json
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
{
"name": "audit",
"displayName": "Audit",
"version": "0.26.0",
"version": "0.27.0",
"description": "Multi-pass codebase audit (process, security, test coverage, documentation, code quality, correctness/intent verification, hazard surface) plus triage. Passes run strictly sequentially; file-scoped subagents within each pass read one file in full and report findings (not fixes), tracked as GitHub issues.",
"author": {
"name": "Rain Open Source Software Ltd",
Expand Down
14 changes: 12 additions & 2 deletions skills/audit/SKILL.md
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
---
name: audit
description: Codebase audit — seven review dimensions (process, security, test coverage, documentation, code quality, correctness/intent, hazard surface) plus triage. Runs at a DECLARED scope — `whole-repo` (every source file across all languages), `pr:<number>` (the diff plus its ramifications: callees, callers, siblings, and every claim the PR makes) or `paths:<globs>` — and is never diff-only at any of them. Reports problems (never fixes them, never "works correctly"), severity-rates each, attaches a concrete proposed fix, and tracks whole-repo findings as GitHub issues; triage then re-validates each finding against live source and applies fixes TDD-style. Triggers on "audit this codebase", "security review", "full audit", "review the whole repo for bugs/coverage/docs/quality/correctness/hazards", "review this PR's diff plus its ramifications", "find what's wrong before an external audit".
version: 0.26.0
version: 0.27.0
---

# Codebase Audit (multi-dimension, declared scope)
Expand Down Expand Up @@ -183,6 +183,16 @@ Exclude auto-generated files (bindings, build artifacts, `*.pointers.sol` and si
Severity: **MEDIUM** — the codehash pin remains the trust anchor, but unverified source blocks integrators and explorer users outright and signals a verification pipeline that will fail the same way on the next deploy; **HIGH** when consumers integrate against explorer-verified source (an ABI or interface read off the explorer) and the unverified network is one they use.

Without this a repo pins only a single "current" address/codehash and **every bytecode-changing PR collides with it** — the prod-deploy test goes red until a redeploy (the premerge chicken-and-egg). Reference implementations: raindex `LibRaindexDeploy.sol` + `LibRaindexDeployTaggedConstants.t.sol`, and `rain.math.float`. The mechanism itself is shared — `LibSnapshot` in `rain-sol-codegen` (`deployTag` / `freezeSnapshot`, carrying the guard that reverts rather than rewrite a frozen record without a `[package].version` bump); flag a repo hand-rolling its own copy, and flag a freeze path with **no such guard** (**HIGH**) since it silently rewrites the record consumers of the published release pin against.
- **Release-lifecycle split (rainlanguage): every repo is a library repo or a deploy repo, never both.** The org runs two mutually exclusive release lifecycles, each a rainix reusable (referenced `@main`, per **Shared-CI reusable workflow refs pin `@main`**): **library** — `rainix-autopublish.yaml`: publish to soldeer on every content-changing merge to `main`; `[package].version` is the NEXT unpublished version, deliberately one ahead of the registry, bumped on every merge; the repo exports an importable abstract surface and carries NO per-tag deploy-pin record. **Deploy** — `rainix-tag-release.yaml`: publish only on a manual `sol-v*` release tag; `[package].version` is the LAST released version, moving only at release time and in lockstep with the frozen `src/generated/<tag>/` record **Deploy-pin completeness & release historicals** prescribes (the on-chain deploy itself is a separate, earlier manual dispatch — rainlanguage/rainix#282 — so a flaky per-network broadcast never gates the one-shot tag publish). **Identify the repo's DECLARED kind FIRST, from the `.deploy` naming convention**: a repo named `*.deploy` (st0x.deploy, rain.factory.deploy) declares itself a deploy repo; any other name declares a library repo. The name is the org's canonical statement of which lifecycle the repo is ON — it is the first thing a consumer, a reviewer, or this audit reads, and everything else in the repo must conform to it. Then derive two observables and flag ANY disagreement with the declared kind (the declaration is the standard; the observables are the evidence):
- **Nature** — does it carry the frozen deploy record? `src/generated/<tag>/*.pointers.sol` tag dirs and/or a `Lib*Deploy*.sol` pinning `*_DEPLOYED_ADDRESS` / `*_DEPLOYED_CODEHASH` constants — the artifacts **Deterministic production deployments** and **Deploy-pin completeness & release historicals** define. A NON-`.deploy` repo carrying the record contradicts its own name (the record belongs in a `*.deploy` sibling — the split); a `*.deploy` repo without one is a deploy repo that froze nothing, which escalates per the flag list below. Historical drift both ways is exactly what this catches: pre-split rain.factory carried the record under a library name (rain.factory#46's cut), and st0x.deploy carried the `.deploy` name while on the library lifecycle (the treadmill below).
- **Lifecycle** — which reusable do its workflows call? Parse `.github/workflows/*.y*ml` and read the ACTIVE jobs' `uses:` values, matching only the canonical references `rainlanguage/rainix/.github/workflows/rainix-autopublish.yaml@main` and `rainlanguage/rainix/.github/workflows/rainix-tag-release.yaml@main` — never a filename substring, which a commented-out line, a doc snippet, or a fork's copy would satisfy. Malformed or ambiguous workflow YAML is a manual-review case, not a lifecycle match (fail closed, per **Fail-closed on input**). The calling workflow's own filename proves nothing (rain.factory and rain.factory.deploy, opposite lifecycles, both name theirs `package-release.yaml`).

**Flag:**
- **Deploy repo on the library lifecycle** (frozen record + `rainix-autopublish`) — **HIGH**. Every merge bumps `[package].version` while the frozen record's `DEPLOY_TAG` stands still, so the `version == DEPLOY_TAG` identity test (`testDeployTag`) is red on `main` between every merge and the next deploy — the canary is exactly a test that can only be green in the instant after a deploy. A permanently-red check trains every reviewer to wave red CI through, which is worse than having no check. Measured precedents: st0x.deploy's `testDeployTag` red for weeks, climbing `0_1_8 != 0_1_9` through `0_1_29` across a single day of merges (drove rainlanguage/rainix#280); rain.factory autopublishing while carrying `src/generated/0_1_3..0_1_5/` snapshots (rainlanguage/rain.factory#46, the split). The identity test is the canary, NOT the defect — the proposed fix is never to weaken, skip, or delete it; it is to move the repo to `rainix-tag-release`, or, when the repo also publishes a library surface consumers import, SPLIT it: the library keeps the name and the importable surface (consumers unaffected), a new `*.deploy` repo takes the concrete contracts, the deploy-pin lib, the frozen snapshots, and the deploy scripts, consuming the library via soldeer (rain.factory#46's cut).
- **Both reusables referenced from one repo's workflows** — the same finding stated structurally (one repo on both lifecycles at once), and the same **HIGH**, whatever the repo's nature.
- **A mismatch without the treadmill** — **LOW/MEDIUM**, consistency: a `rainix-tag-release` caller with no frozen record — though check the stronger rule first: if the repo actually deploys, freezing nothing is the no-release-record **HIGH** of **Deploy-pin completeness & release historicals**, not a consistency LOW — or a vestigial record in a repo whose lifecycle no longer claims it (an autopublish library still carrying snapshot leftovers a split should have taken, with no deploy path or identity test governing them: they rot silently instead of loudly).

**Non-finding:** a repo whose name, nature and lifecycle all agree — a `*.deploy` repo with the frozen record on `rainix-tag-release`, or a library-named repo with no record on `rainix-autopublish` — including either half of a completed split pair. This is a cross-repo invariant single-repo CI cannot state: the repo cannot know it should have been split, and in-repo the defect surfaces only as "flaky red CI", already normalized — so the audit owns it. Hazard cat. 9 reaches the same finding as a release-record hazard.
Comment thread
coderabbitai[bot] marked this conversation as resolved.
- **ERC-165 interface discovery.** `supportsInterface` is a discovery contract consumers trust to route/validate, so hold it to four rules:
- **Use OZ `ERC165` + `super`, never a hand-rolled base check** (code-quality / correctness): a contract MUST inherit OpenZeppelin's `ERC165` and delegate the base case to `super.supportsInterface(interfaceId)`, adding only its own id(s) — `interfaceId == type(IFooV1).interfaceId || super.supportsInterface(interfaceId)`. Declaring `is IERC165` and writing the base membership check literally (`interfaceId == type(IERC165).interfaceId`) re-implements library logic and drifts from canonical (it's how the `0xffffffff` sentinel or a future base change gets silently mishandled) — a finding **even when it happens to be correct today**.
- **No vacuous advertisement** (hazard: dead surface): a contract that implements `supportsInterface` but advertises **only** `type(IERC165).interfaceId` (or nothing beyond it) is vacuous — *every* ERC-165 contract returns true for `IERC165`, so it conveys nothing. Either advertise a real contract-specific interface, or remove the dead ERC-165 surface.
Expand Down Expand Up @@ -276,7 +286,7 @@ Categories (starting points, NOT exhaustive — anything fitting the framing que
6. **Cross-repo / cross-language drift** — anywhere this repo is the **producer OR consumer** of state crossing a repo/language/runtime boundary without a contract pinning the shape (a token list this repo holds that another repo's JSON registry consumes — *or should* consume; event signatures decoded by indexers in another language without a generated binding; selectors frontends compute independently). If a unilateral change on either side would silently break the other and nothing here enforces the contract, file it — the structural fix may live in another repo, but the finding belongs here whenever the hazard is observable from here.
7. **Manual / out-of-band operational steps** — anything depending on a human remembering to act at the right time ("after bumping X, regenerate pointers manually" → better: a CI check failing on mismatch; "rotate the mint key every 90 days" → better: an on-chain timelock). Each is a finding; the report must propose the structural change removing the step.
8. **Defaults that encode brittle assumptions** — defaults/fallbacks/convenience APIs that work today because of a property the system happens to have but would silently break if it changes (a `balanceOf` view assuming total supply < 2^128 that silently wraps; a storage-slot derivation hardcoding an OZ namespace string that breaks on the next OZ major; a fixture using `block.timestamp = 1` masking a sign-comparison bug). For each: what property does it depend on, and is that property pinned by a test/type or implicit? If implicit, file it.
9. **Non-deterministic / unpinnable deployments** — a production deploy path whose resulting address is not deterministic (nonce-dependent `new` / CREATE, or a deployer/impl whose constructor args vary its init bytecode) cannot be codehash-pinned, cross-repo address-pinned, upgrade-lock-checked, or reproducibly verified — so config silently drifts and deploys can't be independently reproduced. The structural fix is a deterministic deployer (Zoltu CREATE2 over fixed bytecode; CREATE2 clones for instances) with all config hardcoded as constants; the full prescriptive rule lives in the Domain rules' **Deterministic production deployments** — here the finding is the unpinnable-address hazard itself (no pin / verification / reproducibility). HIGH when it blocks pinning or verification of a production artifact. A *missing, incomplete or non-frozen* release record is the same hazard for *past* releases — **no record at all** (hand-maintained address/codehash/creation-code literals, or no pins whatsoever), a pin missing creation/runtime bytecode, a dependency-versioned suffix, or a "frozen" tag whose bytecode re-derives from current source. Note the deploy path being deterministic does NOT discharge this: a Zoltu deploy with nothing frozen per release is still unreproducible for every release already published. See the Domain rules' **Deploy-pin completeness & release historicals**.
9. **Non-deterministic / unpinnable deployments** — a production deploy path whose resulting address is not deterministic (nonce-dependent `new` / CREATE, or a deployer/impl whose constructor args vary its init bytecode) cannot be codehash-pinned, cross-repo address-pinned, upgrade-lock-checked, or reproducibly verified — so config silently drifts and deploys can't be independently reproduced. The structural fix is a deterministic deployer (Zoltu CREATE2 over fixed bytecode; CREATE2 clones for instances) with all config hardcoded as constants; the full prescriptive rule lives in the Domain rules' **Deterministic production deployments** — here the finding is the unpinnable-address hazard itself (no pin / verification / reproducibility). HIGH when it blocks pinning or verification of a production artifact. A *missing, incomplete or non-frozen* release record is the same hazard for *past* releases — **no record at all** (hand-maintained address/codehash/creation-code literals, or no pins whatsoever), a pin missing creation/runtime bytecode, a dependency-versioned suffix, or a "frozen" tag whose bytecode re-derives from current source. Note the deploy path being deterministic does NOT discharge this: a Zoltu deploy with nothing frozen per release is still unreproducible for every release already published. See the Domain rules' **Deploy-pin completeness & release historicals**. The repo's release *lifecycle* is part of the same surface: frozen per-tag snapshots released under the library lifecycle (`rainix-autopublish`) desync `[package].version` from the frozen `DEPLOY_TAG` on every merge, leaving the `version == DEPLOY_TAG` identity test permanently red — classifier, flags, and the split fix live in the Domain rules' **Release-lifecycle split**.

**Every Pass-6 finding names all four parts** (incomplete = revise before filing): (1) **the hazard** — what mistake the shape makes more likely; (2) **the shape that creates it** — the duplicate / ordering / manual step / brittle default; (3) **the realistic scenario** where it silently lands in production; (4) **the structural fix** — generator+check, enumeration test, or redesign. Pass-6 severity: HIGH = corrupts user state/money in a realistic scenario (wrong bridge address, stale codehash admitting a malicious impl, missed key rotation); MEDIUM = silently breaks a feature for some users (website missing a token, indexer crashes on an unknown event); LOW = breaks a dev workflow not production; INFO = a convention kept by discipline today that could be cheaply enforced.

Expand Down
Loading