fix(RingHTLC): bounded withdraw + authenticated hop membership (audit HL-05, HL-06)

.github/workflows/ci.yml

@@ -0,0 +1,35 @@
+name: CI
+
+on:
+  push:
+    branches: [master]
+  pull_request:
+
+jobs:
+  hardhat:
+    name: Hardhat tests
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v4
+      - uses: actions/setup-node@v4
+        with:
+          node-version: 20
+          cache: npm
+      - run: npm ci
+      - run: npx hardhat compile
+      - run: npx hardhat test
+
+  foundry:
+    name: Foundry tests (audit suite)
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v4
+      - uses: actions/setup-node@v4
+        with:
+          node-version: 20
+          cache: npm
+      # OpenZeppelin resolves via the node_modules remapping in foundry.toml
+      - run: npm ci
+      - uses: foundry-rs/foundry-toolchain@v1
+      - run: forge install foundry-rs/forge-std --no-git
+      - run: forge test --match-path "test-forge/*" -vv

.gitignore

@@ -6,3 +6,5 @@ typechain-types/
 coverage/
 .env
 *.log
+out-forge/
+lib/

README.md

@@ -96,21 +96,22 @@ REPORT_GAS=true npx hardhat test
 ### Contract
 
 ```solidity
-// Create an ETH hop
-ringHTLC.createHopETH(ringId, hopId, receiver, hashlock, timelock);
+// Create an ETH hop — hopId MUST equal computeHopId(ringId, hopIndex, msg.sender, receiver)
+ringHTLC.createHopETH(ringId, hopId, hopIndex, receiver, hashlock, timelock);
 
 // Create an ERC20 hop (approve first)
 token.approve(address(ringHTLC), amount);
-ringHTLC.createHopERC20(ringId, hopId, receiver, token, amount, hashlock, timelock);
+ringHTLC.createHopERC20(ringId, hopId, hopIndex, receiver, token, amount, hashlock, timelock);
 
 // Withdraw with preimage
 ringHTLC.withdraw(hopId, preimage);
 
 // Refund after timeout
 ringHTLC.refund(hopId);
 
-// View helpers
-ringHTLC.isRingSettled(ringId);
+// View helpers — derive expectedHopIds from the ring definition via
+// computeHopId; never accept the list from an untrusted party
+ringHTLC.isRingSettled(ringId, expectedHopIds);
 ringHTLC.getRingHopIds(ringId);
 ringHTLC.computeHopId(ringId, hopIndex, sender, receiver);
 ```

contracts/RingHTLC.sol

@@ -13,7 +13,15 @@ import "@openzeppelin/contracts/utils/ReentrancyGuard.sol";
  * every hop — either the entire ring settles, or every participant refunds.
  *
  * Timelocks are staggered: the last hop expires first so refunds cascade
- * safely in reverse order if settlement fails.
+ * safely in reverse order if settlement fails. Withdraw is bounded by the
+ * hop timelock (audit HL-05), so withdraw and refund are temporally
+ * exclusive — the staggered gaps are the window in which earlier hops can
+ * still claim after a later hop's preimage reveal.
+ *
+ * NOTE (informational, audit HL-05): this contract hashes preimages with
+ * keccak256; the core HashLock HTLCs (EVM/Sui/BTC) use sha256. A hashlock
+ * from the main system will NOT validate here and vice versa — do not
+ * compose the two without a hash-function bridge.
  */
 contract RingHTLC is ReentrancyGuard {
     using SafeERC20 for IERC20;
@@ -65,13 +73,20 @@ contract RingHTLC is ReentrancyGuard {
      * @notice Create an ETH-funded hop in a ring.
      * @param ringId    Unique ring identifier (computed off-chain by coordinator)
      * @param hopId     Deterministic hop ID = keccak256(ringId, hopIndex, sender, receiver)
+     * @param hopIndex  Position of this hop in the ring (binds hopId — audit HL-06)
      * @param receiver  Address that will receive funds on withdrawal
      * @param hashlock  keccak256(preimage) — shared across all hops in the ring
      * @param timelock  Unix timestamp after which sender can refund
+     * @dev   HL-06: hopId is verified against computeHopId(ringId, hopIndex,
+     *        msg.sender, receiver), so a hop slot is unforgeable — an outsider
+     *        cannot occupy another participant's expected hopId. Combined with
+     *        the expected-hop-set form of isRingSettled, junk hops created
+     *        under someone else's ringId cannot grief settlement detection.
      */
     function createHopETH(
         bytes32 ringId,
         bytes32 hopId,
+        uint8 hopIndex,
         address receiver,
         bytes32 hashlock,
         uint256 timelock
@@ -80,6 +95,11 @@ contract RingHTLC is ReentrancyGuard {
         require(receiver != address(0), "BadReceiver");
         require(timelock > block.timestamp, "BadTimelock");
         require(hops[hopId].status == HopStatus.INVALID, "HopExists");
+        // HL-06: enforce the deterministic hop ID the docs always promised
+        require(
+            hopId == keccak256(abi.encodePacked(ringId, hopIndex, msg.sender, receiver)),
+            "BadHopId"
+        );
 
         hops[hopId] = Hop({
             ringId: ringId,
@@ -104,6 +124,7 @@ contract RingHTLC is ReentrancyGuard {
     function createHopERC20(
         bytes32 ringId,
         bytes32 hopId,
+        uint8 hopIndex,
         address receiver,
         address token,
         uint256 amount,
@@ -115,6 +136,11 @@ contract RingHTLC is ReentrancyGuard {
         require(token != address(0), "UseETH");
         require(timelock > block.timestamp, "BadTimelock");
         require(hops[hopId].status == HopStatus.INVALID, "HopExists");
+        // HL-06: enforce the deterministic hop ID (see createHopETH)
+        require(
+            hopId == keccak256(abi.encodePacked(ringId, hopIndex, msg.sender, receiver)),
+            "BadHopId"
+        );
 
         IERC20(token).safeTransferFrom(msg.sender, address(this), amount);
 
@@ -139,10 +165,17 @@ contract RingHTLC is ReentrancyGuard {
     /**
      * @notice Withdraw funds from a hop by revealing the preimage.
      * @dev    Anyone can call this — funds always go to the designated receiver.
+     *         HL-05: withdraw is bounded by the hop timelock (mirrors the core
+     *         EVM/Sui HTLCs), making withdraw and refund temporally exclusive.
+     *         Without the bound, both were valid simultaneously after expiry —
+     *         a first-tx-wins race that could leave a ring part-WITHDRAWN /
+     *         part-REFUNDED. The ring's staggered timelocks are exactly the
+     *         windows in which earlier hops can still claim after a reveal.
      */
     function withdraw(bytes32 hopId, bytes32 preimage) external nonReentrant {
         Hop storage hop = hops[hopId];
         require(hop.status == HopStatus.ACTIVE, "NotActive");
+        require(block.timestamp < hop.timelock, "Expired");
         require(keccak256(abi.encodePacked(preimage)) == hop.hashlock, "BadPreimage");
 
         hop.status = HopStatus.WITHDRAWN;
@@ -190,12 +223,29 @@ contract RingHTLC is ReentrancyGuard {
         return _ringHopIds[ringId].length;
     }
 
-    /// @notice Check if every hop in the ring has been withdrawn
-    function isRingSettled(bytes32 ringId) external view returns (bool) {
-        bytes32[] memory ids = _ringHopIds[ringId];
-        if (ids.length == 0) return false;
-        for (uint256 i = 0; i < ids.length; i++) {
-            if (hops[ids[i]].status != HopStatus.WITHDRAWN) return false;
+    /**
+     * @notice Check if every EXPECTED hop in the ring has been withdrawn.
+     * @param  ringId         The ring to check.
+     * @param  expectedHopIds The coordinator's full hop set for this ring
+     *                        (deterministic IDs from computeHopId).
+     * @dev    HL-06: the previous form iterated every hop ever pushed under a
+     *         ringId — an unauthenticated bytes32 — so any outsider could
+     *         append a self-funded junk hop and force the result false
+     *         forever. The caller now supplies its expected hop set; junk
+     *         hops under the same ringId are simply not consulted, and a
+     *         forged "expected" hop cannot exist because createHop* binds
+     *         hopId to (ringId, hopIndex, sender, receiver).
+     */
+    function isRingSettled(bytes32 ringId, bytes32[] calldata expectedHopIds)
+        external
+        view
+        returns (bool)
+    {
+        if (expectedHopIds.length == 0) return false;
+        for (uint256 i = 0; i < expectedHopIds.length; i++) {
+            Hop storage hop = hops[expectedHopIds[i]];
+            if (hop.ringId != ringId) return false;
+            if (hop.status != HopStatus.WITHDRAWN) return false;
         }
         return true;
     }

foundry.toml

@@ -0,0 +1,12 @@
+[profile.default]
+src = "contracts"
+out = "out-forge"
+libs = ["node_modules", "lib"]
+test = "test-forge"
+remappings = ["@openzeppelin/=node_modules/@openzeppelin/", "forge-std/=lib/forge-std/src/"]
+fs_permissions = []
+
+[invariant]
+runs = 64
+depth = 32
+fail_on_revert = false

test-forge/RingHTLC.audit.t.sol

@@ -0,0 +1,186 @@
+// SPDX-License-Identifier: MIT
+pragma solidity ^0.8.24;
+
+// AUDIT (Phase 2) - Foundry fuzz/invariant + adversarial tests for RingHTLC.
+// Repo had no Foundry config and a single happy-path Hardhat test file.
+//
+// Post-remediation (audit PR-11): the RING-01/RING-02 tests originally PASSED
+// by demonstrating the unsafe behavior. They now assert the SAFE behavior:
+//   RING-01 (HL-05): withdraw is bounded by the hop timelock.
+//   RING-02 (HL-06): hopId binding is enforced at creation, and isRingSettled
+//                    takes the coordinator's expected hop set, so junk hops
+//                    under a victim ringId cannot grief settlement detection.
+
+import "forge-std/Test.sol";
+import "../contracts/RingHTLC.sol";
+
+contract RingHTLCAuditTest is Test {
+    RingHTLC internal htlc;
+
+    address internal alice = address(0xA11CE);
+    address internal bob = address(0xB0B);
+
+    bytes32 internal constant PRE = bytes32(uint256(0xC0FFEE));
+    bytes32 internal HASH;
+
+    function setUp() public {
+        htlc = new RingHTLC();
+        HASH = keccak256(abi.encodePacked(PRE));
+        vm.deal(alice, 100 ether);
+        vm.deal(bob, 100 ether);
+    }
+
+    /// HL-06: hop IDs are binding-enforced — helper mirrors computeHopId.
+    function _hopId(bytes32 ringId, uint8 idx, address sender, address receiver)
+        internal
+        pure
+        returns (bytes32)
+    {
+        return keccak256(abi.encodePacked(ringId, idx, sender, receiver));
+    }
+
+    // ── INV-PRE: preimage soundness ────────────────────────────────────────
+    function testFuzz_withdraw_requires_correct_preimage(bytes32 wrongPre) public {
+        vm.assume(wrongPre != PRE);
+        bytes32 ring = bytes32("ring1");
+        bytes32 hopId = _hopId(ring, 0, alice, bob);
+        vm.prank(alice);
+        htlc.createHopETH{value: 1 ether}(ring, hopId, 0, bob, HASH, block.timestamp + 1 hours);
+
+        vm.expectRevert(bytes("BadPreimage"));
+        htlc.withdraw(hopId, wrongPre);
+
+        // correct preimage works
+        uint256 before = bob.balance;
+        htlc.withdraw(hopId, PRE);
+        assertEq(bob.balance, before + 1 ether, "receiver paid exactly the hop amount");
+    }
+
+    // ── INV-NODBL: a hop can never be both withdrawn and refunded ───────────
+    function test_no_double_resolve_withdraw_then_refund() public {
+        bytes32 ring = bytes32("ring1");
+        bytes32 hopId = _hopId(ring, 0, alice, bob);
+        vm.prank(alice);
+        htlc.createHopETH{value: 1 ether}(ring, hopId, 0, bob, HASH, block.timestamp + 1 hours);
+        htlc.withdraw(hopId, PRE);
+        vm.warp(block.timestamp + 2 hours);
+        vm.prank(alice);
+        vm.expectRevert(bytes("NotActive"));
+        htlc.refund(hopId);
+    }
+
+    function test_no_double_resolve_refund_then_withdraw() public {
+        bytes32 ring = bytes32("ring1");
+        bytes32 hopId = _hopId(ring, 0, alice, bob);
+        vm.prank(alice);
+        htlc.createHopETH{value: 1 ether}(ring, hopId, 0, bob, HASH, block.timestamp + 1 hours);
+        vm.warp(block.timestamp + 2 hours);
+        vm.prank(alice);
+        htlc.refund(hopId);
+        vm.expectRevert(bytes("NotActive"));
+        htlc.withdraw(hopId, PRE);
+    }
+
+    // ── RING-01 / HL-05 FIXED: withdraw is bounded by the hop timelock ──────
+    // Pre-fix this test PASSED by demonstrating a successful withdraw AFTER
+    // expiry (race window with refund). It now asserts the safe behavior.
+    function test_RING01_withdraw_rejected_after_timelock_expiry() public {
+        bytes32 ring = bytes32("ring1");
+        bytes32 hopId = _hopId(ring, 0, alice, bob);
+        vm.prank(alice);
+        htlc.createHopETH{value: 1 ether}(ring, hopId, 0, bob, HASH, block.timestamp + 1 hours);
+
+        vm.warp(block.timestamp + 2 hours); // PAST the timelock
+
+        vm.expectRevert(bytes("Expired"));
+        htlc.withdraw(hopId, PRE);
+
+        // and the sender's refund path is intact — temporally exclusive
+        uint256 before = alice.balance;
+        vm.prank(alice);
+        htlc.refund(hopId);
+        assertEq(alice.balance, before + 1 ether, "sender refunded after expiry");
+    }
+
+    function test_RING01_withdraw_succeeds_before_timelock_expiry() public {
+        bytes32 ring = bytes32("ring1");
+        bytes32 hopId = _hopId(ring, 0, alice, bob);
+        vm.prank(alice);
+        htlc.createHopETH{value: 1 ether}(ring, hopId, 0, bob, HASH, block.timestamp + 1 hours);
+
+        uint256 before = bob.balance;
+        htlc.withdraw(hopId, PRE);
+        assertEq(bob.balance, before + 1 ether, "withdraw inside the timelock window");
+    }
+
+    // ── RING-02 / HL-06 FIXED: membership binding + expected-set settlement ─
+    function test_RING02_forged_hopId_rejected_at_creation() public {
+        bytes32 ring = bytes32("victim-ring");
+        address attacker = address(0xBEEF);
+        vm.deal(attacker, 10 ether);
+
+        // Attacker tries to occupy ALICE's expected slot (sender=alice in the
+        // binding) — msg.sender is the attacker, so the binding cannot match.
+        bytes32 alicesSlot = _hopId(ring, 0, alice, bob);
+        vm.prank(attacker);
+        vm.expectRevert(bytes("BadHopId"));
+        htlc.createHopETH{value: 1 wei}(ring, alicesSlot, 0, bob, HASH, block.timestamp + 1 hours);
+
+        // Arbitrary junk IDs are rejected too.
+        vm.prank(attacker);
+        vm.expectRevert(bytes("BadHopId"));
+        htlc.createHopETH{value: 1 wei}(ring, keccak256("junk"), 0, attacker, HASH, block.timestamp + 1 hours);
+    }
+
+    function test_RING02_junk_hop_cannot_grief_expected_set_settlement() public {
+        bytes32 ring = bytes32("victim-ring");
+
+        // Legit single-hop ring, withdrawn.
+        bytes32 legit = _hopId(ring, 0, alice, bob);
+        vm.prank(alice);
+        htlc.createHopETH{value: 1 ether}(ring, legit, 0, bob, HASH, block.timestamp + 1 hours);
+        htlc.withdraw(legit, PRE);
+
+        // Mallory creates a hop under the victim's ringId with her OWN valid
+        // binding (the only remaining way in) — it lands in _ringHopIds...
+        address attacker = address(0xBEEF);
+        vm.deal(attacker, 10 ether);
+        bytes32 junk = _hopId(ring, 7, attacker, attacker);
+        vm.prank(attacker);
+        htlc.createHopETH{value: 1 wei}(ring, junk, 7, attacker, keccak256("other"), block.timestamp + 1 hours);
+        assertEq(htlc.getRingHopCount(ring), 2, "junk hop exists under the ringId");
+
+        // ...but the coordinator checks ITS expected set — settlement holds.
+        bytes32[] memory expected = new bytes32[](1);
+        expected[0] = legit;
+        assertTrue(htlc.isRingSettled(ring, expected), "HL-06: junk hop no longer griefs settlement");
+    }
+
+    function test_RING02_expected_set_rejects_foreign_and_unsettled_hops() public {
+        bytes32 ring = bytes32("ring-a");
+        bytes32 otherRing = bytes32("ring-b");
+
+        bytes32 hopA = _hopId(ring, 0, alice, bob);
+        vm.prank(alice);
+        htlc.createHopETH{value: 1 ether}(ring, hopA, 0, bob, HASH, block.timestamp + 1 hours);
+
+        bytes32 hopB = _hopId(otherRing, 0, bob, alice);
+        vm.prank(bob);
+        htlc.createHopETH{value: 1 ether}(otherRing, hopB, 0, alice, HASH, block.timestamp + 1 hours);
+        htlc.withdraw(hopB, PRE);
+
+        // Unsettled hop in the set → false.
+        bytes32[] memory setA = new bytes32[](1);
+        setA[0] = hopA;
+        assertFalse(htlc.isRingSettled(ring, setA), "unsettled hop -> not settled");
+
+        // A WITHDRAWN hop from a DIFFERENT ring cannot be passed off as ours.
+        bytes32[] memory setForeign = new bytes32[](1);
+        setForeign[0] = hopB;
+        assertFalse(htlc.isRingSettled(ring, setForeign), "foreign ring hop -> not settled");
+
+        // Empty expected set is never settled.
+        bytes32[] memory empty = new bytes32[](0);
+        assertFalse(htlc.isRingSettled(ring, empty), "empty set -> not settled");
+    }
+}