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..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, @@ -284,8 +285,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 +1348,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)) { @@ -1506,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); @@ -1545,6 +1558,15 @@ 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) { @@ -1554,7 +1576,7 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin implem gasLimit: params.gasLimit, recoveryDestination: params.recoveryDestination || '', userKey: params.userKey || '', - walletContractAddress: address, + walletContractAddress: tssBaseAddress ?? address, derivationSeed: '', isTss: params.isTss, eip1559: { @@ -1565,7 +1587,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 +1642,9 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin implem } this.validateRecoveryParams(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 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) @@ -1898,7 +1918,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 +2416,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 +2441,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 +2463,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 +2525,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 +2549,7 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin implem gasLimit, params.eip1559, params.replayProtectionOptions, - recoverParams.backupKey as string + commonKeyChain ); } @@ -2562,7 +2621,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..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,11 +213,9 @@ export class EthLikeToken extends AbstractEthLikeNewCoins { throw new Error('invalid recoveryDestination'); } - 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 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-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..a08476e1d56 100644 --- a/modules/sdk-coin-avaxc/src/avaxc.ts +++ b/modules/sdk-coin-avaxc/src/avaxc.ts @@ -15,13 +15,14 @@ import { EthereumNetwork, } from '@bitgo/statics'; import { + classifyRecoveryKeys, BaseCoin, BaseTransaction, BitGoBase, common, + ErrorNoInputToRecover, FeeEstimateOptions, FullySignedTransaction, - getIsUnsignedSweep, InvalidAddressError, IWallet, KeyPair, @@ -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,9 @@ export class AvaxC extends AbstractEthLikeNewCoins { throw new Error('invalid recoveryDestination'); } - // TODO (BG-56531): add support for krs - const isUnsignedSweep = getIsUnsignedSweep(params); - - // Clean up whitespace from entered values - let userKey = params.userKey.replace(/\s/g, ''); - const backupKey = params.backupKey.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)); @@ -677,6 +704,13 @@ export class AvaxC extends AbstractEthLikeNewCoins { } backupKeyAddress = keyPair.getAddress(); } + await this.assertRecoveryKeysAreWalletSigners( + userKey, + backupKeyAddress, + params.walletContractAddress, + params.apiKey + ); + const backupKeyNonce = await this.getAddressNonce(backupKeyAddress); // get balance of backupKey to ensure funds are available to pay fees @@ -703,6 +737,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..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, @@ -79,11 +79,10 @@ export class Etc extends AbstractEthLikeCoin { */ async recover(params: RecoverOptions): Promise { this.validateRecoveryParams(params); - const isUnsignedSweep = getIsUnsignedSweep(params); - // Clean up whitespace from entered values - let userKey = params.userKey.replace(/\s/g, ''); - const backupKey = params.backupKey.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/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..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,17 +151,14 @@ export class Erc20Token extends Eth { throw new Error('invalid recoveryDestination'); } - const isKrsRecovery = getIsKrsRecovery(params); - const isUnsignedSweep = params.isUnsignedSweep ?? getIsUnsignedSweep(params); + const recoveryKeys = classifyRecoveryKeys(params); + const { backupKey, isKrsRecovery, isUnsignedSweep } = recoveryKeys; + let userKey = recoveryKeys.userKey; 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..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,17 +151,14 @@ export class Erc721Token extends Eth { throw new Error('invalid recoveryDestination'); } - const isKrsRecovery = getIsKrsRecovery(params); - const isUnsignedSweep = params.isUnsignedSweep ?? getIsUnsignedSweep(params); + const recoveryKeys = classifyRecoveryKeys(params); + const { backupKey, isKrsRecovery, isUnsignedSweep } = recoveryKeys; + let userKey = recoveryKeys.userKey; 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..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,16 +949,14 @@ export class Erc7984Token extends Eth { throw new Error('invalid recoveryDestination'); } - const isKrsRecovery = getIsKrsRecovery(params); - const isUnsignedSweep = params.isUnsignedSweep ?? getIsUnsignedSweep(params); + const recoveryKeys = classifyRecoveryKeys(params); + const { backupKey, isKrsRecovery, isUnsignedSweep } = recoveryKeys; + let userKey = recoveryKeys.userKey; 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..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, @@ -205,17 +204,15 @@ export class Eth extends AbstractEthLikeNewCoins { } this.validateRecoveryParams(params); - const isKrsRecovery = getIsKrsRecovery(params); - const isUnsignedSweep = params.isUnsignedSweep ?? getIsUnsignedSweep(params); + + const recoveryKeys = classifyRecoveryKeys(params); + const { backupKey, isKrsRecovery, isUnsignedSweep } = recoveryKeys; + let userKey = recoveryKeys.userKey; 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..0d10a96285a --- /dev/null +++ b/modules/sdk-coin-eth/test/unit/ethSafeRecovery.ts @@ -0,0 +1,353 @@ +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('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) + .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..5025754f7fe 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); @@ -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(); @@ -527,12 +528,20 @@ 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); 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);