fix(sdk-core): wire resolveEffectiveTxParams into EddsaMPCv2Utils (WCI-1111) - #9394
Conversation
4807ac9 to
394740d
Compare
7e84969 to
1da40ae
Compare
1da40ae to
eaae085
Compare
eaae085 to
ab7b2e9
Compare
ab7b2e9 to
63e90fc
Compare
63e90fc to
691bdbf
Compare
691bdbf to
ce54229
Compare
ce54229 to
0490122
Compare
0490122 to
e2ca6c4
Compare
e2ca6c4 to
f4c6b60
Compare
f4c6b60 to
0c44ba0
Compare
0c44ba0 to
9a3286b
Compare
9a3286b to
9d6b87c
Compare
0db73c7 to
71040ea
Compare
71040ea to
3c040fc
Compare
9ba6e8d to
3b026eb
Compare
3b026eb to
18f65dd
Compare
18f65dd to
cff0118
Compare
4b4f135 to
a70696a
Compare
a70696a to
9cb52b1
Compare
7183351 to
7322e7d
Compare
7322e7d to
8e11038
Compare
Replace the unsafe `params.txParams || { recipients: [] }` fallback in
`EddsaMPCv2Utils.signRequestBase` with `resolveEffectiveTxParams`, which
throws `InvalidTransactionError` when recipients cannot be resolved and
the intent is not a known no-recipient type. This is the remediation for
Trail of Bits finding TOB-BITGOEDMPC-1 (WCI-1100): the EdDSA MPCv2
re-sign path previously substituted an empty-recipients object, allowing
`verifyTransaction` to be bypassed on coins that only validate when
recipients are non-empty (SOL, VET, Tempo, TRON).
ECDSA already used this pattern (ecdsaMPCv2.ts:958,965); this ports it
to EdDSA MPCv2. MPCv1 (eddsa.ts) is explicitly out of scope.
Also removes the now-redundant EdDSA MPCv2 pre-fetch block from
`wallet.ts:signTransactionTss` and the matching pre-derivation in
`BaseTssUtils.recreateTxRequest`, along with the `txParamsFromIntent`
helper function they both called. `resolveEffectiveTxParams` inside
`signRequestBase` now owns intent-based recipient derivation, so the
wallet layer no longer needs to pre-derive before calling `signTxRequest`.
Ticket: WCI-1111
Session-Id: 1c178dac-6528-4ee7-937d-974216871d68
Task-Id: e91df1ba-6cf4-4b0c-8df2-2588f555481e
8e11038 to
1ecc1f5
Compare
f524fa0 to
85b5bb8
Compare
85b5bb8 to
ddc5199
Compare
SOL authorize stake account operations (intentType 'authorize') have no explicit transfer recipient. Without this entry, resolveEffectiveTxParams would throw InvalidTransactionError when signing an authorize transaction with no buildParams. Ticket: WCI-1111 Session-Id: 1c178dac-6528-4ee7-937d-974216871d68 Task-Id: e91df1ba-6cf4-4b0c-8df2-2588f555481e
ddc5199 to
fba4bbd
Compare
There was a problem hiding this comment.
Requesting changes specifically on the bare 'authorize' allowlist entry in NO_RECIPIENT_TX_TYPES.
The EdDSA resolveEffectiveTxParams wiring looks correct for WCI-1111.
Removing |
abhijit0943
left a comment
There was a problem hiding this comment.
Discussed offline - Approving.
Agree with keeping authorize on the no-recipient allowlist for now so we don't break that signing path in this PR. That matches current behavior; proper intent-field validation for authorize belongs in a follow-up (coin-layer verifyTransaction), as noted in WCI-1110 / discussion here.
Please open a follow-up ticket for that and link it on this PR before merge if possible. The EdDSA resolveEffectiveTxParams wiring LGTM for WCI-1111.
Ticket -> WCI-1243 |
What
signRequestBase: replaced the vulnerableparams.txParams || { recipients: [] }fallback withresolveEffectiveTxParams(txRequest, params.txParams, this.baseCoin.getChain()). The function throwsInvalidTransactionErrorwhen recipients cannot be resolved and the intent is not a recognised no-recipient type.signTransactionTss: removed the EdDSA MPCv2 special-case block that pre-fetched the txRequest and calledtxParamsFromIntentbefore handing off tosignTxRequest. That pre-fetch was introduced to work around the missing guard; now thatresolveEffectiveTxParamsowns intent-based derivation insidesignRequestBase(which already fetches the txRequest internally when given a string ID), the wallet-layer duplication is redundant. Removed the now-unusedtxParamsFromIntentimport.resolveEffectiveTxParamsguard suite tosignTxRequest.tscovering:stakingAuthorizeattack vector (no txParams, non-allowlisted intent → throws)txParamswith non-allowlisted intent → throwsdeactivateintentType with no txParams → succeedsconsolidateintentType with no txParams → succeedsstakingRequestIdsignal → succeedsWhy
Trail of Bits finding TOB-BITGOEDMPC-1 (WCI-1100): the EdDSA MPCv2 re-sign path silently substituted an empty-recipients object when
txParamswas absent. Several coin-levelverifyTransactionimplementations (SOL, VET, Tempo, TRON) skip output-matching validation whenrecipients.length === 0, allowing a compromised BitGo server to present a malicioustxHexthat signs without any client-side validation.ECDSA already used
resolveEffectiveTxParamsfor fail-closed behaviour (ecdsaMPCv2.ts:958,965andecdsa.ts:821,828). This PR ports the same pattern to EdDSA MPCv2. MPCv1 (eddsa.ts) is explicitly out of scope per the ticket.Depends on WCI-1110 (base branch
wci-1110-fix-resolve-effective-tx-params-sol), which fixesresolveEffectiveTxParamsto preservetokenNamefor SOL token transfers and adds thedeactivate/delegate/closeAssociatedTokenAccountallowlist entries needed by EdDSA coins.Test plan
signTxRequestthrowsInvalidTransactionErrorforstakingAuthorizeintent with no recipients and no txParamssignTxRequestthrows for emptytxParams.recipientswith non-allowlisted intentsignTxRequestsucceeds fordeactivateandconsolidate(allowlisted no-recipient intents)signTxRequestsucceeds when intent provides recipients and txParams is absentsignTxRequestsucceeds for staking intents withstakingRequestIdTicket: WCI-1111