Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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", () => {
Expand Down
16 changes: 13 additions & 3 deletions app/__tests__/hooks/useCreateMarket-keeper-register.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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:<slab>:<minute>$`,
// 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",
Expand Down
105 changes: 105 additions & 0 deletions app/__tests__/lib/keeper-register-proof.test.ts
Original file line number Diff line number Diff line change
@@ -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:<slabAddress>:<unix-minute>`.
*
* 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");
});
});
32 changes: 26 additions & 6 deletions app/app/api/playground/keeper-register/route.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -127,22 +131,28 @@ 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
* `keeper-register:<slabAddress>:<unix-minute>` for some minute within the tolerance
* 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 {
Expand Down Expand Up @@ -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",
Expand All @@ -336,7 +354,9 @@ export async function POST(req: NextRequest) {
return NextResponse.json(
{
error:
'Signature verification failed. Ensure you signed "keeper-register:<slabAddress>:<unix-minute>" ' +
"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 },
Expand Down
37 changes: 34 additions & 3 deletions app/hooks/useCreateMarket.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -482,9 +483,28 @@ async function registerMarketWithKeeper(
// deployer-signed proof — sign `keeper-register:<slabAddress>:<unix-minute>`
// 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");
Expand Down Expand Up @@ -844,8 +864,19 @@ async function attemptFreshBatchedLaunch(ctx: FreshBatchContext): Promise<FreshB
let keeperProofSignature: string | null = null;
if (isKeeperOracle && params.dexPoolAddress && wallet.signMessage) {
try {
// #2505 / #2468 — same bound message as the retry path above.
const unixMinute = Math.floor(Date.now() / 60_000);
const proofMsg = new TextEncoder().encode(`keeper-register:${slabPk.toBase58()}:${unixMinute}`);
const proofMsg = buildKeeperRegisterProofMessage(
{
slabAddress: slabPk.toBase58(),
dexPoolAddress: params.dexPoolAddress ?? "",
mainnetCA: params.mainnetCA ?? "",
dexType: normalizeDexType(params.dexType) ?? params.dexType ?? "",
symbol: params.symbol ?? undefined,
label: undefined,
},
unixMinute,
);
const sig = await wallet.signMessage(proofMsg);
keeperProofSignature = Buffer.from(sig).toString("base64");
} catch { /* non-fatal — the "Retry registration" button covers this */ }
Expand Down
102 changes: 102 additions & 0 deletions app/lib/keeper-register-proof.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
/**
* The message a market deployer signs to authorize
* `POST /api/playground/keeper-register`.
*
* #2505 / #2468: the H1v2 "stateless deployer proof" replaced an earlier
* nonce+challenge scheme to remove a serverless race — correctly. But in dropping
* the nonce it also dropped PAYLOAD BINDING, and the message became:
*
* keeper-register:<slabAddress>:<unix-minute>
*
* 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<string, string> = {
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));
}
Loading