From b2832ac1515d3872f993c494775d9e7fbf2d7723 Mon Sep 17 00:00:00 2001 From: Pranish Nepal Date: Fri, 30 Oct 2026 10:45:19 -0400 Subject: [PATCH 1/2] feat: accept Safe keycard params-EVM TSS recovery Safe wallets derive one child key set from a 4-slot keycard, which buildSafeRecoverKeyParams maps into coin.recover(). Four coin families either rejected that parameter shape or skipped the error contracts the Safe recovery orchestrator relies on to decide whether to advance, retry or abort. abstract-eth: unsigned TSS sweeps accept a bitgoKey-only shape alongside the legacy userKey/backupKey pair, and verify the derived m/0 base address against walletContractAddress before any balance query. MPCV2 coins now dispatch to buildUnsignedSweepTxnMPCv2 from the base class, fixing bsc, arbeth, opeth and bera, which declare the feature but inherited the MPCv1 body and threw "Unsupported public key" on any common keychain. sdk-coin-stx: derive the 2-of-3 multisig address from the three child pubs and compare it to rootAddress, with zero network calls. sdk-coin-avaxc: assert the user and backup keys are signers on the wallet contract before spending gas, via a JSON-RPC queryIsWalletSigner override, since avax.network is not Etherscan-compatible. abstract-cosmos: derive senderAddress from bitgoKey in unsigned mode rather than trusting rootAddress, and compare the two in both modes, ignoring an optional ?memoId= suffix and bech32 casing. RecoverOptions.userKey and .backupKey become optional so the unsigned Safe shape type-checks. Every downstream consumer still classifies sweep and KRS recovery from the raw keys, as before, so that detection is unchanged. Two intended behaviour changes for existing callers: - Unsigned EVM TSS sweeps throw RecoveryKeyMismatchError when walletContractAddress is not the m/0 base address. The signed path already enforced this, and WRW labels the field "This is also the wallet's base address". Sweeps using a derivationSeed are exempt: they target a derived child no caller can predict. - AVAXC recovery throws ErrorNoInputToRecover on a zero balance instead of building a zero-value transaction, matching eth Adds end-to-end Safe recovery suites for eth, stx, avaxc and cosmos covering signed and unsigned happy paths, wrong passphrase, empty balance and provider failure, plus a bsc test pinning the MPCv2 dispatch TICKET: WCN-3946 --- modules/abstract-cosmos/src/cosmosCoin.ts | 48 ++- modules/abstract-cosmos/src/lib/constants.ts | 2 + .../src/abstractEthLikeNewCoins.ts | 133 +++++-- modules/abstract-eth/src/ethLikeToken.ts | 12 +- .../test/unit/cosmosSafeRecovery.ts | 246 +++++++++++++ modules/sdk-coin-avaxc/src/avaxc.ts | 48 ++- .../sdk-coin-avaxc/test/resources/avaxc.ts | 8 + modules/sdk-coin-avaxc/test/unit/avaxc.ts | 16 +- .../test/unit/avaxcSafeRecovery.ts | 316 +++++++++++++++++ modules/sdk-coin-bsc/package.json | 1 + .../test/unit/bscUnsignedSweepTss.ts | 77 ++++ modules/sdk-coin-etc/src/etc.ts | 11 +- modules/sdk-coin-eth/package.json | 2 + modules/sdk-coin-eth/src/erc20Token.ts | 18 +- modules/sdk-coin-eth/src/erc721Token.ts | 18 +- modules/sdk-coin-eth/src/erc7984Token.ts | 16 +- modules/sdk-coin-eth/src/eth.ts | 13 +- modules/sdk-coin-eth/test/unit/eth.ts | 2 +- .../sdk-coin-eth/test/unit/ethSafeRecovery.ts | 335 ++++++++++++++++++ modules/sdk-coin-evm/src/evmCoin.ts | 10 - modules/sdk-coin-islm/test/unit/islm.ts | 6 +- modules/sdk-coin-polygon/test/unit/polygon.ts | 2 +- modules/sdk-coin-stx/src/stx.ts | 21 +- modules/sdk-coin-stx/test/fixtures.ts | 3 + modules/sdk-coin-stx/test/unit/stx.ts | 5 +- .../sdk-coin-stx/test/unit/stxSafeRecovery.ts | 229 ++++++++++++ modules/sdk-coin-tempo/src/tempo.ts | 9 +- 27 files changed, 1502 insertions(+), 105 deletions(-) create mode 100644 modules/sdk-coin-atom/test/unit/cosmosSafeRecovery.ts create mode 100644 modules/sdk-coin-avaxc/test/unit/avaxcSafeRecovery.ts create mode 100644 modules/sdk-coin-bsc/test/unit/bscUnsignedSweepTss.ts create mode 100644 modules/sdk-coin-eth/test/unit/ethSafeRecovery.ts create mode 100644 modules/sdk-coin-stx/test/unit/stxSafeRecovery.ts diff --git a/modules/abstract-cosmos/src/cosmosCoin.ts b/modules/abstract-cosmos/src/cosmosCoin.ts index 9271a2e5754..e282f50452e 100644 --- a/modules/abstract-cosmos/src/cosmosCoin.ts +++ b/modules/abstract-cosmos/src/cosmosCoin.ts @@ -15,6 +15,7 @@ import { multisigTypes, ParsedTransaction, ParseTransactionOptions, + RecoveryKeyMismatchError, RecoveryProviderError, SignedTransaction, SigningError, @@ -48,7 +49,7 @@ import { TransactionBuildParams, BalanceResult, } from './lib'; -import { ROOT_PATH } from './lib/constants'; +import { COMPRESSED_PUBLIC_KEY_LENGTH, ROOT_PATH } from './lib/constants'; import utils from './lib/utils'; import { auditEcdsaPrivateKey } from '@bitgo/sdk-lib-mpc'; @@ -212,21 +213,44 @@ export class CosmosCoin extends BaseCoin { }> { const MPC = new Ecdsa(); + let publicKey: string; + let keyShares: KeyShares | undefined; + if (isUnsignedSweep) { - return { - senderAddress: params.rootAddress as string, - publicKey: MPC.deriveUnhardened(params.bitgoKey || '', ROOT_PATH).slice(0, 66), - }; + const bitgoKey = (params.bitgoKey || '').replace(/\s/g, ''); + if (!bitgoKey) { + throw new Error('missing bitgoKey'); + } + publicKey = this.derivePublicKey(MPC, bitgoKey); + } else { + keyShares = await this.getKeyShares(params); + publicKey = this.derivePublicKey(MPC, keyShares.commonKeyChain); } - const keyShares = await this.getKeyShares(params); - const publicKey = MPC.deriveUnhardened(keyShares.commonKeyChain, ROOT_PATH).slice(0, 66); + const senderAddress = this.getAddressFromPublicKey(publicKey); + if (params.rootAddress && !this.isSameBaseAddress(senderAddress, params.rootAddress)) { + throw new RecoveryKeyMismatchError( + `derived address ${senderAddress} does not match rootAddress ${params.rootAddress}` + ); + } - return { - senderAddress: this.getAddressFromPublicKey(publicKey), - publicKey, - keyShares, - }; + return { senderAddress, publicKey, keyShares }; + } + + /** + * Compares two cosmos addresses ignoring an optional `?memoId=` suffix and bech32 casing, + * both of which `isValidAddress` accepts on a rootAddress. + */ + private isSameBaseAddress(a: string, b: string): boolean { + const base = (address: string) => address.split('?memoId=')[0].toLowerCase(); + return base(a) === base(b); + } + + /** + * Derives the compressed public key of the wallet's root address from a common keychain + */ + private derivePublicKey(MPC: Ecdsa, commonKeyChain: string): string { + return MPC.deriveUnhardened(commonKeyChain, ROOT_PATH).slice(0, COMPRESSED_PUBLIC_KEY_LENGTH); } /** diff --git a/modules/abstract-cosmos/src/lib/constants.ts b/modules/abstract-cosmos/src/lib/constants.ts index caf466a9303..4c9f1e6e0c6 100644 --- a/modules/abstract-cosmos/src/lib/constants.ts +++ b/modules/abstract-cosmos/src/lib/constants.ts @@ -9,4 +9,6 @@ export const groupProposalMsgTypeUrl = '/cosmos.group.v1.MsgSubmitProposal'; export const groupVoteMsgTypeUrl = '/cosmos.group.v1.MsgVote'; export const UNAVAILABLE_TEXT = 'UNAVAILABLE'; export const ROOT_PATH = 'm/0'; +/** length in hex characters of a compressed secp256k1 public key */ +export const COMPRESSED_PUBLIC_KEY_LENGTH = 66; export const sendMsgType = '/types.MsgSend'; // thorchain uses this custom message type diff --git a/modules/abstract-eth/src/abstractEthLikeNewCoins.ts b/modules/abstract-eth/src/abstractEthLikeNewCoins.ts index 820369009ff..f3520f31893 100644 --- a/modules/abstract-eth/src/abstractEthLikeNewCoins.ts +++ b/modules/abstract-eth/src/abstractEthLikeNewCoins.ts @@ -284,8 +284,12 @@ export type NonTSSRecoverOptions = { export type TSSRecoverOptions = RecoverOptionsWithBytes | NonTSSRecoverOptions; export type RecoverOptions = { - userKey: string; - backupKey: string; + /** [encrypted] xprv, xpub or TSS key share. Omitted by unsigned TSS sweeps which pass `bitgoKey` instead. */ + userKey?: string; + /** [encrypted] xprv, xpub or TSS key share. Omitted by unsigned TSS sweeps which pass `bitgoKey` instead. */ + backupKey?: string; + /** TSS common keychain, the only key an unsigned Safe sweep supplies */ + bitgoKey?: string; walletPassphrase?: string; walletContractAddress: string; // use this as walletBaseAddress for TSS recoveryDestination: string; @@ -1343,21 +1347,26 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin implem * @returns {void} */ validateRecoveryParams(params: RecoverOptions): void { - if (params.userKey === undefined) { - throw new Error('missing userKey'); - } + // Unsigned TSS sweeps (Safe recovery) supply the common keychain as `bitgoKey` alone + const isBitgoKeyOnlyTss = params.isTss === true && !params.userKey && !params.backupKey && !!params.bitgoKey; - if (params.backupKey === undefined) { - throw new Error('missing backupKey'); - } + if (!isBitgoKeyOnlyTss) { + if (params.userKey === undefined) { + throw new Error('missing userKey'); + } - if ( - !params.isUnsignedSweep && - params.walletPassphrase === undefined && - !params.userKey.startsWith('xpub') && - !params.isTss - ) { - throw new Error('missing wallet passphrase'); + if (params.backupKey === undefined) { + throw new Error('missing backupKey'); + } + + if ( + !params.isUnsignedSweep && + params.walletPassphrase === undefined && + !params.userKey.startsWith('xpub') && + !params.isTss + ) { + throw new Error('missing wallet passphrase'); + } } if (params.walletContractAddress === undefined || !this.isValidAddress(params.walletContractAddress)) { @@ -1548,13 +1557,19 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin implem for (let i = startIdx; i < endIdx; i++) { const consolidationAddress = this.getConsolidationAddress(params, i); for (const address of consolidationAddress) { + // A TSS recovery sweeps the m/0 address derived from the common keychain and never reads + // walletContractAddress, so the forwarder address was never the address being swept here. + // Pass the address actually swept, which is also what the key mismatch check compares. + const recoverWalletContractAddress = params.isTss + ? this.deriveAddressFromPublicKey(this.getTssCommonKeychain(params), 0) + : address; const recoverParams = { apiKey: params.apiKey, backupKey: params.backupKey || '', gasLimit: params.gasLimit, recoveryDestination: params.recoveryDestination || '', userKey: params.userKey || '', - walletContractAddress: address, + walletContractAddress: recoverWalletContractAddress, derivationSeed: '', isTss: params.isTss, eip1559: { @@ -1565,7 +1580,7 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin implem chain: params.replayProtectionOptions?.chain || 0, hardfork: params.replayProtectionOptions?.hardfork || 'london', }, - bitgoKey: '', + bitgoKey: params.bitgoKey || '', ignoreAddressTypes: [], }; let recoveryTransaction; @@ -1620,11 +1635,18 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin implem } this.validateRecoveryParams(params); - const isUnsignedSweep = params.isUnsignedSweep ?? getIsUnsignedSweep(params); + // validateRecoveryParams guarantees both keys are present outside of unsigned TSS sweeps, + // which never reach this non-TSS path. Classify from the RAW keys, as before the type change: + // stripping whitespace first would flip the result for a key pasted with a leading newline. + const rawUserKey = params.userKey ?? ''; + const rawBackupKey = params.backupKey ?? ''; + const isUnsignedSweep = + params.isUnsignedSweep ?? + getIsUnsignedSweep({ userKey: rawUserKey, backupKey: rawBackupKey, isTss: params.isTss }); // Clean up whitespace from entered values - let userKey = params.userKey.replace(/\s/g, ''); - const backupKey = params.backupKey.replace(/\s/g, ''); + let userKey = rawUserKey.replace(/\s/g, ''); + const backupKey = rawBackupKey.replace(/\s/g, ''); const gasLimit = new optionalDeps.ethUtil.BN(this.setGasLimit(params.gasLimit)); const gasPrice = params.eip1559 ? new optionalDeps.ethUtil.BN(params.eip1559.maxFeePerGas) @@ -1898,7 +1920,7 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin implem this.validateEvmBasedRecoveryParams(params); // Clean up whitespace from entered values - const userKey = params.userKey.replace(/\s/g, ''); + const userKey = (params.userKey ?? '').replace(/\s/g, ''); const bitgoFeeAddress = params.bitgoFeeAddress?.replace(/\s/g, '').toLowerCase() as string; const bitgoDestinationAddress = params.bitgoDestinationAddress?.replace(/\s/g, '').toLowerCase() as string; const recoveryDestination = params.recoveryDestination?.replace(/\s/g, '').toLowerCase() as string; @@ -2396,8 +2418,8 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin implem ): Promise { this.validateRecoveryParams(params); // Clean up whitespace from entered values - const userPublicOrPrivateKeyShare = params.userKey.replace(/\s/g, ''); - const backupPrivateOrPublicKeyShare = params.backupKey.replace(/\s/g, ''); + const userPublicOrPrivateKeyShare = (params.userKey ?? '').replace(/\s/g, ''); + const backupPrivateOrPublicKeyShare = (params.backupKey ?? '').replace(/\s/g, ''); if ( getIsUnsignedSweep({ @@ -2421,11 +2443,7 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin implem const derivedCommonKeyChain = MPC.deriveUnhardened(commonKeyChain, 'm/0'); const backupKeyPair = new KeyPairLib({ pub: derivedCommonKeyChain.slice(0, 66) }); const baseAddress = backupKeyPair.getAddress(); - if (baseAddress.toLowerCase() !== params.walletContractAddress.toLowerCase()) { - throw new RecoveryKeyMismatchError( - `derived wallet base address ${baseAddress} does not match the given wallet contract address ${params.walletContractAddress}` - ); - } + this.assertTssBaseAddressMatchesWallet(baseAddress, params); const unsignedTx = (await this.buildTssRecoveryTxn(baseAddress, gasPrice, gasLimit, params)).tx; const messageHash = unsignedTx.getMessageToSign(true); const signature = await ECDSAUtils.signRecoveryMpcV2(messageHash, userKeyShare, backupKeyShare, commonKeyChain); @@ -2447,20 +2465,56 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin implem return { gasLimit, gasPrice }; } + /** + * Resolves the TSS common keychain for a recovery. Legacy callers put it in `backupKey` (and + * `userKey`); Safe unsigned sweeps supply it as `bitgoKey` alone. + * @param {RecoverOptions} params + * @returns {string} the common keychain, stripped of whitespace + */ + protected getTssCommonKeychain(params: { userKey?: string; backupKey?: string; bitgoKey?: string }): string { + return (params.backupKey || params.userKey || params.bitgoKey || '').replace(/\s/g, ''); + } + + /** + * Fails closed when the address derived from the TSS common keychain is not the wallet being + * recovered, before any balance or fee query is made. + * @param {string} baseAddress - address derived from the common keychain + * @param {RecoverOptions} params + */ + protected assertTssBaseAddressMatchesWallet(baseAddress: string, params: RecoverOptions): void { + // This guard is the check itself: without an address to compare against there is nothing + // stopping a wrong keychain, so refuse rather than skip. + if (!params.walletContractAddress) { + throw new Error('invalid walletContractAddress'); + } + if (baseAddress.toLowerCase() !== params.walletContractAddress.toLowerCase()) { + throw new RecoveryKeyMismatchError( + `derived wallet base address ${baseAddress} does not match the given wallet contract address ${params.walletContractAddress}` + ); + } + } + protected async buildUnsignedSweepTxnTSS(params: RecoverOptions): Promise { - const userPublicOrPrivateKeyShare = params.userKey.replace(/\s/g, ''); - const backupPrivateOrPublicKeyShare = params.backupKey.replace(/\s/g, ''); + // MPCv2 coins derive the base address from the common keychain. Not every MPCv2 coin overrides + // this method, so dispatch on the coin feature here rather than relying on each subclass. + if (this.staticsCoin?.features.includes(CoinFeature.MPCV2)) { + return this.buildUnsignedSweepTxnMPCv2(params); + } + + const commonKeyChain = this.getTssCommonKeychain(params); + const userPublicOrPrivateKeyShare = (params.userKey ?? '').replace(/\s/g, '') || commonKeyChain; const { gasLimit, gasPrice } = await this.getGasValues(params); - const backupKeyPair = new KeyPairLib({ pub: backupPrivateOrPublicKeyShare }); + const backupKeyPair = new KeyPairLib({ pub: commonKeyChain }); const baseAddress = backupKeyPair.getAddress(); + this.assertTssBaseAddressMatchesWallet(baseAddress, params); const { txInfo, tx, nonce } = await this.buildTssRecoveryTxn(baseAddress, gasPrice, gasLimit, params); return this.formatForOfflineVaultTSS( txInfo, tx, userPublicOrPrivateKeyShare, - backupPrivateOrPublicKeyShare, + commonKeyChain, gasPrice, gasLimit, nonce, @@ -2473,13 +2527,20 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin implem const { gasLimit, gasPrice } = await this.getGasValues(params); const recoverParams = params as RecoverOptions; - this.validateUnsignedSweepTSSParams(recoverParams); + await this.validateUnsignedSweepTSSParams(recoverParams); + const commonKeyChain = this.getTssCommonKeychain(recoverParams); const derivationPath = recoverParams.derivationSeed ? getDerivationPath(recoverParams.derivationSeed) : 'm/0'; const MPC = new Ecdsa(); - const derivedCommonKeyChain = MPC.deriveUnhardened(recoverParams.backupKey as string, derivationPath); + const derivedCommonKeyChain = MPC.deriveUnhardened(commonKeyChain, derivationPath); const backupKeyPair = new KeyPairLib({ pub: derivedCommonKeyChain.slice(0, 66) }); const baseAddress = backupKeyPair.getAddress(); + // Only the m/0 base address is one the caller can know in advance and pass as + // walletContractAddress. A derivationSeed deliberately sweeps a pseudorandom child + // (m/999999/x/y), so there is nothing meaningful to compare it against. + if (!recoverParams.derivationSeed) { + this.assertTssBaseAddressMatchesWallet(baseAddress, params); + } const { txInfo, tx, nonce } = await this.buildTssRecoveryTxn(baseAddress, gasPrice, gasLimit, params); return this.buildTxRequestForOfflineVaultMPCv2( txInfo, @@ -2490,7 +2551,7 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin implem gasLimit, params.eip1559, params.replayProtectionOptions, - recoverParams.backupKey as string + commonKeyChain ); } @@ -2562,7 +2623,7 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin implem * @returns {void} */ private async validateUnsignedSweepTSSParams(params: RecoverOptions): Promise { - if (_.isUndefined(params.backupKey) && params.backupKey === '') { + if (this.getTssCommonKeychain(params) === '') { throw new Error('missing commonKeyChain'); } if (!_.isUndefined(params.derivationSeed) && typeof params.derivationSeed !== 'string') { diff --git a/modules/abstract-eth/src/ethLikeToken.ts b/modules/abstract-eth/src/ethLikeToken.ts index 0f2e9a9aab2..78a42d9afeb 100644 --- a/modules/abstract-eth/src/ethLikeToken.ts +++ b/modules/abstract-eth/src/ethLikeToken.ts @@ -213,11 +213,17 @@ export class EthLikeToken extends AbstractEthLikeNewCoins { throw new Error('invalid recoveryDestination'); } - const isUnsignedSweep = params.isUnsignedSweep ?? getIsUnsignedSweep(params); + // Classify from the RAW keys, as before the type change: stripping whitespace first would + // flip the result for a key pasted with a leading newline. + const rawUserKey = params.userKey ?? ''; + const rawBackupKey = params.backupKey ?? ''; + const isUnsignedSweep = + params.isUnsignedSweep ?? + getIsUnsignedSweep({ userKey: rawUserKey, backupKey: rawBackupKey, isTss: params.isTss }); // Clean up whitespace from entered values - let userKey = params.userKey.replace(/\s/g, ''); - const backupKey = params.backupKey.replace(/\s/g, ''); + let userKey = rawUserKey.replace(/\s/g, ''); + const backupKey = rawBackupKey.replace(/\s/g, ''); const gasLimit = new optionalDeps.ethUtil.BN(this.setGasLimit(params.gasLimit)); const gasPrice = params.eip1559 diff --git a/modules/sdk-coin-atom/test/unit/cosmosSafeRecovery.ts b/modules/sdk-coin-atom/test/unit/cosmosSafeRecovery.ts new file mode 100644 index 00000000000..a6ea40a0d35 --- /dev/null +++ b/modules/sdk-coin-atom/test/unit/cosmosSafeRecovery.ts @@ -0,0 +1,246 @@ +import should from 'should'; +import nock from 'nock'; +import { DklsUtils, DklsVrfUtils } from '@bitgo/sdk-lib-mpc'; +import { + buildSafeRecoverKeyParams, + deriveSafeWalletKeys, + Ecdsa, + Environments, + ErrorNoInputToRecover, + RecoveryKeyMismatchError, + RecoveryProviderError, + type DecryptedSafeRoots, + type EcdsaMpcChildKeys, +} from '@bitgo/sdk-core'; +import { TestBitGo, TestBitGoAPI } from '@bitgo/sdk-test'; +import { BitGoAPI } from '@bitgo/sdk-api'; +import { Tatom } from '../../src'; + +const recoveryDestination = 'cosmos1smefkq9yaxgw9c9fhym6qsp746q884czeacn32'; +const chainId = 'theta-testnet-001'; + +/** + * End-to-end Safe Cosmos recovery: derive slot-3 (ecdsaMpc) children from real DKG roots, map them + * with buildSafeRecoverKeyParams, and prove `CosmosCoin.recover()` derives the sender from the keys + * in both modes and refuses to touch a wallet those keys do not control. + * + * Two gaps are pinned here. In unsigned mode `getSenderDetails` used to trust `params.rootAddress` + * verbatim as the sender while deriving a public key it never compared against it — so a wrong + * keycard swept whatever address the caller named. And neither mode checked `rootAddress` at all, + * so a wrong passphrase (which still yields a valid, different common keychain) surfaced as + * "no balance" rather than as a key mismatch. + */ +describe('Safe Cosmos recovery end-to-end', function () { + let bitgo: TestBitGoAPI; + let basecoin: Tatom; + // The configured node URL has a trailing slash, so requests land on `//cosmos/...`; resolve the + // real origin and path so nock matches what superagent actually sends. + let nodeOrigin: string; + let nodePath: (path: string) => string; + + let safeKeys: EcdsaMpcChildKeys; + let otherKeys: EcdsaMpcChildKeys; + let rootAddress: string; + + before(async function () { + bitgo = TestBitGo.decorate(BitGoAPI, { env: 'test' }); + bitgo.safeRegister('tatom', Tatom.createInstance); + bitgo.initializeTestVars(); + basecoin = bitgo.coin('tatom') as Tatom; + const nodeUrl = Environments[bitgo.getEnv()].atomNodeUrl; + nodeOrigin = new URL(nodeUrl).origin; + nodePath = (path: string) => new URL(nodeUrl + path).pathname; + + safeKeys = await deriveEcdsaMpcKeys(); + otherKeys = await deriveEcdsaMpcKeys(); + rootAddress = senderAddressOf(safeKeys.commonKeychain); + }); + + afterEach(function () { + nock.cleanAll(); + }); + + /** Real DKG + VRF roots, exactly as a decrypted Safe keycard would provide for the ecdsaMpc slot. */ + async function ecdsaMpcRoots(): Promise { + const [userDkg, backupDkg] = await DklsUtils.generateDKGKeyShares(); + const [vrfUser, vrfBackup] = await DklsVrfUtils.generateVrfDKGKeyShares(); + return { + secp256k1Multisig: { user: 'u', backup: 'b', bitgo: 'g' }, + ed25519Multisig: { user: 'u', backup: { prv: 'b', chainCode: 'cc' }, bitgo: 'g' }, + ecdsaMpc: { + user: { signing: userDkg.getReducedKeyShare(), vrf: vrfUser.getKeyShare() }, + backup: { signing: backupDkg.getReducedKeyShare(), vrf: vrfBackup.getKeyShare() }, + bitgo: 'g', + }, + eddsaMpc: { + user: { signing: Buffer.alloc(0), vrf: Buffer.alloc(0) }, + backup: { signing: Buffer.alloc(0), vrf: Buffer.alloc(0) }, + bitgo: 'g', + }, + }; + } + + async function deriveEcdsaMpcKeys(): Promise { + const derived = await deriveSafeWalletKeys({ + coin: 'tatom', + slot: 'ecdsaMpc', + account: 0, + roots: await ecdsaMpcRoots(), + }); + if (derived.slot !== 'ecdsaMpc') { + throw new Error('expected ecdsaMpc slot'); + } + return derived.keys; + } + + /** The wallet's address: the `m/0` child of the common keychain, as getSenderDetails derives it. */ + function senderAddressOf(commonKeychain: string): string { + return basecoin.getAddressFromPublicKey(new Ecdsa().deriveUnhardened(commonKeychain, 'm/0').slice(0, 66)); + } + + function keyParams(keys: EcdsaMpcChildKeys, mode: 'signed' | 'unsigned') { + return buildSafeRecoverKeyParams({ bitgo, coin: 'tatom', slot: 'ecdsaMpc', keys, mode }); + } + + // comfortably above GAS_AMOUNT so processBalances has something left to sweep + const FUNDED_BALANCE = [{ denom: 'uatom', amount: '10000000' }]; + + function stubNodeQueries(address: string, balances = FUNDED_BALANCE) { + nock(nodeOrigin) + .get(nodePath('/cosmos/base/tendermint/v1beta1/blocks/latest')) + .reply(200, { block: { header: { chain_id: chainId } } }); + nock(nodeOrigin) + .get(nodePath(`/cosmos/auth/v1beta1/accounts/${address}`)) + .reply(200, { account: { account_number: '12345', sequence: '7' } }); + nock(nodeOrigin) + .get(nodePath(`/cosmos/bank/v1beta1/balances/${address}`)) + .reply(200, { balances }); + } + + it('recovers a Safe Cosmos wallet in unsigned mode, deriving the sender from bitgoKey', async function () { + const params = await keyParams(safeKeys, 'unsigned'); + if (params.mode !== 'unsigned') { + throw new Error('expected unsigned key params'); + } + stubNodeQueries(rootAddress); + + const recovery = await basecoin.recover({ ...params, rootAddress, recoveryDestination }); + + should.exist(recovery.signableHex); + const signableHex = recovery.signableHex as string; + // The sender baked into the unsigned payload is the address derived from bitgoKey. + signableHex.should.containEql(Buffer.from(rootAddress).toString('hex')); + signableHex.should.containEql(Buffer.from(recoveryDestination).toString('hex')); + nock.pendingMocks().should.have.length(0); + }); + + it('recovers a Safe Cosmos wallet in unsigned mode when no rootAddress is supplied', async function () { + const params = await keyParams(safeKeys, 'unsigned'); + stubNodeQueries(rootAddress); + + const recovery = await basecoin.recover({ ...params, recoveryDestination }); + + should.exist(recovery.signableHex); + const signableHex = recovery.signableHex as string; + // Without a rootAddress the derived sender is the only address queried, which is what the + // mocks are keyed on: an undefined sender would have left them pending. + signableHex.should.containEql(Buffer.from(rootAddress).toString('hex')); + nock.pendingMocks().should.have.length(0); + }); + + it('recovers a Safe Cosmos wallet in signed mode via the real MPCv2 signing path', async function () { + const params = await keyParams(safeKeys, 'signed'); + if (params.mode !== 'signed') { + throw new Error('expected signed key params'); + } + stubNodeQueries(rootAddress); + + const recovery = await basecoin.recover({ ...params, rootAddress, recoveryDestination }); + + should.exist(recovery); + recovery.should.hasOwnProperty('serializedTx'); + }); + + it('fails closed with RecoveryKeyMismatchError in unsigned mode on a wrong passphrase', async function () { + // A wrong passphrase decrypts to a valid but different keycard, so the derived sender is not + // the wallet being recovered. + const params = await keyParams(otherKeys, 'unsigned'); + stubNodeQueries(rootAddress); + + let caughtError: unknown; + try { + await basecoin.recover({ ...params, rootAddress, recoveryDestination }); + } catch (e) { + caughtError = e; + } + + (caughtError as Error).should.be.instanceOf(RecoveryKeyMismatchError); + (caughtError as Error).message.should.match(/does not match rootAddress/); + // No node call was made: the derivation and comparison are entirely offline. + nock.pendingMocks().should.have.length(3); + }); + + it('fails closed with RecoveryKeyMismatchError in signed mode too', async function () { + const params = await keyParams(otherKeys, 'signed'); + stubNodeQueries(rootAddress); + + let caughtError: unknown; + try { + await basecoin.recover({ ...params, rootAddress, recoveryDestination }); + } catch (e) { + caughtError = e; + } + + (caughtError as Error).should.be.instanceOf(RecoveryKeyMismatchError); + nock.pendingMocks().should.have.length(3); + }); + + it('reports an empty wallet as ErrorNoInputToRecover once the keys do match', async function () { + const params = await keyParams(safeKeys, 'unsigned'); + stubNodeQueries(rootAddress, []); + + let caughtError: unknown; + try { + await basecoin.recover({ ...params, rootAddress, recoveryDestination }); + } catch (e) { + caughtError = e; + } + + (caughtError as Error).should.be.instanceOf(ErrorNoInputToRecover); + (caughtError as Error).should.not.be.instanceOf(RecoveryKeyMismatchError); + }); + + it('surfaces a dead node as RecoveryProviderError, not as an empty wallet', async function () { + const params = await keyParams(safeKeys, 'unsigned'); + nock(nodeOrigin) + .get(nodePath('/cosmos/base/tendermint/v1beta1/blocks/latest')) + .reply(200, { block: { header: { chain_id: chainId } } }); + nock(nodeOrigin) + .get(nodePath(`/cosmos/auth/v1beta1/accounts/${rootAddress}`)) + .reply(200, { account: { account_number: '12345', sequence: '7' } }); + nock(nodeOrigin) + .get(nodePath(`/cosmos/bank/v1beta1/balances/${rootAddress}`)) + .reply(500, { error: 'node unavailable' }); + + let caughtError: unknown; + try { + await basecoin.recover({ ...params, rootAddress, recoveryDestination }); + } catch (e) { + caughtError = e; + } + + (caughtError as Error).should.be.instanceOf(RecoveryProviderError); + (caughtError as Error).should.not.be.instanceOf(ErrorNoInputToRecover); + }); + + it('rejects an unsigned recovery with no bitgoKey instead of deriving from an empty string', async function () { + let caughtError: unknown; + try { + await basecoin.recover({ bitgoKey: ' ', recoveryDestination }); + } catch (e) { + caughtError = e; + } + + (caughtError as Error).message.should.equal('missing bitgoKey'); + }); +}); diff --git a/modules/sdk-coin-avaxc/src/avaxc.ts b/modules/sdk-coin-avaxc/src/avaxc.ts index f82999ab3ea..4057fcfaa09 100644 --- a/modules/sdk-coin-avaxc/src/avaxc.ts +++ b/modules/sdk-coin-avaxc/src/avaxc.ts @@ -19,6 +19,7 @@ import { BaseTransaction, BitGoBase, common, + ErrorNoInputToRecover, FeeEstimateOptions, FullySignedTransaction, getIsUnsignedSweep, @@ -425,6 +426,35 @@ export class AvaxC extends AbstractEthLikeNewCoins { return new optionalDeps.ethUtil.BN(tokenBalanceHex.slice(2), 16); } + /** + * Queries the wallet contract (via avax.network) for whether an address is one of its signers. + * Overridden because avax.network speaks JSON-RPC 2.0 rather than the Etherscan REST API the + * base class queries. + * @param {string} walletContractAddress - address of the wallet contract + * @param {string} signerAddress - address to test for membership + * @returns {Promise} true if the address is a signer on the wallet contract + */ + async queryIsWalletSigner(walletContractAddress: string, signerAddress: string, apiKey?: string): Promise { + const signerData = optionalDeps.ethUtil.addHexPrefix( + this.getMethodCallData('isSigner', ['address'], [signerAddress]).toString('hex') + ); + const result = await this.recoveryBlockchainExplorerQuery( + { + jsonrpc: '2.0', + method: 'eth_call', + params: [{ to: walletContractAddress, data: signerData }, 'latest'], + id: 1, + }, + apiKey + ); + if (!result || !result.result || optionalDeps.ethUtil.stripHexPrefix(result.result).length === 0) { + throw new RecoveryProviderError( + `Could not read the signer set of wallet contract ${walletContractAddress} from avax.network, got: ${result?.result}` + ); + } + return optionalDeps.ethAbi.rawDecode(['bool'], optionalDeps.ethUtil.toBuffer(result.result))[0] === true; + } + /** * Queries the contract (via avax.network) for the next sequence ID * @param {string} address - address of the contract @@ -629,12 +659,16 @@ export class AvaxC extends AbstractEthLikeNewCoins { throw new Error('invalid recoveryDestination'); } + // Classify from the RAW keys, as before the type change: stripping whitespace first would + // flip the result for a key pasted with a leading newline. + const rawUserKey = params.userKey ?? ''; + const rawBackupKey = params.backupKey ?? ''; // TODO (BG-56531): add support for krs - const isUnsignedSweep = getIsUnsignedSweep(params); + const isUnsignedSweep = getIsUnsignedSweep({ userKey: rawUserKey, backupKey: rawBackupKey, isTss: params.isTss }); // Clean up whitespace from entered values - let userKey = params.userKey.replace(/\s/g, ''); - const backupKey = params.backupKey.replace(/\s/g, ''); + let userKey = rawUserKey.replace(/\s/g, ''); + const backupKey = rawBackupKey.replace(/\s/g, ''); // Set new tx fees (using default config values from platform) const gasLimit = new optionalDeps.ethUtil.BN(this.setGasLimit(params.gasLimit)); @@ -677,6 +711,8 @@ export class AvaxC extends AbstractEthLikeNewCoins { } backupKeyAddress = keyPair.getAddress(); } + await this.assertRecoveryKeysAreWalletSigners(userKey, backupKeyAddress, params.walletContractAddress); + const backupKeyNonce = await this.getAddressNonce(backupKeyAddress); // get balance of backupKey to ensure funds are available to pay fees @@ -703,6 +739,12 @@ export class AvaxC extends AbstractEthLikeNewCoins { txAmount = await this.queryAddressBalance(params.walletContractAddress); } + if (txAmount.lten(0)) { + throw new ErrorNoInputToRecover( + 'There is no balance of this coin or token to recover in this wallet: ' + params.walletContractAddress + ); + } + // build recipients object const recipients = [ { diff --git a/modules/sdk-coin-avaxc/test/resources/avaxc.ts b/modules/sdk-coin-avaxc/test/resources/avaxc.ts index ae75a15126e..4b5217ec4ab 100644 --- a/modules/sdk-coin-avaxc/test/resources/avaxc.ts +++ b/modules/sdk-coin-avaxc/test/resources/avaxc.ts @@ -149,6 +149,13 @@ const backupAddressBalanceResponse = { jsonrpc: '2.0', id: 1, result: '0x9c450d5 const addressTokenBalanceResponse = { jsonrpc: '2.0', id: 1, result: '0x14d1120d7b160000' }; +/** abi-encoded `true` — the wallet contract reports the queried address as a signer */ +const isSignerResponse = { + jsonrpc: '2.0', + id: 1, + result: '0x0000000000000000000000000000000000000000000000000000000000000001', +}; + const sequenceIdResponse1 = { jsonrpc: '2.0', id: 1, @@ -166,6 +173,7 @@ export const endpointResponses = { addressNonceResponse2, backupAddressBalanceResponse, addressTokenBalanceResponse, + isSignerResponse, sequenceIdResponse1, sequenceIdResponse2, }; diff --git a/modules/sdk-coin-avaxc/test/unit/avaxc.ts b/modules/sdk-coin-avaxc/test/unit/avaxc.ts index 98826a5b7fc..02ee49dc609 100644 --- a/modules/sdk-coin-avaxc/test/unit/avaxc.ts +++ b/modules/sdk-coin-avaxc/test/unit/avaxc.ts @@ -1074,9 +1074,23 @@ describe('Avalanche C-Chain', function () { // contract address for 'tavaxc:link' const tokenContractAddress = '0x0b9d5d9136855f6fec3c0993fee6e9ce8a297846'; const sequenceIdData = '0xa0b7967b'; + const isSignerSelector = optionalDeps.ethUtil.addHexPrefix( + optionalDeps.ethAbi.methodID('isSigner', ['address']).toString('hex') + ); const sandBox = sinon.createSandbox(); beforeEach(function () { const callBack = sandBox.stub(AvaxC.prototype, 'recoveryBlockchainExplorerQuery' as keyof AvaxC); + // every recovery first checks that the user and backup keys are signers on the wallet contract + callBack + .withArgs( + sinon.match( + (query) => + query?.method === 'eth_call' && + typeof query?.params?.[0]?.data === 'string' && + query.params[0].data.startsWith(isSignerSelector) + ) + ) + .resolves(endpointResponses.isSignerResponse); callBack .withArgs({ jsonrpc: '2.0', @@ -1116,7 +1130,7 @@ describe('Avalanche C-Chain', function () { params: ['0xe0b1fe098050f2745b450de419b5cafc7e826699', 'latest'], id: 1, }) - .resolves({ jsonrpc: '2.0', id: 1, result: '0x0' }); + .resolves({ jsonrpc: '2.0', id: 1, result: '0x9c450d592ee800' }); callBack .withArgs({ jsonrpc: '2.0', diff --git a/modules/sdk-coin-avaxc/test/unit/avaxcSafeRecovery.ts b/modules/sdk-coin-avaxc/test/unit/avaxcSafeRecovery.ts new file mode 100644 index 00000000000..c0548cc5c33 --- /dev/null +++ b/modules/sdk-coin-avaxc/test/unit/avaxcSafeRecovery.ts @@ -0,0 +1,316 @@ +import should from 'should'; +import nock from 'nock'; +import { bip32 } from '@bitgo/secp256k1'; +import { + buildSafeRecoverKeyParams, + common, + deriveSafeWalletKeys, + ErrorNoInputToRecover, + RecoveryKeyMismatchError, + RecoveryProviderError, + type DecryptedSafeRoots, + type Secp256k1MultisigChildKeys, +} from '@bitgo/sdk-core'; +import { optionalDeps } from '@bitgo/abstract-eth'; +import { TestBitGo, TestBitGoAPI } from '@bitgo/sdk-test'; +import { BitGoAPI } from '@bitgo/sdk-api'; +import { TavaxC, KeyPair as AvaxcKeyPair } from '../../src'; + +const walletContractAddress = '0xe0b1fe098050f2745b450de419b5cafc7e826699'; +const recoveryDestination = '0x94b51ebb8c3b90404ad262f42c1588bd94242ecb'; +const rpcUrl = common.Environments.test.avaxcNetworkBaseUrl as string; +const rpcPath = '/ext/bc/C/rpc'; + +const isSignerSelector = optionalDeps.ethAbi.methodID('isSigner', ['address']).toString('hex'); +const nextSequenceIdSelector = optionalDeps.ethAbi.methodID('getNextSequenceId', []).toString('hex'); + +const ENCODED_TRUE = '0x' + '0'.repeat(63) + '1'; +const ENCODED_FALSE = '0x' + '0'.repeat(64); + +/** + * End-to-end Safe AVAXC recovery: derive slot-1 (secp256k1Multisig) children from real roots, map + * them with buildSafeRecoverKeyParams, and prove `AvaxC.recover()` honours the same three contracts + * the base EVM class does. + * + * `AvaxC` overrides `recover()` rather than reusing `recoverEthLike`, so before WCN-3946 it skipped + * `assertRecoveryKeysAreWalletSigners` entirely and happily built a zero-value sweep of an empty + * wallet. It also speaks JSON-RPC 2.0 to `/ext/bc/C/rpc` rather than the Etherscan REST API, so the + * inherited `queryIsWalletSigner` could not have worked here even if it had been called. + */ +describe('Safe AVAXC recovery end-to-end', function () { + let bitgo: TestBitGoAPI; + let basecoin: TavaxC; + + let safeKeys: Secp256k1MultisigChildKeys; + let otherKeys: Secp256k1MultisigChildKeys; + + before(async function () { + bitgo = TestBitGo.decorate(BitGoAPI, { env: 'test' }); + bitgo.safeRegister('tavaxc', TavaxC.createInstance); + bitgo.initializeTestVars(); + basecoin = bitgo.coin('tavaxc') as TavaxC; + + safeKeys = await deriveSecp256k1MultisigKeys([1, 2, 3]); + otherKeys = await deriveSecp256k1MultisigKeys([4, 5, 6]); + }); + + afterEach(function () { + nock.cleanAll(); + }); + + /** Distinct secp256k1 root triplets so user, backup and bitgo children are different keys. */ + function secp256k1Roots([u, b, g]: [number, number, number]): DecryptedSafeRoots { + const root = (fill: number) => bip32.fromSeed(Buffer.alloc(32, fill)); + return { + secp256k1Multisig: { + user: root(u).toBase58(), + backup: root(b).toBase58(), + bitgo: root(g).neutered().toBase58(), + }, + ed25519Multisig: { user: 'u', backup: { prv: 'b', chainCode: 'cc' }, bitgo: 'g' }, + ecdsaMpc: { + user: { signing: Buffer.alloc(0), vrf: Buffer.alloc(0) }, + backup: { signing: Buffer.alloc(0), vrf: Buffer.alloc(0) }, + bitgo: 'g', + }, + eddsaMpc: { + user: { signing: Buffer.alloc(0), vrf: Buffer.alloc(0) }, + backup: { signing: Buffer.alloc(0), vrf: Buffer.alloc(0) }, + bitgo: 'g', + }, + }; + } + + async function deriveSecp256k1MultisigKeys(seeds: [number, number, number]): Promise { + const derived = await deriveSafeWalletKeys({ + coin: 'tavaxc', + slot: 'secp256k1Multisig', + account: 0, + roots: secp256k1Roots(seeds), + }); + if (derived.slot !== 'secp256k1Multisig') { + throw new Error('expected secp256k1Multisig slot'); + } + return derived.keys; + } + + function keyParams(keys: Secp256k1MultisigChildKeys, mode: 'signed' | 'unsigned') { + return buildSafeRecoverKeyParams({ bitgo, coin: 'tavaxc', slot: 'secp256k1Multisig', keys, mode }); + } + + function addressOf(xpub: string): string { + return new AvaxcKeyPair({ pub: xpub }).getAddress().toLowerCase(); + } + + type RpcRequest = { method: string; params: unknown[] }; + type RpcOverrides = { + /** EOA addresses the wallet contract reports as signers; everything else answers false. */ + signers?: string[]; + /** wei balance of the wallet contract being swept */ + walletBalance?: string; + /** wei balance of the backup key EOA, which pays the gas */ + backupBalance?: string; + }; + + /** Records every JSON-RPC method the recovery actually asked for, in order. */ + let rpcCalls: RpcRequest[]; + + function stubRpc(overrides: RpcOverrides = {}): void { + const { signers = [], walletBalance = '0x9c450d592ee800', backupBalance = '0x9c450d592ee800' } = overrides; + rpcCalls = []; + nock(rpcUrl) + .persist() + .post(rpcPath) + .reply(200, (uri, body) => { + const request = body as RpcRequest & { id: number }; + rpcCalls.push({ method: request.method, params: request.params }); + return { jsonrpc: '2.0', id: request.id, result: rpcResult(request, signers, walletBalance, backupBalance) }; + }); + } + + function rpcResult( + request: RpcRequest, + signers: string[], + walletBalance: string, + backupBalance: string + ): string | undefined { + const [first] = request.params as [{ to?: string; data?: string } | string]; + if (request.method === 'eth_getTransactionCount') { + return '0x7'; + } + if (request.method === 'eth_getBalance') { + return (first as string).toLowerCase() === walletContractAddress ? walletBalance : backupBalance; + } + if (request.method === 'eth_call') { + const data = optionalDeps.ethUtil.stripHexPrefix((first as { data: string }).data); + if (data.startsWith(isSignerSelector)) { + // the probed address is the last 20 bytes of the single abi-encoded argument + const probed = `0x${data.slice(-40)}`.toLowerCase(); + return signers.includes(probed) ? ENCODED_TRUE : ENCODED_FALSE; + } + if (data.startsWith(nextSequenceIdSelector)) { + return '0x0000000000000000000000000000000000000000000000000000000000000003'; + } + } + return undefined; + } + + function signersOf(keys: Secp256k1MultisigChildKeys): string[] { + return [addressOf(keys.user.pub), addressOf(keys.backup.pub)]; + } + + it('recovers a Safe AVAXC wallet in signed mode after verifying both keys are contract signers', async function () { + const params = await keyParams(safeKeys, 'signed'); + stubRpc({ signers: signersOf(safeKeys) }); + + const recovery = await basecoin.recover({ + userKey: params.userKey, + backupKey: params.backupKey, + bitgoKey: params.bitgoKey, + walletPassphrase: params.walletPassphrase, + walletContractAddress, + recoveryDestination, + }); + + should.exist(recovery); + (recovery as { tx: string }).tx.should.be.a.String(); + // The signer check ran before the nonce query, not after it. + rpcCalls[0].method.should.equal('eth_call'); + rpcCalls.findIndex((c) => c.method === 'eth_getTransactionCount').should.be.above(1); + }); + + it('recovers a Safe AVAXC wallet in unsigned mode (xpubs only, no passphrase)', async function () { + const params = await keyParams(safeKeys, 'unsigned'); + should.not.exist(params.walletPassphrase); + stubRpc({ signers: signersOf(safeKeys) }); + + const recovery = await basecoin.recover({ + userKey: params.userKey, + backupKey: params.backupKey, + bitgoKey: params.bitgoKey, + walletContractAddress, + recoveryDestination, + }); + + should.exist(recovery); + (recovery as unknown as { txHex: string }).txHex.should.be.a.String(); + (recovery as unknown as { walletContractAddress: string }).walletContractAddress.should.equal( + walletContractAddress + ); + }); + + it('fails closed with RecoveryKeyMismatchError before any balance query on a wrong passphrase', async function () { + // A wrong passphrase decrypts to a valid but different keycard, whose EOAs are not signers on + // the wallet contract. + const params = await keyParams(otherKeys, 'signed'); + stubRpc({ signers: signersOf(safeKeys) }); + + let caughtError: unknown; + try { + await basecoin.recover({ + userKey: params.userKey, + backupKey: params.backupKey, + bitgoKey: params.bitgoKey, + walletPassphrase: params.walletPassphrase, + walletContractAddress, + recoveryDestination, + }); + } catch (e) { + caughtError = e; + } + + (caughtError as Error).should.be.instanceOf(RecoveryKeyMismatchError); + (caughtError as Error).message.should.match(/are not signers of wallet contract/); + // Only the two isSigner probes were made; no balance or nonce query followed. + rpcCalls.every((c) => c.method === 'eth_call').should.be.true(); + }); + + it('reports an empty wallet as ErrorNoInputToRecover once the keys do check out', async function () { + const params = await keyParams(safeKeys, 'unsigned'); + stubRpc({ signers: signersOf(safeKeys), walletBalance: '0x0' }); + + let caughtError: unknown; + try { + await basecoin.recover({ + userKey: params.userKey, + backupKey: params.backupKey, + bitgoKey: params.bitgoKey, + walletContractAddress, + recoveryDestination, + }); + } catch (e) { + caughtError = e; + } + + (caughtError as Error).should.be.instanceOf(ErrorNoInputToRecover); + (caughtError as Error).message.should.match(/no balance of this coin or token to recover/); + }); + + it('keeps an unfunded backup key a plain error, distinct from an empty wallet', async function () { + const params = await keyParams(safeKeys, 'unsigned'); + stubRpc({ signers: signersOf(safeKeys), backupBalance: '0x0' }); + + let caughtError: unknown; + try { + await basecoin.recover({ + userKey: params.userKey, + backupKey: params.backupKey, + bitgoKey: params.bitgoKey, + walletContractAddress, + recoveryDestination, + }); + } catch (e) { + caughtError = e; + } + + (caughtError as Error).message.should.match(/must have a balance of at least/); + (caughtError as Error).should.not.be.instanceOf(ErrorNoInputToRecover); + (caughtError as Error).should.not.be.instanceOf(RecoveryKeyMismatchError); + }); + + it('surfaces a dead avax.network node as RecoveryProviderError, not as a key mismatch', async function () { + const params = await keyParams(safeKeys, 'unsigned'); + nock(rpcUrl).persist().post(rpcPath).reply(500, 'node unavailable'); + + let caughtError: unknown; + try { + await basecoin.recover({ + userKey: params.userKey, + backupKey: params.backupKey, + bitgoKey: params.bitgoKey, + walletContractAddress, + recoveryDestination, + }); + } catch (e) { + caughtError = e; + } + + (caughtError as Error).should.be.instanceOf(RecoveryProviderError); + (caughtError as Error).should.not.be.instanceOf(RecoveryKeyMismatchError); + (caughtError as Error).should.not.be.instanceOf(ErrorNoInputToRecover); + }); + + it('surfaces an empty isSigner response as RecoveryProviderError rather than assuming not-a-signer', async function () { + const params = await keyParams(safeKeys, 'unsigned'); + nock(rpcUrl) + .persist() + .post(rpcPath) + .reply(200, (uri, body) => ({ jsonrpc: '2.0', id: (body as { id: number }).id, result: '0x' })); + + let caughtError: unknown; + try { + await basecoin.recover({ + userKey: params.userKey, + backupKey: params.backupKey, + bitgoKey: params.bitgoKey, + walletContractAddress, + recoveryDestination, + }); + } catch (e) { + caughtError = e; + } + + (caughtError as Error).should.be.instanceOf(RecoveryProviderError); + (caughtError as Error).message.should.match(/Could not read the signer set/); + }); +}); diff --git a/modules/sdk-coin-bsc/package.json b/modules/sdk-coin-bsc/package.json index 96569043af9..085a370b6e8 100644 --- a/modules/sdk-coin-bsc/package.json +++ b/modules/sdk-coin-bsc/package.json @@ -51,6 +51,7 @@ "@bitgo/sdk-test": "^9.1.91", "@ethereumjs/rlp": "^4.0.0", "@ethereumjs/tx": "^3.3.0", + "bn.js": "^5.2.1", "ethereumjs-util": "7.1.5" }, "gitHead": "18e460ddf02de2dbf13c2aa243478188fb539f0c", diff --git a/modules/sdk-coin-bsc/test/unit/bscUnsignedSweepTss.ts b/modules/sdk-coin-bsc/test/unit/bscUnsignedSweepTss.ts new file mode 100644 index 00000000000..97cc1c3e08c --- /dev/null +++ b/modules/sdk-coin-bsc/test/unit/bscUnsignedSweepTss.ts @@ -0,0 +1,77 @@ +import should from 'should'; +import sinon from 'sinon'; +import BN from 'bn.js'; +import { TestBitGo, TestBitGoAPI } from '@bitgo/sdk-test'; +import { BitGoAPI } from '@bitgo/sdk-api'; +import { AbstractEthLikeNewCoins, KeyPair as EthKeyPair, UnsignedSweepTxMPCv2 } from '@bitgo/abstract-eth'; +import { Ecdsa, RecoveryKeyMismatchError } from '@bitgo/sdk-core'; +import { Tbsc } from '../../src'; + +/** + * BSC declares CoinFeature.MPCV2 but never overrode `buildUnsignedSweepTxnTSS`, so it inherited + * the MPCv1 body, which fed the 130-char common keychain straight into `new KeyPair({ pub })` and + * threw `Unsupported public key` — an unsigned TSS sweep was impossible on this coin. + * + * The base class now dispatches MPCV2 coins to `buildUnsignedSweepTxnMPCv2`. These tests pin that + * dispatch for one of the four coins (bsc, arbeth, opeth, bera) that gained it, so the fix cannot + * silently regress. + */ +describe('BSC unsigned TSS sweep (MPCv2 dispatch)', function () { + let bitgo: TestBitGoAPI; + let basecoin: AbstractEthLikeNewCoins; + const sandbox = sinon.createSandbox(); + + // a real MPCv2 common keychain (33-byte pubkey + 32-byte chaincode, hex) + const commonKeyChain = + '0234eb39b22fed523ece7c78da29ba1f1de5b64a6e48013e0914de793bc1df0570' + + 'e779de04758732734d97e54b782c8b336283811af6a2c57bd81438798e1c2446'; + const recoveryDestination = '0xd5ADdE17feD8baed3F32b84AF05B8F2816f7b560'; + let baseAddress: string; + + before(function () { + bitgo = TestBitGo.decorate(BitGoAPI, { env: 'test' }); + bitgo.safeRegister('tbsc', Tbsc.createInstance); + bitgo.initializeTestVars(); + basecoin = bitgo.coin('tbsc') as unknown as AbstractEthLikeNewCoins; + + const pub = new Ecdsa().deriveUnhardened(commonKeyChain, 'm/0').slice(0, 66); + baseAddress = new EthKeyPair({ pub }).getAddress(); + }); + + beforeEach(function () { + sandbox.stub(basecoin, 'queryAddressBalance').resolves(new BN('100000000000000000')); + sandbox.stub(basecoin, 'getAddressNonce').resolves(0); + }); + + afterEach(function () { + sandbox.restore(); + }); + + function recover(walletContractAddress: string) { + return basecoin.recover({ + userKey: commonKeyChain, + backupKey: commonKeyChain, + isTss: true, + walletContractAddress, + recoveryDestination, + gasLimit: 200000, + gasPrice: 20000000000, + // tbsc is not EIP-1559, so a legacy hardfork is required here + replayProtectionOptions: { chain: 97, hardfork: 'petersburg' }, + }); + } + + it('derives the base address from the common keychain instead of rejecting it', async function () { + const sweep = (await recover(baseAddress)) as UnsignedSweepTxMPCv2; + + // The MPCv2 payload shape, not the MPCv1 OfflineVaultTxInfo the broken path would have built. + sweep.txRequests.should.have.length(1); + const unsignedTx = sweep.txRequests[0].transactions[0].unsignedTx; + unsignedTx.derivationPath.should.equal('m/0'); + should.exist(unsignedTx.signableHex); + }); + + it('fails closed when the wallet address is not the one the keychain derives', async function () { + await recover('0xe740412ab65efdbdbe2a5e4c6e01d7bbc54a38a9').should.be.rejectedWith(RecoveryKeyMismatchError); + }); +}); diff --git a/modules/sdk-coin-etc/src/etc.ts b/modules/sdk-coin-etc/src/etc.ts index 2a9ee5eaf1f..484c08b1439 100644 --- a/modules/sdk-coin-etc/src/etc.ts +++ b/modules/sdk-coin-etc/src/etc.ts @@ -79,11 +79,16 @@ export class Etc extends AbstractEthLikeCoin { */ async recover(params: RecoverOptions): Promise { this.validateRecoveryParams(params); - const isUnsignedSweep = getIsUnsignedSweep(params); + + // Classify from the RAW keys, as before the type change: stripping whitespace first would + // flip the result for a key pasted with a leading newline. + const rawUserKey = params.userKey ?? ''; + const rawBackupKey = params.backupKey ?? ''; + const isUnsignedSweep = getIsUnsignedSweep({ userKey: rawUserKey, backupKey: rawBackupKey, isTss: params.isTss }); // Clean up whitespace from entered values - let userKey = params.userKey.replace(/\s/g, ''); - const backupKey = params.backupKey.replace(/\s/g, ''); + let userKey = rawUserKey.replace(/\s/g, ''); + const backupKey = rawBackupKey.replace(/\s/g, ''); const gasLimit = new optionalDeps.ethUtil.BN(this.setGasLimit(params.gasLimit)); const gasPrice = params.eip1559 ? new optionalDeps.ethUtil.BN(params.eip1559.maxFeePerGas) diff --git a/modules/sdk-coin-eth/package.json b/modules/sdk-coin-eth/package.json index 4618c9fc8f3..30888e57b67 100644 --- a/modules/sdk-coin-eth/package.json +++ b/modules/sdk-coin-eth/package.json @@ -56,7 +56,9 @@ }, "devDependencies": { "@bitgo/sdk-api": "^3.1.6", + "@bitgo/sdk-lib-mpc": "^11.6.0", "@bitgo/sdk-test": "^9.1.91", + "@ethereumjs/common": "^2.6.5", "bn.js": "^5.2.1", "tweetnacl": "^1.0.3" }, diff --git a/modules/sdk-coin-eth/src/erc20Token.ts b/modules/sdk-coin-eth/src/erc20Token.ts index f0909455b2d..5a0494becaa 100644 --- a/modules/sdk-coin-eth/src/erc20Token.ts +++ b/modules/sdk-coin-eth/src/erc20Token.ts @@ -152,17 +152,23 @@ export class Erc20Token extends Eth { throw new Error('invalid recoveryDestination'); } - const isKrsRecovery = getIsKrsRecovery(params); - const isUnsignedSweep = params.isUnsignedSweep ?? getIsUnsignedSweep(params); + // Classify from the RAW keys, as before the type change: stripping whitespace first would + // flip the result for a key pasted with a leading newline. + const rawUserKey = params.userKey ?? ''; + const rawBackupKey = params.backupKey ?? ''; + const isKrsRecovery = getIsKrsRecovery({ userKey: rawUserKey, backupKey: rawBackupKey }); + const isUnsignedSweep = + params.isUnsignedSweep ?? + getIsUnsignedSweep({ userKey: rawUserKey, backupKey: rawBackupKey, isTss: params.isTss }); + + // Clean up whitespace from entered values + let userKey = rawUserKey.replace(/\s/g, ''); + const backupKey = rawBackupKey.replace(/\s/g, ''); if (isKrsRecovery) { checkKrsProvider(this, params.krsProvider, { checkCoinFamilySupport: false }); } - // Clean up whitespace from entered values - let userKey = params.userKey.replace(/\s/g, ''); - const backupKey = params.backupKey.replace(/\s/g, ''); - // Set new eth tx fees (default to using platform values if none are provided) const gasPrice = params.eip1559 ? new optionalDeps.ethUtil.BN(params.eip1559.maxFeePerGas) diff --git a/modules/sdk-coin-eth/src/erc721Token.ts b/modules/sdk-coin-eth/src/erc721Token.ts index fb84c9cb511..0c0e0c1575d 100644 --- a/modules/sdk-coin-eth/src/erc721Token.ts +++ b/modules/sdk-coin-eth/src/erc721Token.ts @@ -152,17 +152,23 @@ export class Erc721Token extends Eth { throw new Error('invalid recoveryDestination'); } - const isKrsRecovery = getIsKrsRecovery(params); - const isUnsignedSweep = params.isUnsignedSweep ?? getIsUnsignedSweep(params); + // Classify from the RAW keys, as before the type change: stripping whitespace first would + // flip the result for a key pasted with a leading newline. + const rawUserKey = params.userKey ?? ''; + const rawBackupKey = params.backupKey ?? ''; + const isKrsRecovery = getIsKrsRecovery({ userKey: rawUserKey, backupKey: rawBackupKey }); + const isUnsignedSweep = + params.isUnsignedSweep ?? + getIsUnsignedSweep({ userKey: rawUserKey, backupKey: rawBackupKey, isTss: params.isTss }); + + // Clean up whitespace from entered values + let userKey = rawUserKey.replace(/\s/g, ''); + const backupKey = rawBackupKey.replace(/\s/g, ''); if (isKrsRecovery) { checkKrsProvider(this, params.krsProvider, { checkCoinFamilySupport: false }); } - // Clean up whitespace from entered values - let userKey = params.userKey.replace(/\s/g, ''); - const backupKey = params.backupKey.replace(/\s/g, ''); - // Set new eth tx fees (default to using platform values if none are provided) const gasPrice = params.eip1559 ? new optionalDeps.ethUtil.BN(params.eip1559.maxFeePerGas) diff --git a/modules/sdk-coin-eth/src/erc7984Token.ts b/modules/sdk-coin-eth/src/erc7984Token.ts index fb158ed7e65..05d1173bfe8 100644 --- a/modules/sdk-coin-eth/src/erc7984Token.ts +++ b/modules/sdk-coin-eth/src/erc7984Token.ts @@ -950,16 +950,22 @@ export class Erc7984Token extends Eth { throw new Error('invalid recoveryDestination'); } - const isKrsRecovery = getIsKrsRecovery(params); - const isUnsignedSweep = params.isUnsignedSweep ?? getIsUnsignedSweep(params); + // Classify from the RAW keys, as before the type change: stripping whitespace first would + // flip the result for a key pasted with a leading newline. + const rawUserKey = params.userKey ?? ''; + const rawBackupKey = params.backupKey ?? ''; + const isKrsRecovery = getIsKrsRecovery({ userKey: rawUserKey, backupKey: rawBackupKey }); + const isUnsignedSweep = + params.isUnsignedSweep ?? + getIsUnsignedSweep({ userKey: rawUserKey, backupKey: rawBackupKey, isTss: params.isTss }); + + let userKey = rawUserKey.replace(/\s/g, ''); + const backupKey = rawBackupKey.replace(/\s/g, ''); if (isKrsRecovery) { checkKrsProvider(this, params.krsProvider, { checkCoinFamilySupport: false }); } - let userKey = params.userKey.replace(/\s/g, ''); - const backupKey = params.backupKey.replace(/\s/g, ''); - const gasPrice = params.eip1559 ? new optionalDeps.ethUtil.BN(params.eip1559.maxFeePerGas) : new optionalDeps.ethUtil.BN(this.setGasPrice(params.gasPrice)); diff --git a/modules/sdk-coin-eth/src/eth.ts b/modules/sdk-coin-eth/src/eth.ts index 16b4b7022fd..76a9b566370 100644 --- a/modules/sdk-coin-eth/src/eth.ts +++ b/modules/sdk-coin-eth/src/eth.ts @@ -205,17 +205,18 @@ export class Eth extends AbstractEthLikeNewCoins { } this.validateRecoveryParams(params); - const isKrsRecovery = getIsKrsRecovery(params); - const isUnsignedSweep = params.isUnsignedSweep ?? getIsUnsignedSweep(params); + + // Clean up whitespace from entered values + let userKey = (params.userKey ?? '').replace(/\s/g, ''); + const backupKey = (params.backupKey ?? '').replace(/\s/g, ''); + + const isKrsRecovery = getIsKrsRecovery({ userKey, backupKey }); + const isUnsignedSweep = params.isUnsignedSweep ?? getIsUnsignedSweep({ userKey, backupKey, isTss: params.isTss }); if (isKrsRecovery) { checkKrsProvider(this, params.krsProvider, { checkCoinFamilySupport: false }); } - // Clean up whitespace from entered values - let userKey = params.userKey.replace(/\s/g, ''); - const backupKey = params.backupKey.replace(/\s/g, ''); - // Set new eth tx fees (using default config values from platform) const gasLimit = new optionalDeps.ethUtil.BN(this.setGasLimit(params.gasLimit)); diff --git a/modules/sdk-coin-eth/test/unit/eth.ts b/modules/sdk-coin-eth/test/unit/eth.ts index 6a821017680..9eeb9106d37 100644 --- a/modules/sdk-coin-eth/test/unit/eth.ts +++ b/modules/sdk-coin-eth/test/unit/eth.ts @@ -3509,7 +3509,7 @@ describe('ETH:', function () { recoveryDestination: params.recoveryDestination, // Destination Address gasLimit: 200000, // Gas Limit eip1559: { maxFeePerGas: 20000000000, maxPriorityFeePerGas: 10000000000 }, // Max Fee Per Gas and Max Priority Fee Per Gas - walletContractAddress: params.walletContractAddress, + walletContractAddress: params.address, // base address derived at m/0 from the common keychain isTss: true, replayProtectionOptions: { chain: '42', diff --git a/modules/sdk-coin-eth/test/unit/ethSafeRecovery.ts b/modules/sdk-coin-eth/test/unit/ethSafeRecovery.ts new file mode 100644 index 00000000000..9d42ebefe18 --- /dev/null +++ b/modules/sdk-coin-eth/test/unit/ethSafeRecovery.ts @@ -0,0 +1,335 @@ +import should from 'should'; +import nock from 'nock'; +import Sinon from 'sinon'; +import { DklsUtils, DklsVrfUtils, getDerivationPath } from '@bitgo/sdk-lib-mpc'; +import { + buildSafeRecoverKeyParams, + common, + deriveSafeWalletKeys, + Ecdsa, + ErrorNoInputToRecover, + RecoveryKeyMismatchError, + RecoveryProviderError, + type DecryptedSafeRoots, + type EcdsaMpcChildKeys, +} from '@bitgo/sdk-core'; +import { TestBitGo, TestBitGoAPI } from '@bitgo/sdk-test'; +import { BitGoAPI } from '@bitgo/sdk-api'; +import { + AbstractEthLikeNewCoins, + KeyPair as EthKeyPair, + RecoveryInfo, + UnsignedSweepTxMPCv2, +} from '@bitgo/abstract-eth'; +import { FeeMarketEIP1559Transaction } from '@ethereumjs/tx'; +import * as EthLikeCommon from '@ethereumjs/common'; +import { toBuffer } from 'ethereumjs-util'; +import { Hteth } from '../../src'; +import * as mockData from '../fixtures/eth'; + +const recoveryDestination = '0xd5ADdE17feD8baed3F32b84AF05B8F2816f7b560'; +const baseUrl = common.Environments.test.etherscanBaseUrl as string; + +/** + * End-to-end Safe EVM TSS recovery: derive slot-3 (ecdsaMpc) children from real DKG roots, map them + * with buildSafeRecoverKeyParams, and prove `AbstractEthLikeNewCoins.recover()` accepts both shapes + * the mapper emits — signed `{ userKey, backupKey, bitgoKey, walletPassphrase, isTss }` and unsigned + * `{ bitgoKey, isTss }` with no user or backup key at all. + * + * The unsigned shape is the gap this suite pins: before WCN-3946 `validateRecoveryParams` rejected it + * with `missing userKey`, and the unsigned sweep builders never checked the derived `m/0` base address + * against `walletContractAddress`, so a wrong passphrase silently produced a sweep of someone else's + * wallet instead of failing closed. + */ +describe('Safe EVM TSS recovery end-to-end', function () { + let bitgo: TestBitGoAPI; + let basecoin: AbstractEthLikeNewCoins; + const sandbox = Sinon.createSandbox(); + + // Deriving real DKG roots is the slow part, so each distinct key set is built once. + let safeKeys: EcdsaMpcChildKeys; + let otherKeys: EcdsaMpcChildKeys; + let walletContractAddress: string; + + before(async function () { + bitgo = TestBitGo.decorate(BitGoAPI, { env: 'test' }); + bitgo.safeRegister('hteth', Hteth.createInstance); + bitgo.initializeTestVars(); + basecoin = bitgo.coin('hteth') as unknown as AbstractEthLikeNewCoins; + + safeKeys = await deriveEcdsaMpcKeys(); + otherKeys = await deriveEcdsaMpcKeys(); + walletContractAddress = baseAddressOf(safeKeys.commonKeychain); + }); + + afterEach(function () { + nock.cleanAll(); + sandbox.restore(); + }); + + /** Real DKG + VRF roots, exactly as a decrypted Safe keycard would provide for the ecdsaMpc slot. */ + async function ecdsaMpcRoots(): Promise { + const [userDkg, backupDkg] = await DklsUtils.generateDKGKeyShares(); + const [vrfUser, vrfBackup] = await DklsVrfUtils.generateVrfDKGKeyShares(); + return { + secp256k1Multisig: { user: 'u', backup: 'b', bitgo: 'g' }, + ed25519Multisig: { user: 'u', backup: { prv: 'b', chainCode: 'cc' }, bitgo: 'g' }, + ecdsaMpc: { + user: { signing: userDkg.getReducedKeyShare(), vrf: vrfUser.getKeyShare() }, + backup: { signing: backupDkg.getReducedKeyShare(), vrf: vrfBackup.getKeyShare() }, + bitgo: 'g', + }, + eddsaMpc: { + user: { signing: Buffer.alloc(0), vrf: Buffer.alloc(0) }, + backup: { signing: Buffer.alloc(0), vrf: Buffer.alloc(0) }, + bitgo: 'g', + }, + }; + } + + async function deriveEcdsaMpcKeys(): Promise { + const derived = await deriveSafeWalletKeys({ + coin: 'hteth', + slot: 'ecdsaMpc', + account: 0, + roots: await ecdsaMpcRoots(), + }); + if (derived.slot !== 'ecdsaMpc') { + throw new Error('expected ecdsaMpc slot'); + } + return derived.keys; + } + + /** The wallet's base address: the `m/0` child of the common keychain, exactly as recover() derives it. */ + function baseAddressOf(commonKeychain: string): string { + const pub = new Ecdsa().deriveUnhardened(commonKeychain, 'm/0').slice(0, 66); + return new EthKeyPair({ pub }).getAddress(); + } + + async function unsignedKeyParams(keys: EcdsaMpcChildKeys) { + return buildSafeRecoverKeyParams({ bitgo, coin: 'hteth', slot: 'ecdsaMpc', keys, mode: 'unsigned' }); + } + + function stubBaseAddressBalance(address: string, balance = mockData.getBalanceResponse) { + nock(baseUrl).get('/api').query(mockData.getTxListRequest(address)).reply(200, mockData.getTxListResponse); + nock(baseUrl).get('/api').query(mockData.getBalanceRequest(address)).reply(200, balance); + } + + function pendingBalanceMocks(): string[] { + return nock.pendingMocks(); + } + + it('maps an unsigned Safe recovery to { bitgoKey, isTss, broadcast } with no user or backup key', async function () { + const keyParams = await unsignedKeyParams(safeKeys); + keyParams.should.eql({ + slot: 'ecdsaMpc', + mode: 'unsigned', + bitgoKey: safeKeys.commonKeychain, + isTss: true, + broadcast: false, + }); + }); + + it('builds an unsigned sweep from bitgoKey alone', async function () { + const keyParams = await unsignedKeyParams(safeKeys); + stubBaseAddressBalance(walletContractAddress); + + const sweep = (await basecoin.recover({ + ...keyParams, + walletContractAddress, + recoveryDestination, + gasLimit: 200000, + eip1559: { maxFeePerGas: 20000000000, maxPriorityFeePerGas: 10000000000 }, + replayProtectionOptions: { chain: '560048', hardfork: 'london' }, + })) as UnsignedSweepTxMPCv2; + + should.exist(sweep); + sweep.txRequests.should.have.length(1); + const unsignedTx = sweep.txRequests[0].transactions[0].unsignedTx; + unsignedTx.should.have.properties('serializedTxHex', 'signableHex', 'derivationPath', 'feeInfo'); + unsignedTx.derivationPath.should.equal('m/0'); + // The sweep is built against the common keychain's own base address, not some caller-supplied one. + (unsignedTx.parsedTx as { outputs: { address: string }[] }).outputs[0].address.should.equal(recoveryDestination); + }); + + it('recovers a Safe EVM wallet in signed mode via the real MPCv2 signing path', async function () { + const keyParams = await buildSafeRecoverKeyParams({ + bitgo, + coin: 'hteth', + slot: 'ecdsaMpc', + keys: safeKeys, + mode: 'signed', + }); + if (keyParams.mode !== 'signed') { + throw new Error('expected signed key params'); + } + keyParams.bitgoKey.should.equal(safeKeys.commonKeychain); + stubBaseAddressBalance(walletContractAddress); + + const recovery = (await basecoin.recover({ + ...keyParams, + walletContractAddress, + recoveryDestination, + gasLimit: 200000, + eip1559: { maxFeePerGas: 20000000000, maxPriorityFeePerGas: 10000000000 }, + replayProtectionOptions: { chain: '560048', hardfork: 'london' }, + })) as RecoveryInfo; + + recovery.id.should.be.a.String(); + recovery.tx.should.startWith('0x'); + // A fully signed transaction recovers the base address as its sender. + const signedTx = FeeMarketEIP1559Transaction.fromSerializedTx(toBuffer(recovery.tx), { + common: EthLikeCommon.default.custom({ chainId: 560048 }, { baseChain: 'mainnet', hardfork: 'london' }), + }); + signedTx.getSenderAddress().toString().toLowerCase().should.equal(walletContractAddress.toLowerCase()); + signedTx.to?.toString().toLowerCase().should.equal(recoveryDestination.toLowerCase()); + }); + + it('still accepts the legacy shape that puts the common keychain in userKey and backupKey', async function () { + stubBaseAddressBalance(walletContractAddress); + + const sweep = (await basecoin.recover({ + userKey: safeKeys.commonKeychain, + backupKey: safeKeys.commonKeychain, + isTss: true, + walletContractAddress, + recoveryDestination, + gasLimit: 200000, + eip1559: { maxFeePerGas: 20000000000, maxPriorityFeePerGas: 10000000000 }, + replayProtectionOptions: { chain: '560048', hardfork: 'london' }, + })) as UnsignedSweepTxMPCv2; + + sweep.txRequests.should.have.length(1); + }); + + it('fails closed with RecoveryKeyMismatchError before any balance query when the keys are for another wallet', async function () { + // A wrong passphrase yields a valid but different keycard, so the derived m/0 address does not + // match the wallet being recovered. + const keyParams = await unsignedKeyParams(otherKeys); + stubBaseAddressBalance(walletContractAddress); + + let caughtError: unknown; + try { + await basecoin.recover({ + ...keyParams, + walletContractAddress, + recoveryDestination, + gasLimit: 200000, + eip1559: { maxFeePerGas: 20000000000, maxPriorityFeePerGas: 10000000000 }, + replayProtectionOptions: { chain: '560048', hardfork: 'london' }, + }); + } catch (e) { + caughtError = e; + } + + (caughtError as Error).should.be.instanceOf(RecoveryKeyMismatchError); + (caughtError as Error).message.should.match(/does not match the given wallet contract address/); + // Both explorer mocks are untouched: the mismatch fires before the balance check. + pendingBalanceMocks().should.have.length(2); + }); + + it('refuses to sweep an empty wallet once the keys do match', async function () { + const keyParams = await unsignedKeyParams(safeKeys); + stubBaseAddressBalance(walletContractAddress, { status: '1', message: 'OK', result: '0' }); + + let caughtError: unknown; + try { + await basecoin.recover({ + ...keyParams, + walletContractAddress, + recoveryDestination, + gasLimit: 200000, + eip1559: { maxFeePerGas: 20000000000, maxPriorityFeePerGas: 10000000000 }, + replayProtectionOptions: { chain: '560048', hardfork: 'london' }, + }); + } catch (e) { + caughtError = e; + } + + should.exist(caughtError); + // An empty wallet cannot even cover gas, so the base-class gas guard fires first. What matters + // for the orchestrator is that it is NOT reported as a key mismatch. + (caughtError as Error).message.should.match(/has balance 0 Gwei/); + (caughtError as Error).should.not.be.instanceOf(RecoveryKeyMismatchError); + }); + + it('still builds a derivationSeed sweep, whose base address is a child the caller cannot predict', async function () { + // Regression guard: the m/0 key-mismatch check must NOT apply when a derivationSeed is set. + // getDerivationPath hashes the seed into m/999999/x/y, so the swept address can never equal + // any walletContractAddress the caller could supply, and asserting on it would make every + // derivationSeed sweep impossible. + const derivationSeed = 'abc123'; + const derivedPub = new Ecdsa() + .deriveUnhardened(safeKeys.commonKeychain, getDerivationPath(derivationSeed)) + .slice(0, 66); + const seedAddress = new EthKeyPair({ pub: derivedPub }).getAddress(); + seedAddress.should.not.equal(walletContractAddress); + stubBaseAddressBalance(seedAddress); + + const sweep = (await basecoin.recover({ + userKey: safeKeys.commonKeychain, + backupKey: safeKeys.commonKeychain, + isTss: true, + derivationSeed, + walletContractAddress, + recoveryDestination, + gasLimit: 200000, + eip1559: { maxFeePerGas: 20000000000, maxPriorityFeePerGas: 10000000000 }, + replayProtectionOptions: { chain: '560048', hardfork: 'london' }, + })) as UnsignedSweepTxMPCv2; + + sweep.txRequests.should.have.length(1); + sweep.txRequests[0].transactions[0].unsignedTx.derivationPath.should.equal(getDerivationPath(derivationSeed)); + }); + + it('refuses an unsigned sweep with no walletContractAddress rather than skipping the key check', async function () { + const keyParams = await unsignedKeyParams(safeKeys); + stubBaseAddressBalance(walletContractAddress); + + let caughtError: unknown; + try { + await basecoin.recover({ + ...keyParams, + walletContractAddress: '', + recoveryDestination, + gasLimit: 200000, + eip1559: { maxFeePerGas: 20000000000, maxPriorityFeePerGas: 10000000000 }, + replayProtectionOptions: { chain: '560048', hardfork: 'london' }, + }); + } catch (e) { + caughtError = e; + } + + (caughtError as Error).message.should.match(/invalid walletContractAddress/); + // Nothing was queried: there was never an address to verify the keychain against. + pendingBalanceMocks().should.have.length(2); + }); + + it('surfaces an explorer failure rather than reporting an empty wallet', async function () { + const keyParams = await unsignedKeyParams(safeKeys); + nock(baseUrl) + .get('/api') + .query(mockData.getTxListRequest(walletContractAddress)) + .reply(200, mockData.getTxListResponse); + nock(baseUrl).get('/api').query(mockData.getBalanceRequest(walletContractAddress)).reply(500, 'node unavailable'); + + let caughtError: unknown; + try { + await basecoin.recover({ + ...keyParams, + walletContractAddress, + recoveryDestination, + gasLimit: 200000, + eip1559: { maxFeePerGas: 20000000000, maxPriorityFeePerGas: 10000000000 }, + replayProtectionOptions: { chain: '560048', hardfork: 'london' }, + }); + } catch (e) { + caughtError = e; + } + + (caughtError as Error).should.be.instanceOf(RecoveryProviderError); + // A dead explorer must never be mistaken for an empty wallet or a wrong passphrase. + (caughtError as Error).should.not.be.instanceOf(ErrorNoInputToRecover); + (caughtError as Error).should.not.be.instanceOf(RecoveryKeyMismatchError); + }); +}); diff --git a/modules/sdk-coin-evm/src/evmCoin.ts b/modules/sdk-coin-evm/src/evmCoin.ts index e92406163a4..4da310e95cb 100644 --- a/modules/sdk-coin-evm/src/evmCoin.ts +++ b/modules/sdk-coin-evm/src/evmCoin.ts @@ -15,11 +15,8 @@ import { BaseCoin as StaticsBaseCoin, CoinFeature, coins, CoinFamily } from '@bi import { AbstractEthLikeNewCoins, ERC721TransferBuilder, - OfflineVaultTxInfo, - RecoverOptions, recoveryBlockchainExplorerQuery, TransactionBuilder as EthLikeTransactionBuilder, - UnsignedSweepTxMPCv2, VerifyEthTransactionOptions, } from '@bitgo/abstract-eth'; import { TransactionBuilder } from './lib'; @@ -54,13 +51,6 @@ export class EvmCoin extends AbstractEthLikeNewCoins { return 'ecdsa'; } - protected async buildUnsignedSweepTxnTSS(params: RecoverOptions): Promise { - if (this.staticsCoin?.features.includes(CoinFeature.MPCV2)) { - return this.buildUnsignedSweepTxnMPCv2(params); - } - return super.buildUnsignedSweepTxnTSS(params); - } - /** * Make a query to chain explorer for information such as balance, token balance, solidity calls * @param {Object} query key-value pairs of parameters to append after /api diff --git a/modules/sdk-coin-islm/test/unit/islm.ts b/modules/sdk-coin-islm/test/unit/islm.ts index 3371a561ad7..07de32ac57e 100644 --- a/modules/sdk-coin-islm/test/unit/islm.ts +++ b/modules/sdk-coin-islm/test/unit/islm.ts @@ -340,6 +340,8 @@ describe('Islm', function () { const commonKeyChain = '03924e763697f4388577cf234a57b1675ff5048f3adddec23959d85862f55cc97f6a06c263e' + 'a42166875f0f13acb828c724bd9d67d06c440dff6e308326e9833ab'; + // the m/0 address of commonKeyChain — the wallet these keys actually control + const rootAddress = 'haqq1he0rn7pkqwgdg3xjff8u3afcqcwamshdtrtpav'; beforeEach(function () { sandBox.stub(Islm.prototype, 'getAccountDetails' as keyof Islm).resolves(['0', '0']); @@ -357,7 +359,7 @@ describe('Islm', function () { .resolves({ status: 200, body: { block: { header: { chain_id: 'test-chain' } } } }); await basecoin .recover({ - rootAddress: recoveryDestination, + rootAddress, bitgoKey: commonKeyChain, recoveryDestination, }) @@ -368,7 +370,7 @@ describe('Islm', function () { sandBox.stub(Islm.prototype, 'getChainIdFromNode' as keyof Islm).resolves({ status: 500, body: {} }); await basecoin .recover({ - rootAddress: recoveryDestination, + rootAddress, bitgoKey: commonKeyChain, recoveryDestination, }) diff --git a/modules/sdk-coin-polygon/test/unit/polygon.ts b/modules/sdk-coin-polygon/test/unit/polygon.ts index e6025459163..f08de0e13a0 100644 --- a/modules/sdk-coin-polygon/test/unit/polygon.ts +++ b/modules/sdk-coin-polygon/test/unit/polygon.ts @@ -1283,7 +1283,7 @@ describe('Polygon', function () { recoveryDestination: params.recoveryDestination, gasLimit: 200000, eip1559: { maxFeePerGas: 20000000000, maxPriorityFeePerGas: 10000000000 }, - walletContractAddress: params.walletContractAddress, + walletContractAddress: params.address, // base address derived at m/0 from the common keychain isTss: true, replayProtectionOptions: { chain: '137', diff --git a/modules/sdk-coin-stx/src/stx.ts b/modules/sdk-coin-stx/src/stx.ts index f8654d0a31f..6d6c23f6892 100644 --- a/modules/sdk-coin-stx/src/stx.ts +++ b/modules/sdk-coin-stx/src/stx.ts @@ -11,6 +11,7 @@ import { MethodNotImplementedError, MultisigType, multisigTypes, + RecoveryKeyMismatchError, SignedTransaction, RecoveryProviderError, TransactionParams, @@ -801,6 +802,21 @@ export class Stx extends BaseCoin { const isUnsignedSweep = getIsUnsignedSweep(params); const keys = await getBip32Keys(this.bitgo, params, { requireBitGoXpub: true }); const rootAddressDetails = getAddressDetails(params.rootAddress); + const pubs = [ + publicKeyToHex(keys[0].publicKey), + publicKeyToHex(keys[1].publicKey), + publicKeyToHex(keys[2].publicKey), + ]; + + // Fail closed before touching the node: wrong keys must never reach a balance or nonce query. + const addressVersion = StxLib.Utils.getAddressVersion(rootAddressDetails.address); + const derivedAddress = StxLib.Utils.getSTXAddressFromPubKeys(pubs, addressVersion).address; + if (!StxLib.Utils.isSameBaseAddress(rootAddressDetails.address, derivedAddress)) { + throw new RecoveryKeyMismatchError( + `derived wallet address ${derivedAddress} does not match the given root address ${rootAddressDetails.address}` + ); + } + const [accountBalanceData, accountNonceData] = await Promise.all([ this.getNativeStxBalanceFromNode({ address: rootAddressDetails.address }), this.getAccountNonceFromNode({ address: rootAddressDetails.address }), @@ -809,11 +825,6 @@ export class Stx extends BaseCoin { if (!balance || isNaN(balance)) { throw new ErrorNoInputToRecover('could not find any balance to recover for ' + params.rootAddress); } - const pubs = [ - publicKeyToHex(keys[0].publicKey), - publicKeyToHex(keys[1].publicKey), - publicKeyToHex(keys[2].publicKey), - ]; const destinationAddressDetails = getAddressDetails(params.recoveryDestination); const nonce = diff --git a/modules/sdk-coin-stx/test/fixtures.ts b/modules/sdk-coin-stx/test/fixtures.ts index 48f39ea7422..ebfa8e4f85f 100644 --- a/modules/sdk-coin-stx/test/fixtures.ts +++ b/modules/sdk-coin-stx/test/fixtures.ts @@ -78,6 +78,9 @@ export const HOT_WALLET_KEY_CARD_INFO = { BITGO_PUB_KEY: 'xpub661MyMwAqRbcGeFRie579ey3NRwkdJ2zhEhCK3zhMMXYU3daWoMFFLQrVYmB5EqxPtaSCnEK9YYpb7FKoqkojD3BNtdmtFTYCwb7KywHpKF', WALLET_PASSPHRASE: '#Bondiola1234', + /** neutered BACKUP_KEY — derives the same root address but holds no private key */ + BACKUP_PUB_KEY: + 'xpub661MyMwAqRbcFpgyWjqWYK5bQLBY7Pf78wyzhsWWdKcXyYXDxpArLPQxLyUkvh3XAQ1FLJKHCrfHXjwbzGxk83pofKfzuzdjjYWEH1FoZhi', }; export const COLD_WALLET_PUBLIC_KEY_INFO = { diff --git a/modules/sdk-coin-stx/test/unit/stx.ts b/modules/sdk-coin-stx/test/unit/stx.ts index e30f47d276f..13aa4c973c4 100644 --- a/modules/sdk-coin-stx/test/unit/stx.ts +++ b/modules/sdk-coin-stx/test/unit/stx.ts @@ -761,11 +761,12 @@ describe('STX:', function () { .reply(200, testData.FEE_ESTIMATION_RESPONSE); const recoveryOptions: RecoveryOptions = { - backupKey: testData.COLD_WALLET_PUBLIC_KEY_INFO.BACKUP_KEY, - userKey: resources.xprv1, + backupKey: testData.HOT_WALLET_KEY_CARD_INFO.BACKUP_PUB_KEY, + userKey: testData.HOT_WALLET_KEY_CARD_INFO.USER_KEY, rootAddress: rootAddress, recoveryDestination: testData.DESTINATION_ADDRESS_WRW, bitgoKey: testData.HOT_WALLET_KEY_CARD_INFO.BITGO_PUB_KEY, + walletPassphrase: testData.HOT_WALLET_KEY_CARD_INFO.WALLET_PASSPHRASE, }; await basecoin.recover(recoveryOptions).should.be.rejectedWith('backupKey is not a private key'); }); diff --git a/modules/sdk-coin-stx/test/unit/stxSafeRecovery.ts b/modules/sdk-coin-stx/test/unit/stxSafeRecovery.ts new file mode 100644 index 00000000000..cad7853e27e --- /dev/null +++ b/modules/sdk-coin-stx/test/unit/stxSafeRecovery.ts @@ -0,0 +1,229 @@ +import should from 'should'; +import nock from 'nock'; +import { bip32 } from '@bitgo/secp256k1'; +import { + buildSafeRecoverKeyParams, + deriveSafeWalletKeys, + ErrorNoInputToRecover, + RecoveryKeyMismatchError, + RecoveryProviderError, + type DecryptedSafeRoots, + type Secp256k1MultisigChildKeys, +} from '@bitgo/sdk-core'; +import { TestBitGo, TestBitGoAPI } from '@bitgo/sdk-test'; +import { BitGoAPI } from '@bitgo/sdk-api'; +import { Tstx, StxLib } from '../../src'; +import * as testData from '../fixtures'; + +const recoveryDestination = testData.DESTINATION_ADDRESS_WRW; +const nodeUrl = 'https://api.testnet.hiro.so'; + +/** + * End-to-end Safe STX recovery: derive slot-1 (secp256k1Multisig) children from real roots, map them + * with buildSafeRecoverKeyParams, and prove `Stx.recover()` accepts both modes while verifying that + * the three child pubs really do form the 2-of-3 multisig address being recovered. + * + * The mismatch check is the gap this suite pins: before WCN-3946 `recover()` queried the balance and + * nonce of whatever `rootAddress` it was handed before looking at the keys at all, so a wrong + * passphrase surfaced as `ErrorNoInputToRecover` (or, worse, built a transfer signed with keys that + * do not control the address). The mismatch must now fire with zero network calls. + */ +describe('Safe STX recovery end-to-end', function () { + let bitgo: TestBitGoAPI; + let basecoin: Tstx; + + let safeKeys: Secp256k1MultisigChildKeys; + let otherKeys: Secp256k1MultisigChildKeys; + let rootAddress: string; + + before(async function () { + bitgo = TestBitGo.decorate(BitGoAPI, { env: 'test' }); + bitgo.safeRegister('tstx', Tstx.createInstance); + bitgo.initializeTestVars(); + basecoin = bitgo.coin('tstx') as Tstx; + + safeKeys = await deriveSecp256k1MultisigKeys([1, 2, 3]); + otherKeys = await deriveSecp256k1MultisigKeys([4, 5, 6]); + rootAddress = multisigAddressOf(safeKeys); + }); + + afterEach(function () { + nock.cleanAll(); + }); + + /** Distinct secp256k1 root triplets so user, backup and bitgo children are different keys. */ + function secp256k1Roots([u, b, g]: [number, number, number]): DecryptedSafeRoots { + const root = (fill: number) => bip32.fromSeed(Buffer.alloc(32, fill)); + return { + secp256k1Multisig: { + user: root(u).toBase58(), + backup: root(b).toBase58(), + bitgo: root(g).neutered().toBase58(), + }, + ed25519Multisig: { user: 'u', backup: { prv: 'b', chainCode: 'cc' }, bitgo: 'g' }, + ecdsaMpc: { + user: { signing: Buffer.alloc(0), vrf: Buffer.alloc(0) }, + backup: { signing: Buffer.alloc(0), vrf: Buffer.alloc(0) }, + bitgo: 'g', + }, + eddsaMpc: { + user: { signing: Buffer.alloc(0), vrf: Buffer.alloc(0) }, + backup: { signing: Buffer.alloc(0), vrf: Buffer.alloc(0) }, + bitgo: 'g', + }, + }; + } + + async function deriveSecp256k1MultisigKeys(seeds: [number, number, number]): Promise { + const derived = await deriveSafeWalletKeys({ + coin: 'tstx', + slot: 'secp256k1Multisig', + account: 0, + roots: secp256k1Roots(seeds), + }); + if (derived.slot !== 'secp256k1Multisig') { + throw new Error('expected secp256k1Multisig slot'); + } + return derived.keys; + } + + /** The wallet's 2-of-3 multisig address over the three child pubs, as `recover()` derives it. */ + function multisigAddressOf(keys: Secp256k1MultisigChildKeys): string { + const pubs = [keys.user.pub, keys.backup.pub, keys.bitgo.pub].map((xpub) => StxLib.Utils.xpubToSTXPubkey(xpub)); + return StxLib.Utils.getSTXAddressFromPubKeys(pubs, StxLib.Utils.getAddressVersion(testData.HOT_WALLET_ROOT_ADDRESS)) + .address; + } + + function keyParams(keys: Secp256k1MultisigChildKeys, mode: 'signed' | 'unsigned') { + return buildSafeRecoverKeyParams({ bitgo, coin: 'tstx', slot: 'secp256k1Multisig', keys, mode }); + } + + function stubNodeQueries(address: string, balance = testData.ACCOUNT_BALANCE_RESPONSE) { + nock(nodeUrl).get(`/extended/v2/addresses/${address}/balances/stx`).reply(200, balance); + nock(nodeUrl).get(`/extended/v1/address/${address}/nonces`).reply(200, testData.ACCOUNT_NONCE_RESPONSE); + nock(nodeUrl, { allowUnmocked: true }).post('/v2/fees/transaction').reply(200, testData.FEE_ESTIMATION_RESPONSE); + } + + it('recovers a Safe STX wallet in signed mode', async function () { + const params = await keyParams(safeKeys, 'signed'); + stubNodeQueries(rootAddress); + + const recovery = await basecoin.recover({ + userKey: params.userKey, + backupKey: params.backupKey, + bitgoKey: params.bitgoKey, + walletPassphrase: params.walletPassphrase, + rootAddress, + recoveryDestination, + }); + + should.exist(recovery); + (recovery as { txHex: string }).txHex.should.be.a.String(); + }); + + it('recovers a Safe STX wallet in unsigned mode (xpubs only, no passphrase)', async function () { + const params = await keyParams(safeKeys, 'unsigned'); + should.not.exist(params.walletPassphrase); + stubNodeQueries(rootAddress); + + const recovery = await basecoin.recover({ + userKey: params.userKey, + backupKey: params.backupKey, + bitgoKey: params.bitgoKey, + rootAddress, + recoveryDestination, + }); + + should.exist(recovery); + (recovery as { txHex: string }).txHex.should.be.a.String(); + }); + + it('fails closed with RecoveryKeyMismatchError and zero network calls on a wrong passphrase', async function () { + // A wrong passphrase decrypts to a valid but different keycard, so the three child pubs form a + // different multisig address than the wallet being recovered. + const params = await keyParams(otherKeys, 'signed'); + stubNodeQueries(rootAddress); + + let caughtError: unknown; + try { + await basecoin.recover({ + userKey: params.userKey, + backupKey: params.backupKey, + bitgoKey: params.bitgoKey, + walletPassphrase: params.walletPassphrase, + rootAddress, + recoveryDestination, + }); + } catch (e) { + caughtError = e; + } + + (caughtError as Error).should.be.instanceOf(RecoveryKeyMismatchError); + (caughtError as Error).message.should.match(/does not match the given root address/); + // All three node mocks are untouched: the check is offline and runs before balance and nonce. + nock.pendingMocks().should.have.length(3); + }); + + it('fails closed with RecoveryKeyMismatchError in unsigned mode too', async function () { + const params = await keyParams(otherKeys, 'unsigned'); + stubNodeQueries(rootAddress); + + let caughtError: unknown; + try { + await basecoin.recover({ + userKey: params.userKey, + backupKey: params.backupKey, + bitgoKey: params.bitgoKey, + rootAddress, + recoveryDestination, + }); + } catch (e) { + caughtError = e; + } + + (caughtError as Error).should.be.instanceOf(RecoveryKeyMismatchError); + nock.pendingMocks().should.have.length(3); + }); + + it('reports an empty wallet as ErrorNoInputToRecover once the keys do match', async function () { + const params = await keyParams(safeKeys, 'unsigned'); + stubNodeQueries(rootAddress, { ...testData.ACCOUNT_BALANCE_RESPONSE, balance: '0' }); + + let caughtError: unknown; + try { + await basecoin.recover({ + userKey: params.userKey, + backupKey: params.backupKey, + bitgoKey: params.bitgoKey, + rootAddress, + recoveryDestination, + }); + } catch (e) { + caughtError = e; + } + + (caughtError as Error).should.be.instanceOf(ErrorNoInputToRecover); + }); + + it('surfaces a dead node as RecoveryProviderError, not as an empty wallet', async function () { + const params = await keyParams(safeKeys, 'unsigned'); + nock(nodeUrl).get(`/extended/v2/addresses/${rootAddress}/balances/stx`).reply(500, { error: 'node unavailable' }); + nock(nodeUrl).get(`/extended/v1/address/${rootAddress}/nonces`).reply(200, testData.ACCOUNT_NONCE_RESPONSE); + + let caughtError: unknown; + try { + await basecoin.recover({ + userKey: params.userKey, + backupKey: params.backupKey, + bitgoKey: params.bitgoKey, + rootAddress, + recoveryDestination, + }); + } catch (e) { + caughtError = e; + } + + (caughtError as Error).should.be.instanceOf(RecoveryProviderError); + (caughtError as Error).should.not.be.instanceOf(ErrorNoInputToRecover); + }); +}); diff --git a/modules/sdk-coin-tempo/src/tempo.ts b/modules/sdk-coin-tempo/src/tempo.ts index f7fddb897a7..f61f13bed0e 100644 --- a/modules/sdk-coin-tempo/src/tempo.ts +++ b/modules/sdk-coin-tempo/src/tempo.ts @@ -481,8 +481,8 @@ export class Tempo extends AbstractEthLikeNewCoins { params: RecoverOptions ): Promise { this.validateRecoveryParams(params); - const userKey = params.userKey.replace(/\s/g, ''); - const backupKey = params.backupKey.replace(/\s/g, ''); + const userKey = (params.userKey ?? '').replace(/\s/g, ''); + const backupKey = (params.backupKey ?? '').replace(/\s/g, ''); if (getIsUnsignedSweep({ userKey, backupKey, isTss: params.isTss })) { return this.buildUnsignedSweepTxnTSS(params); @@ -527,7 +527,10 @@ export class Tempo extends AbstractEthLikeNewCoins { * Called by recoverTSS when plain public key shares are provided. */ protected async buildUnsignedSweepTxnTSS(params: RecoverOptions): Promise { - const backupKey = params.backupKey.replace(/\s/g, ''); + const backupKey = this.getTssCommonKeychain(params); + if (!backupKey) { + throw new Error('missing commonKeyChain'); + } const derivationPath = params.derivationSeed ? getDerivationPath(params.derivationSeed) : 'm/0'; const MPC = new Ecdsa(); const derivedCommonKeyChain = MPC.deriveUnhardened(backupKey, derivationPath); From 1fdd01ce8d9929032ce0b59a8c713601ce8e521d Mon Sep 17 00:00:00 2001 From: Pranish Nepal Date: Fri, 30 Oct 2026 12:27:00 -0400 Subject: [PATCH 2/2] fix(abstract-eth): address review feedback on Safe keycard recovery - eth.ts classified sweep/KRS recovery from whitespace-stripped keys while every sibling used the raw keys; extract classifyRecoveryKeys so the two cannot drift apart again - tempo: fail closed on a wrong keychain like the rest of the EVM TSS family - avaxc: forward apiKey to the signer check - cover the base-class missing commonKeyChain path, which also pins that validateUnsignedSweepTSSParams is awaited Aligning getConsolidationAddress onto the same keychain fallback was tried and reverted: recoverConsolidations is called with backupKey and no userKey, so it scanned an extra derived address and changed which addresses a consolidation sweeps. TICKET: WCN-3946 --- .../src/abstractEthLikeNewCoins.ts | 36 +++++++++---------- modules/abstract-eth/src/ethLikeToken.ts | 16 +++------ modules/sdk-coin-avaxc/src/avaxc.ts | 22 ++++++------ modules/sdk-coin-etc/src/etc.ts | 14 +++----- modules/sdk-coin-eth/src/erc20Token.ts | 18 +++------- modules/sdk-coin-eth/src/erc721Token.ts | 18 +++------- modules/sdk-coin-eth/src/erc7984Token.ts | 17 +++------ modules/sdk-coin-eth/src/eth.ts | 12 +++---- .../sdk-coin-eth/test/unit/ethSafeRecovery.ts | 18 ++++++++++ modules/sdk-coin-tempo/src/tempo.ts | 6 ++++ modules/sdk-coin-tempo/test/unit/tempo.ts | 34 +++++++++++++++++- .../sdk-core/src/bitgo/recovery/initiate.ts | 27 ++++++++++++++ 12 files changed, 135 insertions(+), 103 deletions(-) diff --git a/modules/abstract-eth/src/abstractEthLikeNewCoins.ts b/modules/abstract-eth/src/abstractEthLikeNewCoins.ts index f3520f31893..ff45cad8def 100644 --- a/modules/abstract-eth/src/abstractEthLikeNewCoins.ts +++ b/modules/abstract-eth/src/abstractEthLikeNewCoins.ts @@ -2,6 +2,7 @@ * @prettier */ import { + classifyRecoveryKeys, AddressCoinSpecific, BitGoBase, BuildNftTransferDataOptions, @@ -1515,6 +1516,9 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin implem } } + // NOTE: deliberately keyed on params.userKey only, not getTssCommonKeychain. Resolving the + // TSS fallback chain here would scan an extra derived address whenever a caller supplies + // backupKey without userKey, changing which addresses a consolidation sweeps. if (params.userKey) { try { const derivedAddress = this.deriveAddressFromPublicKey(params.userKey, index); @@ -1554,22 +1558,25 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin implem const consolidatedTransactions: any[] = []; let lastScanIndex = startIdx; + // An unsigned TSS sweep builds from the m/0 address derived from the common keychain and + // never reads walletContractAddress, so the forwarder address was never the address being + // swept here. Pass the address actually swept, which is what the key mismatch check compares. + // Loop-invariant, and only for the unsigned shape: in the signed shape userKey/backupKey hold + // encrypted key shares, and deriving from that ciphertext would throw before the loop starts. + const isTssUnsignedSweep = params.isTss === true && classifyRecoveryKeys(params).isUnsignedSweep; + const tssCommonKeychain = isTssUnsignedSweep ? this.getTssCommonKeychain(params) : ''; + const tssBaseAddress = tssCommonKeychain ? this.deriveAddressFromPublicKey(tssCommonKeychain, 0) : undefined; + for (let i = startIdx; i < endIdx; i++) { const consolidationAddress = this.getConsolidationAddress(params, i); for (const address of consolidationAddress) { - // A TSS recovery sweeps the m/0 address derived from the common keychain and never reads - // walletContractAddress, so the forwarder address was never the address being swept here. - // Pass the address actually swept, which is also what the key mismatch check compares. - const recoverWalletContractAddress = params.isTss - ? this.deriveAddressFromPublicKey(this.getTssCommonKeychain(params), 0) - : address; const recoverParams = { apiKey: params.apiKey, backupKey: params.backupKey || '', gasLimit: params.gasLimit, recoveryDestination: params.recoveryDestination || '', userKey: params.userKey || '', - walletContractAddress: recoverWalletContractAddress, + walletContractAddress: tssBaseAddress ?? address, derivationSeed: '', isTss: params.isTss, eip1559: { @@ -1635,18 +1642,9 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin implem } this.validateRecoveryParams(params); - // validateRecoveryParams guarantees both keys are present outside of unsigned TSS sweeps, - // which never reach this non-TSS path. Classify from the RAW keys, as before the type change: - // stripping whitespace first would flip the result for a key pasted with a leading newline. - const rawUserKey = params.userKey ?? ''; - const rawBackupKey = params.backupKey ?? ''; - const isUnsignedSweep = - params.isUnsignedSweep ?? - getIsUnsignedSweep({ userKey: rawUserKey, backupKey: rawBackupKey, isTss: params.isTss }); - - // Clean up whitespace from entered values - let userKey = rawUserKey.replace(/\s/g, ''); - const backupKey = rawBackupKey.replace(/\s/g, ''); + const recoveryKeys = classifyRecoveryKeys(params); + const { backupKey, isUnsignedSweep } = recoveryKeys; + let userKey = recoveryKeys.userKey; const gasLimit = new optionalDeps.ethUtil.BN(this.setGasLimit(params.gasLimit)); const gasPrice = params.eip1559 ? new optionalDeps.ethUtil.BN(params.eip1559.maxFeePerGas) diff --git a/modules/abstract-eth/src/ethLikeToken.ts b/modules/abstract-eth/src/ethLikeToken.ts index 78a42d9afeb..4716ad16b5a 100644 --- a/modules/abstract-eth/src/ethLikeToken.ts +++ b/modules/abstract-eth/src/ethLikeToken.ts @@ -7,12 +7,12 @@ import { bip32 } from '@bitgo/secp256k1'; import { BigNumber } from 'bignumber.js'; import { + classifyRecoveryKeys, BitGoBase, CoinConstructor, NamedCoinConstructor, ErrorNoInputToRecover, RecoveryProviderError, - getIsUnsignedSweep, Util, } from '@bitgo/sdk-core'; import { @@ -213,17 +213,9 @@ export class EthLikeToken extends AbstractEthLikeNewCoins { throw new Error('invalid recoveryDestination'); } - // Classify from the RAW keys, as before the type change: stripping whitespace first would - // flip the result for a key pasted with a leading newline. - const rawUserKey = params.userKey ?? ''; - const rawBackupKey = params.backupKey ?? ''; - const isUnsignedSweep = - params.isUnsignedSweep ?? - getIsUnsignedSweep({ userKey: rawUserKey, backupKey: rawBackupKey, isTss: params.isTss }); - - // Clean up whitespace from entered values - let userKey = rawUserKey.replace(/\s/g, ''); - const backupKey = rawBackupKey.replace(/\s/g, ''); + const recoveryKeys = classifyRecoveryKeys(params); + const { backupKey, isUnsignedSweep } = recoveryKeys; + let userKey = recoveryKeys.userKey; const gasLimit = new optionalDeps.ethUtil.BN(this.setGasLimit(params.gasLimit)); const gasPrice = params.eip1559 diff --git a/modules/sdk-coin-avaxc/src/avaxc.ts b/modules/sdk-coin-avaxc/src/avaxc.ts index 4057fcfaa09..a08476e1d56 100644 --- a/modules/sdk-coin-avaxc/src/avaxc.ts +++ b/modules/sdk-coin-avaxc/src/avaxc.ts @@ -15,6 +15,7 @@ import { EthereumNetwork, } from '@bitgo/statics'; import { + classifyRecoveryKeys, BaseCoin, BaseTransaction, BitGoBase, @@ -22,7 +23,6 @@ import { ErrorNoInputToRecover, FeeEstimateOptions, FullySignedTransaction, - getIsUnsignedSweep, InvalidAddressError, IWallet, KeyPair, @@ -659,16 +659,9 @@ export class AvaxC extends AbstractEthLikeNewCoins { throw new Error('invalid recoveryDestination'); } - // Classify from the RAW keys, as before the type change: stripping whitespace first would - // flip the result for a key pasted with a leading newline. - const rawUserKey = params.userKey ?? ''; - const rawBackupKey = params.backupKey ?? ''; - // TODO (BG-56531): add support for krs - const isUnsignedSweep = getIsUnsignedSweep({ userKey: rawUserKey, backupKey: rawBackupKey, isTss: params.isTss }); - - // Clean up whitespace from entered values - let userKey = rawUserKey.replace(/\s/g, ''); - const backupKey = rawBackupKey.replace(/\s/g, ''); + const recoveryKeys = classifyRecoveryKeys({ ...params, isUnsignedSweep: undefined }); + const { backupKey, isUnsignedSweep } = recoveryKeys; + let userKey = recoveryKeys.userKey; // Set new tx fees (using default config values from platform) const gasLimit = new optionalDeps.ethUtil.BN(this.setGasLimit(params.gasLimit)); @@ -711,7 +704,12 @@ export class AvaxC extends AbstractEthLikeNewCoins { } backupKeyAddress = keyPair.getAddress(); } - await this.assertRecoveryKeysAreWalletSigners(userKey, backupKeyAddress, params.walletContractAddress); + await this.assertRecoveryKeysAreWalletSigners( + userKey, + backupKeyAddress, + params.walletContractAddress, + params.apiKey + ); const backupKeyNonce = await this.getAddressNonce(backupKeyAddress); diff --git a/modules/sdk-coin-etc/src/etc.ts b/modules/sdk-coin-etc/src/etc.ts index 484c08b1439..96e09873ca1 100644 --- a/modules/sdk-coin-etc/src/etc.ts +++ b/modules/sdk-coin-etc/src/etc.ts @@ -12,11 +12,11 @@ import { SignTransactionOptions, } from '@bitgo/abstract-eth'; import { + classifyRecoveryKeys, BaseCoin, BitGoBase, ErrorNoInputToRecover, common, - getIsUnsignedSweep, Util, Recipient, RecoveryProviderError, @@ -80,15 +80,9 @@ export class Etc extends AbstractEthLikeCoin { async recover(params: RecoverOptions): Promise { this.validateRecoveryParams(params); - // Classify from the RAW keys, as before the type change: stripping whitespace first would - // flip the result for a key pasted with a leading newline. - const rawUserKey = params.userKey ?? ''; - const rawBackupKey = params.backupKey ?? ''; - const isUnsignedSweep = getIsUnsignedSweep({ userKey: rawUserKey, backupKey: rawBackupKey, isTss: params.isTss }); - - // Clean up whitespace from entered values - let userKey = rawUserKey.replace(/\s/g, ''); - const backupKey = rawBackupKey.replace(/\s/g, ''); + const recoveryKeys = classifyRecoveryKeys({ ...params, isUnsignedSweep: undefined }); + const { backupKey, isUnsignedSweep } = recoveryKeys; + let userKey = recoveryKeys.userKey; const gasLimit = new optionalDeps.ethUtil.BN(this.setGasLimit(params.gasLimit)); const gasPrice = params.eip1559 ? new optionalDeps.ethUtil.BN(params.eip1559.maxFeePerGas) diff --git a/modules/sdk-coin-eth/src/erc20Token.ts b/modules/sdk-coin-eth/src/erc20Token.ts index 5a0494becaa..94a3e6e23ae 100644 --- a/modules/sdk-coin-eth/src/erc20Token.ts +++ b/modules/sdk-coin-eth/src/erc20Token.ts @@ -2,14 +2,13 @@ * @prettier */ import { + classifyRecoveryKeys, BitGoBase, CoinConstructor, ErrorNoInputToRecover, RecoveryProviderError, Util, checkKrsProvider, - getIsKrsRecovery, - getIsUnsignedSweep, MPCAlgorithm, NamedCoinConstructor, } from '@bitgo/sdk-core'; @@ -152,18 +151,9 @@ export class Erc20Token extends Eth { throw new Error('invalid recoveryDestination'); } - // Classify from the RAW keys, as before the type change: stripping whitespace first would - // flip the result for a key pasted with a leading newline. - const rawUserKey = params.userKey ?? ''; - const rawBackupKey = params.backupKey ?? ''; - const isKrsRecovery = getIsKrsRecovery({ userKey: rawUserKey, backupKey: rawBackupKey }); - const isUnsignedSweep = - params.isUnsignedSweep ?? - getIsUnsignedSweep({ userKey: rawUserKey, backupKey: rawBackupKey, isTss: params.isTss }); - - // Clean up whitespace from entered values - let userKey = rawUserKey.replace(/\s/g, ''); - const backupKey = rawBackupKey.replace(/\s/g, ''); + const recoveryKeys = classifyRecoveryKeys(params); + const { backupKey, isKrsRecovery, isUnsignedSweep } = recoveryKeys; + let userKey = recoveryKeys.userKey; if (isKrsRecovery) { checkKrsProvider(this, params.krsProvider, { checkCoinFamilySupport: false }); diff --git a/modules/sdk-coin-eth/src/erc721Token.ts b/modules/sdk-coin-eth/src/erc721Token.ts index 0c0e0c1575d..1cb72c1d1b4 100644 --- a/modules/sdk-coin-eth/src/erc721Token.ts +++ b/modules/sdk-coin-eth/src/erc721Token.ts @@ -2,14 +2,13 @@ * @prettier */ import { + classifyRecoveryKeys, BitGoBase, CoinConstructor, ErrorNoInputToRecover, RecoveryProviderError, Util, checkKrsProvider, - getIsKrsRecovery, - getIsUnsignedSweep, MPCAlgorithm, NamedCoinConstructor, } from '@bitgo/sdk-core'; @@ -152,18 +151,9 @@ export class Erc721Token extends Eth { throw new Error('invalid recoveryDestination'); } - // Classify from the RAW keys, as before the type change: stripping whitespace first would - // flip the result for a key pasted with a leading newline. - const rawUserKey = params.userKey ?? ''; - const rawBackupKey = params.backupKey ?? ''; - const isKrsRecovery = getIsKrsRecovery({ userKey: rawUserKey, backupKey: rawBackupKey }); - const isUnsignedSweep = - params.isUnsignedSweep ?? - getIsUnsignedSweep({ userKey: rawUserKey, backupKey: rawBackupKey, isTss: params.isTss }); - - // Clean up whitespace from entered values - let userKey = rawUserKey.replace(/\s/g, ''); - const backupKey = rawBackupKey.replace(/\s/g, ''); + const recoveryKeys = classifyRecoveryKeys(params); + const { backupKey, isKrsRecovery, isUnsignedSweep } = recoveryKeys; + let userKey = recoveryKeys.userKey; if (isKrsRecovery) { checkKrsProvider(this, params.krsProvider, { checkCoinFamilySupport: false }); diff --git a/modules/sdk-coin-eth/src/erc7984Token.ts b/modules/sdk-coin-eth/src/erc7984Token.ts index 05d1173bfe8..52ecbc4c4de 100644 --- a/modules/sdk-coin-eth/src/erc7984Token.ts +++ b/modules/sdk-coin-eth/src/erc7984Token.ts @@ -2,11 +2,10 @@ * @prettier */ import { + classifyRecoveryKeys, BitGoBase, CoinConstructor, checkKrsProvider, - getIsKrsRecovery, - getIsUnsignedSweep, MPCAlgorithm, NamedCoinConstructor, RecoveryProviderError, @@ -950,17 +949,9 @@ export class Erc7984Token extends Eth { throw new Error('invalid recoveryDestination'); } - // Classify from the RAW keys, as before the type change: stripping whitespace first would - // flip the result for a key pasted with a leading newline. - const rawUserKey = params.userKey ?? ''; - const rawBackupKey = params.backupKey ?? ''; - const isKrsRecovery = getIsKrsRecovery({ userKey: rawUserKey, backupKey: rawBackupKey }); - const isUnsignedSweep = - params.isUnsignedSweep ?? - getIsUnsignedSweep({ userKey: rawUserKey, backupKey: rawBackupKey, isTss: params.isTss }); - - let userKey = rawUserKey.replace(/\s/g, ''); - const backupKey = rawBackupKey.replace(/\s/g, ''); + const recoveryKeys = classifyRecoveryKeys(params); + const { backupKey, isKrsRecovery, isUnsignedSweep } = recoveryKeys; + let userKey = recoveryKeys.userKey; if (isKrsRecovery) { checkKrsProvider(this, params.krsProvider, { checkCoinFamilySupport: false }); diff --git a/modules/sdk-coin-eth/src/eth.ts b/modules/sdk-coin-eth/src/eth.ts index 76a9b566370..e27ee5cb682 100644 --- a/modules/sdk-coin-eth/src/eth.ts +++ b/modules/sdk-coin-eth/src/eth.ts @@ -5,14 +5,13 @@ import { bip32 } from '@bitgo/secp256k1'; import _ from 'lodash'; import request from 'superagent'; import { + classifyRecoveryKeys, BaseCoin, BitGoBase, checkKrsProvider, common, ErrorNoInputToRecover, FullySignedTransaction, - getIsKrsRecovery, - getIsUnsignedSweep, HalfSignedTransaction, MPCAlgorithm, MultisigType, @@ -206,12 +205,9 @@ export class Eth extends AbstractEthLikeNewCoins { this.validateRecoveryParams(params); - // Clean up whitespace from entered values - let userKey = (params.userKey ?? '').replace(/\s/g, ''); - const backupKey = (params.backupKey ?? '').replace(/\s/g, ''); - - const isKrsRecovery = getIsKrsRecovery({ userKey, backupKey }); - const isUnsignedSweep = params.isUnsignedSweep ?? getIsUnsignedSweep({ userKey, backupKey, isTss: params.isTss }); + const recoveryKeys = classifyRecoveryKeys(params); + const { backupKey, isKrsRecovery, isUnsignedSweep } = recoveryKeys; + let userKey = recoveryKeys.userKey; if (isKrsRecovery) { checkKrsProvider(this, params.krsProvider, { checkCoinFamilySupport: false }); diff --git a/modules/sdk-coin-eth/test/unit/ethSafeRecovery.ts b/modules/sdk-coin-eth/test/unit/ethSafeRecovery.ts index 9d42ebefe18..0d10a96285a 100644 --- a/modules/sdk-coin-eth/test/unit/ethSafeRecovery.ts +++ b/modules/sdk-coin-eth/test/unit/ethSafeRecovery.ts @@ -305,6 +305,24 @@ describe('Safe EVM TSS recovery end-to-end', function () { pendingBalanceMocks().should.have.length(2); }); + it('rejects an unsigned sweep with no key material at all', async function () { + // Also pins that validateUnsignedSweepTSSParams is awaited: before that fix this threw + // inside an un-awaited promise, so the caller saw a downstream BigInt error instead and + // the real rejection surfaced as an unhandled rejection. + await basecoin + .recover({ + userKey: '', + backupKey: '', + isTss: true, + walletContractAddress, + recoveryDestination, + gasLimit: 200000, + eip1559: { maxFeePerGas: 20000000000, maxPriorityFeePerGas: 10000000000 }, + replayProtectionOptions: { chain: '560048', hardfork: 'london' }, + }) + .should.be.rejectedWith('missing commonKeyChain'); + }); + it('surfaces an explorer failure rather than reporting an empty wallet', async function () { const keyParams = await unsignedKeyParams(safeKeys); nock(baseUrl) diff --git a/modules/sdk-coin-tempo/src/tempo.ts b/modules/sdk-coin-tempo/src/tempo.ts index f61f13bed0e..5025754f7fe 100644 --- a/modules/sdk-coin-tempo/src/tempo.ts +++ b/modules/sdk-coin-tempo/src/tempo.ts @@ -500,6 +500,7 @@ export class Tempo extends AbstractEthLikeNewCoins { const derivedCommonKeyChain = MPC.deriveUnhardened(commonKeyChain, 'm/0'); const backupKeyPair = new KeyPair({ pub: derivedCommonKeyChain.slice(0, 66) }); const baseAddress = backupKeyPair.getAddress(); + this.assertTssBaseAddressMatchesWallet(baseAddress, params); const { tx: unsignedTx } = await this.buildTempoSweepTx(baseAddress, params); const serializedHex = await unsignedTx.serialize(); @@ -536,6 +537,11 @@ export class Tempo extends AbstractEthLikeNewCoins { const derivedCommonKeyChain = MPC.deriveUnhardened(backupKey, derivationPath); const backupKeyPair = new KeyPair({ pub: derivedCommonKeyChain.slice(0, 66) }); const baseAddress = backupKeyPair.getAddress(); + // Fail closed on a wrong keychain, as the base class does. A derivationSeed sweeps a + // pseudorandom child the caller cannot predict, so there is nothing to compare against. + if (!params.derivationSeed) { + this.assertTssBaseAddressMatchesWallet(baseAddress, params); + } const { tx, nonce, sweepAmount, gasMarginUnits } = await this.buildTempoSweepTx(baseAddress, params); const serializedHex = await tx.serialize(); diff --git a/modules/sdk-coin-tempo/test/unit/tempo.ts b/modules/sdk-coin-tempo/test/unit/tempo.ts index 8aa88448ae0..369e1be8269 100644 --- a/modules/sdk-coin-tempo/test/unit/tempo.ts +++ b/modules/sdk-coin-tempo/test/unit/tempo.ts @@ -2,7 +2,7 @@ import sinon from 'sinon'; import should from 'should'; import { BitGoAPI } from '@bitgo/sdk-api'; import { TestBitGo, TestBitGoAPI } from '@bitgo/sdk-test'; -import { BitGoBase } from '@bitgo/sdk-core'; +import { BitGoBase, RecoveryKeyMismatchError } from '@bitgo/sdk-core'; import { BaseCoin as StaticsBaseCoin, CoinFamily } from '@bitgo/statics'; import { Tempo } from '../../src/tempo'; const PATH_USD_ADDRESS = '0x20c0000000000000000000000000000000000000'; @@ -12,6 +12,9 @@ const TEST_BACKUP_KEY = '0279be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798' + '0000000000000000000000000000000000000000000000000000000000000001'; const TEST_RECOVERY_DESTINATION = '0x80151ebf635e6ec8a5455258f617be6cda1fbd7e'; +// the m/0 address TEST_BACKUP_KEY derives; recover() always supplies this, so these direct +// calls to the protected builder must too +const TEST_WALLET_BASE_ADDRESS = '0x19332ae0650de6a555f096057603fdc0c821c1f0'; describe('Tempo Recovery', function () { let bitgo: TestBitGoAPI; @@ -140,6 +143,7 @@ describe('Tempo Recovery', function () { const result = (await (basecoin as any).buildUnsignedSweepTxnTSS({ backupKey: TEST_BACKUP_KEY, recoveryDestination: TEST_RECOVERY_DESTINATION, + walletContractAddress: TEST_WALLET_BASE_ADDRESS, tokenContractAddress: PATH_USD_ADDRESS, gasLimit: 1_000_000, eip1559: { maxFeePerGas: 20_000_000_000, maxPriorityFeePerGas: 10_000_000_000 }, @@ -155,6 +159,7 @@ describe('Tempo Recovery', function () { const result = (await (basecoin as any).buildUnsignedSweepTxnTSS({ backupKey: TEST_BACKUP_KEY, recoveryDestination: TEST_RECOVERY_DESTINATION, + walletContractAddress: TEST_WALLET_BASE_ADDRESS, tokenContractAddress: PATH_USD_ADDRESS, gasLimit: 500_000, eip1559: { maxFeePerGas: 20_000_000_000, maxPriorityFeePerGas: 10_000_000_000 }, @@ -169,6 +174,7 @@ describe('Tempo Recovery', function () { .buildUnsignedSweepTxnTSS({ backupKey: TEST_BACKUP_KEY, recoveryDestination: TEST_RECOVERY_DESTINATION, + walletContractAddress: TEST_WALLET_BASE_ADDRESS, tokenContractAddress: PATH_USD_ADDRESS, gasLimit: 1_000_000, eip1559: { maxFeePerGas: 20_000_000_000, maxPriorityFeePerGas: 10_000_000_000 }, @@ -182,6 +188,7 @@ describe('Tempo Recovery', function () { const result = (await (basecoin as any).buildUnsignedSweepTxnTSS({ backupKey: TEST_BACKUP_KEY, recoveryDestination: TEST_RECOVERY_DESTINATION, + walletContractAddress: TEST_WALLET_BASE_ADDRESS, tokenContractAddress: otherToken, gasLimit: 1_000_000, eip1559: { maxFeePerGas: 20_000_000_000, maxPriorityFeePerGas: 10_000_000_000 }, @@ -195,6 +202,7 @@ describe('Tempo Recovery', function () { const result = (await (basecoin as any).buildUnsignedSweepTxnTSS({ backupKey: TEST_BACKUP_KEY, recoveryDestination: TEST_RECOVERY_DESTINATION, + walletContractAddress: TEST_WALLET_BASE_ADDRESS, tokenContractAddress: PATH_USD_ADDRESS, gasLimit: 1_000_000, eip1559: { maxFeePerGas: 20_000_000_000, maxPriorityFeePerGas: 10_000_000_000 }, @@ -205,6 +213,29 @@ describe('Tempo Recovery', function () { tx.signatureShares.should.deepEqual([]); tx.nonce.should.equal(0); }); + + it('fails closed with RecoveryKeyMismatchError when the keychain is for another wallet', async function () { + stubBalanceAndNonce('1000000'); + await (basecoin as any) + .buildUnsignedSweepTxnTSS({ + backupKey: TEST_BACKUP_KEY, + recoveryDestination: TEST_RECOVERY_DESTINATION, + walletContractAddress: '0xe740412ab65efdbdbe2a5e4c6e01d7bbc54a38a9', + tokenContractAddress: PATH_USD_ADDRESS, + gasLimit: 1_000_000, + eip1559: { maxFeePerGas: 20_000_000_000, maxPriorityFeePerGas: 10_000_000_000 }, + }) + .should.be.rejectedWith(RecoveryKeyMismatchError); + }); + + it('rejects a sweep with no key material rather than deriving from an empty keychain', async function () { + await (basecoin as any) + .buildUnsignedSweepTxnTSS({ + recoveryDestination: TEST_RECOVERY_DESTINATION, + walletContractAddress: TEST_WALLET_BASE_ADDRESS, + }) + .should.be.rejectedWith('missing commonKeyChain'); + }); }); describe('recoverTSS — unsigned sweep dispatch', function () { @@ -217,6 +248,7 @@ describe('Tempo Recovery', function () { backupKey: TEST_BACKUP_KEY, isTss: true, recoveryDestination: TEST_RECOVERY_DESTINATION, + walletContractAddress: TEST_WALLET_BASE_ADDRESS, }); buildStub.calledOnce.should.equal(true); diff --git a/modules/sdk-core/src/bitgo/recovery/initiate.ts b/modules/sdk-core/src/bitgo/recovery/initiate.ts index 79c65959d8b..67902dc7d63 100644 --- a/modules/sdk-core/src/bitgo/recovery/initiate.ts +++ b/modules/sdk-core/src/bitgo/recovery/initiate.ts @@ -101,6 +101,33 @@ export function getIsUnsignedSweep({ return backupKey.startsWith('xpub') && userKey.startsWith('xpub'); } +/** + * Resolves the key-derived facts every EVM-like `recover()` needs, in one place. + * + * The two classifications must run against the RAW keys: stripping whitespace first flips the + * result for a key pasted with a leading newline, which would silently reroute a KRS or signed + * recovery into an unsigned sweep. The stripped keys are what callers should actually use, so + * both are returned together and the pairing cannot drift apart again. + * + * @param params - the recovery options, whose keys may be absent on an unsigned TSS sweep + * @returns whitespace-stripped keys alongside the sweep/KRS classification + */ +export function classifyRecoveryKeys(params: { + userKey?: string; + backupKey?: string; + isTss?: boolean; + isUnsignedSweep?: boolean; +}): { userKey: string; backupKey: string; isKrsRecovery: boolean; isUnsignedSweep: boolean } { + const userKey = params.userKey ?? ''; + const backupKey = params.backupKey ?? ''; + return { + userKey: userKey.replace(/\s/g, ''), + backupKey: backupKey.replace(/\s/g, ''), + isKrsRecovery: getIsKrsRecovery({ userKey, backupKey }), + isUnsignedSweep: params.isUnsignedSweep ?? getIsUnsignedSweep({ userKey, backupKey, isTss: params.isTss }), + }; +} + function parseKeyAsXprv(key: string, source: string): BIP32Interface { try { return bip32.fromBase58(key);