Skip to content

Commit

Permalink
Merge pull request #4916 from BitGo/BTC-1451.fix-validate-address
Browse files Browse the repository at this point in the history
fix(utxo): descriptor address validation
  • Loading branch information
OttoAllmendinger authored Sep 20, 2024
2 parents 755063b + 7d67509 commit 31148da
Show file tree
Hide file tree
Showing 12 changed files with 184 additions and 119 deletions.
1 change: 1 addition & 0 deletions modules/abstract-utxo/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@
"@types/bluebird": "^3.5.25",
"@types/lodash": "^4.14.121",
"@types/superagent": "4.1.15",
"io-ts": "2.1.3",
"bignumber.js": "^9.0.2",
"bitcoinjs-message": "npm:@bitgo-forks/[email protected]",
"bluebird": "^3.5.3",
Expand Down
93 changes: 27 additions & 66 deletions modules/abstract-utxo/src/abstractUtxoCoin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ import { isReplayProtectionUnspent } from './replayProtection';
import { signAndVerifyPsbt, signAndVerifyWalletTransaction } from './sign';
import { supportedCrossChainRecoveries } from './config';
import { explainPsbt, explainTx, getPsbtTxInputs, getTxInputs } from './transaction';
import { Descriptor } from '@bitgo/wasm-miniscript';
import { assertDescriptorWalletAddress } from './descriptor/assertDescriptorWalletAddress';

type UtxoCustomSigningFunction<TNumber extends number | bigint> = {
(params: {
Expand Down Expand Up @@ -109,7 +109,7 @@ type Unspent<TNumber extends number | bigint = number> = bitgo.Unspent<TNumber>;

type RootWalletKeys = bitgo.RootWalletKeys;

type UtxoCoinSpecific = AddressCoinSpecific | DescriptorAddressCoinSpecific;
export type UtxoCoinSpecific = AddressCoinSpecific | DescriptorAddressCoinSpecific;

export interface VerifyAddressOptions<TCoinSpecific extends UtxoCoinSpecific> extends BaseVerifyAddressOptions {
chain?: number;
Expand Down Expand Up @@ -249,21 +249,6 @@ export interface GenerateFixedScriptAddressOptions extends GenerateAddressOption
}[];
}

export type NamedDescriptor = {
/** Name of the descriptor */
name: string;
/** Descriptor value */
value: string;
/** Descriptor signatures */
signatures: string[];
/** Highest address index */
lastIndex: number;
};

export interface GenerateDescriptorAddressOptions extends GenerateAddressOptions {
descriptor: NamedDescriptor;
}

export interface AddressDetails {
address: string;
chain: number;
Expand All @@ -274,7 +259,8 @@ export interface AddressDetails {
}

export interface DescriptorAddressCoinSpecific extends AddressCoinSpecific {
descriptor: NamedDescriptor;
descriptorName: string;
descriptorChecksum: string;
}

type UtxoBaseSignTransactionOptions<TNumber extends number | bigint = number> = BaseSignTransactionOptions & {
Expand Down Expand Up @@ -1016,39 +1002,39 @@ export abstract class AbstractUtxoCoin extends BaseCoin {
* @throws {InvalidAddressDerivationPropertyError}
* @throws {UnexpectedAddressError}
*/
async isWalletAddress(params: VerifyAddressOptions<UtxoCoinSpecific>): Promise<boolean> {
async isWalletAddress(params: VerifyAddressOptions<UtxoCoinSpecific>, wallet?: IWallet): Promise<boolean> {
const { address, addressType, keychains, chain, index } = params;

if (!this.isValidAddress(address)) {
throw new InvalidAddressError(`invalid address: ${address}`);
}

let expectedAddress: AddressDetails;
if (params.coinSpecific && 'descriptor' in params.coinSpecific) {
expectedAddress = this.generateAddress({
descriptor: params.coinSpecific.descriptor,
index,
});
} else {
if ((_.isUndefined(chain) && _.isUndefined(index)) || !(_.isFinite(chain) && _.isFinite(index))) {
throw new InvalidAddressDerivationPropertyError(
`address validation failure: invalid chain (${chain}) or index (${index})`
);
if (wallet) {
const walletCoinSpecific = wallet.coinSpecific();
if (walletCoinSpecific && 'descriptors' in walletCoinSpecific) {
assertDescriptorWalletAddress(this.network, params, walletCoinSpecific.descriptors);
return true;
}
}

if (!keychains) {
throw new Error('missing required param keychains');
}
if ((_.isUndefined(chain) && _.isUndefined(index)) || !(_.isFinite(chain) && _.isFinite(index))) {
throw new InvalidAddressDerivationPropertyError(
`address validation failure: invalid chain (${chain}) or index (${index})`
);
}

expectedAddress = this.generateAddress({
addressType: addressType as ScriptType2Of3,
keychains,
threshold: 2,
chain,
index,
});
if (!keychains) {
throw new Error('missing required param keychains');
}

const expectedAddress = this.generateAddress({
addressType: addressType as ScriptType2Of3,
keychains,
threshold: 2,
chain,
index,
});

if (expectedAddress.address !== address) {
throw new UnexpectedAddressError(
`address validation failure: expected ${expectedAddress.address} but got ${address}`
Expand Down Expand Up @@ -1086,15 +1072,6 @@ export abstract class AbstractUtxoCoin extends BaseCoin {
return [KeyIndices.USER, KeyIndices.BACKUP, KeyIndices.BITGO];
}

static isDescriptorWallet(
params:
| GenerateFixedScriptAddressOptions
| GenerateDescriptorAddressOptions
| VerifyAddressOptions<UtxoCoinSpecific>
): params is GenerateDescriptorAddressOptions {
return 'descriptor' in params;
}

/**
* TODO(BG-11487): Remove addressType, segwit, and bech32 params in SDKv6
* Generate an address for a wallet based on a set of configurations
Expand All @@ -1108,28 +1085,12 @@ export abstract class AbstractUtxoCoin extends BaseCoin {
* @param params.bech32 {boolean} Deprecated
* @returns {{chain: number, index: number, coin: number, coinSpecific: {outputScript, redeemScript}}}
*/
generateAddress(params: GenerateFixedScriptAddressOptions | GenerateDescriptorAddressOptions): AddressDetails {
generateAddress(params: GenerateFixedScriptAddressOptions): AddressDetails {
let derivationIndex = 0;
if (_.isInteger(params.index) && (params.index as number) > 0) {
derivationIndex = params.index as number;
}

if (AbstractUtxoCoin.isDescriptorWallet(params)) {
const descriptor = Descriptor.fromString(params.descriptor.value, 'derivable');
const scriptPubkey = Buffer.from(descriptor.atDerivationIndex(derivationIndex).scriptPubkey());
const address = utxolib.address.fromOutputScript(scriptPubkey, this.network);

return {
address,
chain: 0,
index: derivationIndex,
coin: this.getChain(),
coinSpecific: {
descriptor: params.descriptor,
},
};
}

const { keychains, threshold, chain, segwit = false, bech32 = false } = params as GenerateFixedScriptAddressOptions;

let derivationChain = getExternalChainCode('p2sh');
Expand Down
8 changes: 8 additions & 0 deletions modules/abstract-utxo/src/descriptor/NamedDescriptor.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import * as t from 'io-ts';

export const NamedDescriptor = t.type({
name: t.string,
value: t.string,
});

export type NamedDescriptor = t.TypeOf<typeof NamedDescriptor>;
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
import * as assert from 'assert';
import * as t from 'io-ts';
import * as utxolib from '@bitgo/utxo-lib';
import { Descriptor } from '@bitgo/wasm-miniscript';

import { UtxoCoinSpecific, VerifyAddressOptions } from '../abstractUtxoCoin';
import { NamedDescriptor } from './NamedDescriptor';

class DescriptorAddressMismatchError extends Error {
constructor(descriptor: Descriptor, index: number, derivedAddress: string, expectedAddress: string) {
super(
`Address mismatch for descriptor ${descriptor.toString()} at index ${index}: ${derivedAddress} !== ${expectedAddress}`
);
}
}

export function assertDescriptorWalletAddress(
network: utxolib.Network,
params: VerifyAddressOptions<UtxoCoinSpecific>,
descriptors: unknown
): void {
assert(params.coinSpecific);
assert(t.array(NamedDescriptor).is(descriptors));
assert('descriptorName' in params.coinSpecific);
assert('descriptorChecksum' in params.coinSpecific);
const descriptorName = params.coinSpecific.descriptorName;
const descriptorChecksum = params.coinSpecific.descriptorChecksum;
const namedDescriptor = descriptors.find((d) => d.name === descriptorName);
if (!namedDescriptor) {
throw new Error(`Descriptor ${descriptorName} not found`);
}
const descriptor = Descriptor.fromString(namedDescriptor.value, 'derivable');
const checksum = descriptor.toString().slice(-8);
if (checksum !== descriptorChecksum) {
throw new Error(
`Descriptor checksum mismatch for descriptor name=${descriptorName}: ${checksum} !== ${descriptorChecksum}`
);
}
const derivedScript = Buffer.from(descriptor.atDerivationIndex(params.index).scriptPubkey());
const derivedAddress = utxolib.address.fromOutputScript(derivedScript, network);
if (params.address !== derivedAddress) {
throw new DescriptorAddressMismatchError(descriptor, params.index, derivedAddress, params.address);
}
}
3 changes: 3 additions & 0 deletions modules/abstract-utxo/src/descriptor/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export { Miniscript, Descriptor } from '@bitgo/wasm-miniscript';
export { assertDescriptorWalletAddress } from './assertDescriptorWalletAddress';
export { NamedDescriptor } from './NamedDescriptor';
2 changes: 2 additions & 0 deletions modules/abstract-utxo/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,5 @@ export * from './config';
export * from './recovery';
export * from './replayProtection';
export * from './sign';

export * as descriptor from './descriptor';
3 changes: 1 addition & 2 deletions modules/bitgo/.mocharc.yml
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,4 @@ reporter: 'min'
reporter-option:
- 'cdn=true'
- 'json=false'
exit: true
spec: ['test/v2/unit/**/*.ts', test/unit/**/*.ts]
exit: true
2 changes: 1 addition & 1 deletion modules/bitgo/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
"webpack-dev": "cross-env NODE_ENV=development webpack",
"webpack-prod": "NODE_OPTIONS=--max-old-space-size=4096 cross-env NODE_ENV=production webpack",
"test": "npm run coverage",
"unit-test": "mocha",
"unit-test": "mocha 'test/v2/unit/**/*.ts' 'test/unit/**/*.ts'",
"coverage": "nyc -- npm run unit-test",
"integration-test": "nyc -- mocha \"test/v2/integration/**/*.ts\"",
"browser-test": "karma start karma.conf.js",
Expand Down
60 changes: 12 additions & 48 deletions modules/bitgo/test/v2/unit/coins/abstractUtxoCoin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,10 @@ import { BitGo } from '../../../../src/bitgo';
import {
AbstractUtxoCoin,
AbstractUtxoCoinWallet,
NamedDescriptor,
Output,
TransactionExplanation,
TransactionParams,
} from '@bitgo/abstract-utxo';
import * as assert from 'node:assert';

describe('Abstract UTXO Coin:', () => {
describe('Parse Transaction:', () => {
Expand Down Expand Up @@ -555,8 +553,18 @@ describe('Abstract UTXO Coin:', () => {
keySignatures: {},
outputs: [],
missingOutputs: [],
explicitExternalOutputs: [{ address: 'external_address', amount: '10000' }],
implicitExternalOutputs: [{ address: 'external_address_2', amount: '15' }],
explicitExternalOutputs: [
{
address: 'external_address',
amount: '10000',
},
],
implicitExternalOutputs: [
{
address: 'external_address_2',
amount: '15',
},
],
changeOutputs: [],
explicitExternalSpendAmount: BigInt(10000),
implicitExternalSpendAmount: BigInt(15),
Expand Down Expand Up @@ -584,48 +592,4 @@ describe('Abstract UTXO Coin:', () => {
bitcoinMock.restore();
});
});

describe('descriptor wallets', () => {
const bitgo: BitGo = TestBitGo.decorate(BitGo, { env: 'mock' });
const coin = bitgo.coin('tbtc') as AbstractUtxoCoin;

const rawDescriptor: NamedDescriptor = {
name: 'foo',
value:
"sh(multi(2,[00000000/111'/222]xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL,xpub68NZiKmJWnxxS6aaHmn81bvJeTESw724CRDs6HbuccFQN9Ku14VQrADWgqbhhTHBaohPX4CjNLf9fq9MYo6oDaPPLPxSb7gwQN3ih19Zm4Y/0/*))",
signatures: [],
lastIndex: 0,
};

it('should create an address', async () => {
const address = coin.generateAddress({ descriptor: rawDescriptor, index: 0 });

should.exist(address);
should.deepEqual(address.address, '2NAukrNbhcZNNZ83zFPN48X44FE5qNdYgDv');
assert('descriptor' in address.coinSpecific);
should.deepEqual(address.coinSpecific.descriptor, rawDescriptor);
});

it('should check if isWalletAddress', async () => {
const matchingAddressResult = await coin.isWalletAddress({
coinSpecific: { descriptor: rawDescriptor },
index: 0,
address: '2NAukrNbhcZNNZ83zFPN48X44FE5qNdYgDv',
});
should.equal(matchingAddressResult, true);

try {
await coin.isWalletAddress({
coinSpecific: { descriptor: rawDescriptor },
index: 1,
address: '2NAukrNbhcZNNZ83zFPN48X44FE5qNdYgDv',
});
throw '';
} catch (e) {
e.message.should.equal(
'address validation failure: expected 2N49LHwLRJ8oNXzLYWJM6sVVawC6QeC9B54 but got 2NAukrNbhcZNNZ83zFPN48X44FE5qNdYgDv'
);
}
});
});
});
Loading

0 comments on commit 31148da

Please sign in to comment.