Skip to content

External audit triage: all 18 findings addressed#450

Open
thedavidmeister wants to merge 15 commits intomainfrom
2026-03-23-audit
Open

External audit triage: all 18 findings addressed#450
thedavidmeister wants to merge 15 commits intomainfrom
2026-03-23-audit

Conversation

@thedavidmeister
Copy link
Copy Markdown
Contributor

@thedavidmeister thedavidmeister commented Mar 23, 2026

Summary

Triages all 18 findings from the external audit report (Report_rain.interpreter_2.0_mar_2026.pdf).

Security fixes (H01, M01-M06, L01-L03)

  • H01: Fix off-by-one in MAX_STACK_RHS_OFFSET that corrupts LHS counter
  • M05: Enforce MAX_STACK_RHS_OFFSET in endLine() for empty-RHS input lines
  • M06: Validate extern integrity return values against operand
  • L02: Add outputs mask to LibOpCall
  • L03: Add inputs assertion to LibOpCall
  • M01-M04, L01: Verified fixes from prior commits with test coverage

Informational fixes (I01-I08)

  • I01: Document unreachable MalformedHexLiteral as defensive fallback
  • I04: Add OutOfBoundsConstantRead check in LibOpExtern.integrity()
  • I05: Replace implicit mload(state) with explicit field access in LibOpStack
  • I06: Float identity fuzz test added upstream (rain.math.float)
  • I07: Document integrity/runtime separation design principle
  • I08: Move checkParseMemoryOverflow() inside LibParse.parse()

Other

  • Update rain.interpreter.interface submodule (includes latest rain.math.float)
  • Pointer rebuild for parser bytecode change

Test plan

  • All new tests pass locally
  • CI passes

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes

    • Fixed bytecode parsing overflow and stack-boundary edge cases; prevented LHS/RHS corruption.
    • Strengthened integrity checks to detect mismatched extern/call input/output counts and out-of-bounds constant reads.
    • Aligned runtime decoding with tightened integrity rules.
  • Documentation

    • Added README section clarifying deploy-time bytecode validation and runtime/check separation.
  • Tests

    • Added and expanded integrity and parse boundary tests to improve robustness.

thedavidmeister and others added 7 commits March 23, 2026 17:35
RHS offset 62 causes pushOpToSource to write into the LHS counter byte
at state + 0x5F instead of a per-word ops counter. Lower the constant
from 0x3f to 0x3e so highwater() rejects offset 62 before it can
collide.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
endLine() adds lineLHSItems to totalRHSTopLevel without checking the
limit when a line has only LHS items and no RHS. Add the same
>= MAX_STACK_RHS_OFFSET check that highwater() uses so large input
declarations revert with ParseStackOverflow instead of corrupting
parser state.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
LibOpExtern.integrity() passed through whatever externIntegrity()
returned without checking it matched the operand-encoded IO counts.
A malicious extern could lie consistently at parse and integrity time.
Now assert the returned inputs and outputs match the operand, reverting
with ExternIntegrityInputsMismatch or ExternIntegrityOutputsMismatch.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
L02: outputs was extracted via unmasked right shift (>> 0x14), reading
bits above the 4-bit field. Add & 0x0F mask to both integrity() and
run() for consistency with other opcodes.

L03: integrity() returned sourceInputs from the bytecode without
comparing against the operand-encoded inputs. Add an explicit
equality assertion that reverts with CallInputsMismatchSource on
mismatch, making the opcode self-validating.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Opcode integrity() functions report stack IO to integrityCheck2, which
is the single validation authority. Opcode run() functions trust that
integrity has already validated the bytecode and do not duplicate
checks — runtime gas efficiency depends on this separation.

Documented in CLAUDE.md, README.md, and integrityCheck2 NatSpec.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
I01: Add comment noting MalformedHexLiteral revert is a defensive
unreachable fallback in LibParseLiteralHex.

I04: Add OutOfBoundsConstantRead check in LibOpExtern.integrity()
matching the LibOpConstant pattern.

I05: Replace implicit mload(state) with explicit state.stackBottoms
field access in LibOpStack.run().

I08: Move checkParseMemoryOverflow() inside LibParse.parse() after
buildBytecode/subParseWords so it enforces the 16-bit pointer limit
internally. Remove redundant modifier from RainterpreterParser.unsafeParse.

Rebuild pointers for the parser bytecode change.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Update interface submodule to latest main (includes float dep with
identity fuzz test). All 18 external audit findings now triaged.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Mar 23, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 0e26482f-e876-47d0-ae24-05c62ed5dc4a

📥 Commits

Reviewing files that changed from the base of the PR and between 19ab7bc and 2aaa660.

📒 Files selected for processing (1)
  • test/src/lib/op/call/LibOpCall.t.sol
🚧 Files skipped from review as they are similar to previous changes (1)
  • test/src/lib/op/call/LibOpCall.t.sol

📝 Walkthrough

Walkthrough

Bytecode integrity validation is centralized in integrityCheck2; opcode integrity() functions report exact stack IO for deploy-time checks while run() omits them. Extern/call integrity mismatches now revert with new errors. Parse stack RHS boundary corrected from 63 to 62; tests updated and RainlangParser.unsafeParse modifier removed.

Changes

Cohort / File(s) Summary
Errors & Integrity Docs
src/error/ErrExtern.sol, src/error/ErrIntegrity.sol, src/lib/integrity/LibIntegrityCheck.sol, README.md
Added ExternIntegrityInputsMismatch, ExternIntegrityOutputsMismatch, and CallInputsMismatchSource. Documented that integrityCheck2 is the authoritative deploy-time bytecode validator and run() assumes prior validation.
Extern Instruction Validation
src/lib/op/00/LibOpExtern.sol
integrity() now bounds-check constant dispatch index, calls extern.externIntegrity(...), validates returned inputs/outputs against expected values, and reverts with the new extern integrity errors on mismatch.
Call Instruction Validation
src/lib/op/call/LibOpCall.sol
integrity() extracts/masks operand inputs (bits 16–19) and outputs (4 bits), and reverts with CallInputsMismatchSource if operand-encoded inputs differ from the callee source; run() masks outputs similarly.
Stack Operation Access
src/lib/op/00/LibOpStack.sol
run() caches state.stackBottoms into a local Pointer[] memory before assembly access to load stackBottom; runtime behavior unchanged.
Parse State & Parse Flow
src/lib/parse/LibParseState.sol, src/lib/parse/LibParse.sol
Reduced MAX_STACK_RHS_OFFSET from 0x3f to 0x3e (63→62), added runtime overflow guard in endLine() for empty RHS case, and invoked LibParseState.checkParseMemoryOverflow() inside LibParse.parse() before returning.
Parse Literal Doc
src/lib/parse/literal/LibParseLiteralHex.sol
Added a defensive comment marking an else branch as unreachable due to prior hex-bounds constraints; logic unchanged.
Parser Entry Signature
src/concrete/RainlangParser.sol
Removed the checkParseMemoryOverflow modifier from unsafeParse() signature; callers are now responsible for invoking the check.
Extern Integrity Tests
test/src/lib/op/00/LibOpExtern.t.sol
Added setupExternIntegrity() helper, refactored happy-path to expect exact inputs/outputs, and added tests that assert reverts for extern inputs/outputs mismatches with the new errors.
Call Instruction Tests
test/src/lib/op/call/LibOpCall.t.sol
Added tests for 4-bit output masking and operand-inputs mismatch (CallInputsMismatchSource), and adjusted existing test signatures to derive source inputs from bytecode.
Parse Boundary Tests
test/src/lib/parse/LibParseState.endLine.unboundedLHS.t.sol, test/src/lib/parse/LibParseState.pushOpToSource.LHSCorruption.t.sol
Added tests verifying endLine() overflow behavior at the corrected 62-item boundary and demonstrating LHS counter corruption at offset 62 (vs safe at 61).
Parse Highwater Tests
test/src/lib/parse/LibParseState.highwater.t.sol, test/src/lib/parse/LibParseState.highwaterOverflow.t.sol
Updated comments and initial state values to reflect the corrected 62-item boundary; test expectations unchanged.

Sequence Diagram(s)

mermaid
sequenceDiagram
participant Deployer
participant LibIntegrityCheck as IntegrityChecker
participant ExternContract as Extern
participant Runtime
Deployer->>IntegrityChecker: deploy bytecode -> integrityCheck2
IntegrityChecker->>Extern: call externIntegrity(...)
Extern-->>IntegrityChecker: (actualInputs, actualOutputs)
IntegrityChecker-->>Deployer: accept or revert (ExternIntegrity* / CallInputsMismatchSource)
Note over Runtime,IntegrityChecker: Later at runtime
Runtime->>Runtime: run() assumes prior integrity check (no revalidation)

Estimated Code Review Effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Poem

🐰
Bytecode checked at deploy-time's light,
Stack counts tidy, offsets set right.
Externs and calls now must agree,
Parser trimmed and tests run free.
A little hop for correctness—hooray! 🥕

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title clearly and specifically summarizes the main purpose: triaging and addressing all 18 audit findings. This directly aligns with the changeset scope.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch 2026-03-23-audit

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
src/lib/parse/LibParseState.sol (1)

81-87: Clarify that this is an exclusive bound.

MAX_STACK_RHS_OFFSET now stores the first invalid offset (0x3e), not the highest valid one. Rewording the lead sentence here would make the boundary harder to misread next time.

✏️ Suggested wording
-/// `@dev` Maximum RHS offset value. The RHS offset is stored as a single
+/// `@dev` Exclusive upper bound for the RHS offset. The RHS offset is stored as a single
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/lib/parse/LibParseState.sol` around lines 81 - 87, The comment for
MAX_STACK_RHS_OFFSET is misleading because the constant holds the first invalid
offset (0x3e) rather than the maximum valid offset; update the docstring for
MAX_STACK_RHS_OFFSET to explicitly state it is an exclusive upper bound (first
invalid offset = 0x3e) and that the highest valid RHS offset is 0x3d (61), and
preserve the existing explanation about why offset 62 would overflow into the
LHS counter byte (referencing topLevel0, topLevel1, and pushOpToSource) so
readers don’t misinterpret the constant's meaning.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/lib/parse/LibParseState.sol`:
- Around line 81-87: The comment for MAX_STACK_RHS_OFFSET is misleading because
the constant holds the first invalid offset (0x3e) rather than the maximum valid
offset; update the docstring for MAX_STACK_RHS_OFFSET to explicitly state it is
an exclusive upper bound (first invalid offset = 0x3e) and that the highest
valid RHS offset is 0x3d (61), and preserve the existing explanation about why
offset 62 would overflow into the LHS counter byte (referencing topLevel0,
topLevel1, and pushOpToSource) so readers don’t misinterpret the constant's
meaning.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: a6cc9f15-4fec-476b-b98e-6d40933a2e32

📥 Commits

Reviewing files that changed from the base of the PR and between 2780cdd and 66cb789.

⛔ Files ignored due to path filters (8)
  • CLAUDE.md is excluded by !CLAUDE.md
  • audit/2026-03-23-01/triage.md is excluded by !audit/**
  • flake.lock is excluded by !**/*.lock
  • foundry.lock is excluded by !**/*.lock
  • src/generated/Rainlang.pointers.sol is excluded by !**/generated/**
  • src/generated/Rainterpreter.pointers.sol is excluded by !**/generated/**
  • src/generated/RainterpreterExpressionDeployer.pointers.sol is excluded by !**/generated/**
  • src/generated/RainterpreterParser.pointers.sol is excluded by !**/generated/**
📒 Files selected for processing (18)
  • README.md
  • lib/rain.interpreter.interface
  • src/concrete/RainterpreterParser.sol
  • src/error/ErrExtern.sol
  • src/error/ErrIntegrity.sol
  • src/lib/integrity/LibIntegrityCheck.sol
  • src/lib/op/00/LibOpExtern.sol
  • src/lib/op/00/LibOpStack.sol
  • src/lib/op/call/LibOpCall.sol
  • src/lib/parse/LibParse.sol
  • src/lib/parse/LibParseState.sol
  • src/lib/parse/literal/LibParseLiteralHex.sol
  • test/src/lib/op/00/LibOpExtern.t.sol
  • test/src/lib/op/call/LibOpCall.t.sol
  • test/src/lib/parse/LibParseState.endLine.unboundedLHS.t.sol
  • test/src/lib/parse/LibParseState.highwater.t.sol
  • test/src/lib/parse/LibParseState.highwaterOverflow.t.sol
  • test/src/lib/parse/LibParseState.pushOpToSource.LHSCorruption.t.sol

thedavidmeister and others added 7 commits March 25, 2026 22:31
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The audit branch added a runtime inputs check in LibOpCall.run() that
reverts with CallInputsMismatchSource before the integrity-level
BadOpInputsLength check fires.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant