From 14a7142182dce66fb9900301d18ae4e482feac6b Mon Sep 17 00:00:00 2001 From: Pranish Nepal Date: Thu, 22 Oct 2026 16:11:25 -0400 Subject: [PATCH] feat: enable Safe recovery for HBAR - Cut Hbar.recover() over to the lazy ExternalEd25519Signer, dropping the pre-made externalBackupSignature hook (mirrors the ALGO cutover) - Register hbar/thbar in ED25519_MULTISIG_COIN_ENCODERS; omit rootAddres for HBAR since Hedera account IDs are network-assigned - Echo derived public keys in signed Safe recovery for HBAR and ALGO, leaving legacy paths unchanged - Fix external signer mutation: verify against the original message byte - Validate bitgoKey before node queries and signing Ticket: WCN-2897 --- modules/sdk-coin-algo/src/algo.ts | 20 ++ modules/sdk-coin-algo/test/unit/algo.ts | 4 + .../test/unit/algoSafeRecovery.ts | 97 ++++++-- modules/sdk-coin-hbar/src/hbar.ts | 91 ++++++-- modules/sdk-coin-hbar/test/unit/hbar.ts | 100 +++++++- .../test/unit/hbarSafeRecovery.ts | 221 ++++++++++++++++++ .../src/bitgo/safe/derivableEd25519Pub.ts | 6 +- modules/sdk-core/src/bitgo/safe/recovery.ts | 47 ++-- .../unit/bitgo/safe/derivableEd25519Pub.ts | 16 ++ .../sdk-core/test/unit/bitgo/safe/recovery.ts | 46 +++- 10 files changed, 576 insertions(+), 72 deletions(-) create mode 100644 modules/sdk-coin-hbar/test/unit/hbarSafeRecovery.ts diff --git a/modules/sdk-coin-algo/src/algo.ts b/modules/sdk-coin-algo/src/algo.ts index ddeb9adff5..92d50b7739 100644 --- a/modules/sdk-coin-algo/src/algo.ts +++ b/modules/sdk-coin-algo/src/algo.ts @@ -170,6 +170,10 @@ export interface RecoveryOptions { externalBackupSigner?: ExternalEd25519Signer; } +/** + * Signed recovery result. Optional key/account fields are populated for Safe recovery when + * `externalBackupSigner` is supplied. They expose only public material. + */ interface RecoveryInfo { id: string; tx: string; @@ -180,6 +184,14 @@ interface RecoveryInfo { genesisId: string; genesisHash: string; note?: string; + /** Derived user public key encoded as an ALGO address, never the encrypted `userKey` input. */ + userKey?: string; + /** Derived backup public key encoded as an ALGO address, not a private key or seed. */ + backupKey?: string; + /** BitGo public key encoded as an ALGO address. */ + bitgoKey?: string; + /** Wallet address requiring two of the three keys to authorize a transfer; not an individual key. */ + rootAddress?: string; } export interface OfflineVaultTxInfo { @@ -1073,6 +1085,14 @@ export class Algo extends BaseCoin { genesisId: genesisId, genesisHash: genesisHash, note: txJson.note ? Buffer.from(txJson.note.buffer).toString('utf-8') : undefined, + ...(externalBackupSigner + ? { + userKey: userKeyAddress, + backupKey: backupKeyAddress, + bitgoKey: this.stellarAddressToAlgoAddress(params.bitgoKey), + rootAddress: params.rootAddress, + } + : {}), }; } diff --git a/modules/sdk-coin-algo/test/unit/algo.ts b/modules/sdk-coin-algo/test/unit/algo.ts index 0ce0ac066b..74f01eca14 100644 --- a/modules/sdk-coin-algo/test/unit/algo.ts +++ b/modules/sdk-coin-algo/test/unit/algo.ts @@ -820,6 +820,10 @@ describe('ALGO:', function () { recovery.should.have.property('coin', 'talgo'); recovery.should.have.property('firstRound'); recovery.should.have.property('lastRound'); + assert.deepStrictEqual( + Object.keys(recovery).sort(), + ['tx', 'id', 'coin', 'fee', 'firstRound', 'lastRound', 'genesisId', 'genesisHash', 'note'].sort() + ); getBalanceStub.callCount.should.equal(1); const factory = new AlgoLib.TransactionBuilderFactory(coins.get('algo')); const txBuilder = factory.from(recovery.tx); diff --git a/modules/sdk-coin-algo/test/unit/algoSafeRecovery.ts b/modules/sdk-coin-algo/test/unit/algoSafeRecovery.ts index 4e10f8a873..e5fb1ef991 100644 --- a/modules/sdk-coin-algo/test/unit/algoSafeRecovery.ts +++ b/modules/sdk-coin-algo/test/unit/algoSafeRecovery.ts @@ -1,3 +1,4 @@ +import assert from 'assert'; import should from 'should'; import { Talgo } from '../../src'; import { TestBitGo, TestBitGoAPI } from '@bitgo/sdk-test'; @@ -10,6 +11,7 @@ import { deriveSafeWalletKeys, encodeDerivableEd25519Pub, encodeEd25519ChainCode, + RecoveryKeyMismatchError, type DecryptedSafeRoots, } from '@bitgo/sdk-core'; import algosdk from 'algosdk'; @@ -25,13 +27,18 @@ import algosdk from 'algosdk'; */ describe('Safe ALGO recovery end-to-end', function () { let bitgo: TestBitGoAPI; - let basecoin; + let basecoin: Algo; + const sandbox = Sinon.createSandbox(); + + afterEach(function () { + sandbox.restore(); + }); before(function () { bitgo = TestBitGo.decorate(BitGoAPI, { env: 'mock' }); bitgo.safeRegister('talgo', Talgo.createInstance); bitgo.initializeTestVars(); - basecoin = bitgo.coin('talgo'); + basecoin = bitgo.coin('talgo') as Algo; }); // The ed25519Multisig root fixture the sdk-core unit tests pin (with a distinct BitGo root, so the @@ -72,7 +79,6 @@ describe('Safe ALGO recovery end-to-end', function () { const recoveryDestination = 'GB3YETD5TSTBAIYGYHVWU3O3I7XGOB44HOZA5MOEF5M23CLLZKRQLEVAOA'; it('recovers a Safe ALGO wallet whose rootAddress equals multisigAddress(1,2,[user,backup,bitgo])', async function () { - const sandbox = Sinon.createSandbox(); sandbox.stub(Algo.prototype, 'getAccountBalance').resolves(nativeBalance); const roots = ed25519Roots(); @@ -106,7 +112,8 @@ describe('Safe ALGO recovery end-to-end', function () { algosdk.encodeAddress(Buffer.from(derived.keys.bitgo.pub, 'hex')), ], }); - keyParams.rootAddress.should.equal(expectedRoot); + assert(keyParams.rootAddress); + assert.strictEqual(keyParams.rootAddress, expectedRoot); const recovery = await basecoin.recover({ userKey: keyParams.userKey, @@ -120,19 +127,64 @@ describe('Safe ALGO recovery end-to-end', function () { externalBackupSigner: keyParams.externalBackupSigner, }); + assert('tx' in recovery); recovery.should.have.property('tx'); + assert.strictEqual(recovery.userKey, algosdk.encodeAddress(Buffer.from(derived.keys.user.pub, 'hex'))); + assert.strictEqual(recovery.backupKey, algosdk.encodeAddress(Buffer.from(derived.keys.backup.pub, 'hex'))); + assert.strictEqual(recovery.bitgoKey, algosdk.encodeAddress(Buffer.from(derived.keys.bitgo.pub, 'hex'))); + assert.strictEqual(recovery.rootAddress, expectedRoot); + assert.notStrictEqual(recovery.userKey, keyParams.userKey); + recovery.should.not.have.property('walletPassphrase'); + recovery.should.not.have.property('externalBackupSigner'); const decoded = algosdk.decodeSignedTransaction(Buffer.from(recovery.tx as string, 'base64')); should(decoded.msig).not.be.undefined(); const msigPk = algosdk.decodeAddress(expectedRoot).publicKey; algosdk.verifyMultisig(decoded.txn.bytesToSign(), decoded.msig!, msigPk).should.equal(true); + }); - sandbox.restore(); + it('returns the derived public keys for an unsigned Safe recovery without encrypting or decrypting', async function () { + sandbox.stub(Algo.prototype, 'getAccountBalance').resolves(nativeBalance); + const encryptSpy = sandbox.spy(bitgo, 'encrypt'); + const decryptSpy = sandbox.spy(bitgo, 'decrypt'); + const derived = await deriveSafeWalletKeys({ + coin: 'talgo', + slot: 'ed25519Multisig', + account: 0, + roots: ed25519Roots(), + }); + assert(derived.slot === 'ed25519Multisig'); + const keyParams = await buildSafeRecoverKeyParams({ + bitgo, + coin: 'talgo', + slot: 'ed25519Multisig', + keys: derived.keys, + mode: 'unsigned', + }); + assert(keyParams.mode === 'unsigned'); + assert(keyParams.rootAddress); + + const recovery = await basecoin.recover({ + ...keyParams, + rootAddress: keyParams.rootAddress, + fee, + recoveryDestination, + firstRound: 5002596, + nodeParams, + }); + assert('txHex' in recovery); + const expectedKeys = [derived.keys.user.pub, derived.keys.backup.pub, derived.keys.bitgo.pub].map((pub) => + algosdk.encodeAddress(Buffer.from(pub, 'hex')) + ); + assert.deepStrictEqual([recovery.userKey, recovery.backupKey, recovery.bitgoKey], expectedKeys); + assert.deepStrictEqual(recovery.keys, expectedKeys); + assert.strictEqual(recovery.address, keyParams.rootAddress); + Sinon.assert.notCalled(encryptSpy); + Sinon.assert.notCalled(decryptSpy); }); it('fails closed with RecoveryKeyMismatchError when the user key does not belong to the root', async function () { - const sandbox = Sinon.createSandbox(); - sandbox.stub(Algo.prototype, 'getAccountBalance').resolves(nativeBalance); + const getBalanceStub = sandbox.stub(Algo.prototype, 'getAccountBalance').resolves(nativeBalance); const roots = ed25519Roots(); const derived = await deriveSafeWalletKeys({ coin: 'algo', slot: 'ed25519Multisig', account: 0, roots }); @@ -151,20 +203,21 @@ describe('Safe ALGO recovery end-to-end', function () { } // A rootAddress the derived keys do not control: the fail-closed check fires before any node call. - await basecoin - .recover({ - userKey: keyParams.userKey, - rootAddress: algosdk.encodeAddress(Buffer.alloc(32, 0xee)), - bitgoKey: keyParams.bitgoKey, - walletPassphrase: keyParams.walletPassphrase, - fee, - recoveryDestination, - firstRound: 5002596, - nodeParams, - externalBackupSigner: keyParams.externalBackupSigner, - }) - .should.be.rejectedWith(/does not match the given rootAddress/); - - sandbox.restore(); + await assert.rejects( + () => + basecoin.recover({ + userKey: keyParams.userKey, + rootAddress: algosdk.encodeAddress(Buffer.alloc(32, 0xee)), + bitgoKey: keyParams.bitgoKey, + walletPassphrase: keyParams.walletPassphrase, + fee, + recoveryDestination, + firstRound: 5002596, + nodeParams, + externalBackupSigner: keyParams.externalBackupSigner, + }), + RecoveryKeyMismatchError + ); + Sinon.assert.notCalled(getBalanceStub); }); }); diff --git a/modules/sdk-coin-hbar/src/hbar.ts b/modules/sdk-coin-hbar/src/hbar.ts index 55806f601b..81586a463e 100644 --- a/modules/sdk-coin-hbar/src/hbar.ts +++ b/modules/sdk-coin-hbar/src/hbar.ts @@ -6,8 +6,9 @@ import { BaseCoin, BitGoBase, ErrorNoInputToRecover, - assertValidExternalEd25519Signature, - ExternalEd25519Signature, + assertValidExternalEd25519Signer, + ExternalEd25519Signer, + signWithExternalEd25519Signer, KeyPair, ParsedTransaction, ParseTransactionOptions, @@ -114,7 +115,7 @@ interface VerifyAddressOptions extends BaseVerifyAddressOptions { } export interface RecoveryOptions { - backupKey: string; + backupKey?: string; userKey: string; rootAddress: string; recoveryDestination: string; @@ -124,16 +125,28 @@ export interface RecoveryOptions { nodeId?: string; startTime?: string; tokenId?: string; - /** Optional pre-made signature for the backup child, whose signature must be over the transaction body bytes (`bodyBytes`). */ - externalBackupSignature?: ExternalEd25519Signature; + /** Lazy signer for the seedless backup child; the signed body bytes only exist after recover() builds the transaction. */ + externalBackupSigner?: ExternalEd25519Signer; } +/** + * Signed recovery result. Optional key/account fields are populated for Safe recovery when + * `externalBackupSigner` is supplied. They expose only public material. + */ export interface RecoveryInfo { id: string; tx: string; coin: string; startTime: string; nodeId: string; + /** Derived user public key with the Hedera public-key prefix, never the encrypted `userKey` input. */ + userKey?: string; + /** Derived backup public key with the Hedera public-key prefix, not a private key or seed. */ + backupKey?: string; + /** BitGo public key with the Hedera public-key prefix, when supplied by the caller. */ + bitgoKey?: string; + /** Caller-supplied Hedera account ID (for example, `0.0.12345`), not a public or private key. */ + rootAddress?: string; } export interface OfflineVaultTxInfo { @@ -566,9 +579,11 @@ export class Hbar extends BaseCoin { * @param params */ public async recover(params: RecoveryOptions): Promise { + const backupKey = params.backupKey; const isUnsignedSweep = - (params.backupKey.startsWith(PUBLIC_KEY_PREFIX) && params.userKey.startsWith(PUBLIC_KEY_PREFIX)) || - (Utils.isValidPublicKey(params.userKey) && Utils.isValidPublicKey(params.backupKey)); + backupKey !== undefined && + ((backupKey.startsWith(PUBLIC_KEY_PREFIX) && params.userKey.startsWith(PUBLIC_KEY_PREFIX)) || + (Utils.isValidPublicKey(params.userKey) && Utils.isValidPublicKey(backupKey))); // Validate the root address if (!this.isValidAddress(params.rootAddress)) { @@ -600,8 +615,19 @@ export class Hbar extends BaseCoin { throw new Error('start time is required for unsigned sweep'); } - if (params.externalBackupSignature) { - assertValidExternalEd25519Signature(params.externalBackupSignature); + if (params.externalBackupSigner) { + assertValidExternalEd25519Signer(params.externalBackupSigner); + } + + // Normalize the caller-supplied BitGo public key before any node query or signing: a malformed + // key must fail fast rather than surface after the backup signer has already run. + let bitgoPub: string | undefined; + if (params.externalBackupSigner && params.bitgoKey) { + try { + bitgoPub = new HbarKeyPair({ pub: params.bitgoKey }).getKeys().pub; + } catch (e) { + throw new Error('invalid bitgoKey, got: ' + params.bitgoKey); + } } if (!isUnsignedSweep && !params.walletPassphrase) { @@ -612,13 +638,18 @@ export class Hbar extends BaseCoin { let backUp: string | undefined; let userPub: string | undefined; let backupPub: string | undefined; + const externalBackupSigner = params.externalBackupSigner; + const externalBackupPub = externalBackupSigner?.publicKey; if (!isUnsignedSweep) { try { userPrv = await this.bitgo.decrypt({ input: params.userKey, password: params.walletPassphrase }); - if (params.externalBackupSignature) { - backupPub = new HbarKeyPair({ pub: params.externalBackupSignature.publicKey }).getKeys().pub; + if (externalBackupPub) { + backupPub = new HbarKeyPair({ pub: externalBackupPub }).getKeys().pub; } else { - backUp = await this.bitgo.decrypt({ input: params.backupKey, password: params.walletPassphrase }); + if (!backupKey) { + throw new Error('backupKey is required for non-bitgo recovery when no external backup signer is provided'); + } + backUp = await this.bitgo.decrypt({ input: backupKey, password: params.walletPassphrase }); backupPub = new HbarKeyPair({ prv: backUp }).getKeys().pub; } } catch (e) { @@ -635,8 +666,8 @@ export class Hbar extends BaseCoin { } // validate backupKey for unsigned sweep - if (isUnsignedSweep && !Utils.isValidPublicKey(params.backupKey)) { - throw new Error('invalid backupKey, got: ' + params.backupKey); + if (isUnsignedSweep && backupKey !== undefined && !Utils.isValidPublicKey(backupKey)) { + throw new Error('invalid backupKey, got: ' + backupKey); } const { address: destinationAddress, memoId } = Utils.getAddressDetails(params.recoveryDestination); @@ -646,13 +677,13 @@ export class Hbar extends BaseCoin { const userRaw = PublicKey.fromString(userPub ?? params.userKey) .toStringRaw() .toLowerCase(); - const backupRaw = PublicKey.fromString(backupPub ?? params.backupKey) + const backupRaw = PublicKey.fromString(backupPub ?? backupKey!) .toStringRaw() .toLowerCase(); if (!onChainSigners.has(userRaw) || !onChainSigners.has(backupRaw)) { throw new RecoveryKeyMismatchError( `user key ${userPub ?? params.userKey} or backup key ${ - backupPub ?? params.backupKey + backupPub ?? backupKey } is not in the key list of account ${params.rootAddress}` ); } @@ -711,7 +742,7 @@ export class Hbar extends BaseCoin { nodeId: txJson.node, memo: txJson.memo, userKey: params.userKey, - backupKey: params.backupKey, + backupKey, bitgoKey: params.bitgoKey, maxFee: fee, address: params.rootAddress, @@ -723,17 +754,19 @@ export class Hbar extends BaseCoin { txBuilder.sign({ key: userPrv }); - const externalBackupSignature = params.externalBackupSignature; - if (externalBackupSignature) { - txBuilder.signature( - externalBackupSignature.signature, - new HbarKeyPair({ pub: externalBackupSignature.publicKey }) - ); - } else { + if (!externalBackupPub) { txBuilder.sign({ key: backUp }); } - const tx = await txBuilder.build(); + const tx = (await txBuilder.build()) as Transaction; + + if (externalBackupSigner) { + // Slot-2 Safe recovery: the seedless backup child signs lazily, over the exact transaction the + // user key just signed. A pre-made signature cannot cover the body bytes, since the amount and + // start time are only known once the transaction is built. + const signature = await signWithExternalEd25519Signer(externalBackupSigner, tx.hederaTx.bodyBytes); + tx.addSignature(signature, new HbarKeyPair({ pub: externalBackupSigner.publicKey })); + } return { tx: tx.toBroadcastFormat(), @@ -741,6 +774,14 @@ export class Hbar extends BaseCoin { coin: this.getChain(), startTime: tx.toJson().startTime, nodeId: tx.toJson().node, + ...(externalBackupSigner + ? { + userKey: userPub, + backupKey: backupPub, + bitgoKey: bitgoPub, + rootAddress: params.rootAddress, + } + : {}), }; } diff --git a/modules/sdk-coin-hbar/test/unit/hbar.ts b/modules/sdk-coin-hbar/test/unit/hbar.ts index b5f9695c32..408a42e51b 100644 --- a/modules/sdk-coin-hbar/test/unit/hbar.ts +++ b/modules/sdk-coin-hbar/test/unit/hbar.ts @@ -16,6 +16,7 @@ import { } from '@bitgo/sdk-core'; import { PublicKey } from '@hashgraph/sdk'; import { proto } from '@hashgraph/proto'; +import nacl from 'tweetnacl'; import { TxData, Transfer } from '../../src/lib/iface'; import * as TestData from '../fixtures/hbar'; @@ -1028,6 +1029,7 @@ describe('Hedera Hashgraph:', function () { recovery.should.have.property('tx'); recovery.should.have.property('coin', 'thbar'); recovery.should.have.property('nodeId', defaultNodeId); + assert.deepStrictEqual(Object.keys(recovery).sort(), ['tx', 'id', 'coin', 'startTime', 'nodeId'].sort()); getBalanceStub.callCount.should.equal(1); const txBuilder = basecoin.getBuilderFactory().from(recovery.tx); const tx = await txBuilder.build(); @@ -1045,27 +1047,107 @@ describe('Hedera Hashgraph:', function () { recovery.should.have.property('id', rootAddress + '@' + txJson.startTime); }); - it('should sign with an external backup signature without decrypting the backup key', async function () { + const externalSignerRecoveryParams = { + userKey, + rootAddress, + walletPassphrase, + recoveryDestination: recoveryDestination + '?memoId=' + memo, + }; + + it('should sign with an external backup signer without decrypting the backup key', async function () { const getBalanceStub = sandBox .stub(Hbar.prototype, 'getAccountBalance') .resolves({ hbars: formatBalanceResponse(balance), tokens: [] }); + + // Prepare the fixture signer before spying: recover() must only decrypt the user key. + const backupPrv = await bitgo.decrypt({ input: backupKey, password: walletPassphrase }); + const backupKeys = new KeyPair({ prv: backupPrv }).getKeys(true); + assert(backupKeys.prv); + const backupSecretKey = nacl.sign.keyPair.fromSeed(Buffer.from(backupKeys.prv, 'hex')).secretKey; + const sign = sandBox.spy(async (message: Uint8Array) => ({ + signature: Buffer.from(nacl.sign.detached(message, backupSecretKey)).toString('hex'), + })); const decryptSpy = sandBox.spy(bitgo, 'decrypt'); const recovery = await basecoin.recover({ - userKey, - backupKey, - rootAddress, - walletPassphrase, - recoveryDestination: recoveryDestination + '?memoId=' + memo, - externalBackupSignature: { - publicKey: signedBackupPub.slice(24), - signature: 'ab'.repeat(64), + ...externalSignerRecoveryParams, + externalBackupSigner: { + publicKey: backupKeys.pub, + sign, }, }); recovery.should.have.property('tx'); + assert.strictEqual(recovery.userKey, signedUserPub); + assert.strictEqual(recovery.backupKey, signedBackupPub); + assert.strictEqual(recovery.bitgoKey, undefined); + assert.strictEqual(recovery.rootAddress, rootAddress); getBalanceStub.callCount.should.equal(1); decryptSpy.callCount.should.equal(1); + sign.callCount.should.equal(1); + + const { bodyBytes, sigMap } = proto.Transaction.decode(Buffer.from(recovery.tx, 'hex')); + assert(sigMap?.sigPair); + assert.deepStrictEqual(Buffer.from(sign.firstCall.args[0]), Buffer.from(bodyBytes)); + assert.deepStrictEqual( + sigMap.sigPair.map((pair) => Buffer.from(pair.pubKeyPrefix ?? []).toString('hex')).sort(), + [PublicKey.fromString(signedUserPub).toStringRaw().toLowerCase(), backupKeys.pub].sort() + ); + for (const pair of sigMap.sigPair) { + assert(pair.ed25519); + assert(pair.pubKeyPrefix); + assert(nacl.sign.detached.verify(bodyBytes, pair.ed25519, pair.pubKeyPrefix)); + } + }); + + it('calls the lazy signer with the tx bytes and rejects a non-verifying signature', async function () { + const getBalanceStub = sandBox + .stub(Hbar.prototype, 'getAccountBalance') + .resolves({ hbars: formatBalanceResponse(balance), tokens: [] }); + const decryptSpy = sandBox.spy(bitgo, 'decrypt'); + const sign = sandBox.stub().resolves({ signature: 'ab'.repeat(64) }); + + await assert.rejects( + () => + basecoin.recover({ + ...externalSignerRecoveryParams, + externalBackupSigner: { + publicKey: PublicKey.fromString(signedBackupPub).toStringRaw().toLowerCase(), + sign, + }, + }), + /does not verify against its public key/ + ); + + getBalanceStub.callCount.should.equal(1); + decryptSpy.callCount.should.equal(1); + sign.callCount.should.equal(1); + assert(sign.firstCall.args[0] instanceof Uint8Array); + sign.firstCall.args[0].length.should.be.greaterThan(0); + }); + + it('rejects a lazy external backup signer with a malformed public key or signature', async function () { + sandBox + .stub(Hbar.prototype, 'getAccountBalance') + .resolves({ hbars: formatBalanceResponse(balance), tokens: [] }); + + for (const { publicKey, signature, expectedError } of [ + { publicKey: 'not-hex', signature: 'ab'.repeat(64), expectedError: /expected 32 bytes/ }, + { + publicKey: PublicKey.fromString(signedBackupPub).toStringRaw().toLowerCase(), + signature: 'ab', + expectedError: /expected 64 bytes/, + }, + ]) { + await assert.rejects( + () => + basecoin.recover({ + ...externalSignerRecoveryParams, + externalBackupSigner: { publicKey, sign: async () => ({ signature }) }, + }), + expectedError + ); + } }); it('should throw for invalid rootAddress', async function () { diff --git a/modules/sdk-coin-hbar/test/unit/hbarSafeRecovery.ts b/modules/sdk-coin-hbar/test/unit/hbarSafeRecovery.ts new file mode 100644 index 0000000000..4e319d2178 --- /dev/null +++ b/modules/sdk-coin-hbar/test/unit/hbarSafeRecovery.ts @@ -0,0 +1,221 @@ +import assert from 'assert'; +import Sinon from 'sinon'; +import nacl from 'tweetnacl'; +import { Hbar as HbarUnit } from '@hashgraph/sdk'; +import { proto } from '@hashgraph/proto'; +import { coins } from '@bitgo/statics'; +import { BitGoAPI } from '@bitgo/sdk-api'; +import { TestBitGo, TestBitGoAPI } from '@bitgo/sdk-test'; +import { + buildSafeRecoverKeyParams, + deriveSafeWalletKeys, + encodeDerivableEd25519Pub, + encodeEd25519ChainCode, + encodeHbarPublicKeyDer, + ErrorNoInputToRecover, + RecoveryKeyMismatchError, + RecoveryProviderError, + type DecryptedSafeRoots, + type Ed25519MultisigChildKeys, +} from '@bitgo/sdk-core'; +import { Hbar, Thbar, Transaction } from '../../src'; + +// Real derivation, encryption, and signing from decrypted keycard roots. Only chain queries are mocked. +describe('Safe HBAR recovery end-to-end', function () { + const sandbox = Sinon.createSandbox(); + let bitgo: TestBitGoAPI; + let basecoin: Hbar; + let keys: Ed25519MultisigChildKeys; + let getAccountKeysStub: Sinon.SinonStub; + let getAccountBalanceStub: Sinon.SinonStub; + + const recoveryParams = { + rootAddress: '0.0.12345', + recoveryDestination: '0.0.99999', + startTime: '1700000010.123456789', + maxFee: '10000000', + }; + + before(async function () { + bitgo = TestBitGo.decorate(BitGoAPI, { env: 'mock' }); + bitgo.safeRegister('thbar', Thbar.createInstance); + bitgo.initializeTestVars(); + basecoin = bitgo.coin('thbar') as Hbar; + + const seed = 'SAAACAQDAQCQMBYIBEFAWDANBYHRAEISCMKBKFQXDAMRUGY4DUPB6NKI'; + const bitgoRootPub = Buffer.from(nacl.sign.keyPair.fromSeed(Buffer.alloc(32, 0x22)).publicKey); + const roots: DecryptedSafeRoots = { + secp256k1Multisig: { user: 'u', backup: 'b', bitgo: 'g' }, + ed25519Multisig: { + user: seed, + backup: { prv: seed, chainCode: encodeEd25519ChainCode(Buffer.alloc(32, 0xab)) }, + bitgo: encodeDerivableEd25519Pub(bitgoRootPub, Buffer.alloc(32, 0xcd)), + }, + 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', + }, + }; + const derived = await deriveSafeWalletKeys({ coin: 'thbar', slot: 'ed25519Multisig', account: 0, roots }); + assert(derived.slot === 'ed25519Multisig'); + keys = derived.keys; + }); + + beforeEach(function () { + sandbox.useFakeTimers({ now: 1700000000000, toFake: ['Date'] }); + getAccountKeysStub = sandbox + .stub(basecoin, 'getAccountKeys') + .resolves([keys.user.pub, keys.backup.pub, keys.bitgo.pub]); + getAccountBalanceStub = sandbox + .stub(basecoin, 'getAccountBalance') + .resolves({ hbars: new HbarUnit(10).toString(), tokens: [] }); + }); + + afterEach(function () { + sandbox.restore(); + }); + + async function signedKeyParams() { + const params = await buildSafeRecoverKeyParams({ + bitgo, + coin: 'thbar', + slot: 'ed25519Multisig', + keys, + mode: 'signed', + }); + assert(params.mode === 'signed'); + return { + ...params, + externalBackupSigner: { + ...params.externalBackupSigner, + sign: sandbox.spy(params.externalBackupSigner.sign), + }, + }; + } + + it('returns public keys and a transaction signed by the derived user and seedless backup children', async function () { + const keyParams = await signedKeyParams(); + assert(!('rootAddress' in keyParams)); + assert.strictEqual(new Set([keys.user.pub, keys.backup.pub, keys.bitgo.pub]).size, 3); + const decryptSpy = sandbox.spy(bitgo, 'decrypt'); + const broadcastStub = sandbox.stub(basecoin, 'broadcastTransaction').rejects(new Error('must not broadcast')); + + const recovery = await basecoin.recover({ ...recoveryParams, ...keyParams }); + assert('tx' in recovery); + assert.strictEqual(recovery.coin, 'thbar'); + assert.strictEqual(recovery.rootAddress, recoveryParams.rootAddress); + assert.strictEqual(recovery.userKey, encodeHbarPublicKeyDer(Buffer.from(keys.user.pub, 'hex'))); + assert.strictEqual(recovery.backupKey, encodeHbarPublicKeyDer(Buffer.from(keys.backup.pub, 'hex'))); + assert.strictEqual(recovery.bitgoKey, encodeHbarPublicKeyDer(Buffer.from(keys.bitgo.pub, 'hex'))); + assert.notStrictEqual(recovery.userKey, keyParams.userKey); + assert.deepStrictEqual( + Object.keys(recovery).sort(), + ['tx', 'id', 'coin', 'startTime', 'nodeId', 'userKey', 'backupKey', 'bitgoKey', 'rootAddress'].sort() + ); + const serialized = JSON.stringify(recovery); + for (const secret of [keys.user.seed, keys.backup.kL, keys.backup.kR, keyParams.walletPassphrase]) { + assert(!serialized.includes(secret)); + } + + const { bodyBytes, sigMap } = proto.Transaction.decode(Buffer.from(recovery.tx, 'hex')); + assert(sigMap?.sigPair); + assert.deepStrictEqual( + sigMap.sigPair.map((pair) => Buffer.from(pair.pubKeyPrefix ?? []).toString('hex')).sort(), + [keys.user.pub, keys.backup.pub].sort() + ); + for (const pair of sigMap.sigPair) { + assert(pair.ed25519); + assert(pair.pubKeyPrefix); + assert(nacl.sign.detached.verify(bodyBytes, pair.ed25519, pair.pubKeyPrefix)); + } + Sinon.assert.calledOnce(keyParams.externalBackupSigner.sign); + assert.deepStrictEqual(Buffer.from(keyParams.externalBackupSigner.sign.firstCall.args[0]), Buffer.from(bodyBytes)); + Sinon.assert.calledOnce(decryptSpy); + Sinon.assert.notCalled(broadcastStub); + + const transaction = new Transaction(coins.get('thbar')); + transaction.fromRawTransaction(recovery.tx); + const json = transaction.toJson(); + assert.strictEqual(json.from, recoveryParams.rootAddress); + assert.strictEqual(json.to, recoveryParams.recoveryDestination); + assert.strictEqual(json.amount, '990000000'); + assert.strictEqual(json.startTime, recoveryParams.startTime); + assert.strictEqual(json.fee, Number(recoveryParams.maxFee)); + }); + + it('returns public keys and an unsigned transaction without encrypting or decrypting', async function () { + const encryptSpy = sandbox.spy(bitgo, 'encrypt'); + const decryptSpy = sandbox.spy(bitgo, 'decrypt'); + const keyParams = await buildSafeRecoverKeyParams({ + bitgo, + coin: 'thbar', + slot: 'ed25519Multisig', + keys, + mode: 'unsigned', + }); + assert(keyParams.mode === 'unsigned'); + const recovery = await basecoin.recover({ ...recoveryParams, ...keyParams }); + assert('txHex' in recovery); + assert.strictEqual(recovery.address, recoveryParams.rootAddress); + assert.strictEqual(recovery.userKey, encodeHbarPublicKeyDer(Buffer.from(keys.user.pub, 'hex'))); + assert.strictEqual(recovery.backupKey, encodeHbarPublicKeyDer(Buffer.from(keys.backup.pub, 'hex'))); + assert.strictEqual(recovery.bitgoKey, encodeHbarPublicKeyDer(Buffer.from(keys.bitgo.pub, 'hex'))); + const { sigMap } = proto.Transaction.decode(Buffer.from(recovery.txHex, 'hex')); + assert.strictEqual(sigMap?.sigPair?.length ?? 0, 0); + Sinon.assert.notCalled(encryptSpy); + Sinon.assert.notCalled(decryptSpy); + }); + + for (const missingSigner of ['user', 'backup'] as const) { + it(`rejects a missing on-chain ${missingSigner} key before querying the balance or signing`, async function () { + const keyParams = await signedKeyParams(); + getAccountKeysStub.resolves( + [keys.user.pub, keys.backup.pub, keys.bitgo.pub].filter((pub) => pub !== keys[missingSigner].pub) + ); + await assert.rejects(() => basecoin.recover({ ...recoveryParams, ...keyParams }), RecoveryKeyMismatchError); + Sinon.assert.notCalled(getAccountBalanceStub); + Sinon.assert.notCalled(keyParams.externalBackupSigner.sign); + }); + } + + it('rejects a malformed optional bitgoKey before querying the account, the balance, or signing', async function () { + const keyParams = await signedKeyParams(); + await assert.rejects( + () => basecoin.recover({ ...recoveryParams, ...keyParams, bitgoKey: 'not-a-public-key' }), + /invalid bitgoKey/ + ); + Sinon.assert.notCalled(getAccountKeysStub); + Sinon.assert.notCalled(getAccountBalanceStub); + Sinon.assert.notCalled(keyParams.externalBackupSigner.sign); + }); + + it('reports an empty account without invoking the backup signer', async function () { + const keyParams = await signedKeyParams(); + getAccountBalanceStub.resolves({ hbars: new HbarUnit(0).toString(), tokens: [] }); + await assert.rejects(() => basecoin.recover({ ...recoveryParams, ...keyParams }), ErrorNoInputToRecover); + Sinon.assert.notCalled(keyParams.externalBackupSigner.sign); + }); + + for (const query of ['keys', 'balance'] as const) { + it(`preserves a ${query} provider failure instead of treating it as an empty account`, async function () { + const keyParams = await signedKeyParams(); + const failure = new RecoveryProviderError('fixture provider unavailable'); + const queries = { keys: getAccountKeysStub, balance: getAccountBalanceStub }; + queries[query].rejects(failure); + await assert.rejects( + () => basecoin.recover({ ...recoveryParams, ...keyParams }), + (error) => error === failure + ); + Sinon.assert.notCalled(keyParams.externalBackupSigner.sign); + if (query === 'keys') { + Sinon.assert.notCalled(getAccountBalanceStub); + } + }); + } +}); diff --git a/modules/sdk-core/src/bitgo/safe/derivableEd25519Pub.ts b/modules/sdk-core/src/bitgo/safe/derivableEd25519Pub.ts index 9e91fb9265..ed1318b2fb 100644 --- a/modules/sdk-core/src/bitgo/safe/derivableEd25519Pub.ts +++ b/modules/sdk-core/src/bitgo/safe/derivableEd25519Pub.ts @@ -184,7 +184,11 @@ export async function signWithExternalEd25519Signer( message: Uint8Array ): Promise { assertValidExternalEd25519Signer(signer); - const result = await signer.sign(message); + // The signer gets a defensive copy: it must not be able to mutate the authoritative transaction + // bytes, and the signature is verified against the original message, not whatever the signer + // received or modified. + const messageForSigner = Uint8Array.from(message); + const result = await signer.sign(messageForSigner); if (!result || typeof result.signature !== 'string') { throw new Error('Invalid external Ed25519 signer: sign must resolve to { signature: string }'); } diff --git a/modules/sdk-core/src/bitgo/safe/recovery.ts b/modules/sdk-core/src/bitgo/safe/recovery.ts index e789f5bc33..e08ed39375 100644 --- a/modules/sdk-core/src/bitgo/safe/recovery.ts +++ b/modules/sdk-core/src/bitgo/safe/recovery.ts @@ -20,6 +20,8 @@ import { toSoftTwinPath, algoMultisigAddress, encodeAlgoKey, + encodeHbarPublicKeyDer, + encodeHbarPrivateKeyDer, type SafeKeycardRootEntry, } from '@bitgo/sdk-lib-safes'; import { @@ -103,7 +105,8 @@ export type SecpSafeRecoverKeyParams = { * `rootAddress` is the wallet's on-chain address. For ALGO it's the 2-of-3 multisig address over the * three child pubs — derivable here, but `recover()` still requires it as input. Note this is * *derived*, not independently verified against a known wallet address; callers with a recorded - * address should cross-check it themselves. + * address should cross-check it themselves. For HBAR this field is omitted so the caller's + * network-assigned account ID is preserved when the key params are spread into a recovery request. */ export type Ed25519SafeRecoverKeyParams = | { @@ -112,7 +115,7 @@ export type Ed25519SafeRecoverKeyParams = userKey: string; bitgoKey: string; walletPassphrase: string; - rootAddress: string; + rootAddress?: string; /** * Lazy signer for the seedless backup child. The backup child has no seed, so it cannot be * seed-signed; and its signature must cover `tx.bytesToSign()`, which only exists once @@ -127,7 +130,7 @@ export type Ed25519SafeRecoverKeyParams = userKey: string; backupKey: string; bitgoKey: string; - rootAddress: string; + rootAddress?: string; }; export type SafeRecoverKeyParams = SecpSafeRecoverKeyParams | Ed25519SafeRecoverKeyParams; @@ -295,14 +298,28 @@ export async function deriveSafeWalletKeys(params: { /** * Per-coin encoders for the `ed25519Multisig` slot: each slot-2 coin re-encodes the same 32-byte * children into its own address/seed string formats. A coin absent from the registry is not yet - * supported for slot-2 recovery (xlm/hbar are future arms). + * supported for slot-2 recovery (xlm is a future arm). */ +const ALGO_ENCODERS = { + encodePub: encodeAlgoKey, + encodeSeed: encodeAlgoKey, + deriveRootAddress: (userPub: Buffer, backupPub: Buffer, bitgoPub: Buffer) => + algoMultisigAddress(1, 2, [userPub, backupPub, bitgoPub]), +}; + const ED25519_MULTISIG_COIN_ENCODERS: Record< string, - { encodePub: (bytes: Buffer) => string; encodeSeed: (bytes: Buffer) => string } | undefined + | { + encodePub: (bytes: Buffer) => string; + encodeSeed: (bytes: Buffer) => string; + deriveRootAddress?: (userPub: Buffer, backupPub: Buffer, bitgoPub: Buffer) => string; + } + | undefined > = { - algo: { encodePub: encodeAlgoKey, encodeSeed: encodeAlgoKey }, - talgo: { encodePub: encodeAlgoKey, encodeSeed: encodeAlgoKey }, + algo: ALGO_ENCODERS, + talgo: ALGO_ENCODERS, + hbar: { encodePub: encodeHbarPublicKeyDer, encodeSeed: encodeHbarPrivateKeyDer }, + thbar: { encodePub: encodeHbarPublicKeyDer, encodeSeed: encodeHbarPrivateKeyDer }, }; /** @@ -325,7 +342,8 @@ export type SafeRecoverKeyParamsRequest = { * Builds the key fields `coin.recover()` expects from derived Safe child keys. * Supports `secp256k1Multisig` (slot 1) and `ed25519Multisig` (slot 2); the MPC slots fail closed. * Signed mode re-encrypts the child secrets under a fresh ephemeral passphrase (v1/SJCL); unsigned - * mode returns pubs. `rootAddress` is derived from the child keys, not independently verified. + * mode returns pubs. Where supported, `rootAddress` is derived from the child keys, not independently + * verified; otherwise it is omitted so the caller can supply it. */ export async function buildSafeRecoverKeyParams( params: Extract @@ -400,10 +418,11 @@ async function buildEd25519MultisigRecoverKeyParams( const backupPub = Buffer.from(keys.backup.pub, 'hex'); const bitgoPub = Buffer.from(keys.bitgo.pub, 'hex'); - // Algorand wallet addresses are the version-1 2-of-3 multisig address over the three child pubs - // in wallet-creation order. `Algo.recover()` recomputes and enforces the same value and throws - // `RecoveryKeyMismatchError` on any mismatch (sdk-coin-algo/src/algo.ts). - const rootAddress = algoMultisigAddress(1, 2, [userPub, backupPub, bitgoPub]); + // The root address is derivable from child pubs for ALGO (version-1 2-of-3 multisig), but + // not for HBAR (Hedera account IDs are network-assigned). Each coin declares, via its registry + // entry, whether it can derive one; the caller supplies it otherwise. + const rootAddress = encoders.deriveRootAddress?.(userPub, backupPub, bitgoPub); + const rootAddressParams = rootAddress === undefined ? {} : { rootAddress }; if (mode === 'unsigned') { return { @@ -412,7 +431,7 @@ async function buildEd25519MultisigRecoverKeyParams( userKey: encoders.encodePub(userPub), backupKey: encoders.encodePub(backupPub), bitgoKey: encoders.encodePub(bitgoPub), - rootAddress, + ...rootAddressParams, }; } @@ -432,7 +451,7 @@ async function buildEd25519MultisigRecoverKeyParams( userKey, bitgoKey: encoders.encodePub(bitgoPub), walletPassphrase, - rootAddress, + ...rootAddressParams, externalBackupSigner: { publicKey: keys.backup.pub, sign: async (message) => ({ diff --git a/modules/sdk-core/test/unit/bitgo/safe/derivableEd25519Pub.ts b/modules/sdk-core/test/unit/bitgo/safe/derivableEd25519Pub.ts index 40c060b959..df98ad75a0 100644 --- a/modules/sdk-core/test/unit/bitgo/safe/derivableEd25519Pub.ts +++ b/modules/sdk-core/test/unit/bitgo/safe/derivableEd25519Pub.ts @@ -280,5 +280,21 @@ describe('derivableEd25519Pub', function () { signature.should.match(/^[0-9a-f]{128}$/); nacl.sign.detached.verify(message, Buffer.from(signature, 'hex'), keyPair.publicKey).should.equal(true); }); + + it('rejects a signer that mutates its message argument, leaving the original message intact', async function () { + const originalMessage = Uint8Array.from(message); + const mutatingSigner: ExternalEd25519Signer = { + publicKey, + sign: async (m: Uint8Array) => { + m.fill(0x00); + return { signature: Buffer.from(nacl.sign.detached(m, keyPair.secretKey)).toString('hex') }; + }, + }; + await signWithExternalEd25519Signer(mutatingSigner, originalMessage).should.be.rejectedWith( + /does not verify against its public key/ + ); + // The authoritative message must be unchanged: the signer only ever saw a defensive copy. + Buffer.from(originalMessage).equals(Buffer.from(message)).should.equal(true); + }); }); }); diff --git a/modules/sdk-core/test/unit/bitgo/safe/recovery.ts b/modules/sdk-core/test/unit/bitgo/safe/recovery.ts index e59f7dfdb0..7bb788adf1 100644 --- a/modules/sdk-core/test/unit/bitgo/safe/recovery.ts +++ b/modules/sdk-core/test/unit/bitgo/safe/recovery.ts @@ -515,7 +515,7 @@ describe('buildSafeRecoverKeyParams', () => { }).should.be.rejectedWith(/not supported yet/); }); - describe('ed25519Multisig (algo)', () => { + describe('ed25519Multisig', () => { // Golden vectors generated with algosdk: encodeAddress(Buffer.alloc(32, n)) and // multisigAddress({ version: 1, threshold: 2, addrs: [...] }) over the same pubs. const edKeys = { @@ -544,6 +544,7 @@ describe('buildSafeRecoverKeyParams', () => { result.userKey.should.equal('EEQSCIJBEEQSCIJBEEQSCIJBEEQSCIJBEEQSCIJBEEQSCIJBEEQYNI53HM'); result.backupKey.should.equal('KFIVCUKRKFIVCUKRKFIVCUKRKFIVCUKRKFIVCUKRKFIVCUKRKFI33B3AOY'); result.bitgoKey.should.equal('MFQWCYLBMFQWCYLBMFQWCYLBMFQWCYLBMFQWCYLBMFQWCYLBMFQS3KHEHM'); + assert(result.rootAddress); result.rootAddress.should.equal('LHWEIOU2UIPSKCZ3XMKQUKRYY3FM6XTZKBHDMIU737OQQEXQTYSGDFOHDM'); result.userKey.should.match(/^[A-Z2-7]{58}$/); result.should.not.have.property('walletPassphrase'); @@ -606,6 +607,7 @@ describe('buildSafeRecoverKeyParams', () => { result.bitgoKey.should.equal(encodeAlgoKey(Buffer.from(derived.keys.bitgo.pub, 'hex'))); result.bitgoKey.should.match(/^[A-Z2-7]{58}$/); // The wallet's on-chain address over the three distinct children, in wallet-creation order. + assert(result.rootAddress); result.rootAddress.should.equal( algoMultisigAddress(1, 2, [ Buffer.from(derived.keys.user.pub, 'hex'), @@ -633,6 +635,48 @@ describe('buildSafeRecoverKeyParams', () => { .should.equal(true); }); + for (const coin of ['hbar', 'thbar']) { + for (const mode of ['signed', 'unsigned'] as const) { + it(`${coin} ${mode} mode encodes Hedera keys without overwriting the caller's rootAddress`, async () => { + const encrypt = sinon.stub().callsFake(async ({ input }) => `enc(${input})`); + const bitgo = { encrypt } as unknown as BitGoBase; + + const result = await buildSafeRecoverKeyParams({ + bitgo, + coin, + slot: 'ed25519Multisig', + keys: edKeys, + mode, + }); + + assert(!('rootAddress' in result)); + const rootAddress = '0.0.12345'; + assert.strictEqual({ rootAddress, ...result }.rootAddress, rootAddress); + result.bitgoKey.should.equal('302a300506032b6570032100' + edKeys.bitgo.pub); + + if (result.mode === 'unsigned') { + result.userKey.should.equal('302a300506032b6570032100' + edKeys.user.pub); + result.backupKey.should.equal('302a300506032b6570032100' + edKeys.backup.pub); + result.should.not.have.property('walletPassphrase'); + result.should.not.have.property('externalBackupSigner'); + sinon.assert.notCalled(encrypt); + } else { + const encodedSeed = '302e020100300506032b657004220420' + edKeys.user.seed; + result.userKey.should.equal(`enc(${encodedSeed})`); + result.should.not.have.property('backupKey'); + result.externalBackupSigner.publicKey.should.equal(edKeys.backup.pub); + result.walletPassphrase.should.match(/^[0-9a-f]{64}$/); + sinon.assert.calledOnce(encrypt); + sinon.assert.calledWithExactly(encrypt, { + input: encodedSeed, + password: result.walletPassphrase, + encryptionVersion: 1, + }); + } + }); + } + } + it('rejects slot-2 coins that have no registry entry yet', async () => { const bitgo = { encrypt: sinon.stub() } as unknown as BitGoBase; await buildSafeRecoverKeyParams({