Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 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? Grep the `uses:` lines of `.github/workflows/*.y*ml` for `rainix-autopublish.yaml` vs `rainix-tag-release.yaml`; 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