diff --git a/app/__tests__/hooks/useCreateMarket-fresh-batched-registration.test.ts b/app/__tests__/hooks/useCreateMarket-fresh-batched-registration.test.ts index c9302f52..37c27c91 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 960a98fc..89b0f8ac 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 00000000..7bb08ad2 --- /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 52e81f0a..03260467 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 0b509bae..dd634e90 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)); +}