From 2c177db022d3e2f3b8c37a35a707fc1d2609da74 Mon Sep 17 00:00:00 2001 From: Robert Leifke Date: Fri, 18 Sep 2026 17:34:05 -0400 Subject: [PATCH] fix(execution): refuse to submit any action whose signer is not its owner On-chain, ActionVerifier._verifySignerPermission allows signer != owner when sessionKeys[signer][owner] >= block.timestamp. That registry is live on the deployed Matching -- registerSessionKey/deregisterSessionKey/sessionKeys are all in its runtime bytecode, and the on-chain ACTION_TYPEHASH matches the source string including `address signer` -- and NO session key has ever been registered: SessionKeyRegistered has zero logs across the contract's history. So this is a no-op today on a stronger basis than "we do not use session keys": with an empty registry the chain itself rejects a non-owner signer, so no action this executor has ever submitted successfully could have had one. The guard refuses earlier, for free, instead of paying gas to learn it. It exists because that is configuration, not a property of this service. The moment any owner registers a session key, actions signed by it become chain-valid through EVERY module this executor can reach -- deposit, transfer, withdrawal, liquidate, rfq, trade -- and nothing here would have objected: - assertWithdrawalPolicy covers withdrawals only. - assertPayloadConsistency checks signer only when EXPECTED_ACTION_SIGNER is configured, and it is set NOWHERE in infra/ (nor is EXPECTED_ACTION_OWNER). So the settlement path has never checked this at all. There were two call paths to Matching.verifyAndMatch, execute() and withdraw(), sharing only enqueueSend -- a serial queue that takes an opaque closure and never sees an action. Both now route through one private submitVerifyAndMatch, so the check is at the boundary and a third caller inherits it rather than having to remember it. The ABI and the meaning of a failed simulation are passed in, so behaviour on both paths is unchanged. AtomicSigningExecutor is authorised on Matching but this service never calls it. Deliberately absolute: no allowlist, no per-module exception, no config escape. Relaxing it for a specific delegated signer is a separate reviewable change -- the point is that the relaxation has to be deliberate rather than discovered. This binds the service, not the key: anything else holding kms:Sign on the executor key can call Matching directly and never pass through here. Tests assert the guard fires rather than that it exists. The boundary tests point at a closed port and distinguish outcomes by WHICH error arrives: a refusal returns SignerNotOwnerError in ~1ms having touched no socket, while owner-signed traffic takes ~1s and fails with "HTTP request failed", proving it got to simulateContract. An earlier version of those tests asserted only "not SignerNotOwnerError" and passed while the request was dying in ABI encoding, never reaching the RPC -- green for a reason unrelated to the guard. They now pin the transport error so that cannot recur. On the withdrawal path the guard is a backstop that never fires today, because assertWithdrawalPolicy rejects signer != owner first; a test documents that overlap rather than hiding it. withdrawal.ts and the EXPECTED_ACTION_* code are untouched. Co-authored-by: Claude Opus 5 (1M context) --- .../execution/src/executor.submit.test.ts | 146 ++++++++++++++++++ services/execution/src/executor.ts | 88 ++++++----- services/execution/src/signer-guard.test.ts | 90 +++++++++++ services/execution/src/signer-guard.ts | 79 ++++++++++ 4 files changed, 362 insertions(+), 41 deletions(-) create mode 100644 services/execution/src/executor.submit.test.ts create mode 100644 services/execution/src/signer-guard.test.ts create mode 100644 services/execution/src/signer-guard.ts diff --git a/services/execution/src/executor.submit.test.ts b/services/execution/src/executor.submit.test.ts new file mode 100644 index 0000000..a2e3bc1 --- /dev/null +++ b/services/execution/src/executor.submit.test.ts @@ -0,0 +1,146 @@ +import test from 'node:test'; +import assert from 'node:assert/strict'; + +import { encodeAbiParameters, parseAbi } from 'viem'; + +import type { AppConfig } from './config.js'; +import { MatchExecutor } from './executor.js'; +import { SignerNotOwnerError } from './signer-guard.js'; +import type { ExecuteMatchRequest, WithdrawRequest } from './types.js'; +import { WithdrawalRejectedError } from './withdrawal.js'; + +const TRADE_MODULE = '0x12423B366F6F07130961900bE00d05Ea63Acd071'; +const WITHDRAWAL_MODULE = '0x0a10AE2f5D2482cE1e43bC309D430B8861C2b5aB'; +const WRAPPED_USDC = '0x364058aFF6f36E01505fB2Cc870f8B6BD4835e84'; +const MATCHING = '0x9E90A9cD13d859Bd6a08168082FB1F6F7405F191'; +const OWNER = '0x3448ac0A3283951A2AFD5B3A582329ECA43CB47B'; +const OTHER = '0x1661AA54fA390cd916722F971e4A9Fe4c01889fB'; + +/** + * Port 1 is closed, so any RPC use fails immediately and unmistakably. + * + * That is the point: the guard runs before the queue and before `simulateContract`, so a refused + * submission must surface as SignerNotOwnerError and never as a connection error. An owner-signed + * one must do the opposite. These tests distinguish the two by which error arrives, which is what + * pins the guard to the submit boundary rather than to either handler. + */ +const config: AppConfig = { + port: 8081, + host: '127.0.0.1', + rpcUrl: 'http://127.0.0.1:1', + privateKey: '0x1111111111111111111111111111111111111111111111111111111111111111', + chainId: 8453, + executorAddress: '0x19E7E376E7C213B7E7e7e46cc70A5dD086DAff2A', + expectedActionOwner: undefined, + // Left unset exactly as infra/ leaves it: the settlement path's own signer check is inert here, + // which is why the boundary guard has to be the thing that fires. + expectedActionSigner: undefined, + dryRun: true, + waitForReceipt: false, + receiptTimeoutMs: 60_000, + withdrawalAssetAddresses: [], + withdrawalReceiptTimeoutMs: 30_000, +}; + +const deps = { + // Components are NAMED, matching @numo/abis. viem encodes a tuple by name when the ABI has + // them and positionally when it does not, so an unnamed ABI here makes every action encode as + // undefined -- which fails before the RPC and would make the "reaches the RPC" tests below pass + // for the wrong reason. + matchingAbi: parseAbi([ + 'function verifyAndMatch((uint256 subaccountId,uint256 nonce,address module,bytes data,uint256 expiry,address owner,address signer)[] actions,bytes[] signatures,bytes actionData)', + ]), + matchingAddress: MATCHING as `0x${string}`, + tradeModuleAddress: TRADE_MODULE as `0x${string}`, + withdrawal: { moduleAddress: WITHDRAWAL_MODULE as `0x${string}`, assetAddresses: [WRAPPED_USDC as `0x${string}`] }, +}; + +const executor = () => MatchExecutor.create({ ...config }, deps); + +function matchRequest(signer: string = OWNER): ExecuteMatchRequest { + const action = (nonce: string) => ({ + subaccount_id: '15', + nonce, + module: TRADE_MODULE, + data: '0x' as const, + expiry: '1789999999', + owner: OWNER, + signer, + }); + return { + market: 'USDCcNGN-SPOT', + asset_address: WRAPPED_USDC, + module_address: TRADE_MODULE, + maker_order_id: 'maker-1', + taker_order_id: 'taker-1', + actions: [action('1'), action('2')], + signatures: [`0x${'ab'.repeat(65)}`, `0x${'cd'.repeat(65)}`], + order_data: { + taker_account: '19', + taker_fee: '0', + fill_details: [{ filled_account: '15', amount_filled: '1', price: '1', fee: '0' }], + manager_data: '0x', + }, + } as ExecuteMatchRequest; +} + +function withdrawRequest(signer: string = OWNER): WithdrawRequest { + return { + action: { + subaccount_id: '15', + nonce: '7328734720000000', + module: WITHDRAWAL_MODULE, + data: encodeAbiParameters([{ type: 'address' }, { type: 'uint256' }], [WRAPPED_USDC, 1_000_000n]), + expiry: String(Math.floor(Date.now() / 1000) + 600), + owner: OWNER, + signer, + }, + signature: `0x${'ab'.repeat(65)}`, + } as WithdrawRequest; +} + +async function errorFrom(run: () => Promise): Promise { + try { + await run(); + } catch (error) { + return error; + } + assert.fail('expected a rejection, got none'); +} + +test('settlement: a non-owner signer is refused at the boundary, before any RPC', async () => { + const error = await errorFrom(() => executor().then((e) => e.execute(matchRequest(OTHER)))); + assert.ok(error instanceof SignerNotOwnerError, `expected SignerNotOwnerError, got ${String(error)}`); + assert.equal(error.owner, OWNER); + assert.equal(error.signer, OTHER); + assert.equal(error.module, TRADE_MODULE); +}); + +test('settlement: an owner-signed match is unchanged — it reaches the RPC', async () => { + // Asserting the failure is the DEAD SOCKET, not merely "not SignerNotOwnerError". An earlier + // version of this test checked only the latter and passed while the request was dying in + // encoding, never reaching the RPC at all -- green for a reason that had nothing to do with the + // guard. Pinning the transport error is what makes this prove the guard let it through. + const error = await errorFrom(() => executor().then((e) => e.execute(matchRequest(OWNER)))); + assert.ok(!(error instanceof SignerNotOwnerError), `guard rejected owner-signed traffic: ${String(error)}`); + assert.match(String((error as Error).message), /HTTP request failed/); +}); + +test('withdrawal: an owner-signed withdrawal is unchanged — it reaches the RPC', async () => { + const error = await errorFrom(() => executor().then((e) => e.withdraw(withdrawRequest(OWNER)))); + assert.ok(!(error instanceof SignerNotOwnerError), `guard rejected owner-signed traffic: ${String(error)}`); + // describeSimulationRevert returns undefined for a transport failure, so the original error is + // rethrown rather than being reported as a withdrawal that would revert. + assert.ok(!(error instanceof WithdrawalRejectedError), `transport failure misreported as a revert: ${String(error)}`); + assert.match(String((error as Error).message), /HTTP request failed/); +}); + +test('withdrawal: a non-owner signer is refused, by the pre-existing policy check', async () => { + // Documents the overlap rather than hiding it. assertWithdrawalPolicy already rejects + // signer != owner and runs first, so on this path the boundary guard is a backstop that never + // fires today. It becomes load-bearing the moment that check is relaxed for a delegated signer, + // which is exactly when a forgotten path would otherwise open. + const error = await errorFrom(() => executor().then((e) => e.withdraw(withdrawRequest(OTHER)))); + assert.ok(error instanceof WithdrawalRejectedError, `expected WithdrawalRejectedError, got ${String(error)}`); + assert.match(error.message, /signer must be action\.owner/); +}); diff --git a/services/execution/src/executor.ts b/services/execution/src/executor.ts index 94c7fd5..22fbdaa 100644 --- a/services/execution/src/executor.ts +++ b/services/execution/src/executor.ts @@ -14,6 +14,7 @@ import { privateKeyToAccount } from 'viem/accounts'; import type { AppConfig } from './config.js'; import { createKmsAccount } from '@numo/kms-signer'; import { createSerialQueue } from './serial-queue.js'; +import { assertSignerIsOwner, type SubmittedAction } from './signer-guard.js'; import type { ExecuteMatchRequest, ExecuteMatchResponse, WithdrawRequest } from './types.js'; import { WithdrawalRejectedError, @@ -77,23 +78,36 @@ export class MatchExecutor { this.walletClient = createWalletClient({ account: this.account, chain: this.chain, transport: http(config.rpcUrl) }); } - async execute(request: ExecuteMatchRequest): Promise { - assertPayloadConsistency(request, { - tradeModuleAddress: this.deps.tradeModuleAddress, - expectedActionOwner: this.config.expectedActionOwner, - expectedActionSigner: this.config.expectedActionSigner, - }); - - const args = buildVerifyAndMatchArgs(request); + /** + * The one place an action reaches `Matching.verifyAndMatch`. + * + * Both callers -- settlement and withdrawal -- route through here so the signer guard is applied + * once at the boundary rather than once per handler. A seventh module, or a third caller, inherits + * it by construction instead of needing to remember it. + * + * The guard runs BEFORE the queue: a refusal costs no queue slot and no simulation. Everything + * that differs between the two callers -- the ABI, and what a failed simulation means -- is passed + * in, so this method decides nothing about them. + */ + private async submitVerifyAndMatch( + args: readonly [readonly SubmittedAction[], readonly `0x${string}`[], `0x${string}`], + options: { abi: Abi; onSimulationError?: (error: unknown) => never }, + ): Promise<`0x${string}` | 'dry-run'> { + assertSignerIsOwner(args[0]); - const txHash = await this.enqueueSend(async () => { - await this.publicClient.simulateContract({ - account: this.account, - address: this.deps.matchingAddress, - abi: this.deps.matchingAbi, - functionName: 'verifyAndMatch', - args, - }); + return this.enqueueSend(async () => { + try { + await this.publicClient.simulateContract({ + account: this.account, + address: this.deps.matchingAddress, + abi: options.abi, + functionName: 'verifyAndMatch', + args, + }); + } catch (error) { + options.onSimulationError?.(error); + throw error; + } if (this.config.dryRun) { return 'dry-run' as const; @@ -102,12 +116,24 @@ export class MatchExecutor { return this.walletClient.writeContract({ account: this.account, address: this.deps.matchingAddress, - abi: this.deps.matchingAbi, + abi: options.abi, functionName: 'verifyAndMatch', args, chain: this.chain, }); }); + } + + async execute(request: ExecuteMatchRequest): Promise { + assertPayloadConsistency(request, { + tradeModuleAddress: this.deps.tradeModuleAddress, + expectedActionOwner: this.config.expectedActionOwner, + expectedActionSigner: this.config.expectedActionSigner, + }); + + const args = buildVerifyAndMatchArgs(request); + + const txHash = await this.submitVerifyAndMatch(args, { abi: this.deps.matchingAbi }); if (txHash === 'dry-run') { return { accepted: true, tx_hash: 'dry-run' }; @@ -158,35 +184,15 @@ export class MatchExecutor { const args = buildWithdrawArgs(request); const abi = [...this.deps.matchingAbi, ...withdrawalRevertErrorsAbi] as Abi; - const txHash = await this.enqueueSend(async () => { - try { - await this.publicClient.simulateContract({ - account: this.account, - address: this.deps.matchingAddress, - abi, - functionName: 'verifyAndMatch', - args, - }); - } catch (error) { + const txHash = await this.submitVerifyAndMatch(args, { + abi, + onSimulationError: (error) => { const revert = describeSimulationRevert(error); if (revert !== undefined) { throw new WithdrawalRejectedError(`withdrawal would revert: ${revert}`, revert); } throw error; - } - - if (this.config.dryRun) { - return 'dry-run' as const; - } - - return this.walletClient.writeContract({ - account: this.account, - address: this.deps.matchingAddress, - abi, - functionName: 'verifyAndMatch', - args, - chain: this.chain, - }); + }, }); if (txHash === 'dry-run') { diff --git a/services/execution/src/signer-guard.test.ts b/services/execution/src/signer-guard.test.ts new file mode 100644 index 0000000..708e69e --- /dev/null +++ b/services/execution/src/signer-guard.test.ts @@ -0,0 +1,90 @@ +import test from 'node:test'; +import assert from 'node:assert/strict'; + +import { SignerNotOwnerError, assertSignerIsOwner, type GuardLog, type SubmittedAction } from './signer-guard.js'; + +// The live module set on Base, so "module-agnostic" is asserted against the real addresses rather +// than against placeholders. +const DEPOSIT = '0x6540f8d9Eb599b045C05E45cb6a5B1730a806658' as `0x${string}`; +const TRANSFER = '0xEd8f114982FDBb03B70D4AC427bec7A355Cb78e4' as `0x${string}`; +const WITHDRAWAL = '0x0a10AE2f5D2482cE1e43bC309D430B8861C2b5aB' as `0x${string}`; +const LIQUIDATE = '0x25CF912A21e25226F1Bd99E2ADA959cC80dC4338' as `0x${string}`; +const RFQ = '0x8399328AC53a279A3564E49c2cbC82Ce95ee62D3' as `0x${string}`; +const TRADE = '0x12423B366F6F07130961900bE00d05Ea63Acd071' as `0x${string}`; + +const OWNER = '0x3448ac0A3283951A2AFD5B3A582329ECA43CB47B' as `0x${string}`; +const OTHER = '0x1661AA54fA390cd916722F971e4A9Fe4c01889fB' as `0x${string}`; + +function action(over: Partial = {}): SubmittedAction { + return { subaccountId: 15n, module: TRADE, owner: OWNER, signer: OWNER, ...over }; +} + +/** node:assert's throws() returns undefined, so the error has to be caught to be inspected. */ +function thrownBy(run: () => void): SignerNotOwnerError { + try { + run(); + } catch (error) { + assert.ok(error instanceof SignerNotOwnerError, `expected SignerNotOwnerError, got ${String(error)}`); + return error; + } + assert.fail('expected a rejection, got none'); +} + +function capture(): { log: GuardLog; lines: { level: string; message: string; fields: Record }[] } { + const lines: { level: string; message: string; fields: Record }[] = []; + return { log: (level, message, fields) => lines.push({ level, message, fields }), lines }; +} + +test('owner-signed actions pass on every live module', () => { + for (const module of [DEPOSIT, TRANSFER, WITHDRAWAL, LIQUIDATE, RFQ, TRADE]) { + assertSignerIsOwner([action({ module })], capture().log); + } +}); + +test('a non-owner signer is rejected on every live module', () => { + // The rule is module-agnostic on purpose: a session key registered for this owner would be + // chain-valid through all six, so a guard that only covered trade or only covered withdrawal + // would leave the others open. + for (const module of [DEPOSIT, TRANSFER, WITHDRAWAL, LIQUIDATE, RFQ, TRADE]) { + assert.throws(() => assertSignerIsOwner([action({ module, signer: OTHER })], capture().log), SignerNotOwnerError); + } +}); + +test('an empty action list passes', () => { + assertSignerIsOwner([], capture().log); +}); + +test('the first offending action is the one reported', () => { + const { log, lines } = capture(); + const actions = [action(), action({ module: RFQ, signer: OTHER }), action({ module: TRANSFER, signer: OTHER })]; + const error = thrownBy(() => assertSignerIsOwner(actions, log)); + assert.equal(error.index, 1); + assert.equal(error.module, RFQ); + assert.equal(lines.length, 1, 'one refusal, not one per offending action'); +}); + +test('the refusal logs signer, owner and module', () => { + const { log, lines } = capture(); + assert.throws(() => assertSignerIsOwner([action({ signer: OTHER })], log)); + assert.equal(lines[0]?.level, 'error'); + assert.equal(lines[0]?.message, 'action_signer_not_owner'); + assert.deepEqual(lines[0]?.fields, { + index: 0, + owner: OWNER, + signer: OTHER, + module: TRADE, + subaccount_id: '15', + }); +}); + +test('the error names both addresses, so a log line identifies the key', () => { + const error = thrownBy(() => assertSignerIsOwner([action({ signer: OTHER })], capture().log)); + assert.equal(error.name, 'SignerNotOwnerError'); + assert.match(error.message, new RegExp(OTHER)); + assert.match(error.message, new RegExp(OWNER)); +}); + +test('addresses are compared checksum-insensitively', () => { + // Both builders run getAddress, but a differing case must never read as a different signer. + assertSignerIsOwner([action({ owner: OWNER.toLowerCase() as `0x${string}` })], capture().log); +}); diff --git a/services/execution/src/signer-guard.ts b/services/execution/src/signer-guard.ts new file mode 100644 index 0000000..461b744 --- /dev/null +++ b/services/execution/src/signer-guard.ts @@ -0,0 +1,79 @@ +import { getAddress } from 'viem'; + +/** + * Every action this executor submits must be signed by the account's own owner. + * + * On-chain, `ActionVerifier._verifySignerPermission` already allows `signer != owner` when + * `sessionKeys[signer][owner] >= block.timestamp`. That registry is live on the deployed Matching + * (`registerSessionKey`/`deregisterSessionKey` are in its runtime bytecode) and **no session key has + * ever been registered** — `SessionKeyRegistered` has zero logs across the contract's whole history. + * So today the chain rejects a non-owner signer for us, and this guard is a no-op: an action with + * `signer != owner` would revert in `verifyAndMatch` anyway. It refuses earlier, for free, instead + * of paying gas to discover it. + * + * It exists because that is an accident of configuration, not a property of this service. The + * moment any owner registers a session key, actions signed by it become chain-valid on EVERY module + * this executor can reach — trade, rfq, transfer, liquidate, deposit, withdrawal — and nothing here + * would have objected. `assertWithdrawalPolicy` covers only withdrawals, and the settlement path's + * own signer check (`assertPayloadConsistency`) is conditional on `EXPECTED_ACTION_SIGNER`, which is + * set nowhere in `infra/`. So the settlement path has never checked this at all. + * + * Deliberately absolute: no allowlist, no per-module exception, no config escape. Relaxing it for a + * specific delegated signer is a separate, reviewable change — the point of this one is that the + * relaxation has to be deliberate rather than discovered. + * + * This binds the service, not the key. Anything else holding `kms:Sign` on the executor key can call + * `Matching.verifyAndMatch` (or `AtomicSigningExecutor`) directly and never pass through here. + */ + +/** The fields of a built action this guard reads. Both builders produce checksummed addresses. */ +export type SubmittedAction = { + subaccountId: bigint; + module: `0x${string}`; + owner: `0x${string}`; + signer: `0x${string}`; +}; + +/** Distinct from WithdrawalRejectedError: this refuses a submission on any path, not just a withdrawal. */ +export class SignerNotOwnerError extends Error { + constructor( + readonly index: number, + readonly owner: `0x${string}`, + readonly signer: `0x${string}`, + readonly module: `0x${string}`, + ) { + super( + `actions[${index}] is signed by ${signer} for owner ${owner} on module ${module}: ` + + 'this executor submits only owner-signed actions', + ); + this.name = 'SignerNotOwnerError'; + } +} + +export type GuardLog = (level: 'info' | 'error', message: string, fields: Record) => void; + +/** Same shape index.ts gives the canary, so a refusal is one JSON line in the task's logs. */ +const defaultLog: GuardLog = (level, message, fields) => { + process.stdout.write(`${JSON.stringify({ level, msg: message, ...fields })}\n`); +}; + +/** + * @throws SignerNotOwnerError on the first action whose signer is not its owner. + */ +export function assertSignerIsOwner(actions: readonly SubmittedAction[], log: GuardLog = defaultLog): void { + for (const [index, action] of actions.entries()) { + const owner = getAddress(action.owner); + const signer = getAddress(action.signer); + if (owner === signer) continue; + + const module = getAddress(action.module); + log('error', 'action_signer_not_owner', { + index, + owner, + signer, + module, + subaccount_id: action.subaccountId.toString(), + }); + throw new SignerNotOwnerError(index, owner, signer, module); + } +}