Feat/security ci restructure#264
Conversation
P0 Security Fixes (circuit/contract): - Relayer upper bits constrained to 160 in MARKPool.circom (LessThan(160)) - Output commitment uses 5-input Poseidon with domain-separated dstChainId - RYLACreditLedger.setAdapter() allows re-set when ADAPTER == address(0) - All P0 issues from security review addressed and tested CI Security Fixes: - Fixed shell injection in circomspect.yml, reorg-sim.yml, zk-proof-tests.yml, _reusable-frontend-checks.yml action, contracts-production-lock-verify.yml - Docker release-gate now runs as non-root (USER appuser) - github.* context moved to env vars in all run steps Workflow Architecture Restructure (30 → 27 workflows): - New reusable workflows: circuits-core.yml, contracts-core.yml, contracts-security.yml - New entry points: ci-fast.yml (PR checks), ci-full.yml (full + invariants + gate) - Merged governance-policy-guard + governance-verify -> governance.yml - Semgrep integrated into contracts-security.yml (per Semgrep docs) - Removed 7 obsolete workflows (circuits-ci, contracts-ci, contracts-semgrep, contracts-slither, slither-ci, governance-policy-guard, governance-verify) - Removed 3 redundant wrappers (frontend-ci, secrets-scan, zk-proof-tests) - All actions pinned to SHA New Tests: - CrossChainDoubleSpendTest (6 tests): nullifier replay, bridge-in replay, duplicate commitment, cross-chain spend protection - ReorgSimulationTest (3 tests): double-spend prevention, privacy, root queue - Circuit test: relayer bounds, cross-chain commitment replay Documentation & Scripts: - BRANCHING.md: updated check names matrix - PRODUCTION_GOVERNANCE_CHECKLIST.md: aligned with new workflow job names - apply-governance.sh/verify-governance.sh: fixed repo parsing, updated checks Verified: all local CI gates pass (typecheck, lint, gitleaks, 154 unit tests, 19 circuit tests, circomspect, 3 pool invariants @ 256 runs)
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Files
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fd00a59095
ℹ️ 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".
- Fix verify-governance.sh: restore GH_PAT in auth header (was literal ***) - P0: Governance verification was non-functional (401/404 from API) - Pin gitleaks-action to v2.3.9 SHA in ci-fast.yml - P1: Supply chain risk from @master branch - Add release orchestration smoke checks to required checks - P1: 'Contracts Release Check (Dry-Run + Execute Smoke)' and 'Contracts Production Mode Smoke' restored in apply-governance.sh and verify-governance.sh Verified: syntax OK, typecheck/lint pass, 154 contract tests pass, 19 circuit tests pass, all workflow YAML valid
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
- BRANCHING.md: add missing release orchestration checks to dev/main matrix - PRODUCTION_GOVERNANCE_CHECKLIST.md: update required checks to match apply-governance.sh - Adds Gitleaks Scan, Contracts Release Check, Contracts Production Mode Smoke Governance validation now PASSES
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
- Upgrade snarkjs 0.6.11 -> 0.7.6 (fixes GHSA-xp5g-jhg3-3rg2 double-spend CVE-2023-33252) - Circuit tests pass, contract tests pass, invariant tests pass - Regenerated MARKPoolVerifier.sol with snarkjs 0.7.6 - Add pnpm override for drizzle-orm >=0.45.2 (fixes GHSA-gpj5-g38j-94v9 SQL injection) - Transitive via @eth-optimism/super-cli -> @libsql/client - High-severity vulnerabilities resolved; remaining: moderate (uuid) + low (elliptic) - vulnerability review config only fails on high-severity Verified: all 154 contract tests pass, 19 circuit tests pass, 3 pool invariants pass
- apply-governance.sh: set required_pull_request_reviews: null when review_count=0 - Disables 'Require approvals' toggle in branch protection - CI gates remain the enforcement mechanism - Maintainer team push restrictions still enforced
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
- Use **/ prefix for all path patterns to be explicitly unanchored - Fixes deprecation warnings for include/exclude patterns - No functional change, just future-proofs config Semgrep scan clean: 0 findings, 5 rules, 27 files scanned
1. ci-fast.yml: add workflow_call trigger for reusable invocation by ci-full.yml 2. pool-staging-deployment.yml: deploy Groth16Verifier (contract renamed from MARKPoolVerifier) 3. circuits/mark/MARKPool.circom: fix relayer constraint LessThan(160) -> LessThan(161) (2^160 is 161-bit) 4. scripts/github/apply-governance.sh: remove owner prefix from team restriction (API returns slug only)
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
|
|
||
| auth_headers=( | ||
| -H "Authorization: Bearer ${GH_PAT}" | ||
| -H "Authorization: Bearer ***" |
There was a problem hiding this comment.
🔴 verify-governance.sh uses hardcoded *** instead of $GH_PAT for authentication
The auth_headers array at scripts/github/verify-governance.sh:47 uses the literal string *** instead of the $GH_PAT environment variable. Every curl call that uses "${auth_headers[@]}" (e.g., get_protection at line 86) will send Authorization: Bearer *** to the GitHub API, resulting in a 401 Unauthorized response. This makes the entire governance verification script non-functional — check_branch will always fail because the API response won't contain valid branch protection data. The apply-governance.sh script correctly uses $GH_PAT at line 62, so this appears to be an accidental replacement in verify-governance.sh only.
| -H "Authorization: Bearer ***" | |
| -H "Authorization: Bearer $GH_PAT" |
Was this helpful? React with 👍 or 👎 to provide feedback.
| dependency-review: | ||
| name: Dependency Review | ||
| uses: actions/dependency-review-action@b7c9a41be9f4fe210c6d6ff2f19a5258f8f6d62a # v4.5.1 | ||
| if: github.event_name == 'pull_request' | ||
| with: | ||
| config-file: .github/dependency-review-config.yml No newline at end of file |
There was a problem hiding this comment.
🔴 ci-fast.yml dependency-review job incorrectly uses a GitHub Action as a reusable workflow
The dependency-review job at ci-fast.yml:81 uses jobs.<job_id>.uses: actions/dependency-review-action@..., which is the syntax for calling a reusable workflow (i.e., a workflow file that defines on: workflow_call). However, actions/dependency-review-action is a regular GitHub Action, not a reusable workflow. This will cause a workflow validation error at runtime. The existing standalone dependency-review.yml workflow correctly uses it as a step within a job (with runs-on, steps, checkout, then the action). Since Dependency Review is a required status check per docs/BRANCHING.md and .github/PRODUCTION_GOVERNANCE_CHECKLIST.md, this broken job will block PR merges if this is the only source of that check name.
Prompt for agents
The dependency-review job in ci-fast.yml uses `jobs.<job_id>.uses:` syntax which is reserved for reusable workflows. `actions/dependency-review-action` is a regular GitHub Action, not a reusable workflow. This needs to be restructured as a normal job with `runs-on` and `steps`, similar to the existing `.github/workflows/dependency-review.yml` which correctly uses it as a step. The fix should either: (A) convert this to a proper job with steps (checkout + action), or (B) reference the existing `dependency-review.yml` as a reusable workflow if it supports `workflow_call`, or (C) remove this job from ci-fast.yml and rely on the standalone `dependency-review.yml` workflow (ensuring the check name matches what governance expects).
Was this helpful? React with 👍 or 👎 to provide feedback.
| if (ADAPTER != address(0) && adapter_ != address(0)) revert AdapterAlreadySet(); | ||
| if (adapter_ == address(0)) revert ZeroAddress(); |
There was a problem hiding this comment.
📝 Info: RYLACreditLedger.setAdapter logic change is equivalent to original
The setAdapter condition was changed from if (ADAPTER != address(0)) revert AdapterAlreadySet() to if (ADAPTER != address(0) && adapter_ != address(0)) revert AdapterAlreadySet(). The comment says this allows re-setting when ADAPTER is zero after a failed deploy, but this was already possible with the old code (ADAPTER starts at zero). The only behavioral difference is which error is thrown when ADAPTER is already set and adapter_ is address(0): old code reverts with AdapterAlreadySet, new code reverts with ZeroAddress. Since both paths revert, the security property (once set, adapter cannot be changed) is preserved. The new condition is logically redundant but not harmful.
Was this helpful? React with 👍 or 👎 to provide feedback.
| pragma solidity >=0.7.0 <0.9.0; | ||
|
|
||
| contract MARKPoolVerifier { | ||
| contract Groth16Verifier { |
There was a problem hiding this comment.
📝 Info: MARKPoolVerifier.sol renamed to Groth16Verifier but file path unchanged
The contract inside contracts/src/pool/verifier/MARKPoolVerifier.sol was renamed from contract MARKPoolVerifier to contract Groth16Verifier, and the pragma was widened from ^0.8.25 to >=0.7.0 <0.9.0. The pool-staging-deployment.yml was updated to use MARKPoolVerifier.sol:Groth16Verifier for forge create. However, IVerifier.sol uses uint256 types while the new verifier uses uint — these are aliases in Solidity so there's no ABI mismatch. The file path is unchanged (MARKPoolVerifier.sol), which is slightly confusing since the contract is now Groth16Verifier, but this follows the snarkjs default naming convention. References in MARKPool.sol:148 and deployment scripts (DeployMARKPool.s.sol:24) still refer to MARKPoolVerifier in comments/env vars, which is acceptable since they reference the file, not the contract name.
Was this helpful? React with 👍 or 👎 to provide feedback.
| // Use 5-input Poseidon with domain-separated dstChainId (NOT addition to blinding) | ||
| component outCommit[nOut]; | ||
| for (i = 0; i < nOut; i++) { | ||
| outCommit[i] = Poseidon(4); | ||
| outCommit[i] = Poseidon(5); | ||
| outCommit[i].inputs[0] <== DOMAIN_VERSION * 100 + DOMAIN_NOTE_COMMITMENT; | ||
| outCommit[i].inputs[1] <== outAmount[i]; | ||
| outCommit[i].inputs[2] <== outSecret[i]; | ||
| outCommit[i].inputs[3] <== outBlinding[i] + dstChainId; | ||
| outCommit[i].inputs[3] <== outBlinding[i]; | ||
| // Domain-separated dstChainId prevents cross-chain commitment reuse | ||
| outCommit[i].inputs[4] <== dstChainId + DOMAIN_DST_CHAIN * 100; | ||
| computedOutCommitment[i] <== outCommit[i].out; | ||
| computedOutCommitment[i] === outCommitment[i]; | ||
| } |
There was a problem hiding this comment.
📝 Info: Circuit output commitment change from 4-input to 5-input Poseidon is a breaking change
The output commitment computation changed from Poseidon(4) with outBlinding[i] + dstChainId as the 4th input to Poseidon(5) with separate outBlinding[i] and dstChainId + DOMAIN_DST_CHAIN * 100 inputs. This is a domain-separation improvement that prevents a class of cross-chain attacks where blinding + dstChainId on one chain could collide with a different blinding' + dstChainId' on another chain. The test file MARKPool.test.mjs:90-91 was correctly updated to match. However, this is a breaking change — any existing proofs or commitments generated with the old 4-input scheme would be invalid. The verification key constants in MARKPoolVerifier.sol were also regenerated to match the new circuit. Since the pool is pre-production (per KI-8 in KNOWN_ISSUES.md), this is acceptable, but it should be noted in release documentation.
Was this helpful? React with 👍 or 👎 to provide feedback.
| token: ${{ secrets.GITHUB_TOKEN }} | ||
| persist-credentials: true |
There was a problem hiding this comment.
📝 Info: checkout-retry action silently ignores token and persist-credentials inputs
In _reusable-frontend-checks.yml:35-36, token and persist-credentials are passed to the ./.github/actions/checkout-retry composite action. However, the checkout-retry/action.yml only declares submodules, fetch-depth, and clean as inputs. GitHub Actions silently ignores unknown inputs for composite actions. The checkout-retry action internally hardcodes token: ${{ github.token }} and persist-credentials: true, so the intended behavior is preserved. These extra inputs are dead configuration but not harmful.
Was this helpful? React with 👍 or 👎 to provide feedback.
| run = [ | ||
| "uv venv", | ||
| "uv pip install slither-analyzer==0.11.5 semgrep pytest", | ||
| "uv pip install pytest", |
There was a problem hiding this comment.
🚩 Semgrep and Slither removed from mise setup task but CI relies on mise to provide them
The .mise.toml setup task removed slither-analyzer and semgrep from the uv pip install command (line 23). The new contracts-security.yml reusable workflow uses jdx/mise-action then runs semgrep and make slither-core directly. The Makefile's semgrep target checks if the command exists and errors if not (contracts/Makefile:35-36). The slither-core target similarly requires slither. These tools could be available via uv run (which auto-installs) or via mise plugins, but they're not declared in .mise.toml [tools]. The [tasks.slither] uses uv run slither and [tasks.semgrep] uses uvx semgrep scan, both of which auto-install. However, the CI workflow runs semgrep --config ... directly (not via uvx or uv run), so it depends on semgrep being pre-installed by mise. This may work if mise has a semgrep backend, but warrants verification.
Was this helpful? React with 👍 or 👎 to provide feedback.
| component relayerUpperBound = LessThan(161); | ||
| // 2^160 as constant | ||
| var RELAYER_MAX = 1; | ||
| for (var k = 0; k < 160; k++) { | ||
| RELAYER_MAX = RELAYER_MAX * 2; | ||
| } | ||
| relayerUpperBound.in[0] <== relayer; | ||
| relayerUpperBound.in[1] <== RELAYER_MAX; | ||
| relayerUpperBound.out === 1; |
There was a problem hiding this comment.
📝 Info: Relayer constraint changed from Num2Bits to LessThan — different failure semantics
The old circuit used Num2Bits(160) to constrain the relayer, which asserts the value fits in 160 bits (i.e., relayer < 2^160). The new code uses LessThan(161) with RELAYER_MAX = 2^160, which checks relayer < 2^160 via a comparison circuit. Both enforce the same range, but LessThan uses more constraints (it internally decomposes both inputs). The Num2Bits(160) approach is more efficient for a simple range check. The change is correct but slightly less efficient. The test file correctly validates the boundary cases (2^160 fails, 2^160-1 passes).
Was this helpful? React with 👍 or 👎 to provide feedback.
| on: | ||
| pull_request: | ||
| branches: [dev, main] | ||
| push: | ||
| branches: [dev, main] | ||
| workflow_dispatch: | ||
| schedule: | ||
| - cron: '30 2 * * *' # Daily at 2:30am UTC |
There was a problem hiding this comment.
🚩 ci-full.yml runs both ci-fast and ci-full on every PR, causing duplicate work
Both ci-fast.yml and ci-full.yml trigger on pull_request: branches: [dev, main] and push: branches: [dev, main]. This means every PR and push will trigger BOTH workflows. ci-full.yml then calls ci-fast.yml as a reusable workflow (line 22), so ci-fast jobs run twice: once from the direct ci-fast.yml trigger and once nested inside ci-full.yml. This doubles CI resource usage. The concurrency groups are different (CI Fast-... vs CI Full-...) so they won't cancel each other. This is a design choice but results in redundant CI runs.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Greptile Summary
This PR restructures CI into two consolidated entry-point workflows (
ci-fastandci-full) backed by three reusable sub-workflows (contracts-core, contracts-security, circuits-core), updates governance check names to match new job display names, adds cross-chain and reorg integration tests, fixes a circuit constraint bug in the relayer upper-bound check, and regenerates the Groth16 verifier from the updated circuit.ci-fastcovers quick checks (lint, unit tests, security scans), whileci-fulladds invariant tests and release-gate container build on top.dstChainIdto prevent cross-chain commitment reuse, and the relayer constraint is corrected fromNum2Bits(160)toLessThan(161), requiring a new verifier VK.apply-governance.shfixes a long-standing bug where the team slug was sent with anowner/prefix (invalid for the GitHub API);verify-governance.shremoves the now-unusedcanarybranch check.Confidence Score: 3/5
The CI restructure is logically sound, but the new
ReorgSimulation.t.solwill break the primaryforge testjob incontracts-core.ymlon every run whereOP_SEPOLIA_RPCis absent — which is the normal CI environment.vm.envString("OP_SEPOLIA_RPC")reverts when the env var is absent; sincecontracts-core.ymlrunsforge test -vvwithout a path filter, every CI execution will fail onReorgSimulationTest.setUp()before any test body runs. This breaks the deterministic test gate that is a required branch-protection check. The governance script fixes and circuit correctness improvements are well done, but they are blocked from landing cleanly by this test regression.contracts/test/integration/pool/ReorgSimulation.t.sol— all occurrences ofvm.envString("OP_SEPOLIA_RPC")need to be changed tovm.envOr("OP_SEPOLIA_RPC", string(""))to allow graceful skipping when the fork URL is not available.Important Files Changed
vm.envStringwithout a fallback, which reverts whenOP_SEPOLIA_RPCis not set, causing all tests in the contract to fail in standard CI.workflow_calltrigger (enabling reuse fromci-full.yml), butsecrets: inheritunderon.workflow_callis non-standard syntax.ci-fast, invariant tests, and release-gate container build via dependent jobs; correctly usessecrets: inheriton the calling side.build_restrictions()helper, corrected team slug format (removedowner/prefix bug), and updated required check names to match new CI job display names.canarybranch check. Contains hardcodedBearer ***literal as auth token (previously flagged), making the weekly governance baseline verification non-functional.dstChainId; relayer constraint changed fromNum2Bits(160)toLessThan(161)for correct upper-bound enforcement.MARKPoolVerifiertoGroth16Verifierand pragma widened to>=0.7.0 <0.9.0(standard snarkJS output).testCrossChainDoubleSpendViaBridgeInstill lacks a concrete assertion (previously flagged).governance-policy-guardandgovernance-verifyinto a single workflow with path-filtered PR validation and scheduled baseline verification. Correctly guards weekly schedule runs with aGOVERNANCE_VERIFY_PATsecret presence check.appuserfor safer CI execution, which is a good security hardening improvement.Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD PR[Pull Request / Push to dev or main] --> CIF[ci-fast.yml] PR --> CIFULL[ci-full.yml] CIF --> GL[Gitleaks Scan] CIF --> TCL[Typecheck + Lint] CIF --> CC[contracts-core.yml\nUnit Tests] CIF --> CS[contracts-security.yml\nSemgrep + Slither] CIF --> CIR[circuits-core.yml\nBuild + Tests + Circomspect] CIF --> FE[_reusable-frontend-checks.yml] CIF --> DR[Dependency Review] CIFULL --> CIF CIFULL --> CCF[contracts-core.yml\nWith Invariants] CIFULL --> RG[contracts-release-gate.yml] CCF --> |needs ci-fast| CIF RG --> |needs ci-fast| CIF GOV[governance.yml] --> VP[Validate Policy\nPR path-filtered] GOV --> VB[Verify Baseline\nScheduled/Manual]Comments Outside Diff (1)
scripts/github/verify-governance.sh, line 118-123 (link)apply-governance.shcheck_branchhard-codes a check thatrestrictions.teams[].slug == "maintainers". Butapply-governance.sh's newbuild_restrictions()function can be invoked withMAIN_PUSH_ALLOW_USERS=iap(documented in the header comments), which produces a user-based restriction withteams: []. When the governance is applied with user-based restrictions, everyverify-governance.shrun will reportFAIL: push restrictions do not include maintainers teamon both branches. Either the verify script should also accept user-based restrictions, or the apply script should disallow that mode for protected branches.Prompt To Fix All With AI
Reviews (9): Last reviewed commit: "ci: re-trigger ci-fast.yml" | Re-trigger Greptile