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); + } +}