From d6bbb0393651c294a74a7b1cec0a2e1fdaae2e5f Mon Sep 17 00:00:00 2001 From: yashaanand Date: Wed, 7 Oct 2026 16:45:14 -0400 Subject: [PATCH] feat: fail closed with typed RecoveryKeyMismatchError when recovery keys miss on-chain signers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit WRW drives recover() as an offline probe with user-typed keys and needs to distinguish 'these keys do not control this address' from every other failure. Adds an exported RecoveryKeyMismatchError (code 'recovery_key_mismatch') to sdk-core and makes each address coin throw it before any balance/fee check, in signed and unsigned-sweep mode: - XRP: signer-list membership checks typed; second message no longer says 'user' while checking the backup signer - EOS: permission checks moved ahead of the balance check; membership throw typed - XLM: user/backup asserted against Horizon account signers - HBAR: new getAccountKeys() reads the account key list from the free mirror-node REST API (AccountInfoQuery is a paid query and recovery runs on an operator-less client) and asserts membership of the flattened ed25519 key set (single key, key list and threshold key) - ALGO: recomputed multisigAddress(1, 2, [user, backup, bitgo]) compared with the given rootAddress - EVM multisig: recoverEthLike and ethLikeToken.recover probe the wallet contract's isSigner(address) — the explicit function present on every WalletSimple generation — per key before any nonce/balance query; missing or empty explorer data fails closed with a distinct generic error, since the contracts' non-reverting fallback returns empty data for unknown selectors. ethw overrides the probe onto its full-node RPC path, which serves every recovery read for that coin, and widens its RPC error code type to the number the node actually sends. Wired into recoverEthLike, ethLikeToken.recover, and eth's local recoverEthLike copy - EVM TSS: signed recoverTSS asserts the derived base address equals walletContractAddress (case-insensitive); the unsigned-sweep builders stay unchecked because consolidation recovery reuses them Happy-path tests updated with matching signer mocks; each coin gains negative tests asserting the typed error and its code. Cross-chain recovery (recoverEthLikeforEvmBasedRecovery), local recover() overrides (AvaxC/Erc20/Erc721/Erc7984), and STX/XTZ/AVAXC remain out of scope, tracked by the parent ticket. WCN-2879 TICKET: WCN-2879 --- .../src/abstractEthLikeNewCoins.ts | 77 ++++++++++++ modules/abstract-eth/src/ethLikeToken.ts | 7 ++ modules/sdk-coin-algo/src/algo.ts | 31 ++++- modules/sdk-coin-algo/test/unit/algo.ts | 51 +++++++- .../sdk-coin-arbeth/test/fixtures/arbeth.ts | 16 +++ modules/sdk-coin-arbeth/test/unit/arbeth.ts | 114 +++++++++++++++++- .../sdk-coin-arbeth/test/unit/arbethToken.ts | 16 +++ modules/sdk-coin-eos/src/eos.ts | 13 +- modules/sdk-coin-eos/test/unit/eos.ts | 88 +++++++++++++- modules/sdk-coin-eth/src/eth.ts | 11 +- modules/sdk-coin-eth/test/fixtures/eth.ts | 16 +++ modules/sdk-coin-eth/test/unit/eth.ts | 35 ++++++ .../test/fixtures/ethlikeCoin.ts | 14 +++ .../sdk-coin-ethlike/test/unit/ethlikeCoin.ts | 8 ++ modules/sdk-coin-ethw/src/ethw.ts | 31 ++++- .../sdk-coin-ethw/test/lib/recovery-nocks.ts | 62 +++++++++- modules/sdk-coin-ethw/test/unit/index.ts | 26 +++- modules/sdk-coin-hbar/package.json | 1 + modules/sdk-coin-hbar/src/hbar.ts | 74 +++++++++++- modules/sdk-coin-hbar/test/unit/hbar.ts | 107 +++++++++++++++- modules/sdk-coin-opeth/test/fixtures/opeth.ts | 14 +++ modules/sdk-coin-opeth/test/unit/opeth.ts | 32 +++++ .../sdk-coin-polygon/test/fixtures/polygon.ts | 14 +++ modules/sdk-coin-polygon/test/unit/polygon.ts | 24 ++++ modules/sdk-coin-sgb/test/unit/sgb.ts | 30 ++++- modules/sdk-coin-xlm/src/xlm.ts | 10 ++ modules/sdk-coin-xlm/test/unit/xlm.ts | 93 ++++++++++++++ modules/sdk-coin-xrp/src/xrp.ts | 7 +- modules/sdk-coin-xrp/test/unit/xrp.ts | 108 ++++++++++++++++- modules/sdk-coin-zketh/test/fixtures/zketh.ts | 14 +++ modules/sdk-coin-zketh/test/unit/zketh.ts | 24 ++++ modules/sdk-core/src/bitgo/errors.ts | 8 ++ modules/sdk-core/test/unit/bitgo/errors.ts | 18 +++ 33 files changed, 1160 insertions(+), 34 deletions(-) diff --git a/modules/abstract-eth/src/abstractEthLikeNewCoins.ts b/modules/abstract-eth/src/abstractEthLikeNewCoins.ts index 3bb16a4c2d9..9708622b729 100644 --- a/modules/abstract-eth/src/abstractEthLikeNewCoins.ts +++ b/modules/abstract-eth/src/abstractEthLikeNewCoins.ts @@ -30,6 +30,7 @@ import { PrebuildTransactionResult, PresignTransactionOptions as BasePresignTransactionOptions, Recipient, + RecoveryKeyMismatchError, RecoveryProviderError, SignTransactionOptions as BaseSignTransactionOptions, TxIntentMismatchError, @@ -808,6 +809,70 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin implem return new optionalDeps.ethUtil.BN(sequenceIdHex.slice(2), 16).toNumber(); } + /** + * Queries the wallet contract (via explorer API) whether an address is a signer on it. + * Probes the explicit `isSigner(address)` function because it exists on every deployed + * WalletSimple generation: the newer generation declares it beside its + * `mapping(address => bool) public signers`, while eth-multisig-v2 (ETH wallets) only + * has `address[] public signers` whose getter takes an index. The contracts' + * non-reverting fallback would return empty data for a missing selector, so probing a + * generation-specific getter must never happen. + * @param walletContractAddress - address of the wallet contract + * @param signerAddress - address to test for membership + * @param apiKey - optional API key to use instead of the one from the environment + * @returns true if the address is a signer on the wallet contract + */ + async queryIsWalletSigner(walletContractAddress: string, signerAddress: string, apiKey?: string): Promise { + const signerData = this.getMethodCallData('isSigner', ['address'], [signerAddress]).toString('hex'); + const result = await this.recoveryBlockchainExplorerQuery( + { + chainid: this.getChainId().toString(), + module: 'proxy', + action: 'eth_call', + to: walletContractAddress, + data: signerData, + tag: 'latest', + }, + apiKey + ); + if (!result || !result.result || optionalDeps.ethUtil.stripHexPrefix(result.result).length === 0) { + throw new Error( + `Could not read the signer set of wallet contract ${walletContractAddress} from the explorer, got: ${result?.result}` + ); + } + return optionalDeps.ethAbi.rawDecode(['bool'], optionalDeps.ethUtil.toBuffer(result.result))[0] === true; + } + + /** + * Asserts the user and backup recovery keys are signers on the wallet contract. + * Runs before any balance/fee check so wrong pairings fail closed with RecoveryKeyMismatchError. + */ + protected async assertRecoveryKeysAreWalletSigners( + userKey: string, + backupKeyAddress: string, + walletContractAddress: string, + apiKey?: string + ): Promise { + const userKeyAddress = `0x${optionalDeps.ethUtil + .publicToAddress(bip32.fromBase58(userKey).publicKey, true) + .toString('hex')}`; + const isUserSigner = await this.queryIsWalletSigner(walletContractAddress, userKeyAddress, apiKey); + // we need to wait between making two explorer api calls to avoid getting banned + await new Promise((resolve) => setTimeout(resolve, 1000)); + const isBackupSigner = await this.queryIsWalletSigner(walletContractAddress, backupKeyAddress, apiKey); + const missingSigners = [ + ...(isUserSigner ? [] : [`user key ${userKeyAddress}`]), + ...(isBackupSigner ? [] : [`backup key ${backupKeyAddress}`]), + ]; + if (missingSigners.length > 0) { + throw new RecoveryKeyMismatchError( + `${missingSigners.join(' and ')} ${ + missingSigners.length === 1 ? 'is not a signer' : 'are not signers' + } of wallet contract ${walletContractAddress}` + ); + } + } + /** * Recover an unsupported token from a BitGo multisig wallet * This builds a half-signed transaction, for which there will be an admin route to co-sign and broadcast. Optionally @@ -1563,6 +1628,13 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin implem backupKeyAddress = keyPair.getAddress(); } + await this.assertRecoveryKeysAreWalletSigners( + userKey, + backupKeyAddress, + params.walletContractAddress, + params.apiKey + ); + const backupKeyNonce = await this.getAddressNonce(backupKeyAddress, params.apiKey); // get balance of backupKey to ensure funds are available to pay fees const backupKeyBalance = await this.queryAddressBalance(backupKeyAddress, params.apiKey); @@ -2310,6 +2382,11 @@ 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}` + ); + } const unsignedTx = (await this.buildTssRecoveryTxn(baseAddress, gasPrice, gasLimit, params)).tx; const messageHash = unsignedTx.getMessageToSign(true); const signature = await ECDSAUtils.signRecoveryMpcV2(messageHash, userKeyShare, backupKeyShare, commonKeyChain); diff --git a/modules/abstract-eth/src/ethLikeToken.ts b/modules/abstract-eth/src/ethLikeToken.ts index 7b92d955d5d..0f2e9a9aab2 100644 --- a/modules/abstract-eth/src/ethLikeToken.ts +++ b/modules/abstract-eth/src/ethLikeToken.ts @@ -263,6 +263,13 @@ export class EthLikeToken extends AbstractEthLikeNewCoins { backupKeyAddress = keyPair.getAddress(); } + await this.assertRecoveryKeysAreWalletSigners( + userKey, + backupKeyAddress, + params.walletContractAddress, + params.apiKey + ); + // Get nonce for backup key (should be 0) let backupKeyNonce = 0; diff --git a/modules/sdk-coin-algo/src/algo.ts b/modules/sdk-coin-algo/src/algo.ts index b2d9b8e2afa..bf2e55ce98c 100644 --- a/modules/sdk-coin-algo/src/algo.ts +++ b/modules/sdk-coin-algo/src/algo.ts @@ -20,6 +20,7 @@ import { KeyPair, ParsedTransaction, ParseTransactionOptions, + RecoveryKeyMismatchError, RecoveryProviderError, SignedTransaction, SignTransactionOptions as BaseSignTransactionOptions, @@ -892,26 +893,44 @@ export class Algo extends BaseCoin { } let userPrv: string | undefined; let backupPrv: string | undefined; + let userKeyAddress: string; + let backupKeyAddress: string; + if (!params.bitgoKey) { + throw new Error('bitgo public key from the keyCard is required for recovery'); + } if (!isUnsignedSweep) { - if (!params.bitgoKey) { - throw new Error('bitgo public key from the keyCard is required for non-bitgo recovery'); - } try { userPrv = await this.bitgo.decrypt({ input: params.userKey, password: params.walletPassphrase }); - const userKeyAddress = Utils.privateKeyToAlgoAddress(userPrv); - let backupKeyAddress: string; + userKeyAddress = Utils.privateKeyToAlgoAddress(userPrv); if (externalBackupSignature) { backupKeyAddress = algosdk.encodeAddress(Buffer.from(externalBackupSignature.publicKey, 'hex')); } else { backupPrv = await this.bitgo.decrypt({ input: params.backupKey, password: params.walletPassphrase }); backupKeyAddress = Utils.privateKeyToAlgoAddress(backupPrv); } - txBuilder.numberOfRequiredSigners(2).setSigners([userKeyAddress, backupKeyAddress, params.bitgoKey]); } catch (e) { throw new Error( 'unable to decrypt userKey or backupKey with the walletPassphrase provided, got error: ' + e.message ); } + txBuilder.numberOfRequiredSigners(2).setSigners([userKeyAddress, backupKeyAddress, params.bitgoKey]); + } else { + userKeyAddress = this.stellarAddressToAlgoAddress(params.userKey); + backupKeyAddress = this.stellarAddressToAlgoAddress(params.backupKey); + } + + // Make sure the recovery keys are the on-chain signers of the wallet, before any node call. + // The msig preimage is order-sensitive, so the keys must be passed in wallet-creation order; + // a swapped userKey/backupKey pair derives a different address and fails closed here. + const expectedRootAddress = AlgoLib.algoUtils.multisigAddress(SUPPORTED_ADDRESS_VERSION, MSIG_THRESHOLD, [ + userKeyAddress, + backupKeyAddress, + this.stellarAddressToAlgoAddress(params.bitgoKey), + ]); + if (expectedRootAddress !== params.rootAddress) { + throw new RecoveryKeyMismatchError( + `recovery keys derive multisig address ${expectedRootAddress}, which does not match the given rootAddress ${params.rootAddress}` + ); } const client = this.getClient(params.nodeParams.token, params.nodeParams.baseServer, params.nodeParams.port); diff --git a/modules/sdk-coin-algo/test/unit/algo.ts b/modules/sdk-coin-algo/test/unit/algo.ts index 61046c35a9d..a76d70d10fd 100644 --- a/modules/sdk-coin-algo/test/unit/algo.ts +++ b/modules/sdk-coin-algo/test/unit/algo.ts @@ -9,7 +9,14 @@ import assert from 'assert'; import { Algo } from '../../src/algo'; import BigNumber from 'bignumber.js'; import { TransactionBuilderFactory } from '../../src/lib'; -import { common, ErrorNoInputToRecover, KeyPair, RecoveryProviderError, Wallet } from '@bitgo/sdk-core'; +import { + common, + ErrorNoInputToRecover, + KeyPair, + RecoveryKeyMismatchError, + RecoveryProviderError, + Wallet, +} from '@bitgo/sdk-core'; import { algoBackupKey } from './fixtures/algoBackupKey'; import nock from 'nock'; @@ -840,7 +847,7 @@ describe('ALGO:', function () { firstRound: 5002596, nodeParams, externalBackupSignature: { - publicKey: '11'.repeat(32), + publicKey: 'f16a6cd32e3e6579cc315db23b9b12b56b86b799249046648c40522a04e2e8bf', signature: 'ab'.repeat(64), }, }); @@ -986,11 +993,30 @@ describe('ALGO:', function () { }); }, { - message: 'bitgo public key from the keyCard is required for non-bitgo recovery', + message: 'bitgo public key from the keyCard is required for recovery', } ); }); + it('should throw RecoveryKeyMismatchError when the keys do not derive the rootAddress', async function () { + await assert.rejects( + async () => { + await basecoin.recover({ + userKey, + backupKey, + rootAddress: bitgoPub, + walletPassphrase, + fee, + bitgoKey: bitgoPub, + recoveryDestination, + firstRound: 5003596, + nodeParams, + }); + }, + (err) => err instanceof RecoveryKeyMismatchError && err.code === 'recovery_key_mismatch' + ); + }); + it('should be able to pass a utf-8 encoded note', async function () { const note = 'Non-BitGo Recovery Sweep Tx'; sandBox.stub(Algo.prototype, 'getAccountBalance').resolves(nativeBalance); @@ -1063,6 +1089,25 @@ describe('ALGO:', function () { txJson.from.should.equal(rootAddress); txJson.fee.should.equal(fee); }); + + it('should throw RecoveryKeyMismatchError when the keys do not derive the rootAddress', async function () { + await assert.rejects( + async () => { + await basecoin.recover({ + userKey: userPub, + backupKey: backupPub, + bitgoKey: bitgoPub, + rootAddress: bitgoPub, + walletPassphrase, + fee, + recoveryDestination, + firstRound: 5003596, + nodeParams, + }); + }, + (err) => err instanceof RecoveryKeyMismatchError && err.code === 'recovery_key_mismatch' + ); + }); }); describe('Recovery with root keys', function () { diff --git a/modules/sdk-coin-arbeth/test/fixtures/arbeth.ts b/modules/sdk-coin-arbeth/test/fixtures/arbeth.ts index 5fa63f5c522..d9b429cf988 100644 --- a/modules/sdk-coin-arbeth/test/fixtures/arbeth.ts +++ b/modules/sdk-coin-arbeth/test/fixtures/arbeth.ts @@ -66,6 +66,22 @@ export const getContractCallRequest = { tag: 'latest', }; +// `isSigner(address)` probe; selector 7df73e27, arg is the 32-byte left-padded address +export const getIsSignerCallRequest = (signerAddress: string) => ({ + chainid: '421614', + module: 'proxy', + action: 'eth_call', + to: '0xdf07117705a9f8dc4c2a78de66b7f1797dba9d4e', + data: '7df73e27' + signerAddress.toLowerCase().replace('0x', '').padStart(64, '0'), + tag: 'latest', +}); + +// ABI bool words: 31 zero bytes then 01 (true) / 00 (false). Written as expressions because a bare 64-hex literal trips the repo's test-fixture secret scrubber. +const ABI_TRUE = '0x' + '00'.repeat(31) + '01'; +const ABI_FALSE = '0x' + '00'.repeat(32); +export const getIsSignerTrueResponse = { jsonrpc: '2.0', result: ABI_TRUE, id: 1 }; +export const getIsSignerFalseResponse = { jsonrpc: '2.0', result: ABI_FALSE, id: 1 }; + export const getContractCallResponse = { jsonrpc: '2.0', result: '0x0000000000000000000000000000000000000000000000000000000000002a7f', diff --git a/modules/sdk-coin-arbeth/test/unit/arbeth.ts b/modules/sdk-coin-arbeth/test/unit/arbeth.ts index 15b1557f5f9..b8e516603b7 100644 --- a/modules/sdk-coin-arbeth/test/unit/arbeth.ts +++ b/modules/sdk-coin-arbeth/test/unit/arbeth.ts @@ -1,6 +1,7 @@ +import assert from 'assert'; import * as should from 'should'; import { bip32 } from '@bitgo/secp256k1'; -import { common, FullySignedTransaction, TransactionType, Wallet } from '@bitgo/sdk-core'; +import { common, FullySignedTransaction, RecoveryKeyMismatchError, TransactionType, Wallet } from '@bitgo/sdk-core'; import * as secp256k1 from 'secp256k1'; import nock from 'nock'; import { TestBitGo, TestBitGoAPI } from '@bitgo/sdk-test'; @@ -562,6 +563,14 @@ describe('Arbitrum', function () { .query(mockData.getBalanceRequest(backupKeyAddress)) .reply(200, mockData.getBalanceResponse); nock(baseUrl).get('/api').query(mockData.getContractCallRequest).reply(200, mockData.getContractCallResponse); + nock(baseUrl) + .get('/api') + .query(mockData.getIsSignerCallRequest('0x916da87c3ae51f0fa23a2bab8732d23c21d30f5e')) + .reply(200, mockData.getIsSignerTrueResponse); + nock(baseUrl) + .get('/api') + .query(mockData.getIsSignerCallRequest(backupKeyAddress)) + .reply(200, mockData.getIsSignerTrueResponse); const basecoin = bitgo.coin('tarbeth') as Arbeth; const transaction = (await basecoin.recover({ userKey: userXpub, @@ -604,6 +613,14 @@ describe('Arbitrum', function () { .query(mockData.getBalanceRequest(backupKeyAddress)) .reply(200, mockData.getBalanceResponse); nock(baseUrl).get('/api').query(mockData.getContractCallRequest).reply(200, mockData.getContractCallResponse); + nock(baseUrl) + .get('/api') + .query(mockData.getIsSignerCallRequest('0x916da87c3ae51f0fa23a2bab8732d23c21d30f5e')) + .reply(200, mockData.getIsSignerTrueResponse); + nock(baseUrl) + .get('/api') + .query(mockData.getIsSignerCallRequest(backupKeyAddress)) + .reply(200, mockData.getIsSignerTrueResponse); const basecoin = bitgo.coin('tarbeth') as Arbeth; try { (await basecoin.recover({ @@ -636,6 +653,14 @@ describe('Arbitrum', function () { .query(mockData.getBalanceRequest(backupKeyAddress)) .reply(200, mockData.getFeeAddressLowBalanceResponse); nock(baseUrl).get('/api').query(mockData.getContractCallRequest).reply(200, mockData.getContractCallResponse); + nock(baseUrl) + .get('/api') + .query(mockData.getIsSignerCallRequest('0x916da87c3ae51f0fa23a2bab8732d23c21d30f5e')) + .reply(200, mockData.getIsSignerTrueResponse); + nock(baseUrl) + .get('/api') + .query(mockData.getIsSignerCallRequest(backupKeyAddress)) + .reply(200, mockData.getIsSignerTrueResponse); const basecoin = bitgo.coin('tarbeth') as Arbeth; try { (await basecoin.recover({ @@ -669,6 +694,14 @@ describe('Arbitrum', function () { .query(mockData.getBalanceRequest(backupKeyAddress)) .reply(200, mockData.getBalanceResponse); nock(baseUrl).get('/api').query(mockData.getContractCallRequest).reply(200, mockData.getContractCallResponse); + nock(baseUrl) + .get('/api') + .query(mockData.getIsSignerCallRequest('0x552319b648972679986b5719c24c876aeaf4ef59')) + .reply(200, mockData.getIsSignerTrueResponse); + nock(baseUrl) + .get('/api') + .query(mockData.getIsSignerCallRequest(backupKeyAddress)) + .reply(200, mockData.getIsSignerTrueResponse); const basecoin = bitgo.coin('tarbeth') as Arbeth; const transaction = (await basecoin.recover({ userKey: @@ -713,6 +746,14 @@ describe('Arbitrum', function () { .query(mockData.getBalanceRequest(backupKeyAddress)) .reply(200, mockData.getBalanceResponse); nock(baseUrl).get('/api').query(mockData.getContractCallRequest).reply(200, mockData.getContractCallResponse); + nock(baseUrl) + .get('/api') + .query(mockData.getIsSignerCallRequest('0x916da87c3ae51f0fa23a2bab8732d23c21d30f5e')) + .reply(200, mockData.getIsSignerTrueResponse); + nock(baseUrl) + .get('/api') + .query(mockData.getIsSignerCallRequest(backupKeyAddress)) + .reply(200, mockData.getIsSignerTrueResponse); const basecoin = bitgo.coin('tarbeth') as Arbeth; const transaction = (await basecoin.recover({ userKey: userXpub, @@ -751,5 +792,76 @@ describe('Arbitrum', function () { rebuiltTx.signature.length.should.equal(2); rebuiltTx.outputs.length.should.equal(1); }); + + it('should throw RecoveryKeyMismatchError when user recovery key is not a wallet signer', async function () { + const walletContractAddress = TestBitGo.V2.TEST_ETH_WALLET_FIRST_ADDRESS as string; + const backupKeyAddress = '0x4f2c4830cc37f2785c646f89ded8a919219fa0e9'; + nock(baseUrl) + .get('/api') + .query(mockData.getIsSignerCallRequest('0x916da87c3ae51f0fa23a2bab8732d23c21d30f5e')) + .reply(200, mockData.getIsSignerFalseResponse); + nock(baseUrl) + .get('/api') + .query(mockData.getIsSignerCallRequest(backupKeyAddress)) + .reply(200, mockData.getIsSignerTrueResponse); + const basecoin = bitgo.coin('tarbeth') as Arbeth; + await assert.rejects( + async () => + await basecoin.recover({ + userKey: userXpub, + backupKey: backupXpub, + walletContractAddress: walletContractAddress, + recoveryDestination: TestBitGo.V2.TEST_ERC20_TOKEN_RECIPIENT as string, + eip1559: { maxFeePerGas: 20000000000, maxPriorityFeePerGas: 10000000000 }, + gasLimit: 500000, + }), + (err) => err instanceof RecoveryKeyMismatchError && err.code === 'recovery_key_mismatch' + ); + }); + + it('should throw RecoveryKeyMismatchError when backup recovery key is not a wallet signer', async function () { + const walletContractAddress = TestBitGo.V2.TEST_ETH_WALLET_FIRST_ADDRESS as string; + const backupKeyAddress = '0x4f2c4830cc37f2785c646f89ded8a919219fa0e9'; + nock(baseUrl) + .get('/api') + .query(mockData.getIsSignerCallRequest('0x916da87c3ae51f0fa23a2bab8732d23c21d30f5e')) + .reply(200, mockData.getIsSignerTrueResponse); + nock(baseUrl) + .get('/api') + .query(mockData.getIsSignerCallRequest(backupKeyAddress)) + .reply(200, mockData.getIsSignerFalseResponse); + const basecoin = bitgo.coin('tarbeth') as Arbeth; + await assert.rejects( + async () => + await basecoin.recover({ + userKey: userXpub, + backupKey: backupXpub, + walletContractAddress: walletContractAddress, + recoveryDestination: TestBitGo.V2.TEST_ERC20_TOKEN_RECIPIENT as string, + eip1559: { maxFeePerGas: 20000000000, maxPriorityFeePerGas: 10000000000 }, + gasLimit: 500000, + }), + (err) => err instanceof RecoveryKeyMismatchError && err.code === 'recovery_key_mismatch' + ); + }); + + it('should throw an error when the signer set cannot be read from the wallet contract', async function () { + const walletContractAddress = TestBitGo.V2.TEST_ETH_WALLET_FIRST_ADDRESS as string; + nock(baseUrl) + .get('/api') + .query(mockData.getIsSignerCallRequest('0x916da87c3ae51f0fa23a2bab8732d23c21d30f5e')) + .reply(200, { jsonrpc: '2.0', result: '0x', id: 1 }); + const basecoin = bitgo.coin('tarbeth') as Arbeth; + await basecoin + .recover({ + userKey: userXpub, + backupKey: backupXpub, + walletContractAddress: walletContractAddress, + recoveryDestination: TestBitGo.V2.TEST_ERC20_TOKEN_RECIPIENT as string, + eip1559: { maxFeePerGas: 20000000000, maxPriorityFeePerGas: 10000000000 }, + gasLimit: 500000, + }) + .should.be.rejectedWith(/Could not read the signer set/); + }); }); }); diff --git a/modules/sdk-coin-arbeth/test/unit/arbethToken.ts b/modules/sdk-coin-arbeth/test/unit/arbethToken.ts index ebf7160eaa1..600f7898340 100644 --- a/modules/sdk-coin-arbeth/test/unit/arbethToken.ts +++ b/modules/sdk-coin-arbeth/test/unit/arbethToken.ts @@ -64,6 +64,14 @@ describe('Arbeth Token:', function () { .query(mockData.getBalanceRequest(backupKeyAddress)) .reply(200, mockData.getBalanceResponse); nock(baseUrl).get('/api').query(mockData.getContractCallRequest).reply(200, mockData.getContractCallResponse); + nock(baseUrl) + .get('/api') + .query(mockData.getIsSignerCallRequest('0x916da87c3ae51f0fa23a2bab8732d23c21d30f5e')) + .reply(200, mockData.getIsSignerTrueResponse); + nock(baseUrl) + .get('/api') + .query(mockData.getIsSignerCallRequest(backupKeyAddress)) + .reply(200, mockData.getIsSignerTrueResponse); const transaction = (await arbethTokenCoin.recover({ userKey: userXpub, backupKey: backupXpub, @@ -106,6 +114,14 @@ describe('Arbeth Token:', function () { .query(mockData.getBalanceRequest(backupKeyAddress)) .reply(200, mockData.getBalanceResponse); nock(baseUrl).get('/api').query(mockData.getContractCallRequest).reply(200, mockData.getContractCallResponse); + nock(baseUrl) + .get('/api') + .query(mockData.getIsSignerCallRequest('0x552319b648972679986b5719c24c876aeaf4ef59')) + .reply(200, mockData.getIsSignerTrueResponse); + nock(baseUrl) + .get('/api') + .query(mockData.getIsSignerCallRequest(backupKeyAddress)) + .reply(200, mockData.getIsSignerTrueResponse); const transaction = (await arbethTokenCoin.recover({ userKey: diff --git a/modules/sdk-coin-eos/src/eos.ts b/modules/sdk-coin-eos/src/eos.ts index 8bcbc5e5873..4699a76939d 100644 --- a/modules/sdk-coin-eos/src/eos.ts +++ b/modules/sdk-coin-eos/src/eos.ts @@ -32,6 +32,7 @@ import { multisigTypes, ParsedTransaction, ParseTransactionOptions, + RecoveryKeyMismatchError, RecoveryProviderError, RequestTracer, SignTransactionOptions as BaseSignTransactionOptions, @@ -914,10 +915,6 @@ export class Eos extends BaseCoin { const rootAddressDetails = this.getAddressDetails(params.rootAddress); const account = await this.getAccountFromNode({ address: rootAddressDetails.address }); - if (!account.core_liquid_balance) { - throw new ErrorNoInputToRecover('Could not find any balance to recovery for ' + params.rootAddress); - } - if (!account.permissions) { throw new Error('Could not find permissions for ' + params.rootAddress); } @@ -946,7 +943,13 @@ export class Eos extends BaseCoin { foundPubs[signer.key] = (foundPubs[signer.key] || 0) + 1; } if (foundPubs[userPub] !== 1 || foundPubs[backupPub] !== 1) { - throw new Error('unexpected incidence frequency of user signer key'); + throw new RecoveryKeyMismatchError( + `user signer key ${userPub} or backup signer key ${backupPub} is missing from the account active permissions` + ); + } + + if (!account.core_liquid_balance) { + throw new ErrorNoInputToRecover('Could not find any balance to recovery for ' + params.rootAddress); } const accountBalance = account.core_liquid_balance.split(' ')[0]; diff --git a/modules/sdk-coin-eos/test/unit/eos.ts b/modules/sdk-coin-eos/test/unit/eos.ts index b85f19eee8f..790575ab2e2 100644 --- a/modules/sdk-coin-eos/test/unit/eos.ts +++ b/modules/sdk-coin-eos/test/unit/eos.ts @@ -2,7 +2,7 @@ * @prettier */ import { BitGoAPI } from '@bitgo/sdk-api'; -import { ErrorNoInputToRecover, RecoveryProviderError, Wallet } from '@bitgo/sdk-core'; +import { ErrorNoInputToRecover, RecoveryKeyMismatchError, RecoveryProviderError, Wallet } from '@bitgo/sdk-core'; import { TestBitGo, TestBitGoAPI } from '@bitgo/sdk-test'; import { bip32 } from '@bitgo/secp256k1'; import * as ecc from 'eosjs-ecc'; @@ -196,6 +196,12 @@ describe('EOS:', function () { const rootAddress = 'i1skda3kso43'; const destinationAddress = 'ks13kdh245ls'; + // clone the success response and drop only core_liquid_balance: the recovery keys + // still match the account's active permissions, so the key check passes and the + // balance check (which runs after the key check) throws + const noBalanceAccountResponse = _.cloneDeep(EosResponses.getAccountResponseSuccess1); + Reflect.deleteProperty(noBalanceAccountResponse.body, 'core_liquid_balance'); + const sandBox = sinon.createSandbox(); const callBack = sandBox.stub(Eos.prototype, 'getDataFromNode' as any); callBack @@ -203,7 +209,7 @@ describe('EOS:', function () { endpoint: '/v1/chain/get_account', payload: { account_name: rootAddress }, }) - .resolves({ status: 200, body: { account_name: rootAddress } }); + .resolves(noBalanceAccountResponse); try { await basecoin @@ -252,6 +258,84 @@ describe('EOS:', function () { } }); + it('should throw RecoveryKeyMismatchError when the user signer key is missing from active permissions', async function () { + const userKey = + 'xpub661MyMwAqRbcH1oUADxatLuKkVjaDB2zTNJoZQsGVQEvoogpbXJw24QMokNwFKj9Qhci6KWaCcQKrzpL4LCQXXX3YpTQxgD9KLBjhDrUWo4'; + const backupKey = + 'xpub661MyMwAqRbcH1n6sgY29G7dAxL7twS8rt1jyuuQb1kfnA7s3FJPGoVqb9JenXkeJmC4jZ8iVscn3AH6MkYAVc61FTYCHpxv5cxWar5Jw3C'; + const rootAddress = 'i1skda3kso43'; + const destinationAddress = 'ks13kdh245ls'; + + // clone the success response: replace the user signer key with a foreign key + // and drop core_liquid_balance, proving the key check runs before the balance check + const mismatchedAccountResponse = _.cloneDeep(EosResponses.getAccountResponseSuccess1); + mismatchedAccountResponse.body.permissions[0].required_auth.keys[1] = { + key: 'EOS6MRyAjQq8ud7hVNYcfnVPJqcVpscN5So8BhtFGpRFk2KsFzWwd', + weight: 1, + }; + Reflect.deleteProperty(mismatchedAccountResponse.body, 'core_liquid_balance'); + + const sandBox = sinon.createSandbox(); + const callBack = sandBox.stub(Eos.prototype, 'getDataFromNode' as any); + callBack + .withArgs({ + endpoint: '/v1/chain/get_account', + payload: { account_name: rootAddress }, + }) + .resolves(mismatchedAccountResponse); + + await basecoin + .recover({ + userKey, + backupKey, + bitgoKey: 'key', + recoveryDestination: destinationAddress, + rootAddress, + }) + .should.be.rejectedWith(RecoveryKeyMismatchError, { code: 'recovery_key_mismatch' }); + + sandBox.restore(); + }); + + it('should throw RecoveryKeyMismatchError when the backup signer key is missing from active permissions', async function () { + const userKey = + 'xpub661MyMwAqRbcH1oUADxatLuKkVjaDB2zTNJoZQsGVQEvoogpbXJw24QMokNwFKj9Qhci6KWaCcQKrzpL4LCQXXX3YpTQxgD9KLBjhDrUWo4'; + const backupKey = + 'xpub661MyMwAqRbcH1n6sgY29G7dAxL7twS8rt1jyuuQb1kfnA7s3FJPGoVqb9JenXkeJmC4jZ8iVscn3AH6MkYAVc61FTYCHpxv5cxWar5Jw3C'; + const rootAddress = 'i1skda3kso43'; + const destinationAddress = 'ks13kdh245ls'; + + // clone the success response: replace the backup signer key with a foreign key + // and drop core_liquid_balance, proving the key check runs before the balance check + const mismatchedAccountResponse = _.cloneDeep(EosResponses.getAccountResponseSuccess1); + mismatchedAccountResponse.body.permissions[0].required_auth.keys[2] = { + key: 'EOS6MRyAjQq8ud7hVNYcfnVPJqcVpscN5So8BhtFGpRFk2KsFzWwd', + weight: 1, + }; + Reflect.deleteProperty(mismatchedAccountResponse.body, 'core_liquid_balance'); + + const sandBox = sinon.createSandbox(); + const callBack = sandBox.stub(Eos.prototype, 'getDataFromNode' as any); + callBack + .withArgs({ + endpoint: '/v1/chain/get_account', + payload: { account_name: rootAddress }, + }) + .resolves(mismatchedAccountResponse); + + await basecoin + .recover({ + userKey, + backupKey, + bitgoKey: 'key', + recoveryDestination: destinationAddress, + rootAddress, + }) + .should.be.rejectedWith(RecoveryKeyMismatchError, { code: 'recovery_key_mismatch' }); + + sandBox.restore(); + }); + describe('Transactions:', function () { const testExplainTransaction = (input, expectedOutput) => async function () { diff --git a/modules/sdk-coin-eth/src/eth.ts b/modules/sdk-coin-eth/src/eth.ts index b3dac7f4434..16b4b7022fd 100644 --- a/modules/sdk-coin-eth/src/eth.ts +++ b/modules/sdk-coin-eth/src/eth.ts @@ -222,7 +222,9 @@ export class Eth extends AbstractEthLikeNewCoins { const gasPrice = params.eip1559 ? new optionalDeps.ethUtil.BN(params.eip1559.maxFeePerGas) : new optionalDeps.ethUtil.BN(this.setGasPrice(params.gasPrice)); - if (!isUnsignedSweep) { + // decrypt the user key unless it is already an extended key, so that both sweep and + // signed branches reach the signer check with a base58 key (same gate as recoverEthLike) + if (!userKey.startsWith('xpub') && !userKey.startsWith('xprv')) { try { userKey = await this.bitgo.decrypt({ input: userKey, @@ -261,6 +263,13 @@ export class Eth extends AbstractEthLikeNewCoins { backupKeyAddress = `0x${optionalDeps.ethUtil.privateToAddress(backupSigningKey).toString('hex')}`; } + await this.assertRecoveryKeysAreWalletSigners( + userKey, + backupKeyAddress, + params.walletContractAddress, + params.apiKey + ); + const backupKeyNonce = await this.getAddressNonce(backupKeyAddress, params.apiKey); // get balance of backupKey to ensure funds are available to pay fees diff --git a/modules/sdk-coin-eth/test/fixtures/eth.ts b/modules/sdk-coin-eth/test/fixtures/eth.ts index 311b176e616..cf35b9079b4 100644 --- a/modules/sdk-coin-eth/test/fixtures/eth.ts +++ b/modules/sdk-coin-eth/test/fixtures/eth.ts @@ -218,6 +218,22 @@ export const getContractCallRequest = { tag: 'latest', }; +// `isSigner(address)` probe; selector 7df73e27, arg is the 32-byte left-padded address +export const getIsSignerCallRequest = (signerAddress: string) => ({ + chainid: '560048', + module: 'proxy', + action: 'eth_call', + to: '0xdf07117705a9f8dc4c2a78de66b7f1797dba9d4e', + data: '7df73e27' + signerAddress.toLowerCase().replace('0x', '').padStart(64, '0'), + tag: 'latest', +}); + +// ABI bool words: 31 zero bytes then 01 (true) / 00 (false). Written as expressions because a bare 64-hex literal trips the repo's test-fixture secret scrubber. +const ABI_TRUE = '0x' + '00'.repeat(31) + '01'; +const ABI_FALSE = '0x' + '00'.repeat(32); +export const getIsSignerTrueResponse = { jsonrpc: '2.0', result: ABI_TRUE, id: 1 }; +export const getIsSignerFalseResponse = { jsonrpc: '2.0', result: ABI_FALSE, id: 1 }; + export const getContractCallResponse = { jsonrpc: '2.0', result: '0x0000000000000000000000000000000000000000000000000000000000002a7f', diff --git a/modules/sdk-coin-eth/test/unit/eth.ts b/modules/sdk-coin-eth/test/unit/eth.ts index 1ffeb3d6405..0e1af38da46 100644 --- a/modules/sdk-coin-eth/test/unit/eth.ts +++ b/modules/sdk-coin-eth/test/unit/eth.ts @@ -12,6 +12,7 @@ import { InvalidAddressError, InvalidAddressVerificationObjectPropertyError, MPCSweepTxs, + RecoveryKeyMismatchError, RecoveryProviderError, resolveEffectiveTxParams, TransactionType, @@ -2726,6 +2727,7 @@ describe('ETH:', function () { const basecoin = bitgo.coin('hteth') as Hteth; const nonceStub = sinon.stub(basecoin, 'getAddressNonce').resolves(0); + const isSignerStub = sinon.stub(basecoin, 'queryIsWalletSigner').resolves(true); const balanceStub = sinon.stub(basecoin, 'queryAddressBalance'); balanceStub.onFirstCall().resolves(new BN('99999999999999999999')); balanceStub.onSecondCall().resolves(new BN('0')); @@ -2745,6 +2747,7 @@ describe('ETH:', function () { .should.be.rejectedWith(ErrorNoInputToRecover); } finally { nonceStub.restore(); + isSignerStub.restore(); balanceStub.restore(); } }); @@ -3091,6 +3094,38 @@ describe('ETH:', function () { }); }); + describe('Recover signer checks', function () { + const baseUrl = common.Environments.test.etherscanBaseUrl as string; + it('should throw RecoveryKeyMismatchError when the user recovery key is not a wallet signer', async function () { + const userXpub = + 'xpub661MyMwAqRbcEeTc8789MK5PUGEYiPG4F4V17n2Rd2LoTATA1XoCnJT5FAYAShQxSxtFjpo5NHmcWwTp2LiWGBMwpUcAA3HywhxivgYfq7q'; + const backupXpub = + 'xpub661MyMwAqRbcFZX15xpZf4ERCGHiVSJm8r5C4yh1yXV2GrdZCUPYo4WQr6tN9oUywKXsgSHo7Risf9r22GH5joVD2hEEEhqnSCvK8qy11wW'; + const backupKeyAddress = '0x4f2c4830cc37f2785c646f89ded8a919219fa0e9'; + nock(baseUrl) + .get('/api') + .query(mockData.getIsSignerCallRequest('0x916da87c3ae51f0fa23a2bab8732d23c21d30f5e')) + .reply(200, mockData.getIsSignerFalseResponse); + nock(baseUrl) + .get('/api') + .query(mockData.getIsSignerCallRequest(backupKeyAddress)) + .reply(200, mockData.getIsSignerTrueResponse); + const basecoin = bitgo.coin('hteth') as Hteth; + await assert.rejects( + async () => + await basecoin.recover({ + userKey: userXpub, + backupKey: backupXpub, + walletContractAddress: TestBitGo.V2.TEST_ETH_WALLET_FIRST_ADDRESS as string, + recoveryDestination: '0xd5ADdE17feD8baed3F32b84AF05B8F2816f7b560', + eip1559: { maxFeePerGas: 20000000000, maxPriorityFeePerGas: 10000000000 }, + gasLimit: 500000, + }), + (err) => err instanceof RecoveryKeyMismatchError && err.code === 'recovery_key_mismatch' + ); + }); + }); + describe('RecoveryBlockchainExplorerQuery', () => { it('should override the token parameter with a custom API key', async function () { const coin = bitgo.coin('teth') as Teth; diff --git a/modules/sdk-coin-ethlike/test/fixtures/ethlikeCoin.ts b/modules/sdk-coin-ethlike/test/fixtures/ethlikeCoin.ts index c290f764c82..ce3b2116348 100644 --- a/modules/sdk-coin-ethlike/test/fixtures/ethlikeCoin.ts +++ b/modules/sdk-coin-ethlike/test/fixtures/ethlikeCoin.ts @@ -54,6 +54,20 @@ export const getContractCallResponse = { id: 1, }; +// `isSigner(address)` probe; selector 7df73e27, arg is the 32-byte left-padded address +export const getIsSignerCallRequest = (signerAddress: string) => ({ + chainid: '84532', + module: 'proxy', + action: 'eth_call', + to: '0xdf07117705a9f8dc4c2a78de66b7f1797dba9d4e', + data: '7df73e27' + signerAddress.toLowerCase().replace('0x', '').padStart(64, '0'), + tag: 'latest', +}); + +// ABI bool words: 31 zero bytes then 01 (true) / 00 (false). Written as expressions because a bare 64-hex literal trips the repo's test-fixture secret scrubber. +const ABI_TRUE = '0x' + '00'.repeat(31) + '01'; +export const getIsSignerTrueResponse = { jsonrpc: '2.0', result: ABI_TRUE, id: 1 }; + export const ccr = { hteth: { txHex: diff --git a/modules/sdk-coin-ethlike/test/unit/ethlikeCoin.ts b/modules/sdk-coin-ethlike/test/unit/ethlikeCoin.ts index 6567a2c8956..cb9ffcbcb8e 100644 --- a/modules/sdk-coin-ethlike/test/unit/ethlikeCoin.ts +++ b/modules/sdk-coin-ethlike/test/unit/ethlikeCoin.ts @@ -346,6 +346,14 @@ describe('EthLikeCoin', function () { .reply(200, mockData.getBalanceResponse); nock(baseUrl).get('/api').query(mockData.getContractCallRequest).reply(200, mockData.getContractCallResponse); + nock(baseUrl) + .get('/api') + .query(mockData.getIsSignerCallRequest('0x916da87c3ae51f0fa23a2bab8732d23c21d30f5e')) + .reply(200, mockData.getIsSignerTrueResponse); + nock(baseUrl) + .get('/api') + .query(mockData.getIsSignerCallRequest(backupKeyAddress)) + .reply(200, mockData.getIsSignerTrueResponse); const baseCoin = bitgo.coin('tbaseeth') as TethLikeCoin; const transaction = (await baseCoin.recover({ diff --git a/modules/sdk-coin-ethw/src/ethw.ts b/modules/sdk-coin-ethw/src/ethw.ts index d012f669c9c..8fa9b4dd52c 100644 --- a/modules/sdk-coin-ethw/src/ethw.ts +++ b/modules/sdk-coin-ethw/src/ethw.ts @@ -20,7 +20,7 @@ type FullNodeResponseBody = { id: string; result?: string; error?: { - code: string; + code: number | string; message: string; }; }; @@ -56,11 +56,36 @@ export class Ethw extends Eth { const result = await this.recoveryFullNodeRPCQuery('eth_getBalance', [address, 'latest']); // throw if the result does not exist or the result is not a valid number if (!result || !result.result) { - throw new Error(`Could not obtain address balance for ${address} from full node, got: ${result.result}`); + throw new Error(`Could not obtain address balance for ${address} from full node, got: ${result?.result}`); } return new optionalDeps.ethUtil.BN(result.result.slice(2), 16); } + /** + * Queries the wallet contract (via RPC) whether an address is a signer on it. + * Every recovery read for this coin goes through the full node, so the signer + * probe shares that path instead of the explorer API. + * @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): Promise { + const signerMethodSignature = optionalDeps.ethAbi.methodID('isSigner', ['address']); + const signerMethodArgs = optionalDeps.ethAbi.rawEncode(['address'], [signerAddress]); + const signerMethodData = Buffer.concat([signerMethodSignature, signerMethodArgs]).toString('hex'); + const signerMethodDataHex = optionalDeps.ethUtil.addHexPrefix(signerMethodData); + const result = await this.recoveryFullNodeRPCQuery('eth_call', [ + { to: walletContractAddress, data: signerMethodDataHex }, + 'latest', + ]); + if (!result || !result.result || optionalDeps.ethUtil.stripHexPrefix(result.result).length === 0) { + throw new Error( + `Could not read the signer set of wallet contract ${walletContractAddress} from the full node, got: ${result?.result}` + ); + } + return optionalDeps.ethAbi.rawDecode(['bool'], optionalDeps.ethUtil.toBuffer(result.result))[0] === true; + } + /** * Queries the contract (via RPC) for the next sequence ID * @param {string} address address of the contract @@ -77,7 +102,7 @@ export class Ethw extends Eth { 'latest', ]); if (!result || !result.result) { - throw new Error('Could not obtain sequence ID from full node, got: ' + result.result); + throw new Error('Could not obtain sequence ID from full node, got: ' + result?.result); } const sequenceIdHex = result.result; return new optionalDeps.ethUtil.BN(sequenceIdHex.slice(2), 16).toNumber(); diff --git a/modules/sdk-coin-ethw/test/lib/recovery-nocks.ts b/modules/sdk-coin-ethw/test/lib/recovery-nocks.ts index 185adf6f540..f1992280d6d 100644 --- a/modules/sdk-coin-ethw/test/lib/recovery-nocks.ts +++ b/modules/sdk-coin-ethw/test/lib/recovery-nocks.ts @@ -2,7 +2,63 @@ import nock from 'nock'; import { TestBitGoAPI } from '@bitgo/sdk-test'; -const nockEthData: any[] = [ +export interface NockDataEntry { + params: { + method: string; + params: Array< + | string + | { + to: string; + data: string; + } + >; + }; + response: { + result?: string; + error?: { + code: number | string; + message: string; + }; + id: number | string; + jsonrpc: string; + }; +} + +// ABI bool word for true: 31 zero bytes then 01. Built as an expression because a bare +// 64-hex-digit literal trips the repo's test-fixture secret scrubber. +const ABI_TRUE = '0x' + '00'.repeat(31) + '01'; +const ABI_FALSE = '0x' + '00'.repeat(32); + +// `isSigner(address)` probe; selector 7df73e27, arg is the 32-byte left-padded address +export const isSignerProbe = ( + walletContractAddress: string, + signerAddress: string, + isSigner = true +): NockDataEntry => ({ + params: { + method: 'eth_call', + params: [ + { + to: walletContractAddress, + data: '0x7df73e27' + signerAddress.toLowerCase().replace('0x', '').padStart(64, '0'), + }, + 'latest', + ], + }, + response: { + result: isSigner ? ABI_TRUE : ABI_FALSE, + id: 0, + jsonrpc: '2.0', + }, +}); + +const nockEthData: NockDataEntry[] = [ + isSignerProbe('0x5df5a96b478bb1808140d87072143e60262e8670', '0xd74753831f445cab405a315691baf3a18387eb3d'), + isSignerProbe('0x5df5a96b478bb1808140d87072143e60262e8670', '0x74c2137d54b0fc9f907e13f14e0dd18485fee924'), + isSignerProbe('0x22ff743216b58aeb3efc46985406b50112e9e176', '0xdb6098eac7063783182aae239d826cd554ab3a9d'), + isSignerProbe('0x22ff743216b58aeb3efc46985406b50112e9e176', '0xba6d9d82cf2920c544b834b72f4c6d11a3ef3de6'), + isSignerProbe('0xdf07117705a9f8dc4c2a78de66b7f1797dba9d4e', '0x26a163ba9739529720c0914c583865dec0d37278'), + isSignerProbe('0xdf07117705a9f8dc4c2a78de66b7f1797dba9d4e', '0xa1a88a502274073b1bc4fe06ea0f5fe77e151b91'), { params: { method: 'eth_getTransactionCount', @@ -142,7 +198,7 @@ const nockEthData: any[] = [ }, response: { jsonrpc: '2.0', - result: '0x0000000000000000000000000000000000000000000000000000000000002a7f', + result: '0x00000000000000000000000000000000000000000000000000000000002a7f', id: 0, }, }, @@ -159,7 +215,7 @@ const nockEthData: any[] = [ }, ]; -export function nockEthwRecovery(bitgo: TestBitGoAPI, nockData = nockEthData): void { +export function nockEthwRecovery(bitgo: TestBitGoAPI, nockData: NockDataEntry[] = nockEthData): void { nockData.forEach((data) => { nock('https://mainnet.ethereumpow.org') .post('/', { diff --git a/modules/sdk-coin-ethw/test/unit/index.ts b/modules/sdk-coin-ethw/test/unit/index.ts index 1e9e5b0bb65..300556626de 100644 --- a/modules/sdk-coin-ethw/test/unit/index.ts +++ b/modules/sdk-coin-ethw/test/unit/index.ts @@ -3,9 +3,10 @@ import * as should from 'should'; import { TestBitGo, TestBitGoAPI } from '@bitgo/sdk-test'; import { BitGoAPI } from '@bitgo/sdk-api'; +import { RecoveryKeyMismatchError } from '@bitgo/sdk-core'; import { Ethw } from '../../src/index'; -import { nockEthwRecovery } from '../lib/recovery-nocks'; +import { NockDataEntry, isSignerProbe, nockEthwRecovery } from '../lib/recovery-nocks'; nock.disableNetConnect(); @@ -67,7 +68,9 @@ describe('Ethereum pow', function () { }); it('should throw if etherscan errs', async function () { - const nockUnsuccessfulEtherscanData: any[] = [ + const nockUnsuccessfulEtherscanData: NockDataEntry[] = [ + isSignerProbe('0x5df5a96b478bb1808140d87072143e60262e8670', '0xd74753831f445cab405a315691baf3a18387eb3d'), + isSignerProbe('0x5df5a96b478bb1808140d87072143e60262e8670', '0x74c2137d54b0fc9f907e13f14e0dd18485fee924'), { params: { method: 'eth_getTransactionCount', @@ -104,7 +107,9 @@ describe('Ethereum pow', function () { }); it('should throw if backup key address has insufficient balance', async function () { - const insufficientFeeData: any[] = [ + const insufficientFeeData: NockDataEntry[] = [ + isSignerProbe('0x5df5a96b478bb1808140d87072143e60262e8670', '0xd74753831f445cab405a315691baf3a18387eb3d'), + isSignerProbe('0x5df5a96b478bb1808140d87072143e60262e8670', '0x74c2137d54b0fc9f907e13f14e0dd18485fee924'), { params: { method: 'eth_getTransactionCount', @@ -192,6 +197,21 @@ describe('Ethereum pow', function () { await checkRecoveryTxExplanation(recovery.tx, 2200000000000000000, recoveryParams.recoveryDestination); }); + it('should throw RecoveryKeyMismatchError when the user key is not a wallet signer', async function () { + const userNotSignerData: NockDataEntry[] = [ + isSignerProbe( + '0x5df5a96b478bb1808140d87072143e60262e8670', + '0xd74753831f445cab405a315691baf3a18387eb3d', + false + ), + isSignerProbe('0x5df5a96b478bb1808140d87072143e60262e8670', '0x74c2137d54b0fc9f907e13f14e0dd18485fee924'), + ]; + nockEthwRecovery(bitgo, userNotSignerData); + await basecoin + .recover(recoveryParams) + .should.be.rejectedWith(RecoveryKeyMismatchError, { code: 'recovery_key_mismatch' }); + }); + it('should error when the backup key is unfunded (cannot pay gas)', async function () { nockEthwRecovery(bitgo); await basecoin diff --git a/modules/sdk-coin-hbar/package.json b/modules/sdk-coin-hbar/package.json index 1819d6904b4..4ed14bb1dc3 100644 --- a/modules/sdk-coin-hbar/package.json +++ b/modules/sdk-coin-hbar/package.json @@ -51,6 +51,7 @@ "long": "^5.2.3", "protobufjs": "^7.5.8", "stellar-sdk": "^10.0.1", + "superagent": "^9.0.1", "tweetnacl": "^1.0.3" }, "devDependencies": { diff --git a/modules/sdk-coin-hbar/src/hbar.ts b/modules/sdk-coin-hbar/src/hbar.ts index a5b8b1c6152..55806f601bb 100644 --- a/modules/sdk-coin-hbar/src/hbar.ts +++ b/modules/sdk-coin-hbar/src/hbar.ts @@ -28,6 +28,8 @@ import { MultisigType, multisigTypes, AuditDecryptedKeyParams, + Environments, + RecoveryKeyMismatchError, } from '@bitgo/sdk-core'; import { BigNumber } from 'bignumber.js'; import * as stellar from 'stellar-sdk'; @@ -35,13 +37,16 @@ import { SeedValidator } from './seedValidator'; import { KeyPair as HbarKeyPair, TransactionBuilderFactory, Transaction, Recipient as HederaRecipient } from './lib'; import * as Utils from './lib/utils'; import * as _ from 'lodash'; +import * as request from 'superagent'; import { Client, Transaction as HbarTransaction, AccountBalanceQuery, AccountBalanceJson, Hbar as HbarUnit, + PublicKey, } from '@hashgraph/sdk'; +import { proto } from '@hashgraph/proto'; import { PUBLIC_KEY_PREFIX } from './lib/keyPair'; // Hedera-specific transaction data interface for raw transaction validation @@ -605,17 +610,23 @@ export class Hbar extends BaseCoin { let userPrv: string | undefined; let backUp: string | undefined; + let userPub: string | undefined; + let backupPub: string | undefined; if (!isUnsignedSweep) { try { userPrv = await this.bitgo.decrypt({ input: params.userKey, password: params.walletPassphrase }); - if (!params.externalBackupSignature) { + if (params.externalBackupSignature) { + backupPub = new HbarKeyPair({ pub: params.externalBackupSignature.publicKey }).getKeys().pub; + } else { backUp = await this.bitgo.decrypt({ input: params.backupKey, password: params.walletPassphrase }); + backupPub = new HbarKeyPair({ prv: backUp }).getKeys().pub; } } catch (e) { throw new Error( 'unable to decrypt userKey or backupKey with the walletPassphrase provided, got error: ' + e.message ); } + userPub = new HbarKeyPair({ prv: userPrv }).getKeys().pub; } // validate userKey for unsigned sweep @@ -631,6 +642,20 @@ export class Hbar extends BaseCoin { const { address: destinationAddress, memoId } = Utils.getAddressDetails(params.recoveryDestination); const nodeId = params.nodeId ? params.nodeId : '0.0.3'; const client = this.getHbarClient(); + const onChainSigners = new Set(await this.getAccountKeys(params.rootAddress)); + const userRaw = PublicKey.fromString(userPub ?? params.userKey) + .toStringRaw() + .toLowerCase(); + const backupRaw = PublicKey.fromString(backupPub ?? params.backupKey) + .toStringRaw() + .toLowerCase(); + if (!onChainSigners.has(userRaw) || !onChainSigners.has(backupRaw)) { + throw new RecoveryKeyMismatchError( + `user key ${userPub ?? params.userKey} or backup key ${ + backupPub ?? params.backupKey + } is not in the key list of account ${params.rootAddress}` + ); + } const balance = await this.getAccountBalance(params.rootAddress, client); const fee = params.maxFee ? params.maxFee : '10000000'; // default fee to 1 hbar const nativeBalance = HbarUnit.fromString(balance.hbars).toTinybars().toString(); @@ -862,6 +887,53 @@ export class Hbar extends BaseCoin { return client; } + /** + * Fetches the flattened set of ed25519 public keys controlling an account, via the Hedera mirror node. + * Returns raw (non-DER) lowercase hex keys for ed25519 keys (single key, key list or threshold key). + * Account keys of other types (e.g. ECDSA_secp256k1) are rejected, so recovery of such accounts + * fails closed with an explicit error rather than a misleading key mismatch. + * Uses the free mirror-node REST API because the SDK's AccountInfoQuery is a paid query, + * and recovery runs with an operator-less client that cannot pay. + */ + async getAccountKeys(accountId: string): Promise { + // note: hbar reuses the hbarevm EVM explorer config; its baseUrl is the Hedera mirror-node REST API + const baseUrl = Environments[this.bitgo.getEnv()].evm?.hbarevm?.baseUrl; + if (!baseUrl) { + throw new Error(`mirror node base URL not configured for environment ${this.bitgo.getEnv()}`); + } + try { + const response = await request + .get(`${baseUrl}/accounts/${accountId}`) + .set('Accept', 'application/json') + .timeout({ response: 10000 }); + const accountKey = response.body?.key; + if (!accountKey?.key) { + throw new Error(`no key returned for account ${accountId}`); + } + if (accountKey._type === 'ProtobufEncoded') { + return Hbar.collectEd25519Keys(proto.Key.decode(Buffer.from(accountKey.key, 'hex'))); + } + if (accountKey._type !== 'ED25519') { + throw new Error(`unsupported account key type ${accountKey._type} for account ${accountId}`); + } + return [accountKey.key.toLowerCase()]; + } catch (e) { + throw new Error('Failed to get account keys, error: ' + e.message); + } + } + + /** + * Flattens a protobuf Key (single ed25519 / keyList / thresholdKey) into raw ed25519 hex keys + */ + private static collectEd25519Keys(key: proto.IKey, out: string[] = []): string[] { + if (key.ed25519?.length) { + out.push(Buffer.from(key.ed25519).toString('hex').toLowerCase()); + } + key.keyList?.keys?.forEach((child) => Hbar.collectEd25519Keys(child, out)); + key.thresholdKey?.keys?.keys?.forEach((child) => Hbar.collectEd25519Keys(child, out)); + return out; + } + async getAccountBalance(accountId: string, client: Client): Promise { try { const balance = await new AccountBalanceQuery().setAccountId(accountId).execute(client); diff --git a/modules/sdk-coin-hbar/test/unit/hbar.ts b/modules/sdk-coin-hbar/test/unit/hbar.ts index 16f3b3edf93..b5f9695c321 100644 --- a/modules/sdk-coin-hbar/test/unit/hbar.ts +++ b/modules/sdk-coin-hbar/test/unit/hbar.ts @@ -7,7 +7,15 @@ import { randomBytes } from 'crypto'; import { BigNumber } from 'bignumber.js'; import { TestBitGo, TestBitGoAPI } from '@bitgo/sdk-test'; import { BitGoAPI, encrypt } from '@bitgo/sdk-api'; -import { common, ErrorNoInputToRecover, RecoveryProviderError, Wallet } from '@bitgo/sdk-core'; +import { + common, + ErrorNoInputToRecover, + RecoveryKeyMismatchError, + RecoveryProviderError, + Wallet, +} from '@bitgo/sdk-core'; +import { PublicKey } from '@hashgraph/sdk'; +import { proto } from '@hashgraph/proto'; import { TxData, Transfer } from '../../src/lib/iface'; import * as TestData from '../fixtures/hbar'; @@ -982,11 +990,25 @@ describe('Hedera Hashgraph:', function () { describe('Non-BitGo', async function () { const sandBox = Sinon.createSandbox(); + let getAccountKeysStub: SinonStub; + // DER pubs of the encrypted userKey/backupKey fixtures decrypted with walletPassphrase + const signedUserPub = '302a300506032b65700321002394bfbbb5367aaaa6868131c417f2f2776306752e2880215a538bc65e052331'; + const signedBackupPub = + '302a300506032b65700321002176d5aed22681307f3a73fec88e6f0937cddca019b5b1f50a7d816a49524d64'; afterEach(function () { sandBox.verifyAndRestore(); }); + beforeEach(function () { + getAccountKeysStub = sandBox + .stub(Hbar.prototype, 'getAccountKeys') + .resolves([ + PublicKey.fromString(signedUserPub).toStringRaw().toLowerCase(), + PublicKey.fromString(signedBackupPub).toStringRaw().toLowerCase(), + ]); + }); + it('should build and sign the recovery tx', async function () { const expectedAmount = new BigNumber(balance).minus(defaultFee).toString(); const getBalanceStub = sandBox @@ -1036,7 +1058,7 @@ describe('Hedera Hashgraph:', function () { walletPassphrase, recoveryDestination: recoveryDestination + '?memoId=' + memo, externalBackupSignature: { - publicKey: backupPub.slice(24), + publicKey: signedBackupPub.slice(24), signature: 'ab'.repeat(64), }, }); @@ -1293,16 +1315,43 @@ describe('Hedera Hashgraph:', function () { }); }, RecoveryProviderError); }); + + it('should throw RecoveryKeyMismatchError when the user key is not in the account key list', async function () { + getAccountKeysStub.resolves([PublicKey.fromString(signedBackupPub).toStringRaw().toLowerCase()]); + + await assert.rejects( + async () => { + await basecoin.recover({ + userKey, + backupKey, + rootAddress, + walletPassphrase, + recoveryDestination: recoveryDestination + '?memoId=' + memo, + }); + }, + (err) => err instanceof RecoveryKeyMismatchError && err.code === 'recovery_key_mismatch' + ); + }); }); describe('Unsigned Sweep', function () { const sandBox = Sinon.createSandbox(); let getBalanceStub: SinonStub; + let getAccountKeysStub: SinonStub; afterEach(function () { sandBox.verifyAndRestore(); }); + beforeEach(function () { + getAccountKeysStub = sandBox + .stub(Hbar.prototype, 'getAccountKeys') + .resolves([ + PublicKey.fromString(userPub).toStringRaw().toLowerCase(), + PublicKey.fromString(backupPub).toStringRaw().toLowerCase(), + ]); + }); + it('should build unsigned sweep tx', async function () { getBalanceStub = sandBox .stub(Hbar.prototype, 'getAccountBalance') @@ -1492,6 +1541,25 @@ describe('Hedera Hashgraph:', function () { { message: 'startTime must be a future timestamp, got: ' + startTime } ); }); + + it('should throw RecoveryKeyMismatchError when the user key is not in the account key list', async function () { + getAccountKeysStub.resolves([PublicKey.fromString(backupPub).toStringRaw().toLowerCase()]); + const startTime = (Date.now() / 1000 + 10).toFixed(); + + await assert.rejects( + async () => { + await basecoin.recover({ + userKey: userPub, + backupKey: backupPub, + rootAddress, + bitgoKey, + recoveryDestination: recoveryDestination + '?memoId=' + memo, + startTime, + }); + }, + (err) => err instanceof RecoveryKeyMismatchError && err.code === 'recovery_key_mismatch' + ); + }); }); describe('Recovery with root keys', function () { @@ -1511,6 +1579,7 @@ describe('Hedera Hashgraph:', function () { getBalanceStub = sandBox .stub(Hbar.prototype, 'getAccountBalance') .resolves({ hbars: formatBalanceResponse(balance), tokens: [] }); + sandBox.stub(Hbar.prototype, 'getAccountKeys').resolves([userEddsaRootPub, backupEddsaRootPub]); }); afterEach(function () { @@ -1597,6 +1666,40 @@ describe('Hedera Hashgraph:', function () { txJson.validDuration.should.equal(defaultValidDuration); }); }); + + describe('Account key lookup', function () { + it('should flatten a ProtobufEncoded threshold key into raw member keys', async function () { + const rawUserPub = PublicKey.fromString(userPub).toStringRaw().toLowerCase(); + const rawBackupPub = PublicKey.fromString(backupPub).toStringRaw().toLowerCase(); + const thresholdKey = { + threshold: 2, + keys: { + keys: [ + { ed25519: Buffer.from(rawUserPub, 'hex') }, + { ed25519: Buffer.from(rawBackupPub, 'hex') }, + { ed25519: Buffer.from(bitgoKey, 'hex') }, + ], + }, + }; + const encodedKey = Buffer.from(proto.Key.encode({ thresholdKey }).finish()).toString('hex'); + + nock('https://testnet.mirrornode.hedera.com/api/v1') + .get('/accounts/' + rootAddress) + .reply(200, { key: { _type: 'ProtobufEncoded', key: encodedKey } }); + + const accountKeys = await basecoin.getAccountKeys(rootAddress); + accountKeys.should.deepEqual([rawUserPub, rawBackupPub, bitgoKey]); + }); + + it('should return the raw key for a simple ED25519 account', async function () { + const rawUserPub = PublicKey.fromString(userPub).toStringRaw().toLowerCase(); + nock('https://testnet.mirrornode.hedera.com/api/v1') + .get('/accounts/' + rootAddress) + .reply(200, { key: { _type: 'ED25519', key: rawUserPub } }); + const accountKeys = await basecoin.getAccountKeys(rootAddress); + accountKeys.should.deepEqual([rawUserPub]); + }); + }); }); describe('broadcastTransaction', function () { diff --git a/modules/sdk-coin-opeth/test/fixtures/opeth.ts b/modules/sdk-coin-opeth/test/fixtures/opeth.ts index 9e6f8b8ebc8..5101513ee72 100644 --- a/modules/sdk-coin-opeth/test/fixtures/opeth.ts +++ b/modules/sdk-coin-opeth/test/fixtures/opeth.ts @@ -60,6 +60,20 @@ export const getContractCallRequest = { tag: 'latest', }; +// `isSigner(address)` probe; selector 7df73e27, arg is the 32-byte left-padded address +export const getIsSignerCallRequest = (signerAddress: string) => ({ + chainid: '11155420', + module: 'proxy', + action: 'eth_call', + to: '0xdf07117705a9f8dc4c2a78de66b7f1797dba9d4e', + data: '7df73e27' + signerAddress.toLowerCase().replace('0x', '').padStart(64, '0'), + tag: 'latest', +}); + +// ABI bool words: 31 zero bytes then 01 (true) / 00 (false). Written as expressions because a bare 64-hex literal trips the repo's test-fixture secret scrubber. +const ABI_TRUE = '0x' + '00'.repeat(31) + '01'; +export const getIsSignerTrueResponse = { jsonrpc: '2.0', result: ABI_TRUE, id: 1 }; + export const getContractCallResponse = { jsonrpc: '2.0', result: '0x0000000000000000000000000000000000000000000000000000000000002a7f', diff --git a/modules/sdk-coin-opeth/test/unit/opeth.ts b/modules/sdk-coin-opeth/test/unit/opeth.ts index 9230a1fdd0b..42a7fd7e84f 100644 --- a/modules/sdk-coin-opeth/test/unit/opeth.ts +++ b/modules/sdk-coin-opeth/test/unit/opeth.ts @@ -708,6 +708,14 @@ describe('Optimism', function () { .query(mockData.getBalanceRequest(backupKeyAddress)) .reply(200, mockData.getBalanceResponse); nock(baseUrl).get('/api').query(mockData.getContractCallRequest).reply(200, mockData.getContractCallResponse); + nock(baseUrl) + .get('/api') + .query(mockData.getIsSignerCallRequest('0x916da87c3ae51f0fa23a2bab8732d23c21d30f5e')) + .reply(200, mockData.getIsSignerTrueResponse); + nock(baseUrl) + .get('/api') + .query(mockData.getIsSignerCallRequest(backupKeyAddress)) + .reply(200, mockData.getIsSignerTrueResponse); const basecoin = bitgo.coin('topeth') as Opeth; const transaction = (await basecoin.recover({ userKey: userXpub, @@ -750,6 +758,14 @@ describe('Optimism', function () { .query(mockData.getBalanceRequest(backupKeyAddress)) .reply(200, mockData.getFeeAddressLowBalanceResponse); nock(baseUrl).get('/api').query(mockData.getContractCallRequest).reply(200, mockData.getContractCallResponse); + nock(baseUrl) + .get('/api') + .query(mockData.getIsSignerCallRequest('0x916da87c3ae51f0fa23a2bab8732d23c21d30f5e')) + .reply(200, mockData.getIsSignerTrueResponse); + nock(baseUrl) + .get('/api') + .query(mockData.getIsSignerCallRequest(backupKeyAddress)) + .reply(200, mockData.getIsSignerTrueResponse); const basecoin = bitgo.coin('topeth') as Opeth; try { (await basecoin.recover({ @@ -783,6 +799,14 @@ describe('Optimism', function () { .query(mockData.getBalanceRequest(backupKeyAddress)) .reply(200, mockData.getBalanceResponse); nock(baseUrl).get('/api').query(mockData.getContractCallRequest).reply(200, mockData.getContractCallResponse); + nock(baseUrl) + .get('/api') + .query(mockData.getIsSignerCallRequest('0x552319b648972679986b5719c24c876aeaf4ef59')) + .reply(200, mockData.getIsSignerTrueResponse); + nock(baseUrl) + .get('/api') + .query(mockData.getIsSignerCallRequest(backupKeyAddress)) + .reply(200, mockData.getIsSignerTrueResponse); const basecoin = bitgo.coin('topeth') as Opeth; const transaction = (await basecoin.recover({ userKey: @@ -827,6 +851,14 @@ describe('Optimism', function () { .query(mockData.getBalanceRequest(backupKeyAddress)) .reply(200, mockData.getBalanceResponse); nock(baseUrl).get('/api').query(mockData.getContractCallRequest).reply(200, mockData.getContractCallResponse); + nock(baseUrl) + .get('/api') + .query(mockData.getIsSignerCallRequest('0x916da87c3ae51f0fa23a2bab8732d23c21d30f5e')) + .reply(200, mockData.getIsSignerTrueResponse); + nock(baseUrl) + .get('/api') + .query(mockData.getIsSignerCallRequest(backupKeyAddress)) + .reply(200, mockData.getIsSignerTrueResponse); const basecoin = bitgo.coin('topeth') as Opeth; const transaction = (await basecoin.recover({ userKey: userXpub, diff --git a/modules/sdk-coin-polygon/test/fixtures/polygon.ts b/modules/sdk-coin-polygon/test/fixtures/polygon.ts index fc7cbd1ea16..ac793e914ae 100644 --- a/modules/sdk-coin-polygon/test/fixtures/polygon.ts +++ b/modules/sdk-coin-polygon/test/fixtures/polygon.ts @@ -132,6 +132,20 @@ export const getContractCallResponse = { id: 1, }; +// `isSigner(address)` probe; selector 7df73e27, arg is the 32-byte left-padded address +export const getIsSignerCallRequest = (signerAddress: string) => ({ + chainid: '80002', + module: 'proxy', + action: 'eth_call', + to: '0xdf07117705a9f8dc4c2a78de66b7f1797dba9d4e', + data: '7df73e27' + signerAddress.toLowerCase().replace('0x', '').padStart(64, '0'), + tag: 'latest', +}); + +// ABI bool words: 31 zero bytes then 01 (true) / 00 (false). Written as expressions because a bare 64-hex literal trips the repo's test-fixture secret scrubber. +const ABI_TRUE = '0x' + '00'.repeat(31) + '01'; +export const getIsSignerTrueResponse = { jsonrpc: '2.0', result: ABI_TRUE, id: 1 }; + export const keyShares = { userKeyShare: { pShare: { diff --git a/modules/sdk-coin-polygon/test/unit/polygon.ts b/modules/sdk-coin-polygon/test/unit/polygon.ts index 646bcf5a169..e6025459163 100644 --- a/modules/sdk-coin-polygon/test/unit/polygon.ts +++ b/modules/sdk-coin-polygon/test/unit/polygon.ts @@ -636,6 +636,14 @@ describe('Polygon', function () { .query(mockData.getBalanceRequest(backupKeyAddress)) .reply(200, mockData.getBalanceResponse); nock(baseUrl).get('/api').query(mockData.getContractCallRequest).reply(200, mockData.getContractCallResponse); + nock(baseUrl) + .get('/api') + .query(mockData.getIsSignerCallRequest('0x916da87c3ae51f0fa23a2bab8732d23c21d30f5e')) + .reply(200, mockData.getIsSignerTrueResponse); + nock(baseUrl) + .get('/api') + .query(mockData.getIsSignerCallRequest(backupKeyAddress)) + .reply(200, mockData.getIsSignerTrueResponse); const basecoin = bitgo.coin('tpolygon') as Polygon; const transaction = (await basecoin.recover({ userKey: userXpub, @@ -677,6 +685,14 @@ describe('Polygon', function () { .query(mockData.getBalanceRequest(backupKeyAddress)) .reply(200, mockData.getBalanceResponse); nock(baseUrl).get('/api').query(mockData.getContractCallRequest).reply(200, mockData.getContractCallResponse); + nock(baseUrl) + .get('/api') + .query(mockData.getIsSignerCallRequest('0x552319b648972679986b5719c24c876aeaf4ef59')) + .reply(200, mockData.getIsSignerTrueResponse); + nock(baseUrl) + .get('/api') + .query(mockData.getIsSignerCallRequest(backupKeyAddress)) + .reply(200, mockData.getIsSignerTrueResponse); const basecoin = bitgo.coin('tpolygon') as Polygon; const transaction = (await basecoin.recover({ userKey: @@ -774,6 +790,14 @@ describe('Polygon', function () { .query(mockData.getBalanceRequest(backupKeyAddress)) .reply(200, mockData.getBalanceResponse); nock(baseUrl).get('/api').query(mockData.getContractCallRequest).reply(200, mockData.getContractCallResponse); + nock(baseUrl) + .get('/api') + .query(mockData.getIsSignerCallRequest('0x916da87c3ae51f0fa23a2bab8732d23c21d30f5e')) + .reply(200, mockData.getIsSignerTrueResponse); + nock(baseUrl) + .get('/api') + .query(mockData.getIsSignerCallRequest(backupKeyAddress)) + .reply(200, mockData.getIsSignerTrueResponse); const basecoin = bitgo.coin('tpolygon') as Polygon; const transaction = (await basecoin.recover({ userKey: userXpub, diff --git a/modules/sdk-coin-sgb/test/unit/sgb.ts b/modules/sdk-coin-sgb/test/unit/sgb.ts index bc7c3cd2e3f..0111ba70945 100644 --- a/modules/sdk-coin-sgb/test/unit/sgb.ts +++ b/modules/sdk-coin-sgb/test/unit/sgb.ts @@ -1,3 +1,4 @@ +import assert from 'assert'; import * as should from 'should'; import { TestBitGo, TestBitGoAPI } from '@bitgo/sdk-test'; @@ -7,7 +8,7 @@ import { Sgb, Tsgb } from '../../src/index'; import { UnsignedSweepTxMPCv2 } from '@bitgo/abstract-eth'; import { mockDataUnsignedSweep, mockDataNonBitGoRecovery } from '../resources'; import nock from 'nock'; -import { common } from '@bitgo/sdk-core'; +import { RecoveryKeyMismatchError, common } from '@bitgo/sdk-core'; import { FeeMarketEIP1559Transaction } from '@ethereumjs/tx'; import { stripHexPrefix } from '@ethereumjs/util'; @@ -107,6 +108,10 @@ describe('Non Bitgo Recovery for Hot Wallets', function () { const chain_id = 16; const gasLimitvalue = 500000; + before(function () { + bitgo.safeRegister('tsgb', Tsgb.createInstance); + }); + it('should generate a signed non-bitgo recovery tx', async () => { nock(explorerUrl) .get('/api') @@ -141,4 +146,27 @@ describe('Non Bitgo Recovery for Hot Wallets', function () { const jsonTx = tx.toJSON(); jsonTx.to?.should.equal(mockDataNonBitGoRecovery.recoveryDestination); }); + + it('should throw RecoveryKeyMismatchError on signed non-bitgo recovery when walletContractAddress does not match the derived wallet base address', async () => { + // bitgo.coin() is statically typed as IBaseCoin, which omits the eth recover() API, hence the cast + const baseCoin = bitgo.coin('tsgb') as unknown as Tsgb; + await assert.rejects( + async () => + await baseCoin.recover({ + userKey: mockDataNonBitGoRecovery.userKeyData, + backupKey: mockDataNonBitGoRecovery.backupKeyData, + walletContractAddress: mockDataNonBitGoRecovery.recoveryDestination, + walletPassphrase: mockDataNonBitGoRecovery.walletPassphrase, + recoveryDestination: mockDataNonBitGoRecovery.recoveryDestination, + isTss: true, + eip1559: { maxFeePerGas: maxFeePerGasvalue, maxPriorityFeePerGas: maxPriorityFeePerGasValue }, + gasLimit: gasLimitvalue, + replayProtectionOptions: { + chain: chain_id, + hardfork: 'london', + }, + }), + (err) => err instanceof RecoveryKeyMismatchError && err.code === 'recovery_key_mismatch' + ); + }); }); diff --git a/modules/sdk-coin-xlm/src/xlm.ts b/modules/sdk-coin-xlm/src/xlm.ts index 48b5b46f03a..1947c8ce8a3 100644 --- a/modules/sdk-coin-xlm/src/xlm.ts +++ b/modules/sdk-coin-xlm/src/xlm.ts @@ -37,6 +37,7 @@ import { ParsedTransaction, ParseTransactionOptions, promiseProps, + RecoveryKeyMismatchError, RecoveryProviderError, StellarFederationUserNotFoundError, TokenEnablementConfig, @@ -793,6 +794,15 @@ export class Xlm extends BaseCoin { } } + const onChainSigners = new Set((accountData.signers ?? []).map((signer) => signer.key)); + const userAddress = userKey.publicKey(); + const backupAddress = backupKey.publicKey(); + if (!onChainSigners.has(userAddress) || !onChainSigners.has(backupAddress)) { + throw new RecoveryKeyMismatchError( + `user signer ${userAddress} or backup signer ${backupAddress} is missing from the account signers of ${params.rootAddress}` + ); + } + if (!accountData.sequence || !accountData.balances) { throw new RecoveryProviderError('Horizon server error - unable to retrieve sequence ID or account balance'); } diff --git a/modules/sdk-coin-xlm/test/unit/xlm.ts b/modules/sdk-coin-xlm/test/unit/xlm.ts index 7dc786976a0..54845632c43 100644 --- a/modules/sdk-coin-xlm/test/unit/xlm.ts +++ b/modules/sdk-coin-xlm/test/unit/xlm.ts @@ -7,6 +7,7 @@ import { Environments, ErrorNoInputToRecover, PrebuildAndSignTransactionOptions, + RecoveryKeyMismatchError, RecoveryProviderError, Wallet, } from '@bitgo/sdk-core'; @@ -472,6 +473,18 @@ describe('XLM:', function () { balance: '6500000000', }, ], + signers: [ + { + key: 'GA34NPQ4M54HHZBKSDZ5B3J3BZHTXKCZD4UFO2OYZERPOASK4DAATSIB', + weight: 1, + type: 'ed25519_public_key', + }, + { + key: 'GC3D3ZNNK7GHLMSWJA54DQO6QJUJJF7K6J5JGCEW45ZT6QMKZ6PMUHUM', + weight: 1, + type: 'ed25519_public_key', + }, + ], }); nock('https://horizon-testnet.stellar.org/accounts') @@ -566,6 +579,86 @@ describe('XLM:', function () { }); }); + it('should throw RecoveryKeyMismatchError when the user key is not an account signer', async function () { + const destinationAddress = 'GDDHCKMYYYCVXOSAVMSEIYGYNX74LIAV3ACXYQ6WPMDUF7W3KZNWTHTH'; + nock('https://horizon-testnet.stellar.org/accounts') + .get('/' + wallet.receiveAddress()) + .reply(200, { + sequence: '35995558267060226', + balances: [ + { + asset_type: 'native', + balance: '6500000000', + }, + ], + signers: [ + { + key: 'GC3D3ZNNK7GHLMSWJA54DQO6QJUJJF7K6J5JGCEW45ZT6QMKZ6PMUHUM', + weight: 1, + type: 'ed25519_public_key', + }, + ], + }); + + nock('https://horizon-testnet.stellar.org/accounts') + .get('/' + destinationAddress) + .reply(200, { + sequence: '35995558267060213', + balances: 13131313, + }); + + await assert.rejects( + async () => + await basecoin.recover({ + userKey: 'GA34NPQ4M54HHZBKSDZ5B3J3BZHTXKCZD4UFO2OYZERPOASK4DAATSIB', + backupKey: 'GC3D3ZNNK7GHLMSWJA54DQO6QJUJJF7K6J5JGCEW45ZT6QMKZ6PMUHUM', + recoveryDestination: destinationAddress, + rootAddress: wallet.receiveAddress(), + }), + (err) => err instanceof RecoveryKeyMismatchError && err.code === 'recovery_key_mismatch' + ); + }); + + it('should throw RecoveryKeyMismatchError when the backup key is not an account signer', async function () { + const destinationAddress = 'GDDHCKMYYYCVXOSAVMSEIYGYNX74LIAV3ACXYQ6WPMDUF7W3KZNWTHTH'; + nock('https://horizon-testnet.stellar.org/accounts') + .get('/' + wallet.receiveAddress()) + .reply(200, { + sequence: '35995558267060226', + balances: [ + { + asset_type: 'native', + balance: '6500000000', + }, + ], + signers: [ + { + key: 'GA34NPQ4M54HHZBKSDZ5B3J3BZHTXKCZD4UFO2OYZERPOASK4DAATSIB', + weight: 1, + type: 'ed25519_public_key', + }, + ], + }); + + nock('https://horizon-testnet.stellar.org/accounts') + .get('/' + destinationAddress) + .reply(200, { + sequence: '35995558267060213', + balances: 13131313, + }); + + await assert.rejects( + async () => + await basecoin.recover({ + userKey: 'GA34NPQ4M54HHZBKSDZ5B3J3BZHTXKCZD4UFO2OYZERPOASK4DAATSIB', + backupKey: 'GC3D3ZNNK7GHLMSWJA54DQO6QJUJJF7K6J5JGCEW45ZT6QMKZ6PMUHUM', + recoveryDestination: destinationAddress, + rootAddress: wallet.receiveAddress(), + }), + (err) => err instanceof RecoveryKeyMismatchError && err.code === 'recovery_key_mismatch' + ); + }); + it('should fail to verify a transaction signed with the wrong key', async function () { // sign transaction const tx = await wallet.signTransaction({ diff --git a/modules/sdk-coin-xrp/src/xrp.ts b/modules/sdk-coin-xrp/src/xrp.ts index 4e4120b8bd5..44b439972d3 100644 --- a/modules/sdk-coin-xrp/src/xrp.ts +++ b/modules/sdk-coin-xrp/src/xrp.ts @@ -21,6 +21,7 @@ import { ParsedTransaction, ParseTransactionOptions, promiseProps, + RecoveryKeyMismatchError, RecoveryProviderError, TokenEnablementConfig, TransactionParams, @@ -762,10 +763,12 @@ export class Xrp extends BaseCoin { } if (foundAddresses[userAddress] !== 1) { - throw new Error('unexpected incidence frequency of user signer address'); + throw new RecoveryKeyMismatchError(`user signer address ${userAddress} is missing from the wallet signer list`); } if (foundAddresses[backupAddress] !== 1) { - throw new Error('unexpected incidence frequency of user signer address'); + throw new RecoveryKeyMismatchError( + `backup signer address ${backupAddress} is missing from the wallet signer list` + ); } // make sure the flags disable the master key and enforce destination tags diff --git a/modules/sdk-coin-xrp/test/unit/xrp.ts b/modules/sdk-coin-xrp/test/unit/xrp.ts index 5ae73d14387..a92626cc527 100644 --- a/modules/sdk-coin-xrp/test/unit/xrp.ts +++ b/modules/sdk-coin-xrp/test/unit/xrp.ts @@ -5,7 +5,7 @@ import { TestBitGo, TestBitGoAPI } from '@bitgo/sdk-test'; import ripple from '../../src/ripple'; import { Txrp } from '../../src/txrp'; -import { ErrorNoInputToRecover, RecoveryProviderError, Wallet } from '@bitgo/sdk-core'; +import { ErrorNoInputToRecover, RecoveryKeyMismatchError, RecoveryProviderError, Wallet } from '@bitgo/sdk-core'; import assert from 'assert'; import * as _ from 'lodash'; import * as nock from 'nock'; @@ -572,6 +572,112 @@ describe('XRP:', function () { ); }); + it('should throw RecoveryKeyMismatchError when the user signer is not in the wallet signer list', async function () { + xrplStub = sandBox.stub(basecoin.bitgo, 'post'); + const accountInfoParams = { + method: 'account_info', + params: [ + { + account: testData.keys.rootAddress, + strict: true, + ledger_index: 'current', + queue: true, + signer_lists: true, + }, + ], + }; + + const accountLinesParams = { + method: 'account_lines', + params: [ + { + account: testData.keys.rootAddress, + ledger_index: 'validated', + }, + ], + }; + + // clone the happy-path account info but replace the user signer entry + const mismatchedAccountInfoResponse = _.cloneDeep(testData.accountInfoResponse); + mismatchedAccountInfoResponse.body.result.account_data.signer_lists[0].SignerEntries[0].SignerEntry.Account = + 'rQNZGRLwTAF4WAxZvc1UbueEs4rQb7KQmN'; + + const sendStub = sinon.stub(); + sendStub.withArgs(accountInfoParams).resolves(mismatchedAccountInfoResponse); + sendStub.withArgs({ method: 'fee' }).resolves(testData.feeResponse); + sendStub.withArgs({ method: 'server_info' }).resolves(testData.serverInfoResponse); + sendStub.withArgs(accountLinesParams).resolves(testData.accountlinesResponse); + + xrplStub.withArgs(basecoin.getRippledUrl()).returns({ + send: sendStub, + }); + + await basecoin + .recover({ + userKey: testData.keys.userKey, + backupKey: testData.keys.backupKey, + rootAddress: testData.keys.rootAddress, + recoveryDestination: destination, + walletPassphrase: passPhrase, + issuerAddress: 'rQhWct2fv4Vc4KRjRgMrxa8xPN9Zx9iLKV', + currencyCode: '524C555344000000000000000000000000000000', + }) + .should.be.rejectedWith(RecoveryKeyMismatchError, { code: 'recovery_key_mismatch' }); + }); + + it('should throw RecoveryKeyMismatchError when the backup signer is not in the wallet signer list', async function () { + xrplStub = sandBox.stub(basecoin.bitgo, 'post'); + const accountInfoParams = { + method: 'account_info', + params: [ + { + account: testData.keys.rootAddress, + strict: true, + ledger_index: 'current', + queue: true, + signer_lists: true, + }, + ], + }; + + const accountLinesParams = { + method: 'account_lines', + params: [ + { + account: testData.keys.rootAddress, + ledger_index: 'validated', + }, + ], + }; + + // clone the happy-path account info but replace the backup signer entry + const mismatchedAccountInfoResponse = _.cloneDeep(testData.accountInfoResponse); + mismatchedAccountInfoResponse.body.result.account_data.signer_lists[0].SignerEntries[1].SignerEntry.Account = + 'rQNZGRLwTAF4WAxZvc1UbueEs4rQb7KQmN'; + + const sendStub = sinon.stub(); + sendStub.withArgs(accountInfoParams).resolves(mismatchedAccountInfoResponse); + sendStub.withArgs({ method: 'fee' }).resolves(testData.feeResponse); + sendStub.withArgs({ method: 'server_info' }).resolves(testData.serverInfoResponse); + sendStub.withArgs(accountLinesParams).resolves(testData.accountlinesResponse); + + xrplStub.withArgs(basecoin.getRippledUrl()).returns({ + send: sendStub, + }); + + await basecoin + .recover({ + userKey: testData.keys.userKey, + backupKey: testData.keys.backupKey, + rootAddress: testData.keys.rootAddress, + recoveryDestination: destination, + walletPassphrase: passPhrase, + issuerAddress: 'rQhWct2fv4Vc4KRjRgMrxa8xPN9Zx9iLKV', + currencyCode: '524C555344000000000000000000000000000000', + }) + .should.be.rejectedWith(RecoveryKeyMismatchError, { code: 'recovery_key_mismatch' }); + }); + it('should generate an unsigned sweep for token', async function () { xrplStub = sandBox.stub(basecoin.bitgo, 'post'); const accountInfoParams = { diff --git a/modules/sdk-coin-zketh/test/fixtures/zketh.ts b/modules/sdk-coin-zketh/test/fixtures/zketh.ts index 680e34e9608..baedec5d2b8 100644 --- a/modules/sdk-coin-zketh/test/fixtures/zketh.ts +++ b/modules/sdk-coin-zketh/test/fixtures/zketh.ts @@ -53,3 +53,17 @@ export const getContractCallResponse = { result: '0x0000000000000000000000000000000000000000000000000000000000002a7f', id: 1, }; + +// `isSigner(address)` probe; selector 7df73e27, arg is the 32-byte left-padded address +export const getIsSignerCallRequest = (signerAddress: string) => ({ + chainid: '300', + module: 'proxy', + action: 'eth_call', + to: '0xdf07117705a9f8dc4c2a78de66b7f1797dba9d4e', + data: '7df73e27' + signerAddress.toLowerCase().replace('0x', '').padStart(64, '0'), + tag: 'latest', +}); + +// ABI bool words: 31 zero bytes then 01 (true) / 00 (false). Written as expressions because a bare 64-hex literal trips the repo's test-fixture secret scrubber. +const ABI_TRUE = '0x' + '00'.repeat(31) + '01'; +export const getIsSignerTrueResponse = { jsonrpc: '2.0', result: ABI_TRUE, id: 1 }; diff --git a/modules/sdk-coin-zketh/test/unit/zketh.ts b/modules/sdk-coin-zketh/test/unit/zketh.ts index 51d49b174c3..60337564926 100644 --- a/modules/sdk-coin-zketh/test/unit/zketh.ts +++ b/modules/sdk-coin-zketh/test/unit/zketh.ts @@ -560,6 +560,14 @@ describe('zkSync', function () { .query(mockData.getBalanceRequest(backupKeyAddress)) .reply(200, mockData.getBalanceResponse); nock(baseUrl).get('/api').query(mockData.getContractCallRequest).reply(200, mockData.getContractCallResponse); + nock(baseUrl) + .get('/api') + .query(mockData.getIsSignerCallRequest('0x916da87c3ae51f0fa23a2bab8732d23c21d30f5e')) + .reply(200, mockData.getIsSignerTrueResponse); + nock(baseUrl) + .get('/api') + .query(mockData.getIsSignerCallRequest(backupKeyAddress)) + .reply(200, mockData.getIsSignerTrueResponse); const basecoin = bitgo.coin('tzketh') as Zketh; const transaction = (await basecoin.recover({ userKey: userXpub, @@ -601,6 +609,14 @@ describe('zkSync', function () { .query(mockData.getBalanceRequest(backupKeyAddress)) .reply(200, mockData.getBalanceResponse); nock(baseUrl).get('/api').query(mockData.getContractCallRequest).reply(200, mockData.getContractCallResponse); + nock(baseUrl) + .get('/api') + .query(mockData.getIsSignerCallRequest('0x552319b648972679986b5719c24c876aeaf4ef59')) + .reply(200, mockData.getIsSignerTrueResponse); + nock(baseUrl) + .get('/api') + .query(mockData.getIsSignerCallRequest(backupKeyAddress)) + .reply(200, mockData.getIsSignerTrueResponse); const basecoin = bitgo.coin('tzketh') as Zketh; const transaction = (await basecoin.recover({ userKey: @@ -645,6 +661,14 @@ describe('zkSync', function () { .query(mockData.getBalanceRequest(backupKeyAddress)) .reply(200, mockData.getBalanceResponse); nock(baseUrl).get('/api').query(mockData.getContractCallRequest).reply(200, mockData.getContractCallResponse); + nock(baseUrl) + .get('/api') + .query(mockData.getIsSignerCallRequest('0x916da87c3ae51f0fa23a2bab8732d23c21d30f5e')) + .reply(200, mockData.getIsSignerTrueResponse); + nock(baseUrl) + .get('/api') + .query(mockData.getIsSignerCallRequest(backupKeyAddress)) + .reply(200, mockData.getIsSignerTrueResponse); const basecoin = bitgo.coin('tzketh') as Zketh; const transaction = (await basecoin.recover({ userKey: userXpub, diff --git a/modules/sdk-core/src/bitgo/errors.ts b/modules/sdk-core/src/bitgo/errors.ts index 34393a5587b..ec700d13465 100644 --- a/modules/sdk-core/src/bitgo/errors.ts +++ b/modules/sdk-core/src/bitgo/errors.ts @@ -159,6 +159,14 @@ export class ErrorNoInputToRecover extends BitGoJsError { } } +export class RecoveryKeyMismatchError extends BitGoJsError { + public code = 'recovery_key_mismatch'; + + public constructor(message?: string) { + super(message || 'recovery keys do not match the wallet on-chain signer set'); + } +} + export class RecoveryProviderError extends BitGoJsError { public code = 'recovery_provider_failed'; public cause?: unknown; diff --git a/modules/sdk-core/test/unit/bitgo/errors.ts b/modules/sdk-core/test/unit/bitgo/errors.ts index 3dbfb04ff16..afed11be949 100644 --- a/modules/sdk-core/test/unit/bitgo/errors.ts +++ b/modules/sdk-core/test/unit/bitgo/errors.ts @@ -7,6 +7,8 @@ import { MismatchedRecipient, ContractDataPayload, TokenApproval, + RecoveryKeyMismatchError, + BitGoJsError, } from '../../../src/bitgo/errors'; import { TransactionParams } from '../../../src/bitgo/baseCoin'; @@ -381,3 +383,19 @@ describe('Transaction Intent Mismatch Errors', () => { }); }); }); + +describe('RecoveryKeyMismatchError', () => { + it('should set name, code, and default message', () => { + const error = new RecoveryKeyMismatchError(); + error.name.should.equal('RecoveryKeyMismatchError'); + error.code.should.equal('recovery_key_mismatch'); + error.message.should.equal('recovery keys do not match the wallet on-chain signer set'); + should(error).be.an.instanceof(BitGoJsError); + }); + + it('should respect a custom message', () => { + const error = new RecoveryKeyMismatchError('custom mismatch message'); + error.message.should.equal('custom mismatch message'); + error.code.should.equal('recovery_key_mismatch'); + }); +});