fix(#2505,#2468): bind the registration parameters into the keeper-register proof - #2532
Merged
Merged
Conversation
…gister proof
The H1v2 "stateless deployer proof" signed:
keeper-register:<slabAddress>:<unix-minute>
That authorises the SLAB and nothing else. It says nothing about the pool being
registered, the mainnet CA, the dex type, the symbol or the label — so one
captured signature authorised registering that slab against ANY pool (#2468),
and the parameters the route actually acts on were never covered by the thing
verifying them (#2505).
Both issues are one root cause and are fixed together.
The message now binds a canonical encoding of every acted-on parameter, built by
a shared module (lib/keeper-register-proof.ts) that BOTH the client and the route
import — so the two cannot drift. The sibling route POST /api/markets already
binds its complete canonical payload this way; this brings keeper-register to the
same standard.
WHAT THIS DELIBERATELY DOES NOT FIX. The signature is still valid across the
~6-minute tolerance window and is still not single-use. Closing that needs a
server-side nonce store — which is exactly what H1v2 REMOVED to fix a serverless
race, so reintroducing it here would trade a replay window for a correctness bug.
Payload binding shrinks the CONSEQUENCE instead: a captured signature can now
only replay the same registration, which is idempotent, rather than authorising a
substituted pool. Stated in the module header rather than left for someone to
discover.
Two details that would have broken this quietly:
* The client sends the NORMALIZED dexType (normalizeDexType(...)), not the raw
DexScreener id. The signature binds the normalized value, because that is what
the body carries and therefore what the route binds. Signing the raw id would
have failed verification on every market whose dexId differs from the keeper
vocabulary — and only those, so it would have looked intermittent.
* Absent optionals encode as EMPTY rather than being omitted. Omitting them
would let {symbol: "X", label: absent} and {symbol: absent, label: "X"} produce
the same message — two different registrations sharing one signature.
TWO EXISTING TESTS PINNED THE VULNERABILITY and are inverted, not deleted:
* useCreateMarket-keeper-register.test.ts asserted the signed message matched
`^keeper-register:<slab>:<minute>$` — the unbound format itself. It now asserts
the pool is covered, which is the #2468 attack stated as an assertion.
* useCreateMarket-fresh-batched-registration.test.ts searched the source for the
literal "keeper-register:". It now asserts the shared builder is used.
Negative control: reverting the message to slab+minute fails 4 of the 8 new tests.
Launch suite: 3136 passed / 16 skipped / 0 failed.
Refs: #2505, #2468
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NgoNgagkvw7i5SSRC3FJ8D
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (6)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe keeper registration proof now binds the slab, pool, CA, DEX type, symbol, label, and Unix minute. A shared canonical builder is used by both registration client paths and the server route. Tests cover binding, canonicalization, delimiter handling, and message format. ChangesKeeper registration proof
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: 🟠 High · up to The registration proof now binds the main target identifiers, but the endpoint still accepts and stores unsigned market metadata and authority-related configuration. A valid proof can therefore be reused for the authorized target while changing the stored market record, leaving a high-impact authorization gap that should be fixed before merge. Sequence Diagram(s)sequenceDiagram
participant Client
participant buildKeeperRegisterProofMessage
participant KeeperRegisterRoute
Client->>buildKeeperRegisterProofMessage: Build proof from registration parameters
Client->>KeeperRegisterRoute: Submit parameters and signature
KeeperRegisterRoute->>buildKeeperRegisterProofMessage: Rebuild proof from request parameters
KeeperRegisterRoute->>KeeperRegisterRoute: Verify signature
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Linked Issues checkExplanation The PR satisfies payload binding, canonical client-server message construction, existing validation preservation, and regression coverage for parameter tampering. It does not eliminate or reduce the approximately six-to-seven-minute replay window, add durable single-use replay protection, or provide replay-prevention and clock-skew regression coverage required by issue Resolution Reduce or eliminate the replay window, preferably with durable single-use replay protection that remains safe in the serverless environment. Add regression tests for replay prevention and clock skew while preserving the implemented payload-binding changes. [
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
This was referenced Sep 2, 2026
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
1 participant
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.
Closes #2505 and #2468 — one root cause, fixed together.
The H1v2 "stateless deployer proof" signed:
That authorises the slab and nothing else. It says nothing about the pool being registered, the mainnet CA, the dex type, the symbol or the label — so one captured signature authorised registering that slab against any pool (#2468), and the parameters the route acts on were never covered by the thing verifying them (#2505).
The message now binds a canonical encoding of every acted-on parameter, built by a shared module that both the client and the route import, so the two cannot drift. The sibling route
POST /api/marketsalready binds its complete canonical payload this way; this brings keeper-register to the same standard.What this deliberately does not fix
The signature is still valid across the ~6-minute window and is still not single-use.
Closing that needs a server-side nonce store — which is exactly what H1v2 removed to fix a serverless race. Reintroducing it here would trade a replay window for a correctness bug.
Payload binding shrinks the consequence instead: a captured signature can now only replay the same registration, which is idempotent, rather than authorising a substituted pool. This is stated in the module header rather than left to be rediscovered.
Two details that would have broken it quietly
The client sends the normalized
dexType, not the raw DexScreener id. The signature binds the normalized value, because that is what the body carries and therefore what the route binds. Signing the raw id would fail verification on only those markets whose dexId differs from the keeper vocabulary — so it would have looked intermittent rather than broken.Absent optionals encode as empty, not omitted. Omitting them would let
{symbol: "X", label: absent}and{symbol: absent, label: "X"}produce the same message — two different registrations sharing one signature.Two existing tests pinned the vulnerability
Inverted, not deleted:
useCreateMarket-keeper-register.test.tsasserted the signed message matched^keeper-register:<slab>:<minute>$— the unbound format itself. It now asserts the pool is covered, which is the keeper-register deployer proof is not bound to the pool it registers #2468 attack stated as an assertion.useCreateMarket-fresh-batched-registration.test.tssearched the source for the literal"keeper-register:". It now asserts the shared builder is used.Verification
tscclean🤖 Generated with Claude Code
https://claude.ai/code/session_01NgoNgagkvw7i5SSRC3FJ8D
Summary by CodeRabbit
Security
Bug Fixes