security/version: fix critical version gaps#267
Merged
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8f0c6b040e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
iap
added a commit
that referenced
this pull request
Jun 7, 2026
- Add workflow_call to ci-fast.yml and contracts-release-gate.yml - Fix stale push.paths in contracts-release-gate.yml (container -> gate) - Remove vacuous testSetAdapterAllowsReSetWhenZero - Pin Foundry to 1.7.1 (1.8.0 doesn't exist in releases) - Align Python to 3.12 in Slither/Semgrep jobs - Pin circomspect v0.9.0 with correct include path (cd circuits) - Fix Semgrep path patterns for v2 compatibility (**/ prefix) - Update Dockerfile: Node 22->24, Foundry latest->v1.7.1
iap
added a commit
that referenced
this pull request
Jun 7, 2026
- Add version: '1.7.1' to foundry-toolchain in reusable/contracts-core.yml and reusable/contracts-security.yml - Replace vm.envString with vm.envOr (string[] pattern) in ReorgSimulation.t.sol to gracefully skip tests when OP_SEPOLIA_RPC not set - Move fork setup from setUp() to individual test functions via _setupPool() helper - Fix misleading @dev comment in RYLACreditLedger.setAdapter() - clarify ADAPTER can only be set on fresh deployment (ADAPTER == address(0)) All tests pass: contracts (unit + integration), circuits, circomspect, semgrep
- Pin Foundry to 1.7.1 in _reusable-contracts-core.yml and _reusable-contracts-security.yml (1.8.0 doesn't exist) - Fix ReorgSimulation.t.sol: use string array form of vm.envOr to gracefully skip when OP_SEPOLIA_RPC not set; move fork setup to _setupPool() helper - Clarify RYLACreditLedger.setAdapter() @dev comment: ADAPTER can only be set while zero (fresh deploy/retry), permanently immutable once set All local checks pass: contracts (unit), circuits, circomspect, semgrep
2b0b83e to
5227703
Compare
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned Files
|
… input The SHA c09b6c89edd7a04a5a1070d10e5fe8b85b4c0ae2 doesn't exist in foundry-rs/foundry-toolchain. Use c7450ba673e133f5ee30098b3b54f444d3a2ca2d (v1.8.0 tag) with version: '1.7.1' input.
- Remove slither-core job from _reusable-contracts-security.yml - Update ci-fast.yml contracts-security job name and remove run_slither input - Keep Slither in Makefile for local: `make slither-core` Rationale: Slither takes 17+ min and causes GitHub runner preemption. CodeQL + Semgrep + Circomspect + unit tests provide sufficient CI coverage. Run `make slither-core` locally before PR.
iap
commented
Jun 8, 2026
iap
left a comment
Member
Author
There was a problem hiding this comment.
Thanks for the changes. Looks good!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Critical Security Fixes
snarkjs 0.6.11 → 0.7.6 (CRITICAL)
Fixes GHSA-xp5g-jhg3-3rg2 / CVE-2023-33252 - ZK proof double-spend vulnerability
Node.js Runtime Consistency
Tool Pinning
Gas Snapshots Updated
Formatting
Verification
Greptile Summary
This PR upgrades snarkjs from 0.6.11 to 0.7.6 to address a ZK proof double-spend vulnerability, aligns the Node.js runtime to version 24, pins the Foundry toolchain, and refactors the reorg simulation tests to move fork setup into individual test functions.
expectRevertassertions intestReorgDoubleSpendProtectionandtestRootQueueAfterReorg.slither-corejob andrun_slitherinput are dropped from_reusable-contracts-security.yml, leaving only Semgrep; no explanation is given in the PR description.1.8.0, butversion: '1.7.1'is what is actually committed in_reusable-contracts-core.yml.Confidence Score: 3/5
The snarkjs security fix and test refactoring are sound, but the simultaneous and unexplained removal of Slither from CI leaves the contracts without dataflow-level static analysis going forward.
The core security upgrade (snarkjs) is the right move, but removing Slither from the CI security workflow in the same PR means any future Solidity vulnerability that Slither would have caught is now silently missed. This is a meaningful regression in defence-in-depth for a ZK-finance codebase, and the PR description offers no rationale for it.
.github/workflows/_reusable-contracts-security.yml — Slither job removed with no replacement or explanation.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD PR[CI Fast Trigger] --> SEC[contracts-security] PR --> CORE[contracts-core] PR --> CIRC[circuits-core] SEC --> SEM[Semgrep scan] SEC -.->|REMOVED| SLI[Slither analysis] CORE --> FOUNDRY["Setup Foundry v1.7.1\n(PR claims 1.8.0)"] FOUNDRY --> TESTS[forge test] FOUNDRY --> GAS[gas-check snapshot] CIRC --> BUILD[circom build] BUILD --> ZK[snarkjs 0.7.6\nCVE-2023-33252 fixed] BUILD --> CIRC_TESTS[circuit tests] style SLI stroke-dasharray: 5 5, fill:#ffcccc style ZK fill:#ccffccComments Outside Diff (3)
contracts/src/pool/RYLACreditLedger.sol, line 296-299 (link)The
@devand inline comments claimsetAdaptercan "re-set"ADAPTERwhen it has become zero again. However, there is no code path in this contract that writesaddress(0)toADAPTERafter it has been set — the only writer is this function itself, andadapter_ == address(0)is rejected by theZeroAddresscheck on the next line. The new condition is functionally identical to the oldADAPTER != address(0)check for all reachable states: a non-zeroADAPTERis still immutable.Prompt To Fix With AI
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
.github/workflows/ci-full.yml, line 272-275 (link)ci-fast.ymlis not a reusable workflow —ci-full.ymlwill always failGitHub Actions requires any workflow called via
jobs.<job>.uses:to declareon: workflow_callas a trigger.ci-fast.ymlonly declareson: pull_requestandon: push— it has noworkflow_calltrigger. Every invocation ofci-full.yml(scheduled daily runs, push tomain/dev, manualworkflow_dispatch) will fail at workflow validation with an error like "The workflow you are attempting to use as a called workflow is not a reusable workflow." The same applies to therelease-gatejob below, which callscontracts-release-gate.yml— that file also lackson: workflow_call.Prompt To Fix With AI
contracts/test/integration/pool/ReorgSimulation.t.sol, line 139-161 (link)testReorgDoesNotDeanonymizemakes no assertions — passes vacuouslyAfter setting up the fork and calling
_setupPool(), the function only emits twoconsole.logcalls and returns. There is noassertEq,assertTrue, or any other assertion; the test passes unconditionally regardless of contract state. The comment "This is a design-time check - runtime can't easily verify event signatures" acknowledges the gap but leaves the passing test as misleading documentation of an unverified invariant.Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (8): Last reviewed commit: "ci: move Slither to local-only (Option 1..." | Re-trigger Greptile