From 1b579ab7ba8c31250a0807a52714a179bd616a94 Mon Sep 17 00:00:00 2001 From: jhfnetboy Date: Sat, 1 Aug 2026 23:14:56 +0700 Subject: [PATCH] fix(account): derive the deploy chain from config, preflight the RPC (#439) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Same defect PR #434 fixed in GuardianService, in the account-creation flow: `submitCreateWithPasskey` built the deployer wallet with a hardcoded `chain: sepolia` (~409) while `prepareGuardianSetup` bound the guardian acceptance hash to `configService.get("chainId")` (~122). configuration.ts defaults chainId to 10, so unless CHAIN_ID is set the user signs for one chain and the account is deployed on another; the `|| 11155111` sitting next to it was dead code that only hid the divergence. viem does not catch this: `assertCurrentChain` runs only on the json-rpc account branch, and the deployer is a local (private-key) account whose `prepareTransactionRequest` returns `chain.id` without ever issuing `eth_chainId`. So the preflight has to be ours, and it has to happen before the one-time WebAuthn ceremony is spent — otherwise the user burns a ceremony on a deploy that cannot match its own signed digest. - Two new helpers in the existing chain.util.ts: `assertValidChainId` (config validation, no silent fallback) and `assertRpcChain` (compares the endpoint's eth_chainId, takes a `why` string so the error says what would have broken). Both are Nest-free so services keep their own exception mapping. - AccountService gets `getChainId()` / `assertChainMatchesRpc()`; the deployer wallet now uses `resolveChain(this.getChainId())`, and both `prepareGuardianSetup` and `submitCreateWithPasskey` preflight the RPC first. GuardianService still carries its own copy of this logic from #434. Not migrated here on purpose: #441 is open against that same file and the conflict is not worth it — worth a follow-up once that lands, so there is one source of truth. Tests (new account.service.spec.ts, 9 cases; 66 total): across 11155111/10/8453, the acceptance hash's chainId, the QR payload's chainId and the deployer wallet's chain.id are all the configured value; on mismatch both entry points refuse and neither the hash nor the deploy is attempted; an unusable chainId is refused rather than defaulted. Verified non-vacuous by mutation: restoring the hardcoded sepolia fails 2, removing the preflight fails 2. Gates: backend type-check + build + 66 tests + lint + prettier green. Closes #439 Claude-Session: https://claude.ai/code/session_01BxmyQj2A82DfFXu97kKACk --- aastar/src/account/account.service.spec.ts | 158 +++++++++++++++++++++ aastar/src/account/account.service.ts | 63 +++++++- aastar/src/common/utils/chain.util.ts | 44 ++++++ 3 files changed, 260 insertions(+), 5 deletions(-) create mode 100644 aastar/src/account/account.service.spec.ts diff --git a/aastar/src/account/account.service.spec.ts b/aastar/src/account/account.service.spec.ts new file mode 100644 index 0000000..90298ab --- /dev/null +++ b/aastar/src/account/account.service.spec.ts @@ -0,0 +1,158 @@ +import { Test, TestingModule } from "@nestjs/testing"; +import { ConfigService } from "@nestjs/config"; +import { InternalServerErrorException } from "@nestjs/common"; +import { AccountService } from "./account.service"; +import { DatabaseService } from "../database/database.service"; +import { YAAA_SERVER_CLIENT } from "../sdk/sdk.providers"; + +// `uuid` ships ESM-only and this project's jest transform doesn't cover node_modules; +// nothing under test generates an id, so a stub is enough. +jest.mock("uuid", () => ({ v4: () => "00000000-0000-4000-8000-000000000000" })); + +const mockGetChainId = jest.fn(); +const capturedWalletConfigs: any[] = []; + +// Real viem elsewhere (resolveChain/defineChain/parseEther are all used for real); +// only the client factories are swapped so nothing hits the network and we can see +// exactly which `chain` the deployer wallet was built with. +jest.mock("viem", () => { + const actual = jest.requireActual("viem"); + return { + ...actual, + createPublicClient: jest.fn(() => ({ getChainId: mockGetChainId })), + createWalletClient: jest.fn((cfg: any) => { + capturedWalletConfigs.push(cfg); + return { ...cfg }; + }), + }; +}); + +const DEPLOYER_KEY = `0x${"a".repeat(64)}`; +const OWNER = "0x1111111111111111111111111111111111111111"; +const FACTORY = "0x9999999999999999999999999999999999999999"; + +describe("AccountService — chain consistency (issue #439)", () => { + let service: AccountService; + const mockConfigGet = jest.fn(); + const mockEnsureSigner = jest.fn(); + const mockBuildGuardianAcceptanceHash = jest.fn(); + const mockSubmitPreparedCreateAccount = jest.fn(); + + const buildService = async (chainId: unknown) => { + mockConfigGet.mockImplementation((key: string) => { + const cfg: Record = { + ethRpcUrl: "http://localhost:8545", + deployerPrivateKey: DEPLOYER_KEY, + chainId, + }; + return cfg[key]; + }); + + const module: TestingModule = await Test.createTestingModule({ + providers: [ + AccountService, + { provide: ConfigService, useValue: { get: mockConfigGet } }, + { provide: DatabaseService, useValue: { findUserById: jest.fn() } }, + { + provide: YAAA_SERVER_CLIENT, + useValue: { + wallets: { ensureSigner: mockEnsureSigner }, + ethereum: { getFactoryAddress: () => FACTORY }, + accounts: { + buildGuardianAcceptanceHash: mockBuildGuardianAcceptanceHash, + submitPreparedCreateAccount: mockSubmitPreparedCreateAccount, + }, + }, + }, + ], + }).compile(); + + return module.get(AccountService); + }; + + beforeEach(() => { + capturedWalletConfigs.length = 0; + for (const m of [ + mockGetChainId, + mockEnsureSigner, + mockBuildGuardianAcceptanceHash, + mockSubmitPreparedCreateAccount, + ]) { + m.mockReset(); + } + mockEnsureSigner.mockResolvedValue({ address: OWNER }); + mockBuildGuardianAcceptanceHash.mockReturnValue("0xacceptance"); + mockSubmitPreparedCreateAccount.mockResolvedValue({ address: OWNER, deployed: true }); + }); + + // The defect: the deployer wallet hardcoded `sepolia` while the guardian acceptance + // hash and the CREATE_ACCOUNT digest are bound to the configured chainId. On any + // non-Sepolia config the user signs for one chain and the account is deployed on + // another. Same shape as PR #434 fixed in GuardianService. + describe.each([11155111, 10, 8453])("chainId=%i", chainId => { + beforeEach(async () => { + mockGetChainId.mockResolvedValue(chainId); + service = await buildService(chainId); + }); + + it("binds the guardian acceptance hash to the configured chain", async () => { + const prepared = await service.prepareGuardianSetup("user-1", {} as any); + + expect(prepared.chainId).toBe(chainId); + // 4th positional arg of buildGuardianAcceptanceHash(owner, salt, factory, chainId, limit) + expect(mockBuildGuardianAcceptanceHash.mock.calls[0][3]).toBe(chainId); + expect(JSON.parse(prepared.qrPayload).chainId).toBe(chainId); + }); + + it("relays the deploy on that same chain", async () => { + await service.submitCreateWithPasskey("user-1", { + createId: "c-1", + challengeId: "ch-1", + credential: {}, + } as any); + + expect(capturedWalletConfigs).toHaveLength(1); + expect(capturedWalletConfigs[0].chain.id).toBe(chainId); + }); + }); + + // viem does not check this for us: `assertCurrentChain` runs only for json-rpc + // accounts, and the deployer is a local (private-key) account, whose + // prepareTransactionRequest takes `chain.id` on faith without issuing eth_chainId. + describe("RPC/config chain mismatch", () => { + beforeEach(async () => { + mockGetChainId.mockResolvedValue(11155111); // RPC is on Sepolia… + service = await buildService(10); // …but CHAIN_ID says OP mainnet. + }); + + it("refuses to build an acceptance hash the guardian would sign for the wrong chain", async () => { + await expect(service.prepareGuardianSetup("user-1", {} as any)).rejects.toThrow( + /Chain mismatch/ + ); + expect(mockBuildGuardianAcceptanceHash).not.toHaveBeenCalled(); + }); + + it("refuses to relay the deploy, before spending the one-time WebAuthn ceremony", async () => { + await expect( + service.submitCreateWithPasskey("user-1", { + createId: "c-1", + challengeId: "ch-1", + credential: {}, + } as any) + ).rejects.toThrow(/Chain mismatch/); + expect(mockSubmitPreparedCreateAccount).not.toHaveBeenCalled(); + expect(capturedWalletConfigs).toHaveLength(0); + }); + }); + + it("refuses an unusable chainId instead of falling back to a hardcoded default", async () => { + for (const bad of [undefined, 0, -1]) { + mockGetChainId.mockResolvedValue(11155111); + service = await buildService(bad); + await expect(service.prepareGuardianSetup("user-1", {} as any)).rejects.toThrow( + InternalServerErrorException + ); + expect(mockBuildGuardianAcceptanceHash).not.toHaveBeenCalled(); + } + }); +}); diff --git a/aastar/src/account/account.service.ts b/aastar/src/account/account.service.ts index b242b7e..5f3cc44 100644 --- a/aastar/src/account/account.service.ts +++ b/aastar/src/account/account.service.ts @@ -1,4 +1,11 @@ -import { Injectable, Inject, NotFoundException, BadRequestException, Logger } from "@nestjs/common"; +import { + Injectable, + Inject, + NotFoundException, + BadRequestException, + InternalServerErrorException, + Logger, +} from "@nestjs/common"; import { AirAccountServerClient as YAAAServerClient, ALG_ECDSA, @@ -18,9 +25,9 @@ import { } from "./dto/guardian-setup.dto"; import { DatabaseService } from "../database/database.service"; import { ConfigService } from "@nestjs/config"; -import { createWalletClient, http, parseEther, isAddress } from "viem"; +import { createPublicClient, createWalletClient, http, parseEther, isAddress } from "viem"; import { privateKeyToAccount } from "viem/accounts"; -import { sepolia } from "viem/chains"; +import { assertRpcChain, assertValidChainId, resolveChain } from "../common/utils/chain.util"; @Injectable() export class AccountService { @@ -32,6 +39,37 @@ export class AccountService { private configService: ConfigService ) {} + /** + * The single chain id this service operates on — it domain-separates the guardian + * acceptance hash and selects the chain the deploy is broadcast to. One source so + * the two cannot diverge (issue #439, same defect as PR #434 fixed in guardian). + */ + private getChainId(): number { + try { + return assertValidChainId(this.configService.get("chainId")); + } catch (err) { + throw new InternalServerErrorException((err as Error).message); + } + } + + /** + * Preflight: ETH_RPC_URL must actually serve `getChainId()`. viem will not check + * this for us — for a local (private-key) account it takes `chain.id` on faith and + * never issues `eth_chainId` — so a mismatch would otherwise surface only as an + * opaque `eth_sendRawTransaction` rejection, after the user's passkey ceremony. + */ + private async assertChainMatchesRpc(why: string): Promise { + const rpcUrl = this.configService.get("ethRpcUrl"); + if (!rpcUrl) { + throw new InternalServerErrorException("ETH_RPC_URL is not configured"); + } + try { + await assertRpcChain(createPublicClient({ transport: http(rpcUrl) }), this.getChainId(), why); + } catch (err) { + throw new InternalServerErrorException((err as Error).message); + } + } + /** * Converts an ETH amount string (e.g. "1.0") to wei as bigint. * Returns undefined when value is empty/zero (no guard enforcement). @@ -117,9 +155,17 @@ export class AccountService { // Resolve signer address (owner of the future account) const { address: owner } = await this.client.wallets.ensureSigner(userId); + // The guardian signs an acceptance hash domain-separated with this chainId, so the + // RPC had better be that chain — otherwise the guardian approves for one chain and + // the account is created on another. The `|| 11155111` that used to sit here was + // dead (configuration.ts defaults chainId to 10) and only hid that divergence. + await this.assertChainMatchesRpc( + "The guardian acceptance hash would be signed for a chain the account is not created on." + ); + // Pick factory + chainId from ethereum provider const factoryAddress = this.client.ethereum.getFactoryAddress(version); - const chainId = this.configService.get("chainId") || 11155111; + const chainId = this.getChainId(); // Determine salt (use provided or generate random) const salt = dto.salt ?? Math.floor(Math.random() * 1_000_000); @@ -395,6 +441,13 @@ export class AccountService { // which requires the user's physical device. `userId` is therefore not a security check // here (a strict createId↔userId assertion needs an SDK-exposed binding getter); it is // recorded for audit. See PR #399 review H1. + // The CREATE_ACCOUNT digest the user's assertion signs is bound to a chain; the + // deploy must land on that same one. Checked before the relay so a misconfigured + // deployment fails here rather than burning the one-time WebAuthn ceremony. + await this.assertChainMatchesRpc( + "The signed CREATE_ACCOUNT digest would not match the chain the deploy is relayed to." + ); + const deployerKey = this.configService.get("deployerPrivateKey") || process.env.DEPLOYER_PRIVATE_KEY; if (!deployerKey) { @@ -406,7 +459,7 @@ export class AccountService { account: privateKeyToAccount( (deployerKey.startsWith("0x") ? deployerKey : `0x${deployerKey}`) as `0x${string}` ), - chain: sepolia, + chain: resolveChain(this.getChainId()), transport: http(this.configService.get("ethRpcUrl")), }); diff --git a/aastar/src/common/utils/chain.util.ts b/aastar/src/common/utils/chain.util.ts index 8ce5b31..17e9bb5 100644 --- a/aastar/src/common/utils/chain.util.ts +++ b/aastar/src/common/utils/chain.util.ts @@ -67,3 +67,47 @@ export function resolveChain(chainId: number): Chain { rpcUrls: { default: { http: [] } }, }); } + +/** + * Validates a `chainId` read from config. `configuration.ts` always supplies a value, + * so a non-number here means the config wiring itself is broken — worth saying so + * rather than silently falling back to a hardcoded default, which is exactly how the + * broadcast chain drifted away from the signature domain in the first place (#434). + */ +export function assertValidChainId(chainId: unknown): number { + if (typeof chainId !== "number" || !Number.isInteger(chainId) || chainId <= 0) { + throw new Error( + `CHAIN_ID is not configured correctly (got ${JSON.stringify(chainId)}). ` + + "It domain-separates signatures and selects the chain transactions are sent to." + ); + } + return chainId; +} + +/** + * Preflight: the configured chain id must be what the RPC endpoint actually serves. + * + * `resolveChain` alone cannot give this guarantee — see its note on viem skipping + * `assertCurrentChain` for local accounts. Anything that signs for, or broadcasts to, + * a specific chain should call this first so a CHAIN_ID/ETH_RPC_URL mismatch fails + * immediately instead of after a user-visible signing ceremony. + * + * `why` is appended to the error to say what the mismatch would have broken. + */ +export async function assertRpcChain( + client: { getChainId: () => Promise }, + expected: number, + why: string +): Promise { + let actual: number; + try { + actual = await client.getChainId(); + } catch (err) { + throw new Error(`Could not read the chain id from ETH_RPC_URL: ${(err as Error).message}`); + } + if (actual !== expected) { + throw new Error( + `Chain mismatch: CHAIN_ID is ${expected} but ETH_RPC_URL serves chain ${actual}. ${why}` + ); + } +}