From d55437a472e65cbe70a2469170bac9639412f7cb Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 10:14:09 +0000 Subject: [PATCH 1/5] audit: named returns must be locally provable; org default outranks file-local style (0.21.0) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds a Solidity named-return rule to the Shared rules' Domain rules, beside the other Solidity naming conventions those rules leave a gap in: they reach declarations, locals and params but not return declarations. The rule flags only the subset whose assignment cannot be proved from the declaration's own function — loop/branch-only assignment, never-assigned on a reachable path, shadowed on every path by an explicit return, and last-iteration-wins — plus mixed forms inside a single diff at INFO. It is explicitly not a ban and not a conversion mandate, matching the restraint the pragma convention and the stale-soldeer-deps rule already carry. Cross-references Fail-closed on input (the zero value is the permissive default) and Explicit handling of ambiguous sets (last-iteration-wins reached through a return declaration rather than a .find), which is why this is a correctness rule and not a style preference. Separately fixes dimension 4 #1: style consistency measured against the surrounding file inverts the ruling when the file is itself the outlier. The convention is now established from the org-wide default, with file-local consistency as the weaker signal. Kept general — it holds for every convention where one file can be the outlier. Realigns the SKILL.md frontmatter version with plugin.json/marketplace.json, which had drifted to 0.20.0 while the skill still declared 0.19.0. Co-Authored-By: Claude Opus 5 (1M context) --- .claude-plugin/marketplace.json | 2 +- .claude-plugin/plugin.json | 2 +- skills/audit/SKILL.md | 12 ++++++++++-- 3 files changed, 12 insertions(+), 4 deletions(-) diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json index d35fb0a..defadac 100644 --- a/.claude-plugin/marketplace.json +++ b/.claude-plugin/marketplace.json @@ -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.20.0", + "version": "0.21.0", "author": { "name": "Rain Open Source Software Ltd" }, "keywords": ["audit", "security", "code-review", "test-coverage", "correctness", "hazard", "solidity"] } diff --git a/.claude-plugin/plugin.json b/.claude-plugin/plugin.json index a5ad53e..0298916 100644 --- a/.claude-plugin/plugin.json +++ b/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "name": "audit", "displayName": "Audit", - "version": "0.20.0", + "version": "0.21.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", diff --git a/skills/audit/SKILL.md b/skills/audit/SKILL.md index 21e99a7..f3fec20 100644 --- a/skills/audit/SKILL.md +++ b/skills/audit/SKILL.md @@ -1,7 +1,7 @@ --- name: audit description: Full codebase audit — seven review dimensions (process, security, test coverage, documentation, code quality, correctness/intent, hazard surface) plus triage. Reviews EVERY source file across all languages as a whole-repo snapshot (not a diff), reports problems (never fixes them, never "works correctly"), severity-rates each, attaches a concrete proposed fix, and tracks 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", "find what's wrong before an external audit". -version: 0.19.0 +version: 0.21.0 --- # Codebase Audit (whole-repo, multi-dimension) @@ -53,6 +53,14 @@ Exclude auto-generated files (bindings, build artifacts, `*.pointers.sol` and si - **Rounding direction.** All rounding from precision loss (Float→fixed-decimal conversions, integer division) MUST favor **non-interactive** participants (order owner, contract, protocol — contracts count as non-interactive). For each rounding op: identify WHO is non-interactive, WHICH direction it rounds, WHETHER that direction favors the non-interactive party. Flag any rounding that favors **interactive** participants (`msg.sender`, arb callers, external protocols). - **Variable naming.** Flag short/meaningless names (single chars `r`/`n`/`x`, abbreviations `ob`/`cfg`/`val`) for closure params, loop vars, or local bindings as **LOW** (e.g. `raindex_cfg` not `ob`, `network_key` not `nk`). Short-but-meaningful names (`id`, `url`, `key` when unambiguous) are fine. - **Solidity storage-class naming (rainlanguage).** In Solidity, encode a variable's storage class in its name so its nature is legible at the *use* site, not just its declaration: **immutables** are `i` + UpperCamel (`iBeacon`, `iOrchestratorBeacon`); **mutable storage** (state variables that are neither `constant` nor `immutable`) are `s` + UpperCamel (`sBalance`); **locals and function parameters** contain **no underscore** — `fooBar`, never `_fooBar` (leading) or `foo_bar` (internal), since the leading underscore is reserved for the internal-function convention. Flag an `immutable` not named `iFoo`, a mutable state var not named `sFoo`, and any local/param containing an underscore. Applies to all Solidity in scope, **tests and scripts included**; an interface-mandated parameter name or a documented, deliberate exception may be allow-listed, but default to flagging. **LOW/INFO.** (This is Solidity-only — Rust/TS keep their own idioms, e.g. Rust `snake_case` locals are correct.) +- **Solidity named returns must be locally provable (rainlanguage).** The naming rules above reach declarations, locals and params but NOT **return declarations**, and a named return is where Solidity silently supplies a zero/empty default on a path the unnamed form would refuse to compile. The finding is the subset whose assignment **cannot be proved from the declaration's own function**: + - **Assigned only inside a loop or a branch**, so correctness rests on a guard elsewhere in the function. Worked example: `rainlanguage/rain.deploy`'s `LibRainDeploy.deployToNetworks` declares `returns (address deployedAddress)` and assigns it only in the two arms of an `if`/`else` **inside the `for` loop over `networks`** — safe solely because an `if (networks.length == 0) revert NoNetworks();` at the top of the function guarantees at least one iteration. Weaken or drop that guard and the function returns `address(0)` while still compiling, and nothing catches the zero: the derived-address check runs *before* the loop, the codehash check is *inside* it (so it is skipped with the loop), and there is no check after it. The hazard is that the proof sits three levels away from the declaration. **LOW.** + - **Never assigned on a reachable path** — a silent zero/empty return where the unnamed form is a compile error. This is the same fail-open shape as **Fail-closed on input** above, with the zero value as the permissive default. **MEDIUM.** + - **Shadowed on every path by an explicit `return`** — the name is a dead declaration and pure reader tax. **LOW.** + - **Last-iteration-wins** — a named return assigned once per loop iteration returns only the final one. Benign when every iteration computes the same value (as in the example above: a Zoltu address is deterministic, so every network yields the same one), a real defect when they can diverge; this is **Explicit handling of ambiguous sets** above reached through a return declaration rather than a `.find`. **LOW**, **MEDIUM** when the iterations can diverge. + - **Mixed forms introduced by one diff** — cheap and **authoring-time only**, for code the diff *adds*, where the fix is free. `rainlanguage/rain.deploy#21` adds `zoltuAddress(bytes memory creationCode) internal pure returns (address derivedAddress)` (implicit assignment, no `return`) in the same diff as three new helpers written `returns (address)` / `returns (bytes32)` with an explicit `return`. **INFO.** + + The first four stand regardless of when the code was written; the mixed-forms case is new-code only. **This is NOT a ban on named returns and NOT a mandate to convert them.** A named return sometimes genuinely simplifies the source — a value accumulated across branches, a slot written from an `assembly` block, a multi-value tuple whose component names are the documentation — and converting an existing one to unnamed means adding an explicit `return` to every path of a function whose paths you must first prove you enumerated: a semantic-risk edit bought for a style win, worst in exactly the audited deploy code where that trade is least affordable. So the proposed fix for the loop/branch case is NEVER "unname it" — it is to make the assignment **locally provable** (assign a default at the top of the function, or hoist the guard's invariant into the declaration), which is a smaller and safer change than the refactor. **Non-findings, stated so a reviewer does not reach for them:** a named return documenting a component of a multi-value tuple; a named return referenced from an `assembly` block; one assigned unconditionally on entry, or on every branch of a total `if`/`else`; and a file whose local convention is named returns, absent one of the four cases above — consistency with the org default is **INFO** at most and NEVER justifies a sweep (unnamed is the overwhelming org default, ~88% of return components measured across `rain.erc4626.words`, `rain.math.float`, `rain.interpreter` and `rain.deploy`, but a per-file count is not a mandate; see also dimension 4 #1 on judging any convention against the org default rather than the surrounding file). - **Solidity/Foundry test rules.** Always use **specific** revert expectations — never bare `vm.expectRevert()` (matches any revert, can pass for the wrong reason). Use `vm.expectRevert(abi.encodeWithSelector(Error.selector, args...))` or `vm.expectRevert(Error.selector)`; only `vm.expectRevert(bytes(""))` (with a comment) when a revert genuinely carries no data. Per-test `forge-config: default.fuzz.runs` overrides exist intentionally for slow fuzz tests — don't remove without benchmarking (run it, check timing); conversely add a reduced-runs override when an un-overridden fuzz test takes more than a few seconds. - **Derived constants carry a re-derivation test.** A **derived constant** is a hardcoded literal that is the output of a documented formula/derivation. It is a *second source of truth* for a value that already has a canonical derivation, so without a test that recomputes it the two silently diverge — a mistyped slot reads the wrong storage, a stale `TYPEHASH` breaks signature verification, a wrong codehash admits the wrong bytecode. **The formula in a comment is unenforced; only a test enforces it.** For every derived constant, require a test that RE-DERIVES the value from its documented inputs and asserts equality against the literal; a bare literal with a `// keccak256(...)` comment and no such test is a finding. Covers (non-exhaustive): **ERC-7201 storage slots** (`keccak256(abi.encode(uint256(keccak256()) - 1)) & ~bytes32(uint256(0xff))` — the full compliance checklist is in the Security dimension); **EIP-712** domain separators and `*_TYPEHASH` constants; **keccak-derived ids** (role/permission ids, deployment-suite ids, selectors pinned as literals); **codehashes** (`keccak256(RUNTIME_CODE)` pinned as `bytes32`); **deterministic deploy addresses** (Zoltu/CREATE2 addresses pinned as `address`, re-derivable via the CREATE2 formula over the creation code); and **bitmasks / offsets / sizes** derived from a struct layout or spec. Shape: ```solidity @@ -144,7 +152,7 @@ Review all documentation for **completeness and accuracy** against the implement ### 4. Code quality Review for maintainability, consistency, and good abstractions across the whole repo: -1. **Style consistency** — similar code using different patterns for the same thing. +1. **Style consistency** — similar code using different patterns for the same thing. **Establish the convention from the ORG-WIDE default before judging any one file — never from the surrounding file alone.** A file can itself be the outlier, and measuring locally then *inverts* the finding: the deviating majority inside that one file reads as the standard, and the conventional minority beside it gets flagged as the deviation. `rainlanguage/rain.deploy`'s `LibRainDeploy.sol` is 5 named return components to 1 unnamed, against an org default that is overwhelmingly unnamed — so a reviewer measuring file-locally blesses the named returns and flags the unnamed sibling as the inconsistency, which is exactly backwards. So: derive the convention from the widest first-party scope you can observe (the org, else the repo, else the directory), and **when a file-local convention contradicts a wider default, the wider default is the reference and file-local consistency is the weaker signal.** This is general — it holds for every convention where a single file can be the outlier (import style, error style, test structure, naming), not just returns. A whole-file deviation from the org default, with no other defect, is at most **INFO** and never justifies a mass edit; see the Shared rules' **Solidity pragma convention** and **Solidity named returns must be locally provable** for the two cases where the sweep would itself cause harm. 2. **Leaky abstractions** — internal details exposed through public interfaces, implementation concerns crossing module boundaries, tight coupling between things that should be independent. 3. **Commented-out code** — each instance should be reinstated or deleted, not left commented. 4. **Build warnings** — no warnings from the project's toolchain; build warnings are real problems (**LOW or higher, NOT INFO**). From 9d001d6ed3d3e2a4232f707f83bc76f931fc18fb Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 10:17:22 +0000 Subject: [PATCH 2/5] audit: keep the worked example's zero-escape claim revision-independent The pre-loop derived-address check exists only on rain.deploy#21's branch, not on main. State instead what holds on both: every check is either before the loop or inside it, and there is none after it, so the zero escapes either way. Co-Authored-By: Claude Opus 5 (1M context) --- skills/audit/SKILL.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/skills/audit/SKILL.md b/skills/audit/SKILL.md index f3fec20..17a4340 100644 --- a/skills/audit/SKILL.md +++ b/skills/audit/SKILL.md @@ -54,7 +54,7 @@ Exclude auto-generated files (bindings, build artifacts, `*.pointers.sol` and si - **Variable naming.** Flag short/meaningless names (single chars `r`/`n`/`x`, abbreviations `ob`/`cfg`/`val`) for closure params, loop vars, or local bindings as **LOW** (e.g. `raindex_cfg` not `ob`, `network_key` not `nk`). Short-but-meaningful names (`id`, `url`, `key` when unambiguous) are fine. - **Solidity storage-class naming (rainlanguage).** In Solidity, encode a variable's storage class in its name so its nature is legible at the *use* site, not just its declaration: **immutables** are `i` + UpperCamel (`iBeacon`, `iOrchestratorBeacon`); **mutable storage** (state variables that are neither `constant` nor `immutable`) are `s` + UpperCamel (`sBalance`); **locals and function parameters** contain **no underscore** — `fooBar`, never `_fooBar` (leading) or `foo_bar` (internal), since the leading underscore is reserved for the internal-function convention. Flag an `immutable` not named `iFoo`, a mutable state var not named `sFoo`, and any local/param containing an underscore. Applies to all Solidity in scope, **tests and scripts included**; an interface-mandated parameter name or a documented, deliberate exception may be allow-listed, but default to flagging. **LOW/INFO.** (This is Solidity-only — Rust/TS keep their own idioms, e.g. Rust `snake_case` locals are correct.) - **Solidity named returns must be locally provable (rainlanguage).** The naming rules above reach declarations, locals and params but NOT **return declarations**, and a named return is where Solidity silently supplies a zero/empty default on a path the unnamed form would refuse to compile. The finding is the subset whose assignment **cannot be proved from the declaration's own function**: - - **Assigned only inside a loop or a branch**, so correctness rests on a guard elsewhere in the function. Worked example: `rainlanguage/rain.deploy`'s `LibRainDeploy.deployToNetworks` declares `returns (address deployedAddress)` and assigns it only in the two arms of an `if`/`else` **inside the `for` loop over `networks`** — safe solely because an `if (networks.length == 0) revert NoNetworks();` at the top of the function guarantees at least one iteration. Weaken or drop that guard and the function returns `address(0)` while still compiling, and nothing catches the zero: the derived-address check runs *before* the loop, the codehash check is *inside* it (so it is skipped with the loop), and there is no check after it. The hazard is that the proof sits three levels away from the declaration. **LOW.** + - **Assigned only inside a loop or a branch**, so correctness rests on a guard elsewhere in the function. Worked example: `rainlanguage/rain.deploy`'s `LibRainDeploy.deployToNetworks` declares `returns (address deployedAddress)` and assigns it only in the two arms of an `if`/`else` **inside the `for` loop over `networks`** — safe solely because an `if (networks.length == 0) revert NoNetworks();` at the top of the function guarantees at least one iteration. Weaken or drop that guard and the function returns `address(0)` while still compiling, and nothing catches the zero: every check in the function is either *before* the loop (so it has already run against inputs, not against the return) or *inside* it (so it is skipped along with the loop — including the codehash check on `deployedAddress` itself), and there is none after it. The hazard is that the proof sits three levels away from the declaration. **LOW.** - **Never assigned on a reachable path** — a silent zero/empty return where the unnamed form is a compile error. This is the same fail-open shape as **Fail-closed on input** above, with the zero value as the permissive default. **MEDIUM.** - **Shadowed on every path by an explicit `return`** — the name is a dead declaration and pure reader tax. **LOW.** - **Last-iteration-wins** — a named return assigned once per loop iteration returns only the final one. Benign when every iteration computes the same value (as in the example above: a Zoltu address is deterministic, so every network yields the same one), a real defect when they can diverge; this is **Explicit handling of ambiguous sets** above reached through a return declaration rather than a `.find`. **LOW**, **MEDIUM** when the iterations can diverge. From fb7abe7d0a2db62925d10307d0fbabd269be3a0f Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 10:37:22 +0000 Subject: [PATCH 3/5] audit: define "locally provable", scale severity by what the zero gates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The named-return rule turned on a term it never defined — "cannot be proved from the declaration's own function" — leaving a reader to guess at try/catch, at a branch whose complement reverts, and at a total if/else nested in a loop. Two sessions could reach opposite findings on the rule's own worked example, whose shape (an if/else inside a for loop) is indistinguishable from the "total if/else" non-finding as it was written. Provability is now a stated test over paths reaching the function's end, and nesting is judged by that test rather than by the syntax. Cases 2 and 4 carried flat severities while every neighbouring rule scales by what the value gates, so a never-assigned `returns (bool blocked)` capped at MEDIUM where Fail-closed on input rates the same fail-open default CRITICAL/HIGH. Both now escalate on the scale of the rule they cross- reference, and file once rather than twice. The non-findings mixed two kinds without saying so: some justify the named form (assembly, tuple documentation), some establish provability itself. Only the second kind discharges cases 1/2/4, and case 3 does not reach a name that is the documentation. The mixed-forms example cited rain.deploy#21 adding `zoltuAddress` with a named return. That PR's head is now 528a7c7, "style(deploy): drop the named return from zoltuAddress" — the citation is falsified by going and looking, so the case states the shape instead. Dimension 4 #1 mandated an org-wide measurement an audit cannot make: the skill's file discovery globs one repo. It now prefers a Domain rule (which is where the org-wide defaults actually live), then the repo's own documented conventions, then the widest observable scope — and makes no finding at all where nothing wider than the file is observable. A raw majority count no longer overrides a per-file-kind rule, which would have inverted the pragma convention exactly as #1 inverted named returns. The LibRainDeploy count is pinned to `main` and the load-bearing claim made rot-tolerant. Co-Authored-By: Claude Opus 5 (1M context) --- skills/audit/SKILL.md | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/skills/audit/SKILL.md b/skills/audit/SKILL.md index 17a4340..64d89e3 100644 --- a/skills/audit/SKILL.md +++ b/skills/audit/SKILL.md @@ -53,14 +53,16 @@ Exclude auto-generated files (bindings, build artifacts, `*.pointers.sol` and si - **Rounding direction.** All rounding from precision loss (Float→fixed-decimal conversions, integer division) MUST favor **non-interactive** participants (order owner, contract, protocol — contracts count as non-interactive). For each rounding op: identify WHO is non-interactive, WHICH direction it rounds, WHETHER that direction favors the non-interactive party. Flag any rounding that favors **interactive** participants (`msg.sender`, arb callers, external protocols). - **Variable naming.** Flag short/meaningless names (single chars `r`/`n`/`x`, abbreviations `ob`/`cfg`/`val`) for closure params, loop vars, or local bindings as **LOW** (e.g. `raindex_cfg` not `ob`, `network_key` not `nk`). Short-but-meaningful names (`id`, `url`, `key` when unambiguous) are fine. - **Solidity storage-class naming (rainlanguage).** In Solidity, encode a variable's storage class in its name so its nature is legible at the *use* site, not just its declaration: **immutables** are `i` + UpperCamel (`iBeacon`, `iOrchestratorBeacon`); **mutable storage** (state variables that are neither `constant` nor `immutable`) are `s` + UpperCamel (`sBalance`); **locals and function parameters** contain **no underscore** — `fooBar`, never `_fooBar` (leading) or `foo_bar` (internal), since the leading underscore is reserved for the internal-function convention. Flag an `immutable` not named `iFoo`, a mutable state var not named `sFoo`, and any local/param containing an underscore. Applies to all Solidity in scope, **tests and scripts included**; an interface-mandated parameter name or a documented, deliberate exception may be allow-listed, but default to flagging. **LOW/INFO.** (This is Solidity-only — Rust/TS keep their own idioms, e.g. Rust `snake_case` locals are correct.) -- **Solidity named returns must be locally provable (rainlanguage).** The naming rules above reach declarations, locals and params but NOT **return declarations**, and a named return is where Solidity silently supplies a zero/empty default on a path the unnamed form would refuse to compile. The finding is the subset whose assignment **cannot be proved from the declaration's own function**: - - **Assigned only inside a loop or a branch**, so correctness rests on a guard elsewhere in the function. Worked example: `rainlanguage/rain.deploy`'s `LibRainDeploy.deployToNetworks` declares `returns (address deployedAddress)` and assigns it only in the two arms of an `if`/`else` **inside the `for` loop over `networks`** — safe solely because an `if (networks.length == 0) revert NoNetworks();` at the top of the function guarantees at least one iteration. Weaken or drop that guard and the function returns `address(0)` while still compiling, and nothing catches the zero: every check in the function is either *before* the loop (so it has already run against inputs, not against the return) or *inside* it (so it is skipped along with the loop — including the codehash check on `deployedAddress` itself), and there is none after it. The hazard is that the proof sits three levels away from the declaration. **LOW.** - - **Never assigned on a reachable path** — a silent zero/empty return where the unnamed form is a compile error. This is the same fail-open shape as **Fail-closed on input** above, with the zero value as the permissive default. **MEDIUM.** +- **Solidity named returns must be locally provable (rainlanguage).** The naming rules above reach declarations, locals and params but NOT **return declarations**, and a named return is where Solidity silently supplies a zero/empty default on a path the unnamed form would refuse to compile. The finding is the subset whose assignment **cannot be proved from the declaration's own function**, where *provable* means: on **every path that reaches the function's end** (its closing brace or a bare `return;`), the name is either assigned or the path reverts — judged from that function's body alone. A path reaching the end unassigned returns the zero/empty value. Judge nesting by that test rather than by the syntax: a total `if`/`else` (or a `try`/`catch` assigning in both arms) nested inside a loop or another conditional is NOT total, because the enclosing construct can be skipped. + - **Assigned only inside a loop or a branch**, where a guard elsewhere in the function is what makes every path assign it — provable, but only via that guard, so correctness rests on code far from the declaration (if no guard makes it total, it is the next case instead). Worked example: `rainlanguage/rain.deploy`'s `LibRainDeploy.deployToNetworks` declares `returns (address deployedAddress)` and assigns it only in the two arms of an `if`/`else` **inside the `for` loop over `networks`** — safe solely because an `if (networks.length == 0) revert NoNetworks();` at the top of the function guarantees at least one iteration. Weaken or drop that guard and the function returns `address(0)` while still compiling, and nothing catches the zero: every check in the function is either *before* the loop (so it has already run against inputs, not against the return) or *inside* it (so it is skipped along with the loop — including the codehash check on `deployedAddress` itself), and there is none after it. The hazard is that the proof sits three levels away from the declaration. **LOW.** + - **Never assigned on a reachable path** — no guard makes it total, so a silent zero/empty return where the unnamed form is a compile error. This is the same fail-open shape as **Fail-closed on input** above, with the zero value as the permissive default. **MEDIUM**, and higher on that rule's own scale by what the zero gates — **CRITICAL/HIGH** where it bypasses auth, lifts a spend/write bound, or destroys data. Read the sense before rating it: `returns (bool valid)` falling through to `false` fails *closed*, but a `returns (bool blocked)` / `(bool paused)` falling through to `false` reads as "proceed", and a `returns (address recipient)` falling through to `address(0)` sends funds nowhere — those are not MEDIUMs. - **Shadowed on every path by an explicit `return`** — the name is a dead declaration and pure reader tax. **LOW.** - - **Last-iteration-wins** — a named return assigned once per loop iteration returns only the final one. Benign when every iteration computes the same value (as in the example above: a Zoltu address is deterministic, so every network yields the same one), a real defect when they can diverge; this is **Explicit handling of ambiguous sets** above reached through a return declaration rather than a `.find`. **LOW**, **MEDIUM** when the iterations can diverge. - - **Mixed forms introduced by one diff** — cheap and **authoring-time only**, for code the diff *adds*, where the fix is free. `rainlanguage/rain.deploy#21` adds `zoltuAddress(bytes memory creationCode) internal pure returns (address derivedAddress)` (implicit assignment, no `return`) in the same diff as three new helpers written `returns (address)` / `returns (bytes32)` with an explicit `return`. **INFO.** + - **Last-iteration-wins** — a named return assigned once per loop iteration returns only the final one. Benign when every iteration computes the same value (as in the example above: a Zoltu address is deterministic, so every network yields the same one), a real defect when they can diverge; this is **Explicit handling of ambiguous sets** above reached through a return declaration rather than a `.find`. **LOW**, **MEDIUM** when the iterations can diverge, and **HIGH** on that rule's own scale when the value returned drives a verdict, permission, transfer, or destructive action. + - **Mixed forms introduced by one diff** — cheap and **authoring-time only**, for code the diff *adds*, where the fix is free. The shape is one diff adding adjacent helpers in both forms — a `returns (address derivedAddress)` filled by implicit assignment with no `return`, beside sibling `returns (address)` / `returns (bytes32)` helpers with an explicit `return`. Pick one form for the code the diff adds. **INFO.** - The first four stand regardless of when the code was written; the mixed-forms case is new-code only. **This is NOT a ban on named returns and NOT a mandate to convert them.** A named return sometimes genuinely simplifies the source — a value accumulated across branches, a slot written from an `assembly` block, a multi-value tuple whose component names are the documentation — and converting an existing one to unnamed means adding an explicit `return` to every path of a function whose paths you must first prove you enumerated: a semantic-risk edit bought for a style win, worst in exactly the audited deploy code where that trade is least affordable. So the proposed fix for the loop/branch case is NEVER "unname it" — it is to make the assignment **locally provable** (assign a default at the top of the function, or hoist the guard's invariant into the declaration), which is a smaller and safer change than the refactor. **Non-findings, stated so a reviewer does not reach for them:** a named return documenting a component of a multi-value tuple; a named return referenced from an `assembly` block; one assigned unconditionally on entry, or on every branch of a total `if`/`else`; and a file whose local convention is named returns, absent one of the four cases above — consistency with the org default is **INFO** at most and NEVER justifies a sweep (unnamed is the overwhelming org default, ~88% of return components measured across `rain.erc4626.words`, `rain.math.float`, `rain.interpreter` and `rain.deploy`, but a per-file count is not a mandate; see also dimension 4 #1 on judging any convention against the org default rather than the surrounding file). + The first four stand regardless of when the code was written; the mixed-forms case is new-code only. **This is NOT a ban on named returns and NOT a mandate to convert them.** A named return sometimes genuinely simplifies the source — a value accumulated across branches, a slot written from an `assembly` block, a multi-value tuple whose component names are the documentation — and converting an existing one to unnamed means adding an explicit `return` to every path of a function whose paths you must first prove you enumerated: a semantic-risk edit bought for a style win, worst in exactly the audited deploy code where that trade is least affordable. So the proposed fix for the loop/branch case is NEVER "unname it" — it is to make the assignment **locally provable** (assign a default at the top of the function, or hoist the guard's invariant into the declaration), which is a smaller and safer change than the refactor. + + **Non-findings, stated so a reviewer does not reach for them:** a named return documenting a component of a multi-value tuple; a named return referenced from an `assembly` block; one assigned unconditionally on entry, or on every branch of a total `if`/`else`; and a file whose local convention is named returns, absent one of the four cases above. The first two of those justify the **named form**, not the assignment: a name written from `assembly`, or documenting a tuple component, is still a case 1/2/4 finding when a path reaches the function's end unassigned, and the fix there is the local proof, never unnaming it. Case 3 conversely does not reach a name that IS the documentation — a partially-named tuple (`returns (bool ok, bytes32)`) forces an explicit `return` on every path, so the name is doing the documenting rather than going dead. Cases 2 and 4 are defects **Fail-closed on input** and **Explicit handling of ambiguous sets** reach by another route: file such a finding **once**, here, taking the severity from whichever scale rates it highest. Consistency with the org default is **INFO** at most and NEVER justifies a sweep (unnamed is the overwhelming org default, ~88% of return components measured across `rain.erc4626.words`, `rain.math.float`, `rain.interpreter` and `rain.deploy`, but a per-file count is not a mandate; see also dimension 4 #1 on judging any convention against the widest scope observable rather than the surrounding file). - **Solidity/Foundry test rules.** Always use **specific** revert expectations — never bare `vm.expectRevert()` (matches any revert, can pass for the wrong reason). Use `vm.expectRevert(abi.encodeWithSelector(Error.selector, args...))` or `vm.expectRevert(Error.selector)`; only `vm.expectRevert(bytes(""))` (with a comment) when a revert genuinely carries no data. Per-test `forge-config: default.fuzz.runs` overrides exist intentionally for slow fuzz tests — don't remove without benchmarking (run it, check timing); conversely add a reduced-runs override when an un-overridden fuzz test takes more than a few seconds. - **Derived constants carry a re-derivation test.** A **derived constant** is a hardcoded literal that is the output of a documented formula/derivation. It is a *second source of truth* for a value that already has a canonical derivation, so without a test that recomputes it the two silently diverge — a mistyped slot reads the wrong storage, a stale `TYPEHASH` breaks signature verification, a wrong codehash admits the wrong bytecode. **The formula in a comment is unenforced; only a test enforces it.** For every derived constant, require a test that RE-DERIVES the value from its documented inputs and asserts equality against the literal; a bare literal with a `// keccak256(...)` comment and no such test is a finding. Covers (non-exhaustive): **ERC-7201 storage slots** (`keccak256(abi.encode(uint256(keccak256()) - 1)) & ~bytes32(uint256(0xff))` — the full compliance checklist is in the Security dimension); **EIP-712** domain separators and `*_TYPEHASH` constants; **keccak-derived ids** (role/permission ids, deployment-suite ids, selectors pinned as literals); **codehashes** (`keccak256(RUNTIME_CODE)` pinned as `bytes32`); **deterministic deploy addresses** (Zoltu/CREATE2 addresses pinned as `address`, re-derivable via the CREATE2 formula over the creation code); and **bitmasks / offsets / sizes** derived from a struct layout or spec. Shape: ```solidity @@ -152,7 +154,7 @@ Review all documentation for **completeness and accuracy** against the implement ### 4. Code quality Review for maintainability, consistency, and good abstractions across the whole repo: -1. **Style consistency** — similar code using different patterns for the same thing. **Establish the convention from the ORG-WIDE default before judging any one file — never from the surrounding file alone.** A file can itself be the outlier, and measuring locally then *inverts* the finding: the deviating majority inside that one file reads as the standard, and the conventional minority beside it gets flagged as the deviation. `rainlanguage/rain.deploy`'s `LibRainDeploy.sol` is 5 named return components to 1 unnamed, against an org default that is overwhelmingly unnamed — so a reviewer measuring file-locally blesses the named returns and flags the unnamed sibling as the inconsistency, which is exactly backwards. So: derive the convention from the widest first-party scope you can observe (the org, else the repo, else the directory), and **when a file-local convention contradicts a wider default, the wider default is the reference and file-local consistency is the weaker signal.** This is general — it holds for every convention where a single file can be the outlier (import style, error style, test structure, naming), not just returns. A whole-file deviation from the org default, with no other defect, is at most **INFO** and never justifies a mass edit; see the Shared rules' **Solidity pragma convention** and **Solidity named returns must be locally provable** for the two cases where the sweep would itself cause harm. +1. **Style consistency** — similar code using different patterns for the same thing. **Establish the convention from the WIDEST scope you can actually observe before judging any one file — never from the surrounding file alone.** A file can itself be the outlier, and measuring locally then *inverts* the finding: the deviating majority inside that one file reads as the standard, and the conventional minority beside it gets flagged as the deviation. `rainlanguage/rain.deploy`'s `LibRainDeploy.sol` is predominantly named returns (on `main`, 5 named return components to 1 unnamed) against an org default that is overwhelmingly unnamed — so a reviewer measuring file-locally blesses the named returns and flags the unnamed sibling as the inconsistency, which is exactly backwards. So take the convention, in this order: from a **Domain rule** that states it, where one exists — those rules carry the org-wide defaults precisely because an audit reads ONE repo and cannot measure the org; else from the repo's own `CLAUDE.md` / `AGENTS.md`, since a convention the repo documents deliberately is that repo's reference and not a deviation; else from the widest scope you can observe (the repo, else the directory). **When a file-local convention contradicts a wider one, the wider one is the reference and file-local consistency is the weaker signal.** Two limits keep this from becoming unbounded. Where nothing wider than the file is observable, there is no style-consistency finding to make — do not manufacture one from a two-file sample. And where a Domain rule sets the convention **per file kind**, that rule is the reference and a raw majority count is NOT: most files floating `^` does not make a deliberately pinned `=` on a concrete contract an inconsistency. Otherwise this is general — it holds for every convention where a single file can be the outlier (import style, error style, test structure, naming), not just returns. A whole-file deviation, with no other defect, is at most **INFO** and never justifies a mass edit; see the Shared rules' **Solidity pragma convention** and **Solidity named returns must be locally provable** for the two cases where the sweep would itself cause harm. 2. **Leaky abstractions** — internal details exposed through public interfaces, implementation concerns crossing module boundaries, tight coupling between things that should be independent. 3. **Commented-out code** — each instance should be reinstated or deleted, not left commented. 4. **Build warnings** — no warnings from the project's toolchain; build warnings are real problems (**LOW or higher, NOT INFO**). From ce77a0be6cae207354058c8947ba56f14e387636 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 10:41:21 +0000 Subject: [PATCH 4/5] audit: the unnamed form warns, it is not a compile error MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The rule's central premise was that a named return supplies a zero "on a path the unnamed form would refuse to compile", and case 2 called the unnamed form "a compile error". Both are false. Checked against solc 0.8.25: the unnamed form compiles and emits a WARNING — Unnamed return variable can remain unassigned. Add an explicit return with value to all non-reverting code paths or name the variable. — exit 0. Every unnamed shape with a non-reverting path to the function end warns (empty body, no return statement, return only inside a loop, return in if but not else); a named return draws nothing on any of them; the sole unnamed exception is a completely empty body. That makes the true motivation stronger than the false one. The compiler already detects this class, and it offers "or name the variable" as one of two remedies — so naming the return suppresses the diagnostic on the compiler's own suggestion. Since dimension 4 #4 rates a build warning LOW or higher, the rule is the audit standing in for a signal the name removed, rather than a claim about what compiles. The same probe falsified a sentence added in the previous commit: a partially named tuple does NOT force an explicit return. `returns (bool ok, bytes32)` with no return statement compiles, warning on the unnamed component only. The case-3 carve-out now rests on the name being the component's documentation, which is the real reason, and notes which component solc actually warns about. Prevalence re-measured independently rather than carried over: 86 named / 561 unnamed = 86.7% unnamed across the four repos' src/ function declarations, generated files excluded. "~88%" is reachable only by also counting the `returns` of function TYPES (87.8%), which are types and not declarations, so the figure is now ~87% with its unit stated and the other convention named. The 5-named-to-1-unnamed count for LibRainDeploy.sol on main is confirmed exact (it is 5/2 on rain.deploy#21's head, which is why the pin matters). Co-Authored-By: Claude Opus 5 (1M context) --- skills/audit/SKILL.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/skills/audit/SKILL.md b/skills/audit/SKILL.md index 64d89e3..e22c144 100644 --- a/skills/audit/SKILL.md +++ b/skills/audit/SKILL.md @@ -53,16 +53,16 @@ Exclude auto-generated files (bindings, build artifacts, `*.pointers.sol` and si - **Rounding direction.** All rounding from precision loss (Float→fixed-decimal conversions, integer division) MUST favor **non-interactive** participants (order owner, contract, protocol — contracts count as non-interactive). For each rounding op: identify WHO is non-interactive, WHICH direction it rounds, WHETHER that direction favors the non-interactive party. Flag any rounding that favors **interactive** participants (`msg.sender`, arb callers, external protocols). - **Variable naming.** Flag short/meaningless names (single chars `r`/`n`/`x`, abbreviations `ob`/`cfg`/`val`) for closure params, loop vars, or local bindings as **LOW** (e.g. `raindex_cfg` not `ob`, `network_key` not `nk`). Short-but-meaningful names (`id`, `url`, `key` when unambiguous) are fine. - **Solidity storage-class naming (rainlanguage).** In Solidity, encode a variable's storage class in its name so its nature is legible at the *use* site, not just its declaration: **immutables** are `i` + UpperCamel (`iBeacon`, `iOrchestratorBeacon`); **mutable storage** (state variables that are neither `constant` nor `immutable`) are `s` + UpperCamel (`sBalance`); **locals and function parameters** contain **no underscore** — `fooBar`, never `_fooBar` (leading) or `foo_bar` (internal), since the leading underscore is reserved for the internal-function convention. Flag an `immutable` not named `iFoo`, a mutable state var not named `sFoo`, and any local/param containing an underscore. Applies to all Solidity in scope, **tests and scripts included**; an interface-mandated parameter name or a documented, deliberate exception may be allow-listed, but default to flagging. **LOW/INFO.** (This is Solidity-only — Rust/TS keep their own idioms, e.g. Rust `snake_case` locals are correct.) -- **Solidity named returns must be locally provable (rainlanguage).** The naming rules above reach declarations, locals and params but NOT **return declarations**, and a named return is where Solidity silently supplies a zero/empty default on a path the unnamed form would refuse to compile. The finding is the subset whose assignment **cannot be proved from the declaration's own function**, where *provable* means: on **every path that reaches the function's end** (its closing brace or a bare `return;`), the name is either assigned or the path reverts — judged from that function's body alone. A path reaching the end unassigned returns the zero/empty value. Judge nesting by that test rather than by the syntax: a total `if`/`else` (or a `try`/`catch` assigning in both arms) nested inside a loop or another conditional is NOT total, because the enclosing construct can be skipped. +- **Solidity named returns must be locally provable (rainlanguage).** The naming rules above reach declarations, locals and params but NOT **return declarations**, and a named return is where Solidity silently supplies a zero/empty default. Unnamed, that same fall-through draws solc's *"Unnamed return variable can remain unassigned. Add an explicit return with value to all non-reverting code paths **or name the variable**"* — so naming it suppresses the compiler's own diagnostic, by the compiler's own suggestion (solc 0.8.25: every unnamed shape with a non-reverting path to the end warns, named ones never do, and the sole unnamed exception is a completely empty body). It is NOT a compile error either way. Since dimension 4 #4 rates a build warning **LOW or higher**, this rule is the audit standing in for the signal the name removed. The finding is the subset whose assignment **cannot be proved from the declaration's own function**, where *provable* means: on **every path that reaches the function's end** (its closing brace or a bare `return;`), the name is either assigned or the path reverts — judged from that function's body alone. A path reaching the end unassigned returns the zero/empty value. Judge nesting by that test rather than by the syntax: a total `if`/`else` (or a `try`/`catch` assigning in both arms) nested inside a loop or another conditional is NOT total, because the enclosing construct can be skipped. - **Assigned only inside a loop or a branch**, where a guard elsewhere in the function is what makes every path assign it — provable, but only via that guard, so correctness rests on code far from the declaration (if no guard makes it total, it is the next case instead). Worked example: `rainlanguage/rain.deploy`'s `LibRainDeploy.deployToNetworks` declares `returns (address deployedAddress)` and assigns it only in the two arms of an `if`/`else` **inside the `for` loop over `networks`** — safe solely because an `if (networks.length == 0) revert NoNetworks();` at the top of the function guarantees at least one iteration. Weaken or drop that guard and the function returns `address(0)` while still compiling, and nothing catches the zero: every check in the function is either *before* the loop (so it has already run against inputs, not against the return) or *inside* it (so it is skipped along with the loop — including the codehash check on `deployedAddress` itself), and there is none after it. The hazard is that the proof sits three levels away from the declaration. **LOW.** - - **Never assigned on a reachable path** — no guard makes it total, so a silent zero/empty return where the unnamed form is a compile error. This is the same fail-open shape as **Fail-closed on input** above, with the zero value as the permissive default. **MEDIUM**, and higher on that rule's own scale by what the zero gates — **CRITICAL/HIGH** where it bypasses auth, lifts a spend/write bound, or destroys data. Read the sense before rating it: `returns (bool valid)` falling through to `false` fails *closed*, but a `returns (bool blocked)` / `(bool paused)` falling through to `false` reads as "proceed", and a `returns (address recipient)` falling through to `address(0)` sends funds nowhere — those are not MEDIUMs. + - **Never assigned on a reachable path** — no guard makes it total, so a silent zero/empty return where the unnamed form would have drawn the warning above. This is the same fail-open shape as **Fail-closed on input** above, with the zero value as the permissive default. **MEDIUM**, and higher on that rule's own scale by what the zero gates — **CRITICAL/HIGH** where it bypasses auth, lifts a spend/write bound, or destroys data. Read the sense before rating it: `returns (bool valid)` falling through to `false` fails *closed*, but a `returns (bool blocked)` / `(bool paused)` falling through to `false` reads as "proceed", and a `returns (address recipient)` falling through to `address(0)` sends funds nowhere — those are not MEDIUMs. - **Shadowed on every path by an explicit `return`** — the name is a dead declaration and pure reader tax. **LOW.** - **Last-iteration-wins** — a named return assigned once per loop iteration returns only the final one. Benign when every iteration computes the same value (as in the example above: a Zoltu address is deterministic, so every network yields the same one), a real defect when they can diverge; this is **Explicit handling of ambiguous sets** above reached through a return declaration rather than a `.find`. **LOW**, **MEDIUM** when the iterations can diverge, and **HIGH** on that rule's own scale when the value returned drives a verdict, permission, transfer, or destructive action. - **Mixed forms introduced by one diff** — cheap and **authoring-time only**, for code the diff *adds*, where the fix is free. The shape is one diff adding adjacent helpers in both forms — a `returns (address derivedAddress)` filled by implicit assignment with no `return`, beside sibling `returns (address)` / `returns (bytes32)` helpers with an explicit `return`. Pick one form for the code the diff adds. **INFO.** The first four stand regardless of when the code was written; the mixed-forms case is new-code only. **This is NOT a ban on named returns and NOT a mandate to convert them.** A named return sometimes genuinely simplifies the source — a value accumulated across branches, a slot written from an `assembly` block, a multi-value tuple whose component names are the documentation — and converting an existing one to unnamed means adding an explicit `return` to every path of a function whose paths you must first prove you enumerated: a semantic-risk edit bought for a style win, worst in exactly the audited deploy code where that trade is least affordable. So the proposed fix for the loop/branch case is NEVER "unname it" — it is to make the assignment **locally provable** (assign a default at the top of the function, or hoist the guard's invariant into the declaration), which is a smaller and safer change than the refactor. - **Non-findings, stated so a reviewer does not reach for them:** a named return documenting a component of a multi-value tuple; a named return referenced from an `assembly` block; one assigned unconditionally on entry, or on every branch of a total `if`/`else`; and a file whose local convention is named returns, absent one of the four cases above. The first two of those justify the **named form**, not the assignment: a name written from `assembly`, or documenting a tuple component, is still a case 1/2/4 finding when a path reaches the function's end unassigned, and the fix there is the local proof, never unnaming it. Case 3 conversely does not reach a name that IS the documentation — a partially-named tuple (`returns (bool ok, bytes32)`) forces an explicit `return` on every path, so the name is doing the documenting rather than going dead. Cases 2 and 4 are defects **Fail-closed on input** and **Explicit handling of ambiguous sets** reach by another route: file such a finding **once**, here, taking the severity from whichever scale rates it highest. Consistency with the org default is **INFO** at most and NEVER justifies a sweep (unnamed is the overwhelming org default, ~88% of return components measured across `rain.erc4626.words`, `rain.math.float`, `rain.interpreter` and `rain.deploy`, but a per-file count is not a mandate; see also dimension 4 #1 on judging any convention against the widest scope observable rather than the surrounding file). + **Non-findings, stated so a reviewer does not reach for them:** a named return documenting a component of a multi-value tuple; a named return referenced from an `assembly` block; one assigned unconditionally on entry, or on every branch of a total `if`/`else`; and a file whose local convention is named returns, absent one of the four cases above. The first two of those justify the **named form**, not the assignment: a name written from `assembly`, or documenting a tuple component, is still a case 1/2/4 finding when a path reaches the function's end unassigned, and the fix there is the local proof, never unnaming it. Case 3 conversely does not reach a name that IS the documentation: in a partially-named tuple (`returns (bool ok, bytes32)`) the name is what tells a reader which component is which, so an explicit `return` beside it is not reader tax — and there it is the *unnamed* sibling that solc warns about, not the named one. Cases 2 and 4 are defects **Fail-closed on input** and **Explicit handling of ambiguous sets** reach by another route: file such a finding **once**, here, taking the severity from whichever scale rates it highest. Consistency with the org default is **INFO** at most and NEVER justifies a sweep (unnamed is the overwhelming org default — ~87% of the return components of function *declarations* under `src/`, generated files excluded, measured across `rain.erc4626.words`, `rain.math.float`, `rain.interpreter` and `rain.deploy`; count the `returns` of function *types* too and it is ~88%, so state the unit if you re-measure — but a per-file count is not a mandate; see also dimension 4 #1 on judging any convention against the widest scope observable rather than the surrounding file). - **Solidity/Foundry test rules.** Always use **specific** revert expectations — never bare `vm.expectRevert()` (matches any revert, can pass for the wrong reason). Use `vm.expectRevert(abi.encodeWithSelector(Error.selector, args...))` or `vm.expectRevert(Error.selector)`; only `vm.expectRevert(bytes(""))` (with a comment) when a revert genuinely carries no data. Per-test `forge-config: default.fuzz.runs` overrides exist intentionally for slow fuzz tests — don't remove without benchmarking (run it, check timing); conversely add a reduced-runs override when an un-overridden fuzz test takes more than a few seconds. - **Derived constants carry a re-derivation test.** A **derived constant** is a hardcoded literal that is the output of a documented formula/derivation. It is a *second source of truth* for a value that already has a canonical derivation, so without a test that recomputes it the two silently diverge — a mistyped slot reads the wrong storage, a stale `TYPEHASH` breaks signature verification, a wrong codehash admits the wrong bytecode. **The formula in a comment is unenforced; only a test enforces it.** For every derived constant, require a test that RE-DERIVES the value from its documented inputs and asserts equality against the literal; a bare literal with a `// keccak256(...)` comment and no such test is a finding. Covers (non-exhaustive): **ERC-7201 storage slots** (`keccak256(abi.encode(uint256(keccak256()) - 1)) & ~bytes32(uint256(0xff))` — the full compliance checklist is in the Security dimension); **EIP-712** domain separators and `*_TYPEHASH` constants; **keccak-derived ids** (role/permission ids, deployment-suite ids, selectors pinned as literals); **codehashes** (`keccak256(RUNTIME_CODE)` pinned as `bytes32`); **deterministic deploy addresses** (Zoltu/CREATE2 addresses pinned as `address`, re-derivable via the CREATE2 formula over the creation code); and **bitmasks / offsets / sizes** derived from a struct layout or spec. Shape: ```solidity From 5371cbf493ffb4812a153527f0d32a9724e6092d Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 10:42:17 +0000 Subject: [PATCH 5/5] audit: widen the solc observation to 0.8.35, name the suppressor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The warning behaviour is identical on 0.8.25 and 0.8.35 — same two diagnostics, exit 0 both — so the claim is pinned to the range rather than one version. The worked example now says why dropping the guard is silent: the name is the thing that suppresses the warning. Co-Authored-By: Claude Opus 5 (1M context) --- skills/audit/SKILL.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/skills/audit/SKILL.md b/skills/audit/SKILL.md index e22c144..8741cff 100644 --- a/skills/audit/SKILL.md +++ b/skills/audit/SKILL.md @@ -53,8 +53,8 @@ Exclude auto-generated files (bindings, build artifacts, `*.pointers.sol` and si - **Rounding direction.** All rounding from precision loss (Float→fixed-decimal conversions, integer division) MUST favor **non-interactive** participants (order owner, contract, protocol — contracts count as non-interactive). For each rounding op: identify WHO is non-interactive, WHICH direction it rounds, WHETHER that direction favors the non-interactive party. Flag any rounding that favors **interactive** participants (`msg.sender`, arb callers, external protocols). - **Variable naming.** Flag short/meaningless names (single chars `r`/`n`/`x`, abbreviations `ob`/`cfg`/`val`) for closure params, loop vars, or local bindings as **LOW** (e.g. `raindex_cfg` not `ob`, `network_key` not `nk`). Short-but-meaningful names (`id`, `url`, `key` when unambiguous) are fine. - **Solidity storage-class naming (rainlanguage).** In Solidity, encode a variable's storage class in its name so its nature is legible at the *use* site, not just its declaration: **immutables** are `i` + UpperCamel (`iBeacon`, `iOrchestratorBeacon`); **mutable storage** (state variables that are neither `constant` nor `immutable`) are `s` + UpperCamel (`sBalance`); **locals and function parameters** contain **no underscore** — `fooBar`, never `_fooBar` (leading) or `foo_bar` (internal), since the leading underscore is reserved for the internal-function convention. Flag an `immutable` not named `iFoo`, a mutable state var not named `sFoo`, and any local/param containing an underscore. Applies to all Solidity in scope, **tests and scripts included**; an interface-mandated parameter name or a documented, deliberate exception may be allow-listed, but default to flagging. **LOW/INFO.** (This is Solidity-only — Rust/TS keep their own idioms, e.g. Rust `snake_case` locals are correct.) -- **Solidity named returns must be locally provable (rainlanguage).** The naming rules above reach declarations, locals and params but NOT **return declarations**, and a named return is where Solidity silently supplies a zero/empty default. Unnamed, that same fall-through draws solc's *"Unnamed return variable can remain unassigned. Add an explicit return with value to all non-reverting code paths **or name the variable**"* — so naming it suppresses the compiler's own diagnostic, by the compiler's own suggestion (solc 0.8.25: every unnamed shape with a non-reverting path to the end warns, named ones never do, and the sole unnamed exception is a completely empty body). It is NOT a compile error either way. Since dimension 4 #4 rates a build warning **LOW or higher**, this rule is the audit standing in for the signal the name removed. The finding is the subset whose assignment **cannot be proved from the declaration's own function**, where *provable* means: on **every path that reaches the function's end** (its closing brace or a bare `return;`), the name is either assigned or the path reverts — judged from that function's body alone. A path reaching the end unassigned returns the zero/empty value. Judge nesting by that test rather than by the syntax: a total `if`/`else` (or a `try`/`catch` assigning in both arms) nested inside a loop or another conditional is NOT total, because the enclosing construct can be skipped. - - **Assigned only inside a loop or a branch**, where a guard elsewhere in the function is what makes every path assign it — provable, but only via that guard, so correctness rests on code far from the declaration (if no guard makes it total, it is the next case instead). Worked example: `rainlanguage/rain.deploy`'s `LibRainDeploy.deployToNetworks` declares `returns (address deployedAddress)` and assigns it only in the two arms of an `if`/`else` **inside the `for` loop over `networks`** — safe solely because an `if (networks.length == 0) revert NoNetworks();` at the top of the function guarantees at least one iteration. Weaken or drop that guard and the function returns `address(0)` while still compiling, and nothing catches the zero: every check in the function is either *before* the loop (so it has already run against inputs, not against the return) or *inside* it (so it is skipped along with the loop — including the codehash check on `deployedAddress` itself), and there is none after it. The hazard is that the proof sits three levels away from the declaration. **LOW.** +- **Solidity named returns must be locally provable (rainlanguage).** The naming rules above reach declarations, locals and params but NOT **return declarations**, and a named return is where Solidity silently supplies a zero/empty default. Unnamed, that same fall-through draws solc's *"Unnamed return variable can remain unassigned. Add an explicit return with value to all non-reverting code paths **or name the variable**"* — so naming it suppresses the compiler's own diagnostic, by the compiler's own suggestion (identical on solc 0.8.25 and 0.8.35: every unnamed shape with a non-reverting path to the end warns, named ones never do, and the sole unnamed exception is a completely empty body). It is NOT a compile error either way. Since dimension 4 #4 rates a build warning **LOW or higher**, this rule is the audit standing in for the signal the name removed. The finding is the subset whose assignment **cannot be proved from the declaration's own function**, where *provable* means: on **every path that reaches the function's end** (its closing brace or a bare `return;`), the name is either assigned or the path reverts — judged from that function's body alone. A path reaching the end unassigned returns the zero/empty value. Judge nesting by that test rather than by the syntax: a total `if`/`else` (or a `try`/`catch` assigning in both arms) nested inside a loop or another conditional is NOT total, because the enclosing construct can be skipped. + - **Assigned only inside a loop or a branch**, where a guard elsewhere in the function is what makes every path assign it — provable, but only via that guard, so correctness rests on code far from the declaration (if no guard makes it total, it is the next case instead). Worked example: `rainlanguage/rain.deploy`'s `LibRainDeploy.deployToNetworks` declares `returns (address deployedAddress)` and assigns it only in the two arms of an `if`/`else` **inside the `for` loop over `networks`** — safe solely because an `if (networks.length == 0) revert NoNetworks();` at the top of the function guarantees at least one iteration. Weaken or drop that guard and the function returns `address(0)` while still compiling — silently, since the name is what suppresses the warning — and nothing catches the zero: every check in the function is either *before* the loop (so it has already run against inputs, not against the return) or *inside* it (so it is skipped along with the loop — including the codehash check on `deployedAddress` itself), and there is none after it. The hazard is that the proof sits three levels away from the declaration. **LOW.** - **Never assigned on a reachable path** — no guard makes it total, so a silent zero/empty return where the unnamed form would have drawn the warning above. This is the same fail-open shape as **Fail-closed on input** above, with the zero value as the permissive default. **MEDIUM**, and higher on that rule's own scale by what the zero gates — **CRITICAL/HIGH** where it bypasses auth, lifts a spend/write bound, or destroys data. Read the sense before rating it: `returns (bool valid)` falling through to `false` fails *closed*, but a `returns (bool blocked)` / `(bool paused)` falling through to `false` reads as "proceed", and a `returns (address recipient)` falling through to `address(0)` sends funds nowhere — those are not MEDIUMs. - **Shadowed on every path by an explicit `return`** — the name is a dead declaration and pure reader tax. **LOW.** - **Last-iteration-wins** — a named return assigned once per loop iteration returns only the final one. Benign when every iteration computes the same value (as in the example above: a Zoltu address is deterministic, so every network yields the same one), a real defect when they can diverge; this is **Explicit handling of ambiguous sets** above reached through a return declaration rather than a `.find`. **LOW**, **MEDIUM** when the iterations can diverge, and **HIGH** on that rule's own scale when the value returned drives a verdict, permission, transfer, or destructive action.