From 34544a2d9e89fed3bfc3cfec34a74c4a2fdddcea Mon Sep 17 00:00:00 2001 From: dcccrypto Date: Wed, 2 Sep 2026 22:39:02 +0100 Subject: [PATCH] fix(#2505,#2468): bind the registration parameters into the keeper-register proof MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The H1v2 "stateless deployer proof" signed: keeper-register:: 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::$` — 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: dcccrypto/percolator-launch#2505, dcccrypto/percolator-launch#2468 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01NgoNgagkvw7i5SSRC3FJ8D --- ...eMarket-fresh-batched-registration.test.ts | 8 +- .../useCreateMarket-keeper-register.test.ts | 16 ++- .../lib/keeper-register-proof.test.ts | 105 ++++++++++++++++++ .../api/playground/keeper-register/route.ts | 32 +++++- app/hooks/useCreateMarket.ts | 37 +++++- app/lib/keeper-register-proof.ts | 102 +++++++++++++++++ 6 files changed, 287 insertions(+), 13 deletions(-) create mode 100644 app/__tests__/lib/keeper-register-proof.test.ts create mode 100644 app/lib/keeper-register-proof.ts diff --git a/app/__tests__/hooks/useCreateMarket-fresh-batched-registration.test.ts b/app/__tests__/hooks/useCreateMarket-fresh-batched-registration.test.ts index c9302f528..37c27c91a 100644 --- a/app/__tests__/hooks/useCreateMarket-fresh-batched-registration.test.ts +++ b/app/__tests__/hooks/useCreateMarket-fresh-batched-registration.test.ts @@ -36,7 +36,13 @@ describe("useCreateMarket fresh batched registration", () => { expect(freshBatchSource).not.toContain("/api/markets/challenge"); // The keeper proof is the one remaining signature. - expect(freshBatchSource).toContain("keeper-register:"); + // + // #2505 / #2468: this used to look for the literal "keeper-register:" — the + // old message, which bound the slab and nothing else. The proof is now built + // by the shared module that binds the registration PARAMETERS, so assert on + // the builder rather than on a string that must no longer appear. + expect(freshBatchSource).toContain("buildKeeperRegisterProofMessage"); + expect(freshBatchSource).not.toContain("keeper-register:${"); }); it("does not publish the market until it actually holds collateral", () => { diff --git a/app/__tests__/hooks/useCreateMarket-keeper-register.test.ts b/app/__tests__/hooks/useCreateMarket-keeper-register.test.ts index 960a98fc5..89b0f8acd 100644 --- a/app/__tests__/hooks/useCreateMarket-keeper-register.test.ts +++ b/app/__tests__/hooks/useCreateMarket-keeper-register.test.ts @@ -78,11 +78,21 @@ describe("useCreateMarket — retryKeeperRegistration", () => { }); expect(signMessage).toHaveBeenCalledOnce(); - // The signed message must be the exact H1v2 stateless proof format the server - // reconstructs and verifies — see route.ts's statelessProofMessage(). + // The signed message must be the exact proof the server reconstructs and + // verifies — see route.ts's statelessProofMessage(). + // + // #2505 / #2468 INVERTED: this used to pin `^keeper-register::$`, + // which is precisely the unbound message the two issues report — it authorised + // the slab and nothing else, so one signature covered any pool. The proof now + // binds the registration parameters, and asserting the OLD shape would pin the + // vulnerability. const signedBytes = signMessage.mock.calls[0][0] as Uint8Array; const signedText = new TextDecoder().decode(signedBytes); - expect(signedText).toMatch(new RegExp(`^keeper-register:${SLAB}:\\d+$`)); + expect(signedText.startsWith("keeper-register\n")).toBe(true); + expect(signedText).toContain(SLAB); + // The pool must be covered — this is the #2468 attack, stated as an assertion. + expect(signedText).toContain("dexPoolAddress="); + expect(signedText).not.toMatch(new RegExp(`^keeper-register:${SLAB}:\\d+$`)); expect(fetchMock).toHaveBeenCalledWith( "/api/playground/keeper-register", diff --git a/app/__tests__/lib/keeper-register-proof.test.ts b/app/__tests__/lib/keeper-register-proof.test.ts new file mode 100644 index 000000000..7bb08ad2a --- /dev/null +++ b/app/__tests__/lib/keeper-register-proof.test.ts @@ -0,0 +1,105 @@ +import { describe, it, expect } from "vitest"; +import { + buildKeeperRegisterProofMessage, + canonicalizeKeeperRegisterParams, + type KeeperRegisterProofParams, +} from "@/lib/keeper-register-proof"; + +/** + * GH#2505 / GH#2468 — the stateless deployer proof signed only + * `keeper-register::`. + * + * That authorised the SLAB and nothing else, so one captured signature let a + * caller register that slab against ANY pool (#2468), and the parameters the + * route acts on were never covered by the thing verifying them (#2505). + * + * These tests are about the MESSAGE, which is where the defect lived. If two + * different registrations can produce the same bytes, the signature over those + * bytes cannot distinguish them — no amount of verification logic downstream + * recovers that. + */ + +const base: KeeperRegisterProofParams = { + slabAddress: "7RXTVmGcJMDqqTCFu5ADQRyLDvVZBi3r5U5WXzoULHJV", + dexPoolAddress: "PooLAAAA1111111111111111111111111111111111", + mainnetCA: "So11111111111111111111111111111111111111112", + dexType: "meteora-dlmm", + symbol: "SOL", + label: "SOL/USDC — Meteora DLMM", +}; + +const bytes = (p: KeeperRegisterProofParams, minute = 29_000_000) => + Buffer.from(buildKeeperRegisterProofMessage(p, minute)).toString("utf8"); + +describe("keeper-register proof binds the registration parameters (GH#2505, GH#2468)", () => { + it("a substituted POOL produces a different message — the #2468 attack", () => { + // The whole of #2468: same slab, same minute, different pool. Under the old + // message these were byte-identical, so one signature authorised both. + const attacker = { ...base, dexPoolAddress: "EviLPooL111111111111111111111111111111111" }; + expect(bytes(attacker)).not.toBe(bytes(base)); + }); + + it("every bound field changes the message", () => { + const variants: Array<[string, KeeperRegisterProofParams]> = [ + ["slabAddress", { ...base, slabAddress: "OtherSlab11111111111111111111111111111111" }], + ["dexPoolAddress", { ...base, dexPoolAddress: "OtherPool11111111111111111111111111111111" }], + ["mainnetCA", { ...base, mainnetCA: "OtherMint11111111111111111111111111111111" }], + ["dexType", { ...base, dexType: "raydium-clmm" }], + ["symbol", { ...base, symbol: "JUP" }], + ["label", { ...base, label: "something else" }], + ]; + for (const [field, v] of variants) { + expect(bytes(v), `${field} must be covered by the signature`).not.toBe(bytes(base)); + } + }); + + it("the minute is still bound, so the tolerance window stays finite", () => { + expect(bytes(base, 29_000_000)).not.toBe(bytes(base, 29_000_001)); + }); + + it("is order-independent — client and route cannot disagree by object shape", () => { + const reordered: KeeperRegisterProofParams = { + label: base.label, + symbol: base.symbol, + dexType: base.dexType, + mainnetCA: base.mainnetCA, + dexPoolAddress: base.dexPoolAddress, + slabAddress: base.slabAddress, + }; + expect(bytes(reordered)).toBe(bytes(base)); + }); + + it("an absent optional encodes as empty, not omitted", () => { + // If optionals were dropped rather than emptied, {symbol: undefined} and + // {symbol: ""} would differ while {symbol: undefined, label: "x"} could + // collide with {symbol: "x", label: undefined} — two different registrations + // sharing one signature. + const noSymbol = { ...base, symbol: undefined }; + const emptySymbol = { ...base, symbol: "" }; + expect(bytes(noSymbol)).toBe(bytes(emptySymbol)); + + const swapA: KeeperRegisterProofParams = { ...base, symbol: "X", label: undefined }; + const swapB: KeeperRegisterProofParams = { ...base, symbol: undefined, label: "X" }; + expect(bytes(swapA)).not.toBe(bytes(swapB)); + }); + + it("a field's value cannot imitate the delimiter", () => { + // The separator is ASCII Unit Separator (0x1F), which cannot be typed into + // the wizard or appear in a base58 address. Even so, pin that a value + // containing '=' — which CAN appear in a label — does not shift the parse. + const tricky = { ...base, label: "symbol=JUP" }; + expect(bytes(tricky)).not.toBe(bytes({ ...base, symbol: "JUP" })); + }); + + it("canonicalisation is stable for identical input", () => { + expect(canonicalizeKeeperRegisterParams(base)).toBe(canonicalizeKeeperRegisterParams({ ...base })); + }); + + it("still binds the slab — the property the old message had, kept", () => { + // Guard against "fixing" this by replacing the slab binding rather than + // adding to it. + const msg = bytes(base); + expect(msg).toContain(base.slabAddress); + expect(msg).toContain("keeper-register"); + }); +}); diff --git a/app/app/api/playground/keeper-register/route.ts b/app/app/api/playground/keeper-register/route.ts index 52e81f0a8..032604678 100644 --- a/app/app/api/playground/keeper-register/route.ts +++ b/app/app/api/playground/keeper-register/route.ts @@ -93,6 +93,10 @@ import { NextRequest, NextResponse } from "next/server"; import { checkAdminSecret } from "@/lib/admin-secret"; +import { + buildKeeperRegisterProofMessage, + type KeeperRegisterProofParams, +} from "@/lib/keeper-register-proof"; import { Connection, PublicKey } from "@solana/web3.js"; import nacl from "tweetnacl"; import * as Sentry from "@sentry/nextjs"; @@ -127,8 +131,14 @@ const STATELESS_PROOF_PREFIX = "keeper-register"; const STATELESS_PROOF_WINDOW_BACK_MIN = 5; const STATELESS_PROOF_WINDOW_FWD_MIN = 1; -function statelessProofMessage(slabAddress: string, unixMinute: number): Uint8Array { - return new TextEncoder().encode(`${STATELESS_PROOF_PREFIX}:${slabAddress}:${unixMinute}`); +// #2505 / #2468: the proof now covers the parameters the route ACTS ON, not just +// the slab. Built by the shared module so the client and this verifier cannot +// drift — see lib/keeper-register-proof.ts for what is and is not fixed. +function statelessProofMessage( + params: KeeperRegisterProofParams, + unixMinute: number, +): Uint8Array { + return buildKeeperRegisterProofMessage(params, unixMinute); } /** Verify `signatureBytes` is a valid ed25519 signature by `deployerPubkeyBytes` over @@ -136,13 +146,13 @@ function statelessProofMessage(slabAddress: string, unixMinute: number): Uint8Ar * window of the server's own clock. Stateless — no nonce store, no shared memory * needed between the "issuing" and "verifying" request (there is no issuing request). */ function verifyStatelessDeployerProof( - slabAddress: string, + params: KeeperRegisterProofParams, deployerPubkeyBytes: Uint8Array, signatureBytes: Uint8Array, ): boolean { const nowMinute = Math.floor(Date.now() / 60_000); for (let d = -STATELESS_PROOF_WINDOW_BACK_MIN; d <= STATELESS_PROOF_WINDOW_FWD_MIN; d++) { - const msg = statelessProofMessage(slabAddress, nowMinute + d); + const msg = statelessProofMessage(params, nowMinute + d); try { if (nacl.sign.detached.verify(msg, signatureBytes, deployerPubkeyBytes)) return true; } catch { @@ -326,7 +336,15 @@ export async function POST(req: NextRequest) { // H1v2: stateless deployer proof — no nonce to claim, so no server-stored state to // race across serverless lambda instances (see file header for why this replaced // the nonce+claim scheme). - const sigValid = verifyStatelessDeployerProof(slabAddress, deployerPubkeyBytes, signatureBytes); + // #2505 / #2468: verify against the parameters this request ACTS ON, not the + // slab alone. A signature is now valid for exactly one (slab, pool, CA, type, + // symbol, label) tuple, so a captured one can no longer authorise the same + // slab against a substituted pool. + const sigValid = verifyStatelessDeployerProof( + { slabAddress, dexPoolAddress, mainnetCA: mainnetCA ?? "", dexType: dexType ?? "", symbol, label }, + deployerPubkeyBytes, + signatureBytes, + ); if (!sigValid) { Sentry.captureMessage("[playground/keeper-register] Deployer signature verification failed", { level: "warning", @@ -336,7 +354,9 @@ export async function POST(req: NextRequest) { return NextResponse.json( { error: - 'Signature verification failed. Ensure you signed "keeper-register::" ' + + "Signature verification failed. The proof must cover the registration " + + "parameters, not just the slab — build it with " + + "buildKeeperRegisterProofMessage() from lib/keeper-register-proof and sign " + "with the deployer keypair within the last few minutes.", }, { status: 401 }, diff --git a/app/hooks/useCreateMarket.ts b/app/hooks/useCreateMarket.ts index 0b509bae5..dd634e901 100644 --- a/app/hooks/useCreateMarket.ts +++ b/app/hooks/useCreateMarket.ts @@ -72,6 +72,7 @@ import { parseMarketGroupV17OI, } from "@percolatorct/sdk"; import { PERCOLATOR_NFT_PROGRAM_ID } from "@/lib/nft-program"; +import { buildKeeperRegisterProofMessage } from "@/lib/keeper-register-proof"; import { deriveMarketParams, MIN_LEVERAGE_X, backingSeedPerDomain, leverageFromMarginBps } from "@/lib/market-params"; // v17: SetOracleAuthority (tag 17), PushOraclePrice (tag 16), SetOraclePriceCap (tag 16), // and UpdateConfig (tag 14) do not exist in v17. All oracle + risk params are embedded @@ -482,9 +483,28 @@ async function registerMarketWithKeeper( // deployer-signed proof — sign `keeper-register::` // and the route independently reconstructs + verifies it against a small // window around its own clock. No server-stored nonce (see route.ts header). + // #2505 / #2468: the proof binds the registration PARAMETERS, not just the + // slab — otherwise one signature authorises this slab against any pool. + // Built by the shared module so this and the route cannot drift. const unixMinute = Math.floor(Date.now() / 60_000); - const proofMsg = new TextEncoder().encode( - `keeper-register:${params.slabAddress}:${unixMinute}`, + const proofMsg = buildKeeperRegisterProofMessage( + { + slabAddress: params.slabAddress, + dexPoolAddress: params.dexPoolAddress, + mainnetCA: params.mainnetCA ?? "", + // MUST be the normalized value, because that is what the body below + // carries and therefore what the route binds. Signing the raw dexId + // while sending the normalized one would fail verification on every + // market whose DexScreener id differs from the keeper vocabulary. + dexType: normalizeDexType(params.dexType) ?? params.dexType ?? "", + symbol: params.symbol ?? undefined, + // This path sends no label, so both sides bind the empty string. Left + // explicit rather than omitted: the canonicaliser encodes an absent + // optional as empty precisely so "no label" and "label removed" cannot + // share a signature. + label: undefined, + }, + unixMinute, ); const sig = await wallet.signMessage(proofMsg); keeperSignature = Buffer.from(sig).toString("base64"); @@ -844,8 +864,19 @@ async function attemptFreshBatchedLaunch(ctx: FreshBatchContext): Promise: + * + * That authorizes the SLAB and nothing else. It says nothing about the pool being + * registered, the mainnet CA, the dex type, or the label — so one captured + * signature authorized registering that slab against ANY pool (#2468), and the + * request's actual parameters were never covered by the thing verifying them + * (#2505). + * + * The sibling route `POST /api/markets` already binds a domain-separated message + * over its complete canonical payload (`lib/market-registration-auth.ts`). This + * brings keeper-register to the same standard, minus the nonce. + * + * WHAT THIS DELIBERATELY DOES NOT FIX: the signature remains valid across the + * ~6-minute tolerance window and is not single-use. Closing that needs a + * server-side nonce store — which is exactly what H1v2 removed to fix the + * serverless race, so reintroducing it here would trade a replay window for a + * correctness bug. + * + * Payload binding shrinks the CONSEQUENCE rather than the window: a captured + * signature can now only replay the SAME registration, which is idempotent, + * instead of authorizing a substituted pool. + * + * Shared by the client (`hooks/useCreateMarket.ts`) and the route so the two + * cannot drift. Drift fails closed — the signature simply will not verify — but + * it fails closed at market-creation time, which is a bad moment to find out. + */ + +export const KEEPER_REGISTER_PROOF_PREFIX = "keeper-register"; + +/** + * Field separator: ASCII Unit Separator (0x1F). + * + * Written as an escape rather than a literal so the source stays printable. It + * cannot occur in a base58 address, a token symbol, or a human label typed into + * the wizard, so no field's content can imitate the delimiter and shift another + * field's meaning — the canonicalisation is unambiguous. + */ +const FIELD_SEP = "\u001F"; + +/** The registration parameters the route acts on, and therefore must be signed. */ +export interface KeeperRegisterProofParams { + slabAddress: string; + dexPoolAddress: string; + mainnetCA: string; + dexType: string; + symbol?: string; + label?: string; +} + +/** + * Canonical, order-independent encoding of the bound parameters. + * + * Keys are sorted so the client and the route cannot disagree by object-literal + * order. Absent optionals encode as EMPTY rather than being omitted, so "no + * symbol" and "symbol removed" produce the same message and cannot be swapped + * for one another — omitting them would let two different requests share a + * signature. + */ +export function canonicalizeKeeperRegisterParams( + p: KeeperRegisterProofParams, +): string { + const fields: Record = { + dexPoolAddress: p.dexPoolAddress ?? "", + dexType: p.dexType ?? "", + label: p.label ?? "", + mainnetCA: p.mainnetCA ?? "", + slabAddress: p.slabAddress ?? "", + symbol: p.symbol ?? "", + }; + return Object.keys(fields) + .sort() + .map((k) => `${k}=${fields[k]}`) + .join(FIELD_SEP); +} + +/** + * Build the exact bytes to sign / verify. + * + * `Uint8Array.from` rather than the TextEncoder's own result: TweetNaCl does a + * strict `instanceof Uint8Array` check, and an encoder can return an array from + * another JS realm in some test and browser environments. The sibling auth module + * hit exactly this and documents it; same defence here. + */ +export function buildKeeperRegisterProofMessage( + p: KeeperRegisterProofParams, + unixMinute: number, +): Uint8Array { + const message = [ + KEEPER_REGISTER_PROOF_PREFIX, + String(unixMinute), + canonicalizeKeeperRegisterParams(p), + ].join("\n"); + return Uint8Array.from(new TextEncoder().encode(message)); +}