From 39e3280d32aa2bcaccf13917da384b396f35b822 Mon Sep 17 00:00:00 2001 From: oskarth Date: Mon, 27 Jul 2026 19:06:08 +0800 Subject: [PATCH] fix(diy-validium): reject non-canonical leaf indices in escape hatch _verifyMerkleProof consumed only proof.length bits of leafIndex while claimed was keyed on the full uint256, so one valid (leaf, proof) pair re-verified at leafIndex + k*2^depth and drained the bridge. Require the index to be fully consumed after the proof loop. --- CHANGELOG.md | 1 + pocs/diy-validium/SPEC.md | 9 ++++++++ .../contracts/src/ValidiumBridge.sol | 7 ++++++- .../contracts/test/ValidiumBridge.t.sol | 21 +++++++++++++++++++ 4 files changed, 37 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b123b46..ccbbba6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,6 +28,7 @@ Use `[repo]` for repository-wide changes (CI, templates, docs). - **Changed**: README PoC table synced with current `pocs/` (added `private-identity`, linked published writeups, updated statuses) ### [diy-validium] +- **Fixed**: `escapeWithdraw` replay at non-canonical leaf indices. `_verifyMerkleProof` consumed only `proof.length` bits of `leafIndex` while `claimed` was keyed on the full `uint256`, so one valid `(leaf, proof)` pair re-verified at `leafIndex + k * 2^depth` and drained the bridge. The verifier now requires the index to be fully consumed. Reported by Semih Civelek. - **Changed**: IMAGE_IDs from hardcoded `bytes32(0)` constants to immutable constructor params in all contracts - **Added**: `risc0-ethereum-contracts` for real seal encoding in E2E test - **Added**: E2E test passes real IMAGE_IDs and encoded seals when guest ELFs are compiled diff --git a/pocs/diy-validium/SPEC.md b/pocs/diy-validium/SPEC.md index b5d4d70..9cdc991 100644 --- a/pocs/diy-validium/SPEC.md +++ b/pocs/diy-validium/SPEC.md @@ -386,10 +386,19 @@ function escapeWithdraw( ) external; // Requires: frozen, !claimed[leafIndex], balance > 0 // Verifies: SHA256(pubkey || balance_le || salt) is in the Merkle tree at leafIndex +// Requires: leafIndex < 2^depth (canonical index; see Index Canonicality) // Requires: msg.sender == escapeAddress[pubkey] // Sends: balance tokens to msg.sender ``` +### Index Canonicality + +`claimed` is keyed on the full `uint256 leafIndex`, but Merkle verification only consumes `merkleProof.length` bits of it to derive left/right directions. Every `leafIndex + k * 2^depth` therefore produces the same directions and verifies against the same root, while landing in a distinct `claimed` slot. A single depositor could replay one valid `(leaf, proof)` pair at successive aliases and drain the bridge. + +**Fix: reject non-canonical indices.** After consuming `merkleProof.length` bits, the verifier requires the remaining index bits to be zero, which bounds `leafIndex` to `[0, 2^depth)` and makes the `claimed` key a one-to-one mapping onto tree positions. + +Note that the Rust host and guest carry explicit direction flags (`indices: Vec`) alongside the sibling path rather than re-deriving them from a number, so they are unaffected. The contract is the only place directions are derived from an integer. + ### Front-Running Protection Without address binding, `escapeWithdraw` sends tokens to `msg.sender`. An attacker monitoring the mempool could copy a legitimate escape transaction's calldata and front-run it, stealing the user's funds. diff --git a/pocs/diy-validium/contracts/src/ValidiumBridge.sol b/pocs/diy-validium/contracts/src/ValidiumBridge.sol index 639ae9d..d464ad6 100644 --- a/pocs/diy-validium/contracts/src/ValidiumBridge.sol +++ b/pocs/diy-validium/contracts/src/ValidiumBridge.sol @@ -293,6 +293,11 @@ contract ValidiumBridge { } /// @dev Verify a binary SHA-256 Merkle proof. + /// The loop consumes exactly proof.length bits of `index`, so any index at or + /// above 2**proof.length would verify identically to index % 2**proof.length. + /// Requiring index == 0 after the loop rejects those aliases; without it the + /// same (leaf, proof) re-verifies at index + k*2**depth, and since `claimed` + /// is keyed on the full index each alias is a fresh slot. function _verifyMerkleProof(bytes32 leaf, uint256 index, bytes32[] calldata proof, bytes32 root) internal pure @@ -307,6 +312,6 @@ contract ValidiumBridge { } index >>= 1; } - return computed == root; + return computed == root && index == 0; } } diff --git a/pocs/diy-validium/contracts/test/ValidiumBridge.t.sol b/pocs/diy-validium/contracts/test/ValidiumBridge.t.sol index 7b9f7d6..f56663a 100644 --- a/pocs/diy-validium/contracts/test/ValidiumBridge.t.sol +++ b/pocs/diy-validium/contracts/test/ValidiumBridge.t.sol @@ -553,6 +553,27 @@ contract ValidiumBridgeTest is Test { assertFalse(escapeBridge.claimed(3)); } + /// @dev A leaf index above the tree size must not verify. The proof loop only + /// consumes proof.length bits, so index + k*2^depth would otherwise re-verify + /// against a fresh `claimed` slot and let one account drain the bridge. + function test_escapeWithdraw_revertsShiftedIndexReplay() public { + ValidiumBridge escapeBridge = _deployEscapeBridge(); + _freezeBridge(escapeBridge); + + bytes32[] memory proof0 = _getMerkleProof(0); + vm.prank(alice); + escapeBridge.escapeWithdraw(0, ESCAPE_PUBKEY_0, ESCAPE_BALANCE_0, ESCAPE_SALT_0, proof0); + + // Same leaf, same proof, index shifted by one tree width (4). + for (uint256 i = 1; i < 4; i++) { + vm.prank(alice); + vm.expectRevert(ValidiumBridge.InvalidMerkleProof.selector); + escapeBridge.escapeWithdraw(i * 4, ESCAPE_PUBKEY_0, ESCAPE_BALANCE_0, ESCAPE_SALT_0, proof0); + } + + assertEq(token.balanceOf(alice), ESCAPE_BALANCE_0); + } + // --------------------------------------------------------------- // Frozen guards // ---------------------------------------------------------------