From ab41c93a3ecf2078dfc46e5643e1eb3cf8e4d533 Mon Sep 17 00:00:00 2001 From: Alejo Acosta Date: Thu, 7 Nov 2024 21:17:32 -0300 Subject: [PATCH 1/3] implement helper methods to get change and address HD node --- src/wallet/hdwallet.ts | 72 ++++++++++++-- src/wallet/qi-hdwallet.ts | 184 +++++++++++++++++++++++------------- src/wallet/quai-hdwallet.ts | 86 +++++++++-------- 3 files changed, 228 insertions(+), 114 deletions(-) diff --git a/src/wallet/hdwallet.ts b/src/wallet/hdwallet.ts index 526a5fe9..2a0f88a5 100644 --- a/src/wallet/hdwallet.ts +++ b/src/wallet/hdwallet.ts @@ -105,6 +105,30 @@ export abstract class AbstractHDWallet} senderPaymentCodeInfo - Map of payment code to array ofQi address + * information. * @interface SerializedQiHDWallet */ export interface SerializedQiHDWallet extends SerializedHDWallet { @@ -261,9 +261,18 @@ export class QiHDWallet extends AbstractHDWallet { this._addressesMap.set(QiHDWallet.PRIVATE_KEYS_PATH, privateKeysArray); } - const newAddrInfo = { - pubKey: addressNode.publicKey, + return this._createAndStoreQiAddressInfo(addressNode, account, zone, isChange); + } + + private _createAndStoreQiAddressInfo( + addressNode: HDNodeWallet, + account: number, + zone: Zone, + isChange: boolean, + ): QiAddressInfo { + const qiAddressInfo: QiAddressInfo = { address: addressNode.address, + pubKey: addressNode.publicKey, account, index: addressNode.index, change: isChange, @@ -271,8 +280,9 @@ export class QiHDWallet extends AbstractHDWallet { status: AddressStatus.UNUSED, derivationPath: isChange ? 'BIP44:change' : 'BIP44:external', }; - this._addressesMap.get(isChange ? 'BIP44:change' : 'BIP44:external')?.push(newAddrInfo); - return newAddrInfo; + + this._addressesMap.get(isChange ? 'BIP44:change' : 'BIP44:external')!.push(qiAddressInfo); // _addressesMap is initialized within the constructor + return qiAddressInfo; } /** @@ -951,11 +961,7 @@ export class QiHDWallet extends AbstractHDWallet { if (addressInfo.derivationPath === 'BIP44:external' || addressInfo.derivationPath === 'BIP44:change') { // (BIP44 addresses) - const changeIndex = addressInfo.change ? 1 : 0; - const addressNode = this._root - .deriveChild(addressInfo.account + HARDENED_OFFSET) - .deriveChild(changeIndex) - .deriveChild(addressInfo.index); + const addressNode = this._getAddressNode(addressInfo.account, addressInfo.change, addressInfo.index); return addressNode.privateKey; } else { // (BIP47 addresses) @@ -1290,42 +1296,18 @@ export class QiHDWallet extends AbstractHDWallet { const root = HDNodeWallet.fromMnemonic(mnemonic, path); const wallet = new this(_guard, root); - const validateQiAddressInfo = (addressInfo: QiAddressInfo): void => { - wallet.validateNeuteredAddressInfo(addressInfo); - - if (!Object.values(AddressStatus).includes(addressInfo.status)) { - throw new Error(`Invalid QiAddressInfo: status '${addressInfo.status}' is not a valid AddressStatus`); - } - - if ( - addressInfo.derivationPath !== 'BIP44:external' && - addressInfo.derivationPath !== 'BIP44:change' && - !validatePaymentCode(addressInfo.derivationPath) - ) { - throw new Error( - `Invalid QiAddressInfo: derivationPath '${addressInfo.derivationPath}' is not valid. It should be 'BIP44:external', 'BIP44:change', or a valid BIP47 payment code`, - ); - } - }; - - // First, group addresses by derivation path - const addressesByPath = new Map(); + // validate and import all the wallet addresses for (const addressInfo of serialized.addresses) { - validateQiAddressInfo(addressInfo); + wallet.validateAddressInfo(addressInfo); let key = addressInfo.derivationPath; if (isHexString(key, 32)) { key = QiHDWallet.PRIVATE_KEYS_PATH; } - - if (!addressesByPath.has(key)) { - addressesByPath.set(key, []); + const existingAddresses = wallet._addressesMap.get(key); + if (existingAddresses && existingAddresses.some((addr) => addr.address === addressInfo.address)) { + throw new Error(`Address ${addressInfo.address} already exists in the wallet`); } - addressesByPath.get(key)!.push(addressInfo); - } - - // Then, set all paths in the wallet's address map - for (const [key, addresses] of addressesByPath) { - wallet._addressesMap.set(key, addresses); + wallet._addressesMap.get(key)!.push(addressInfo); } // validate and import the counter party payment code info @@ -1334,7 +1316,7 @@ export class QiHDWallet extends AbstractHDWallet { throw new Error(`Invalid payment code: ${paymentCode}`); } for (const pcInfo of paymentCodeInfoArray) { - validateQiAddressInfo(pcInfo); + wallet.validateAddressInfo(pcInfo); } wallet._paymentCodeSendAddressMap.set(paymentCode, paymentCodeInfoArray); } @@ -1350,6 +1332,90 @@ export class QiHDWallet extends AbstractHDWallet { return wallet; } + protected validateAddressDerivation(info: QiAddressInfo): void { + const addressNode = this._getAddressNode(info.account, info.change, info.index); + + // Validate derived address matches + if (addressNode.address !== info.address) { + throw new Error(`Address mismatch: derived ${addressNode.address} but got ${info.address}`); + } + + // Validate derived public key matches + if (addressNode.publicKey !== info.pubKey) { + throw new Error(`Public key mismatch: derived ${addressNode.publicKey} but got ${info.pubKey}`); + } + + // Validate zone + const zone = getZoneForAddress(addressNode.address); + if (!zone || zone !== info.zone) { + throw new Error(`Zone mismatch: derived ${zone} but got ${info.zone}`); + } + + // Validate it's a valid Qi address + if (!isQiAddress(addressNode.address)) { + throw new Error(`Address ${addressNode.address} is not a valid Qi address`); + } + } + + protected validateExtendedProperties(info: QiAddressInfo): void { + // Validate status + if (!Object.values(AddressStatus).includes(info.status)) { + throw new Error(`Invalid status: ${info.status}`); + } + + // Validate derivation path + if (typeof info.derivationPath !== 'string' || !info.derivationPath) { + throw new Error(`Invalid derivation path: ${info.derivationPath}`); + } + + // Validate derivation path format + this.validateDerivationPath(info.derivationPath, info.change); + } + + /** + * Validates that the derivation path is either a BIP44 path or a valid payment code. + * + * @private + * @param {string} path - The derivation path to validate + * @param {boolean} isChange - Whether this is a change address + * @throws {Error} If the path is invalid + */ + private validateDerivationPath(path: string, isChange: boolean): void { + // Check if it's a BIP44 path + if (path === 'BIP44:external' || path === 'BIP44:change') { + // Validate that the path matches the change flag + const expectedPath = isChange ? 'BIP44:change' : 'BIP44:external'; + if (path !== expectedPath) { + throw new Error( + `BIP44 path mismatch: address marked as ${isChange ? 'change' : 'external'} ` + + `but has path ${path}`, + ); + } + return; + } + + // Check if it's a private key path + if (path === QiHDWallet.PRIVATE_KEYS_PATH) { + if (isChange) { + throw new Error('Imported private key addresses cannot be change addresses'); + } + return; + } + + // If not a BIP44 path or private key, must be a valid payment code + if (!validatePaymentCode(path)) { + throw new Error( + `Invalid derivation path: must be 'BIP44:external', 'BIP44:change', ` + + `'${QiHDWallet.PRIVATE_KEYS_PATH}', or a valid payment code. Got: ${path}`, + ); + } + + // Payment code addresses cannot be change addresses + if (isChange) { + throw new Error('Payment code addresses cannot be change addresses'); + } + } + /** * Validates an array of OutpointInfo objects. This method checks the validity of each OutpointInfo object by * performing the following validations: @@ -1654,6 +1720,9 @@ export class QiHDWallet extends AbstractHDWallet { * @returns {QiAddressInfo} The address info for the new address. */ public addAddress(account: number, addressIndex: number): QiAddressInfo { + if (account < 0 || addressIndex < 0) { + throw new Error('Account and address index must be non-negative integers'); + } return this._addAddress(account, addressIndex, false); } @@ -1665,6 +1734,9 @@ export class QiHDWallet extends AbstractHDWallet { * @returns {QiAddressInfo} The address info for the new address. */ public addChangeAddress(account: number, addressIndex: number): QiAddressInfo { + if (account < 0 || addressIndex < 0) { + throw new Error('Account and address index must be non-negative integers'); + } return this._addAddress(account, addressIndex, true); } @@ -1675,11 +1747,7 @@ export class QiHDWallet extends AbstractHDWallet { if (existingAddresses.some((info) => info.index === addressIndex)) { throw new Error(`Address index ${addressIndex} already exists in wallet under path ${derivationPath}`); } - - const addressNode = this._root - .deriveChild(account + HARDENED_OFFSET) - .deriveChild(isChange ? 1 : 0) - .deriveChild(addressIndex); + const addressNode = this._getAddressNode(account, isChange, addressIndex); const zone = getZoneForAddress(addressNode.address); if (!zone) { throw new Error(`Failed to derive a Qi valid address zone for the index ${addressIndex}`); @@ -1689,25 +1757,7 @@ export class QiHDWallet extends AbstractHDWallet { throw new Error(`Address ${addressNode.address} is not a valid Qi address`); } - const addressInfo: QiAddressInfo = { - pubKey: addressNode.publicKey, - address: addressNode.address, - account, - index: addressIndex, - change: isChange, - zone, - status: AddressStatus.UNUSED, - derivationPath, - }; - - const addresses = this._addressesMap.get(derivationPath); - if (!addresses) { - this._addressesMap.set(derivationPath, [addressInfo]); - } else { - addresses.push(addressInfo); - } - - return addressInfo; + return this._createAndStoreQiAddressInfo(addressNode, account, zone, isChange); } /** diff --git a/src/wallet/quai-hdwallet.ts b/src/wallet/quai-hdwallet.ts index 2e742bc7..d0e40cc2 100644 --- a/src/wallet/quai-hdwallet.ts +++ b/src/wallet/quai-hdwallet.ts @@ -3,7 +3,7 @@ import { HDNodeWallet } from './hdnodewallet.js'; import { QuaiTransactionRequest, Provider, TransactionResponse } from '../providers/index.js'; import { isQuaiAddress, resolveAddress } from '../address/index.js'; import { AllowedCoinType, Zone } from '../constants/index.js'; -import { SerializedHDWallet, HARDENED_OFFSET } from './hdwallet.js'; +import { SerializedHDWallet } from './hdwallet.js'; import { Mnemonic } from './mnemonic.js'; import { TypedDataDomain, TypedDataField } from '../hash/index.js'; import { getZoneForAddress } from '../utils/index.js'; @@ -131,6 +131,31 @@ export class QuaiHDWallet extends AbstractHDWallet { }; } + protected validateAddressDerivation(info: NeuteredAddressInfo): void { + const addressNode = this._getAddressNode(info.account, false, info.index); + + // Validate derived address matches + if (addressNode.address !== info.address) { + throw new Error(`Address mismatch: derived ${addressNode.address} but got ${info.address}`); + } + + // Validate derived public key matches + if (addressNode.publicKey !== info.pubKey) { + throw new Error(`Public key mismatch: derived ${addressNode.publicKey} but got ${info.pubKey}`); + } + + // Validate zone + const zone = getZoneForAddress(addressNode.address); + if (!zone || zone !== info.zone) { + throw new Error(`Zone mismatch: derived ${zone} but got ${info.zone}`); + } + + // Validate it's a valid Quai address + if (!isQuaiAddress(addressNode.address)) { + throw new Error(`Address ${addressNode.address} is not a valid Quai address`); + } + } + /** * Deserializes the given serialized HD wallet data into an instance of QuaiHDWallet. * @@ -150,7 +175,13 @@ export class QuaiHDWallet extends AbstractHDWallet { const wallet = new this(_guard, root); // import the addresses - wallet.importSerializedAddresses(serialized.addresses); + for (const addressInfo of serialized.addresses) { + wallet.validateAddressInfo(addressInfo); + if (wallet._addresses.has(addressInfo.address)) { + throw new Error(`Address ${addressInfo.address} already exists in the wallet`); + } + wallet._addresses.set(addressInfo.address, addressInfo); + } return wallet; } @@ -184,6 +215,9 @@ export class QuaiHDWallet extends AbstractHDWallet { * @returns {NeuteredAddressInfo} The added address info. */ public addAddress(account: number, addressIndex: number): NeuteredAddressInfo { + if (account < 0 || addressIndex < 0) { + throw new Error('Account and address index must be non-negative integers'); + } return this._addAddress(account, addressIndex) as NeuteredAddressInfo; } @@ -205,11 +239,7 @@ export class QuaiHDWallet extends AbstractHDWallet { }); // derive the address node and validate the zone - const changeIndex = 0; - const addressNode = this._root - .deriveChild(account + HARDENED_OFFSET) - .deriveChild(changeIndex) - .deriveChild(addressIndex); + const addressNode = this._getAddressNode(account, false, addressIndex); const zone = getZoneForAddress(addressNode.address); if (!zone) { throw new Error(`Failed to derive a valid address zone for the index ${addressIndex}`); @@ -219,33 +249,7 @@ export class QuaiHDWallet extends AbstractHDWallet { throw new Error(`Address ${addressNode.address} is not a valid Quai address`); } - return this.createAndStoreAddressInfo(addressNode, account, zone); - } - - /** - * Imports addresses from a serialized wallet into the addresses map. Before adding the addresses, a validation is - * performed to ensure the address, public key, and zone match the expected values. - * - * @param {Map} addressMap - The map where the addresses will be imported. - * @param {NeuteredAddressInfo[]} addresses - The array of addresses to be imported, each containing account, index, - * address, pubKey, and zone information. - * @throws {Error} If there is a mismatch between the expected and actual address, public key, or zone. - * @protected - */ - protected importSerializedAddresses(addresses: NeuteredAddressInfo[]): void { - for (const addressInfo of addresses) { - const newAddressInfo = this._addAddress(addressInfo.account, addressInfo.index); - // validate the address info - if (addressInfo.address !== newAddressInfo.address) { - throw new Error(`Address mismatch: ${addressInfo.address} != ${newAddressInfo.address}`); - } - if (addressInfo.pubKey !== newAddressInfo.pubKey) { - throw new Error(`Public key mismatch: ${addressInfo.pubKey} != ${newAddressInfo.pubKey}`); - } - if (addressInfo.zone !== newAddressInfo.zone) { - throw new Error(`Zone mismatch: ${addressInfo.zone} != ${newAddressInfo.zone}`); - } - } + return this._createAndStoreNeuteredAddressInfo(addressNode, account, zone); } /** @@ -283,7 +287,7 @@ export class QuaiHDWallet extends AbstractHDWallet { this.validateZone(zone); const lastIndex = this._findLastUsedIndex(Array.from(this._addresses.values()), accountIndex, zone); const addressNode = this.deriveNextAddressNode(accountIndex, lastIndex + 1, zone, false); - return this.createAndStoreAddressInfo(addressNode, accountIndex, zone); + return this._createAndStoreNeuteredAddressInfo(addressNode, accountIndex, zone); } /** @@ -300,7 +304,11 @@ export class QuaiHDWallet extends AbstractHDWallet { * @returns {NeuteredAddressInfo} - The created NeuteredAddressInfo object. * @protected */ - protected createAndStoreAddressInfo(addressNode: HDNodeWallet, account: number, zone: Zone): NeuteredAddressInfo { + private _createAndStoreNeuteredAddressInfo( + addressNode: HDNodeWallet, + account: number, + zone: Zone, + ): NeuteredAddressInfo { const neuteredAddressInfo: NeuteredAddressInfo = { pubKey: addressNode.publicKey, address: addressNode.address, @@ -358,11 +366,7 @@ export class QuaiHDWallet extends AbstractHDWallet { throw new Error(`Address ${addr} is not known to this wallet`); } - const changeIndex = 0; - return this._root - .deriveChild(addressInfo.account + HARDENED_OFFSET) - .deriveChild(changeIndex) - .deriveChild(addressInfo.index); + return this._getAddressNode(addressInfo.account, false, addressInfo.index); } /** From 0a9a3d56f7ca67b1a79dc5da6fdc3743b17a80fa Mon Sep 17 00:00:00 2001 From: Alejo Acosta Date: Wed, 13 Nov 2024 09:12:42 -0300 Subject: [PATCH 2/3] implement custom address validator and harden addAddress method --- src/wallet/qi-hdwallet.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/wallet/qi-hdwallet.ts b/src/wallet/qi-hdwallet.ts index c0d73e9e..d8175a8c 100644 --- a/src/wallet/qi-hdwallet.ts +++ b/src/wallet/qi-hdwallet.ts @@ -1307,6 +1307,10 @@ export class QiHDWallet extends AbstractHDWallet { if (existingAddresses && existingAddresses.some((addr) => addr.address === addressInfo.address)) { throw new Error(`Address ${addressInfo.address} already exists in the wallet`); } + const existingAddresses = wallet._addressesMap.get(key); + if (existingAddresses && existingAddresses.some((addr) => addr.address === addressInfo.address)) { + throw new Error(`Address ${addressInfo.address} already exists in the wallet`); + } wallet._addressesMap.get(key)!.push(addressInfo); } From 0c62e0144ec48499d1c19c9b310b664d21c9eeb6 Mon Sep 17 00:00:00 2001 From: Alejo Acosta Date: Fri, 15 Nov 2024 12:07:58 -0300 Subject: [PATCH 3/3] fix: remove duplicated code --- src/wallet/qi-hdwallet.ts | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/wallet/qi-hdwallet.ts b/src/wallet/qi-hdwallet.ts index d8175a8c..c0d73e9e 100644 --- a/src/wallet/qi-hdwallet.ts +++ b/src/wallet/qi-hdwallet.ts @@ -1307,10 +1307,6 @@ export class QiHDWallet extends AbstractHDWallet { if (existingAddresses && existingAddresses.some((addr) => addr.address === addressInfo.address)) { throw new Error(`Address ${addressInfo.address} already exists in the wallet`); } - const existingAddresses = wallet._addressesMap.get(key); - if (existingAddresses && existingAddresses.some((addr) => addr.address === addressInfo.address)) { - throw new Error(`Address ${addressInfo.address} already exists in the wallet`); - } wallet._addressesMap.get(key)!.push(addressInfo); }