fix(tempo): support raw session voucher signing#455
Conversation
6796108 to
1862b7b
Compare
commit: |
| type Parameters = Account.getResolver.Parameters & | ||
| Client.getResolver.Parameters & { | ||
| /** Address authorized to sign vouchers. Defaults to the account address. Use when a separate access key (e.g. secp256k1) signs vouchers while the root account funds the channel. */ | ||
| /** Address authorized to sign vouchers. Defaults to the voucher signer address, access key address, or account address. */ |
tempoxyz-cyclops-bot
left a comment
There was a problem hiding this comment.
👁️ Cyclops Review
PR #455 adds delegated voucherSigner support for tempo/session, threading a separate signing account through session() / sessionManager() / ChannelOps, replacing recoverTypedDataAddress with SignatureEnvelope.verify in verifyVoucher, and extracting signVoucherDigest / normalizeVoucherSignature helpers. The plumbing is coherent inside session(), but the verifier change widens what the server accepts off-chain in ways the on-chain settlement path does not mirror, and a pre-existing channel-recovery branch becomes materially more exploitable when clients reuse a stable delegated signer across servers.
🚨 [SECURITY] Cross-channel voucher signing via unvalidated tryRecoverChannel in auto-managed sessions
Severity: Critical
File: src/tempo/client/Session.ts:166-193 (autoManageCredential recovery branch) — not in this PR's diff, so reported here instead of inline
Also affects: src/tempo/client/ChannelOps.ts:219-241 (tryRecoverChannel discards payee/payer/token/authorizedSigner)
In auto-managed mode, when there is no cached entry for the current challenge's (payee, currency, escrow) key, the client takes suggestedChannelId = context?.channelId ?? md?.channelId from the untrusted 402 challenge and calls tryRecoverChannel(). tryRecoverChannel only uses deposit/finalized/settled and discards payee, payer, token, and authorizedSigner. autoManageCredential then stores the recovered entry under the malicious server's key and signs a voucher for that arbitrary channelId.
Because vouchers sign only (channelId, cumulativeAmount) under the escrow/chain EIP-712 domain — not the HTTP challenge or server identity — a malicious payee (Bob) who has observed an unrelated Alice→Charlie channel can present its channelId in his challenge. Alice's client signs a higher cumulative voucher for the Alice→Charlie channel, which Bob can forward to Charlie to spend Alice's funds. This bug pre-dates the PR but is materially more exploitable now that the PR encourages a stable delegated voucherSigner reused across servers.
Recommended Fix: Have tryRecoverChannel return the full on-chain channel state, and in autoManageCredential validate the recovered channel before signing:
if (onChain.payee.toLowerCase() !== payee.toLowerCase()) throw new Error('payee mismatch')
if (onChain.token.toLowerCase() !== currency.toLowerCase()) throw new Error('token mismatch')
if (onChain.payer.toLowerCase() !== account.address.toLowerCase()) throw new Error('payer mismatch')The server-side open path already does analogous (payee, token) checks at src/tempo/server/Session.ts:483-506; mirror them on the client recovery path.
The two in-diff verifyVoucher issues (P256 uncollectability and non-canonical-byte canonicalization mismatch) are detailed in an inline comment on src/tempo/session/Voucher.ts.
Reviewer Callouts
- ⚡
createOpenPayloadlatent gap (src/tempo/client/ChannelOps.ts:126-207): when called directly with onlyvoucherSignerset and noauthorizedSigner, the on-chainopen(...)usesaccount.addressas the authorized signer while the voucher is signed byvoucherSigner.address, producing an immediately unspendable channel. Today onlySession.tscalls this and always pre-fillsauthorizedSigner = voucherSigner.address, but the helper is exported. Consider derivingauthorizedSigner = options.voucherSigner?.address ?? options.authorizedSigner ?? account.addressinside the helper, or asserting the invariant. - ⚡
sessionContextSchema.voucherSigner: z.custom<viem_Account>()(src/tempo/client/Session.ts:31): the custom check is empty, so context-suppliedvoucherSigneris not shape-validated. Safe today because allmi.context.parse(context)call-sites feed local code, but the schema is fragile if any future transport round-tripscontextfrom network input. - ⚡
normalizeVoucherSignaturesilent-pass behavior: ifSignatureEnvelope.from(signature)throws for an unrecognized format, the function returns the original bytes unchanged. Combined with the server never re-canonicalizing inbound bytes, the SDK could emit non-canonical bytes that are parseable elsewhere. Consider failing closed. - ⚡
signVoucherDigestshortcut: the newif (account.sign) return account.sign({ hash })path bypasses viem'ssignTypedDataaction and any EIP-1271 / EIP-6492 / smart-account wrapper logic on aLocalAccount-shaped object. Fine today but a footgun for integrators with custom wrappers. - ⚡ Access-key voucher signing removed: the prior
signVoucher(client, accessKeyAccount, …, authorizedSigner = accessKeyAddress)flow now throws via the newaccessKeyAddressguard. Confirm no downstream consumer still wires an access-key account intosession().
b82f50f to
617ac5a
Compare
617ac5a to
14f5bca
Compare
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
dddb595 to
d378cf5
Compare
Summary
Fixes Tempo session voucher signing for access-key accounts and tightens Tempo charge source handling.
Motivation
Access-key session vouchers need to sign the voucher digest as the access-key address. viem 2.51.0 added
sign({ hash, raw: true }), which lets mppx produce escrow-compatible raw signatures without requiring callers to pass a separate direct signer.Changes
authorizedSigneroptions with method-levelvoucherSigneraccounts insrc/tempo/client/Session.tsandsrc/tempo/client/SessionManager.ts; session context no longer accepts a voucher signer.src/tempo/session/Voucher.tsto use raw access-key signing, unwrap legacy secp256k1 keychain signatures, canonicalize envelopes, and reject non-secp256k1 voucher signatures until TIP-1020 escrow verification is enabled.src/tempo/internal/account.ts.src/tempo/client/Charge.tsto require a chain ID from the challenge or client and use canonical proof sources.viemto 2.51.0 and keptoxon 0.14.24 with the strict workspace dependency policy.Testing
./node_modules/.bin/vp test src/tempo/client/Session.test.ts src/tempo/client/SessionManager.test.ts src/tempo/client/ChannelOps.test.ts src/tempo/client/Charge.test.ts src/tempo/session/Voucher.test.ts./node_modules/.bin/vp test src/tempo/session/Voucher.test.ts src/tempo/client/Session.test.ts src/tempo/client/SessionManager.test.ts src/tempo/client/ChannelOps.test.ts(4 files, 72 tests)CI=true pnpm check:typespnpm checkgit diff --check