From 6d30f56c14e0370be3823e7f5522660921e9e86b Mon Sep 17 00:00:00 2001 From: Marzooqa Naeema Kather Date: Thu, 27 Aug 2026 15:33:50 +0530 Subject: [PATCH] fix(sdk-coin-ada,sdk-coin-iota): validate precomputed EdDSA signing material Discovered while manually verifying ADA/IOTA non-BitGo recovery against Wallet Recovery Wizard on staging: WRW's generic Electron 'recover' IPC handler always passes a second positional argument (openSSLBytes, meant only for EVM-like coins) to every coin's recover() call. Ada.recover() and Iota.recover() accept an optional second positional parameter of their own (precomputedMaterial, used by recoverConsolidations() to avoid re-decrypting the keycard per scanned index), so that ArrayBuffer was silently misinterpreted as signing material, causing: JSON.parse(signingMaterial.userPrv) -> JSON.parse(undefined) -> SyntaxError: "undefined" is not valid JSON Add a shared isEddsaSigningMaterial() type guard to sdk-core (next to EddsaSigningMaterial/getEddsaSigningMaterial/signEddsaMpcV2RecoveryTx) and use it in Ada/Iota's signRecoveryTransaction() before trusting a caller-supplied precomputedMaterial, falling back to real detection otherwise. This mirrors the guard sdk-coin-dot already had in place when it added the same optimization, which is why DOT was unaffected. Verified end-to-end: ADA and IOTA non-BitGo recovery via WRW against staging now build and broadcast signed transactions successfully. TICKET: WCI-1460 --- modules/sdk-coin-ada/src/ada.ts | 6 ++- modules/sdk-coin-ada/test/unit/ada.ts | 26 ++++++++++ modules/sdk-coin-iota/src/iota.ts | 6 ++- modules/sdk-coin-iota/test/unit/iota.ts | 36 ++++++++++++++ .../src/bitgo/utils/tss/eddsa/eddsaMPCv2.ts | 26 ++++++++++ modules/sdk-core/src/index.ts | 1 + .../unit/bitgo/utils/tss/eddsa/eddsaMPCv2.ts | 48 +++++++++++++++++++ 7 files changed, 145 insertions(+), 4 deletions(-) diff --git a/modules/sdk-coin-ada/src/ada.ts b/modules/sdk-coin-ada/src/ada.ts index 520b75dee5..81107a899d 100644 --- a/modules/sdk-coin-ada/src/ada.ts +++ b/modules/sdk-coin-ada/src/ada.ts @@ -34,6 +34,7 @@ import { extractCommonKeychain, TssVerifyAddressOptions, getEddsaSigningMaterial as sharedGetEddsaSigningMaterial, + isEddsaSigningMaterial, signEddsaMpcV2RecoveryTx, EddsaSigningMaterial, decryptKeychainPrivateKey, @@ -459,8 +460,9 @@ export class Ada extends BaseCoin { const userKey = params.userKey.replace(/\s/g, ''); const backupKey = params.backupKey.replace(/\s/g, ''); const adaKeyPair = new AdaKeyPair({ pub: accountId }); - const signingMaterial = - precomputedMaterial ?? (await this.getEddsaSigningMaterial(userKey, params.walletPassphrase)); + const signingMaterial = isEddsaSigningMaterial(precomputedMaterial) + ? precomputedMaterial + : await this.getEddsaSigningMaterial(userKey, params.walletPassphrase); if (signingMaterial.version === 'v2') { const signature = await this.signAdaMpcV2Recovery({ diff --git a/modules/sdk-coin-ada/test/unit/ada.ts b/modules/sdk-coin-ada/test/unit/ada.ts index 2e6a21295b..2ce5222ac7 100644 --- a/modules/sdk-coin-ada/test/unit/ada.ts +++ b/modules/sdk-coin-ada/test/unit/ada.ts @@ -900,6 +900,32 @@ describe('ADA', function () { sandBox.assert.calledOnce(getEddsaMaterialSpy); }); + it('should ignore a malformed precomputedMaterial and detect signing material itself (regression: WRW openSSLBytes collision)', async function () { + // WRW's generic Electron 'recover' IPC handler passes openSSLBytes (an ArrayBuffer) + // as a second positional argument to every coin's recover(), regardless of whether + // that coin uses it. Previously this was blindly trusted as precomputedMaterial, + // producing `JSON.parse(undefined)` -> SyntaxError: "undefined" is not valid JSON. + const getEddsaMaterialSpy = sandBox.spy( + basecoin as unknown as { getEddsaSigningMaterial: unknown }, + 'getEddsaSigningMaterial' + ); + + const res = await basecoin.recover( + { + userKey: mpcV2UserKey, + backupKey: mpcV2BackupKey, + bitgoKey: mpcV2CommonKeyChain, + walletPassphrase, + recoveryDestination: destAddr, + }, + new ArrayBuffer(8) as any + ); + + res.should.not.be.empty(); + res.should.hasOwnProperty('serializedTx'); + sandBox.assert.calledOnce(getEddsaMaterialSpy); + }); + it('should route to MPCv1 path when keycard is MPCv1 (regression)', async function () { callBack .withArgs('address_info', { _addresses: [wrwUser.walletAddress0] }) diff --git a/modules/sdk-coin-iota/src/iota.ts b/modules/sdk-coin-iota/src/iota.ts index c741b1988a..7cef60cda8 100644 --- a/modules/sdk-coin-iota/src/iota.ts +++ b/modules/sdk-coin-iota/src/iota.ts @@ -7,6 +7,7 @@ import { EddsaSigningMaterial, Environments, getEddsaSigningMaterial as sharedGetEddsaSigningMaterial, + isEddsaSigningMaterial, KeyPair, MPCAlgorithm, MPCConsolidationRecoveryOptions, @@ -857,8 +858,9 @@ export class Iota extends BaseCoin { const backupKey = params.backupKey.replace(/\s/g, ''); const bitgoKey = params.bitgoKey.replace(/\s/g, ''); - const signingMaterial = - precomputedMaterial ?? (await this.getEddsaSigningMaterial(userKey, params.walletPassphrase)); + const signingMaterial = isEddsaSigningMaterial(precomputedMaterial) + ? precomputedMaterial + : await this.getEddsaSigningMaterial(userKey, params.walletPassphrase); let signatureBuffer: Buffer; diff --git a/modules/sdk-coin-iota/test/unit/iota.ts b/modules/sdk-coin-iota/test/unit/iota.ts index f33659d54d..19c815f1e9 100644 --- a/modules/sdk-coin-iota/test/unit/iota.ts +++ b/modules/sdk-coin-iota/test/unit/iota.ts @@ -774,6 +774,42 @@ describe('IOTA:', function () { sandBox.assert.notCalled(getTSSSignatureSpy); }); + it('should ignore a malformed precomputedMaterial and detect signing material itself (regression: WRW openSSLBytes collision)', async function () { + // WRW's generic Electron 'recover' IPC handler passes openSSLBytes (an ArrayBuffer) + // as a second positional argument to every coin's recover(), regardless of whether + // that coin uses it. Previously this was blindly trusted as precomputedMaterial, + // producing `JSON.parse(undefined)` -> SyntaxError: "undefined" is not valid JSON. + sandBox.stub(Iota.prototype, 'fetchOwnedObjects' as keyof Iota).resolves([ + { + objectId: '0xc05c765e26e6ae84c78fa245f38a23fb20406a5cf3f61b57bd323a0df9d98003', + version: '195', + digest: validDigest, + balance: '1900000000', + }, + ]); + sandBox.stub(Iota.prototype, 'fetchGasPrice' as keyof Iota).resolves(1000); + sandBox.stub(Iota.prototype, 'estimateGas' as keyof Iota).resolves(1997880); + const getEddsaMaterialSpy = sandBox.spy( + basecoin as unknown as { getEddsaSigningMaterial: unknown }, + 'getEddsaSigningMaterial' + ); + + const res = await basecoin.recover( + { + userKey: mpcV2UserKey, + backupKey: mpcV2BackupKey, + bitgoKey: mpcV2CommonKeyChain, + recoveryDestination, + walletPassphrase, + }, + new ArrayBuffer(8) as any + ); + + res.should.not.be.empty(); + res.should.hasOwnProperty('transactions'); + sandBox.assert.calledOnce(getEddsaMaterialSpy); + }); + it('should throw missing userKey error on MPCv2 path', async function () { sandBox.stub(Iota.prototype, 'fetchOwnedObjects' as keyof Iota).resolves([ { diff --git a/modules/sdk-core/src/bitgo/utils/tss/eddsa/eddsaMPCv2.ts b/modules/sdk-core/src/bitgo/utils/tss/eddsa/eddsaMPCv2.ts index a58a38348b..f993e2a2d5 100644 --- a/modules/sdk-core/src/bitgo/utils/tss/eddsa/eddsaMPCv2.ts +++ b/modules/sdk-core/src/bitgo/utils/tss/eddsa/eddsaMPCv2.ts @@ -1341,6 +1341,31 @@ export async function getEddsaSigningMaterial( return userPrv !== null ? { version: 'v1', userPrv } : { version: 'v2', encryptedUserKey: normalized }; } +/** + * Type guard validating that a value has the shape of EddsaSigningMaterial. + * + * Coin `recover()` implementations accept an optional precomputed signing-material + * parameter (set by recoverConsolidations() to avoid re-decrypting the keycard per + * scanned index). Some callers pass an unrelated second positional argument to + * recover() for other purposes (e.g. openSSLBytes for EVM-like coins); validating + * the shape here prevents that from being misinterpreted as signing material. + */ +export function isEddsaSigningMaterial(value: unknown): value is EddsaSigningMaterial { + if (typeof value !== 'object' || value === null) { + return false; + } + if (!('version' in value)) { + return false; + } + if (value.version === 'v1') { + return 'userPrv' in value && typeof value.userPrv === 'string'; + } + if (value.version === 'v2') { + return 'encryptedUserKey' in value && typeof value.encryptedUserKey === 'string'; + } + return false; +} + /** * Full MPCv2 recovery signing flow: decrypt key shares → validate commonKeyChain → MPS DSG. * Returns raw 64-byte Ed25519 signature Buffer. @@ -1373,5 +1398,6 @@ export const EddsaMPCv2RecoveryFunctions = { getEddsaMpcV2RecoveryKeySharesFromReducedKey, signRecoveryEddsaMPCv2, getEddsaSigningMaterial, + isEddsaSigningMaterial, signEddsaMpcV2RecoveryTx, }; diff --git a/modules/sdk-core/src/index.ts b/modules/sdk-core/src/index.ts index 4995fdb494..318ac244a4 100644 --- a/modules/sdk-core/src/index.ts +++ b/modules/sdk-core/src/index.ts @@ -34,6 +34,7 @@ export { EddsaMPCv2Utils }; export type { EddsaSigningMaterial } from './bitgo/utils/tss/eddsa/eddsaMPCv2'; export { getEddsaSigningMaterial, + isEddsaSigningMaterial, signEddsaMpcV2RecoveryTx, isEddsaMpcV1SigningMaterial, getEddsaMpcV2RecoveryKeySharesFromReducedKey, diff --git a/modules/sdk-core/test/unit/bitgo/utils/tss/eddsa/eddsaMPCv2.ts b/modules/sdk-core/test/unit/bitgo/utils/tss/eddsa/eddsaMPCv2.ts index 8ffa19fe8c..c9d0d5e1f1 100644 --- a/modules/sdk-core/test/unit/bitgo/utils/tss/eddsa/eddsaMPCv2.ts +++ b/modules/sdk-core/test/unit/bitgo/utils/tss/eddsa/eddsaMPCv2.ts @@ -29,6 +29,7 @@ import { SignatureShareType, TxRequest, getEddsaSigningMaterial, + isEddsaSigningMaterial, signEddsaMpcV2RecoveryTx, } from '../../../../../../src'; import { @@ -2078,6 +2079,53 @@ describe('getEddsaSigningMaterial', () => { }); }); +describe('isEddsaSigningMaterial', () => { + it('returns true for a valid v1 shape', () => { + assert.strictEqual(isEddsaSigningMaterial({ version: 'v1', userPrv: '{}' }), true); + }); + + it('returns true for a valid v2 shape', () => { + assert.strictEqual(isEddsaSigningMaterial({ version: 'v2', encryptedUserKey: 'abc123' }), true); + }); + + it('returns false for a v1 shape with a non-string userPrv', () => { + assert.strictEqual(isEddsaSigningMaterial({ version: 'v1', userPrv: 123 }), false); + }); + + it('returns false for a v2 shape with a non-string encryptedUserKey', () => { + assert.strictEqual(isEddsaSigningMaterial({ version: 'v2', encryptedUserKey: 123 }), false); + }); + + it('returns false for an unrecognized version value', () => { + assert.strictEqual(isEddsaSigningMaterial({ version: 'v3', userPrv: '{}' }), false); + }); + + it('returns false for an object missing a version field', () => { + assert.strictEqual(isEddsaSigningMaterial({ userPrv: '{}' }), false); + }); + + it('returns false for null', () => { + assert.strictEqual(isEddsaSigningMaterial(null), false); + }); + + it('returns false for undefined', () => { + assert.strictEqual(isEddsaSigningMaterial(undefined), false); + }); + + it('returns false for primitive values', () => { + assert.strictEqual(isEddsaSigningMaterial('a string'), false); + assert.strictEqual(isEddsaSigningMaterial(42), false); + assert.strictEqual(isEddsaSigningMaterial(true), false); + }); + + it('returns false for an ArrayBuffer (regression: WRW openSSLBytes collision)', () => { + // WRW's generic Electron 'recover' IPC handler passes openSSLBytes as a second + // positional argument to every coin's recover(), colliding with precomputedMaterial + // on coins that accept it. This must be rejected rather than misread as signing material. + assert.strictEqual(isEddsaSigningMaterial(new ArrayBuffer(8)), false); + }); +}); + describe('signEddsaMpcV2RecoveryTx', () => { const derivationPath = 'm/0/0'; const walletPassphrase = 'testPass';