Skip to content

fix(sdk-core): enforce recipient verification in EdDSA TSS signing#9071

Open
mrdanish26 wants to merge 1 commit into
masterfrom
WCN-196/security-issue
Open

fix(sdk-core): enforce recipient verification in EdDSA TSS signing#9071
mrdanish26 wants to merge 1 commit into
masterfrom
WCN-196/security-issue

Conversation

@mrdanish26

@mrdanish26 mrdanish26 commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

TICKET: WCN-196

Problem

EdDSA TSS signing paths have no recipient verification before signing:

  • eddsa.ts (MPC v1): No verifyTransaction call at all. After resolving the
    unsigned tx, the code proceeds directly to MPC signing rounds with zero verification
    that the transaction recipients match the intent.

  • eddsaMPCv2.ts (MPC v2): Has a verifyTransaction call but uses
    params.txParams || { recipients: [] }, which always falls back to an empty
    recipients array — making the recipient check in verifyTransaction a no-op.

This means an attacker who can manipulate the txPrebuild (the actual transaction bytes)
can substitute different recipient addresses without detection on any EdDSA coin
(SOL, ADA, SUI, TON, DOT, APT, NEAR, CANTON, IOTA, TAO).

This is the EdDSA counterpart to the ECDSA fix in #8924.

Fix

  • eddsa.ts: Add verifyTransaction call with resolveEffectiveTxParams to resolve
    recipients from txRequest.intent (server-side truth) and verify them against the
    txPrebuild before signing.

  • eddsaMPCv2.ts: Replace params.txParams || { recipients: [] } with
    resolveEffectiveTxParams(txRequest, params.txParams) so the existing
    verifyTransaction call receives actual recipients for comparison.

  • recipientUtils.ts: Add EdDSA-specific no-recipient transaction types to
    NO_RECIPIENT_TX_TYPES: closeAssociatedTokenAccount (SOL),
    voteDelegation (ADA), transferAcknowledge (CANTON).

Verification

  • Checked EdDSA intent types verified against production logs entries
  • recipientUtils unit tests passing (12/12).

@linear-code

linear-code Bot commented Jun 19, 2026

Copy link
Copy Markdown

WCN-196

@mrdanish26 mrdanish26 force-pushed the WCN-196/security-issue branch from 5c3c229 to 4a14137 Compare June 19, 2026 18:51
@mrdanish26 mrdanish26 marked this pull request as ready for review June 19, 2026 19:20
@mrdanish26 mrdanish26 requested review from a team as code owners June 19, 2026 19:20

@mohammadalfaiyazbitgo mohammadalfaiyazbitgo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, but we should add some unit tests

@mrdanish26 mrdanish26 force-pushed the WCN-196/security-issue branch 2 times, most recently from 089706d to 9da68df Compare June 19, 2026 20:06
@zahin-mohammad

Copy link
Copy Markdown
Contributor

Review: blocking concern — NO_RECIPIENT_TX_TYPES has drifted from the authoritative list in mpcUtils.ts

The approach (EdDSA counterpart to #8924) is sound and the diff is clean — verifyTransaction is correctly scoped to the RequestType.tx branch and message signing is excluded. But I believe the no-recipient allowlist is incomplete, and as a result this PR will cause resolveEffectiveTxParams to throw for several Canton intent types that sign successfully today.

The problem

resolveEffectiveTxParams throws (recipientUtils.ts:113-117) when a tx has no resolvable recipients AND no stakingRequestId AND an intentType not in NO_RECIPIENT_TX_TYPES.

There is already an authoritative no-recipient list in mpcUtils.ts:204-224 — the set of intentTypes exempt from the assert(params.recipients, ...) at build time. Cross-referencing it against the set this PR ships, these no-recipient Canton intent types (all built in wallet.ts:4319-4442, all EdDSA) are missing from NO_RECIPIENT_TX_TYPES:

  • cosignDelegationAccept
  • allocationAllocate
  • allocationAllocateWithdrawn
  • cantonEndInvestorOnboardingOffer
  • cantonEndInvestorOnboardingAccept
  • cantonEndInvestorOnboardingReject
  • cantonParticipantOnboardingRequest

Why it throws (regression, not a no-op)

For these types: mpcUtils.ts:226-260 never sets recipients, so intent.recipients is undefined at sign time → resolveEffectiveTxParams resolves recipients to undefined. They carry no stakingRequestId, and the intentType isn't in the set → throw.

Pre-PR, eddsaMPCv2.ts:437 passed { recipients: [] } straight to canton.verifyTransaction, which early-return trues for exactly these types (canton.ts:126-137) → signing succeeds today. This PR moves the gate earlier, so it now throws before verifyTransaction is ever called. eddsa.ts (MPCv1) had no call at all, so it regresses too.

allocationAllocateWithdrawn is the clearest case — it's in the mpcUtils.ts exempt list and just got a builder days ago (CHALO-685, b57f0bf on master), yet is absent here.

Suggested fix

Derive NO_RECIPIENT_TX_TYPES from — or reconcile it against — the mpcUtils.ts:204-224 list, rather than maintaining two hand-curated lists that have already drifted. At minimum, add the 7 types above.

Separately (pre-existing, from #8924, not introduced here but adjacent): NO_RECIPIENT_TX_TYPES has defiApprove/defiDeposit (camelCase) while the actual intentTypes are defi-approve/defi-deposit (wallet.ts:4556,4571, mpcUtils.ts:222-223) — those entries never match.

One thing to confirm

The SDK-side chain above is fully verifiable in this repo. The remaining inference is the server contract: that WP returns intent.intentType verbatim and doesn't populate intent.recipients for these types. Every SDK signal points to it, but a production Loki query of EdDSA intentTypes (à la the table in #8924) would close it definitively — and the verification note here ("Checked EdDSA intent types verified against production logs entries") would benefit from being that explicit, given a missing entry means customers can't sign legitimate transactions.

Questions

  1. Did the production-log audit cover all EdDSA intentTypes across SOL/ADA/SUI/TON/DOT/APT/NEAR/CANTON/IOTA/TAO, or a subset? Can you share the intentType → count table?
  2. Were the Canton cosignDelegation* / allocation* / *Onboarding* flows considered? They're no-recipient per mpcUtils.ts:204-224 but absent from the set.
  3. Do EdDSA staking intents (DOT bond, SOL/NEAR/SUI/ADA stake) reliably carry stakingRequestId? The throw-bypass for staking depends on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants