fix(RingHTLC): bounded withdraw + authenticated hop membership (audit HL-05, HL-06)#1
Conversation
… HL-05, HL-06) HL-05: withdraw had no upper timelock bound — after a hop's expiry both withdraw and refund were simultaneously valid (first-tx-wins race). A late preimage reveal could leave a ring part-WITHDRAWN/part-REFUNDED, breaking the atomicity the staggered timelocks exist to provide. withdraw now requires block.timestamp < hop.timelock (mirrors the core EVM/Sui HTLCs), making withdraw and refund temporally exclusive. HL-06: ringId was an unauthenticated bytes32 — anyone could append a self-funded junk hop to a victim's ring and force isRingSettled false forever (coordinator griefed indefinitely at dust cost). Two-part fix: - createHopETH/createHopERC20 take hopIndex and enforce hopId == keccak256(ringId, hopIndex, msg.sender, receiver) — hop slots are unforgeable (computeHopId was documented but never enforced) - isRingSettled(ringId, expectedHopIds[]) replaces the unbounded form: the coordinator checks ITS deterministic hop set; junk hops under the same ringId are never consulted, and a foreign/forged hop fails the ringId-match check Also documents the keccak256-vs-sha256 incompatibility with the core HashLock HTLCs in the contract header (audit informational note). Audit suite flipped from demonstrating the unsafe behavior to pinning the safe behavior: forge 8/8 (incl. expired-withdraw rejection + refund-path intact, forged-hopId rejection, junk-hop grief immunity, foreign-hop and empty-set rejection). Hardhat suite updated to the new signatures: 21/21. Breaking ABI change — standalone repo, no deployments (Phase 0 verified not wired into the main settlement path). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 54 minutes and 3 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR strengthens atomic ring settlement security by introducing deterministic hop ID binding, enforcing temporal settlement windows, and requiring caller-supplied expected hop sets for settlement verification. The contract now rejects forged hop IDs at creation, prevents withdrawal after timelock expiry, and prevents junk-hop griefing of settlement detection. Comprehensive Forge audit tests and TypeScript test migration validate all changes. ChangesRing Settlement Security & Verification
🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Independent review (Claude /ultrareview)Diff scope. Audit PR-11 (~320 LOC): HL-05 — Findings
Verified safe (checked explicitly)
Risk assessment
VerdictAPPROVE WITH SUGGESTIONS — fix the CI foundry job (#1) before merge so the new gate is green on day one; #2/#3 are doc touch-ups. |
… API update Review #1: the foundry CI job compiles the contract through the node_modules OpenZeppelin remapping but never ran npm ci — would fail on day one. Review #2: README still showed the pre-HL-06 signatures; updated with the hopIndex param, the expected-set isRingSettled, and the note that expectedHopIds must be derived via computeHopId, never accepted from an untrusted party (review #3). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Review follow-up: all three findings addressed in |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
contracts/RingHTLC.sol (1)
4-6:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFoundry CI fails to resolve OpenZeppelin imports.
The pipeline errors show Forge cannot find
@openzeppelin/paths. Hardhat resolves these vianode_modules/, but Foundry needs explicit remappings.🔧 Proposed fix: add remappings to foundry.toml
[profile.default] # ... existing config ... remappings = [ "`@openzeppelin/`=node_modules/@openzeppelin/", ]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contracts/RingHTLC.sol` around lines 4 - 6, Foundry cannot resolve the `@openzeppelin` imports used in contracts/RingHTLC.sol (the IERC20, SafeERC20, ReentrancyGuard imports); add an explicit remapping for `@openzeppelin/` in foundry.toml (e.g., add "`@openzeppelin/`=node_modules/@openzeppelin/" to the remappings array under the default profile or top-level remappings) so Forge can locate those modules, then rerun forge build to verify resolution.Source: Pipeline failures
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Line 13: Update the actions/checkout@v4 steps to disable persisting the GitHub
token by adding the input persist-credentials: false to each checkout step (both
occurrences of actions/checkout@v4), so the token is not stored in .git/config
and cannot be leaked to subsequent steps or artifacts.
- Around line 13-14: Replace mutable action tags with immutable commit SHAs: for
each usage like actions/checkout@v4 and actions/setup-node@v4 (and the other
occurrences on the file at the two locations referenced), lookup the exact
commit SHA for the desired release in the upstream repos (actions/checkout and
actions/setup-node) and replace the tag reference (e.g., actions/checkout@v4)
with the full commit SHA (e.g., actions/checkout@sha) to pin the workflow to
that immutable commit; do this for both occurrences referenced (lines with
actions/checkout and actions/setup-node and the second pair at 26-27) and verify
the SHAs correspond to the intended release before committing.
- Line 28: The workflow currently runs the unfixed command "forge install
foundry-rs/forge-std --no-git" which pulls the latest default branch; change
that command to pin a specific version by installing a tag or commit, e.g.
replace it with "forge install foundry-rs/forge-std@<tag-or-commit> --no-git"
(use a released tag or exact commit SHA), or alternatively vendor/lock the
dependency by committing lib/forge-std as a git submodule and remove the install
step entirely; update the workflow step that contains "forge install
foundry-rs/forge-std --no-git" accordingly.
- Around line 1-7: The workflow currently uses the default GITHUB_TOKEN
permissions; add an explicit top-level permissions block to scope the token to
read-only (e.g., add a top-level "permissions:" entry with "contents: read" or
other minimal scopes required) so the CI workflow named "CI" only has read
access for push/pull_request triggers; update the YAML near the top (after the
workflow name) to include the permissions mapping.
---
Outside diff comments:
In `@contracts/RingHTLC.sol`:
- Around line 4-6: Foundry cannot resolve the `@openzeppelin` imports used in
contracts/RingHTLC.sol (the IERC20, SafeERC20, ReentrancyGuard imports); add an
explicit remapping for `@openzeppelin/` in foundry.toml (e.g., add
"`@openzeppelin/`=node_modules/@openzeppelin/" to the remappings array under the
default profile or top-level remappings) so Forge can locate those modules, then
rerun forge build to verify resolution.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f74d795e-de1e-41e7-884f-a5492b64972e
📒 Files selected for processing (6)
.github/workflows/ci.yml.gitignorecontracts/RingHTLC.solfoundry.tomltest-forge/RingHTLC.audit.t.soltest/RingHTLC.test.ts
| name: CI | ||
|
|
||
| on: | ||
| push: | ||
| branches: [master] | ||
| pull_request: | ||
|
|
There was a problem hiding this comment.
Explicitly scope permissions to follow least-privilege principle.
The workflow inherits default broad GITHUB_TOKEN permissions. Neither job requires write access, so permissions should be scoped to read-only.
🔐 Proposed fix
name: CI
on:
push:
branches: [master]
pull_request:
+
+permissions:
+ contents: read📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| name: CI | |
| on: | |
| push: | |
| branches: [master] | |
| pull_request: | |
| name: CI | |
| on: | |
| push: | |
| branches: [master] | |
| pull_request: | |
| permissions: | |
| contents: read |
🧰 Tools
🪛 zizmor (1.25.2)
[warning] 1-30: overly broad permissions (excessive-permissions): default permissions used due to no permissions: block
(excessive-permissions)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ci.yml around lines 1 - 7, The workflow currently uses the
default GITHUB_TOKEN permissions; add an explicit top-level permissions block to
scope the token to read-only (e.g., add a top-level "permissions:" entry with
"contents: read" or other minimal scopes required) so the CI workflow named "CI"
only has read access for push/pull_request triggers; update the YAML near the
top (after the workflow name) to include the permissions mapping.
Source: Linters/SAST tools
| name: Hardhat tests | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v4 |
There was a problem hiding this comment.
Set persist-credentials: false to prevent GitHub token leakage.
By default, actions/checkout persists the GitHub token in .git/config, which could leak through artifacts or be accessed by subsequent steps. Since neither job requires git operations after checkout, the token should not be persisted.
🛡️ Proposed fix
- uses: actions/checkout@v4
+ with:
+ persist-credentials: falseApply to both checkout steps (lines 13 and 26).
Also applies to: 26-26
🧰 Tools
🪛 zizmor (1.25.2)
[warning] 13-13: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 13-13: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ci.yml at line 13, Update the actions/checkout@v4 steps to
disable persisting the GitHub token by adding the input persist-credentials:
false to each checkout step (both occurrences of actions/checkout@v4), so the
token is not stored in .git/config and cannot be leaked to subsequent steps or
artifacts.
Source: Linters/SAST tools
| - uses: actions/checkout@v4 | ||
| - uses: actions/setup-node@v4 |
There was a problem hiding this comment.
Pin actions to immutable commit SHAs to prevent supply-chain attacks.
Tag references (@v4, @v1) are mutable and vulnerable to tag-rewriting attacks. If an action repository is compromised, malicious code can be injected into existing tags.
🔒 Proposed fix: pin to commit SHAs
- - uses: actions/checkout@v4
+ - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2
- - uses: actions/setup-node@v4
+ - uses: actions/setup-node@39370e3970a6d050c480ffad4ff0ed4d3fdee5af # v4.1.0
with:
node-version: 20
cache: npm- - uses: actions/checkout@v4
+ - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2
- - uses: foundry-rs/foundry-toolchain@v1
+ - uses: foundry-rs/foundry-toolchain@8e967ec25b47f78a3f3d5a3f967382d391399497 # v1.2.0Also applies to: 26-27
🧰 Tools
🪛 zizmor (1.25.2)
[warning] 13-13: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 13-13: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 14-14: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ci.yml around lines 13 - 14, Replace mutable action tags
with immutable commit SHAs: for each usage like actions/checkout@v4 and
actions/setup-node@v4 (and the other occurrences on the file at the two
locations referenced), lookup the exact commit SHA for the desired release in
the upstream repos (actions/checkout and actions/setup-node) and replace the tag
reference (e.g., actions/checkout@v4) with the full commit SHA (e.g.,
actions/checkout@sha) to pin the workflow to that immutable commit; do this for
both occurrences referenced (lines with actions/checkout and actions/setup-node
and the second pair at 26-27) and verify the SHAs correspond to the intended
release before committing.
Source: Linters/SAST tools
| steps: | ||
| - uses: actions/checkout@v4 | ||
| - uses: foundry-rs/foundry-toolchain@v1 | ||
| - run: forge install foundry-rs/forge-std --no-git |
There was a problem hiding this comment.
Pin forge-std to a specific version to ensure deterministic builds.
Installing foundry-rs/forge-std without a version specifier pulls the latest code from the default branch at build time. This leads to non-deterministic CI runs—different builds may test against different library versions, causing flaky tests or missing regressions.
📌 Proposed fix: pin to a version tag or commit
- - run: forge install foundry-rs/forge-std --no-git
+ - run: forge install foundry-rs/forge-std@v1.9.4 --no-gitAlternatively, commit lib/forge-std as a git submodule in the repository to lock the exact version, then remove this install step entirely.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - run: forge install foundry-rs/forge-std --no-git | |
| - run: forge install foundry-rs/forge-std@v1.9.4 --no-git |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ci.yml at line 28, The workflow currently runs the unfixed
command "forge install foundry-rs/forge-std --no-git" which pulls the latest
default branch; change that command to pin a specific version by installing a
tag or commit, e.g. replace it with "forge install
foundry-rs/forge-std@<tag-or-commit> --no-git" (use a released tag or exact
commit SHA), or alternatively vendor/lock the dependency by committing
lib/forge-std as a git submodule and remove the install step entirely; update
the workflow step that contains "forge install foundry-rs/forge-std --no-git"
accordingly.
1 participant
Audit PR-11 — RingHTLC hardening (HL-05 + HL-06) + CI
Wave-3 remediation from the 2026-06 multi-repo security audit (main repo
AUDIT/FIX-PLAN.md§PR-11). This repo is standalone with no deployments (Phase 0 verified it is not wired into the main settlement path), so the breaking ABI change ships directly.HL-05 (MEDIUM) — withdraw had no upper timelock bound
After a hop's expiry,
withdrawandrefundwere simultaneously valid — a first-tx-wins race. In a staggered-timelock ring, a late preimage reveal could withdraw a hop whose sender already refunded an adjacent hop, leaving the ring part-WITHDRAWN/part-REFUNDED (atomicity break).withdrawnow requiresblock.timestamp < hop.timelock, mirroring the core EVM (HashedTimelockEther.sol:130) and Sui HTLCs — withdraw and refund are temporally exclusive, and the staggered gaps are exactly the post-reveal claim windows.HL-06 (MEDIUM) — unauthenticated ring membership griefed isRingSettled
ringIdwas a caller-chosenbytes32: anyone could append a dust-funded junk hop to a victim's ring;isRingSettlediterated every pushed hop, so the result was false forever. Two-part fix:createHopETH/ERC20takehopIndexand requirehopId == keccak256(ringId, hopIndex, msg.sender, receiver)—computeHopIdwas documented but never enforced. A hop slot is now unforgeable (sender is part of the binding, so an outsider cannot occupy a participant's expected hopId).isRingSettled(ringId, expectedHopIds[])replaces the unbounded form: the coordinator checks its own deterministic hop set; junk hops under the same ringId are never consulted, foreign-ring hops fail theringIdmatch, empty sets are never settled.Also documents the keccak256-vs-sha256 incompatibility with the core HashLock HTLCs in the contract header (audit informational note — do not compose without a hash bridge).
Tests + CI
.github/workflows/ci.yml): Hardhat compile+test and Foundry audit-suite jobs — first CI this repo has had (audit follow-up item).foundry.toml+ the forge audit suite are now tracked (were local-only from the audit phase);lib/+out-forge/gitignored, forge-std installed in CI.Off-chain coordinator
src/coordinator.tsalready computes binding-correct hop IDs and tracks status off-chain only — no change required.🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Testing
Infrastructure