From 20a1deb187d8fb38a42ae87fc8f046dff4b4ba61 Mon Sep 17 00:00:00 2001 From: Jacob Evans Date: Wed, 11 Apr 2018 12:47:17 +1000 Subject: Throw errors when the address is not specified or invalid --- .../src/subproviders/base_wallet_subprovider.ts | 2 +- packages/subproviders/src/subproviders/ledger.ts | 31 ++++++++++------------ .../subproviders/mnemonic_wallet_subprovider.ts | 27 ++++++++++++------- .../subproviders/private_key_wallet_subprovider.ts | 12 ++++----- packages/subproviders/src/types.ts | 4 +-- .../test/integration/ledger_subprovider_test.ts | 5 +++- .../test/unit/ledger_subprovider_test.ts | 5 ++-- .../test/unit/mnemonic_wallet_subprovider_test.ts | 2 +- .../unit/private_key_wallet_subprovider_test.ts | 21 +++++++++++++-- 9 files changed, 67 insertions(+), 42 deletions(-) (limited to 'packages/subproviders') diff --git a/packages/subproviders/src/subproviders/base_wallet_subprovider.ts b/packages/subproviders/src/subproviders/base_wallet_subprovider.ts index 47b45a126..0a9b99ae4 100644 --- a/packages/subproviders/src/subproviders/base_wallet_subprovider.ts +++ b/packages/subproviders/src/subproviders/base_wallet_subprovider.ts @@ -21,7 +21,7 @@ export abstract class BaseWalletSubprovider extends Subprovider { public abstract async getAccountsAsync(): Promise; public abstract async signTransactionAsync(txParams: PartialTxParams): Promise; - public abstract async signPersonalMessageAsync(data: string, address?: string): Promise; + public abstract async signPersonalMessageAsync(data: string, address: string): Promise; /** * This method conforms to the web3-provider-engine interface. diff --git a/packages/subproviders/src/subproviders/ledger.ts b/packages/subproviders/src/subproviders/ledger.ts index 6f66e3018..3c0442e4c 100644 --- a/packages/subproviders/src/subproviders/ledger.ts +++ b/packages/subproviders/src/subproviders/ledger.ts @@ -105,11 +105,9 @@ export class LedgerSubprovider extends BaseWalletSubprovider { public async signTransactionAsync(txParams: PartialTxParams): Promise { LedgerSubprovider._validateTxParams(txParams); const initialDerivedKey = await this._initialDerivedKeyAsync(); - const derivedKey = _.isUndefined(txParams.from) - ? walletUtils._firstDerivedKey(initialDerivedKey) - : this._findDerivedKeyByPublicAddress(initialDerivedKey, txParams.from); + const derivedKey = this._findDerivedKeyByPublicAddress(initialDerivedKey, txParams.from); - this._ledgerClientIfExists = await this._createLedgerClientAsync(); + const ledgerClient = await this._createLedgerClientAsync(); const tx = new EthereumTx(txParams); @@ -121,7 +119,7 @@ export class LedgerSubprovider extends BaseWalletSubprovider { const txHex = tx.serialize().toString('hex'); try { const derivationPath = `${derivedKey.derivationPath}/${derivedKey.derivationIndex}`; - const result = await this._ledgerClientIfExists.signTransaction(derivationPath, txHex); + const result = await ledgerClient.signTransaction(derivationPath, txHex); // Store signature in transaction tx.r = Buffer.from(result.r, 'hex'); tx.s = Buffer.from(result.s, 'hex'); @@ -145,7 +143,7 @@ export class LedgerSubprovider extends BaseWalletSubprovider { } /** * Sign a personal Ethereum signed message. The signing address will be the one - * either the provided address or the first address on the ledger device. + * the provided address. * The Ledger adds the Ethereum signed message prefix on-device. If you've added * the LedgerSubprovider to your app's provider, you can simply send an `eth_sign` * or `personal_sign` JSON RPC request, and this method will be called auto-magically. @@ -154,23 +152,18 @@ export class LedgerSubprovider extends BaseWalletSubprovider { * @param address Address to sign with * @return Signature hex string (order: rsv) */ - public async signPersonalMessageAsync(data: string, address?: string): Promise { + public async signPersonalMessageAsync(data: string, address: string): Promise { if (_.isUndefined(data)) { throw new Error(WalletSubproviderErrors.DataMissingForSignPersonalMessage); } assert.isHexString('data', data); const initialDerivedKey = await this._initialDerivedKeyAsync(); - const derivedKey = _.isUndefined(address) - ? walletUtils._firstDerivedKey(initialDerivedKey) - : this._findDerivedKeyByPublicAddress(initialDerivedKey, address); + const derivedKey = this._findDerivedKeyByPublicAddress(initialDerivedKey, address); - this._ledgerClientIfExists = await this._createLedgerClientAsync(); + const ledgerClient = await this._createLedgerClientAsync(); try { const derivationPath = `${derivedKey.derivationPath}/${derivedKey.derivationIndex}`; - const result = await this._ledgerClientIfExists.signPersonalMessage( - derivationPath, - ethUtil.stripHexPrefix(data), - ); + const result = await ledgerClient.signPersonalMessage(derivationPath, ethUtil.stripHexPrefix(data)); const v = result.v - 27; let vHex = v.toString(16); if (vHex.length < 2) { @@ -192,6 +185,7 @@ export class LedgerSubprovider extends BaseWalletSubprovider { } const ledgerEthereumClient = await this._ledgerEthereumClientFactoryAsync(); this._connectionLock.release(); + this._ledgerClientIfExists = ledgerEthereumClient; return ledgerEthereumClient; } private async _destroyLedgerClientAsync() { @@ -205,11 +199,11 @@ export class LedgerSubprovider extends BaseWalletSubprovider { this._connectionLock.release(); } private async _initialDerivedKeyAsync(): Promise { - this._ledgerClientIfExists = await this._createLedgerClientAsync(); + const ledgerClient = await this._createLedgerClientAsync(); let ledgerResponse; try { - ledgerResponse = await this._ledgerClientIfExists.getAddress( + ledgerResponse = await ledgerClient.getAddress( this._derivationPath, this._shouldAlwaysAskForConfirmation, SHOULD_GET_CHAIN_CODE, @@ -229,6 +223,9 @@ export class LedgerSubprovider extends BaseWalletSubprovider { }; } private _findDerivedKeyByPublicAddress(initalHDKey: DerivedHDKey, address: string): DerivedHDKey { + if (_.isUndefined(address)) { + throw new Error(WalletSubproviderErrors.FromAddressMissingOrInvalid); + } const matchedDerivedKey = walletUtils.findDerivedKeyByAddress(address, initalHDKey, this._addressSearchLimit); if (_.isUndefined(matchedDerivedKey)) { throw new Error(`${WalletSubproviderErrors.AddressNotFound}: ${address}`); diff --git a/packages/subproviders/src/subproviders/mnemonic_wallet_subprovider.ts b/packages/subproviders/src/subproviders/mnemonic_wallet_subprovider.ts index 53013c44c..730453bc6 100644 --- a/packages/subproviders/src/subproviders/mnemonic_wallet_subprovider.ts +++ b/packages/subproviders/src/subproviders/mnemonic_wallet_subprovider.ts @@ -63,6 +63,10 @@ export class MnemonicWalletSubprovider extends BaseWalletSubprovider { */ public setPath(derivationPath: string) { this._derivationPath = derivationPath; + this._derivedKey = { + ...this._derivedKey, + derivationPath: this._derivationPath, + }; } /** * Retrieve the accounts associated with the mnemonic. @@ -88,10 +92,7 @@ export class MnemonicWalletSubprovider extends BaseWalletSubprovider { * @return Signed transaction hex string */ public async signTransactionAsync(txParams: PartialTxParams): Promise { - const derivedKey = _.isUndefined(txParams.from) - ? walletUtils._firstDerivedKey(this._derivedKey) - : this._findDerivedKeyByPublicAddress(txParams.from); - const privateKeyWallet = new PrivateKeyWalletSubprovider(derivedKey.hdKey.privateKey.toString('hex')); + const privateKeyWallet = this._privateKeyWalletFromAddress(txParams.from); const signedTx = privateKeyWallet.signTransactionAsync(txParams); return signedTx; } @@ -105,15 +106,21 @@ export class MnemonicWalletSubprovider extends BaseWalletSubprovider { * @param address Address to sign with * @return Signature hex string (order: rsv) */ - public async signPersonalMessageAsync(data: string, address?: string): Promise { - const derivedKey = _.isUndefined(address) - ? walletUtils._firstDerivedKey(this._derivedKey) - : this._findDerivedKeyByPublicAddress(address); - const privateKeyWallet = new PrivateKeyWalletSubprovider(derivedKey.hdKey.privateKey.toString('hex')); - const sig = await privateKeyWallet.signPersonalMessageAsync(data, derivedKey.address); + public async signPersonalMessageAsync(data: string, address: string): Promise { + const privateKeyWallet = this._privateKeyWalletFromAddress(address); + const sig = await privateKeyWallet.signPersonalMessageAsync(data, address); return sig; } + private _privateKeyWalletFromAddress(address: string): PrivateKeyWalletSubprovider { + const derivedKey = this._findDerivedKeyByPublicAddress(address); + const privateKeyHex = derivedKey.hdKey.privateKey.toString('hex'); + const privateKeyWallet = new PrivateKeyWalletSubprovider(privateKeyHex); + return privateKeyWallet; + } private _findDerivedKeyByPublicAddress(address: string): DerivedHDKey { + if (_.isUndefined(address)) { + throw new Error(WalletSubproviderErrors.FromAddressMissingOrInvalid); + } const matchedDerivedKey = walletUtils.findDerivedKeyByAddress( address, this._derivedKey, diff --git a/packages/subproviders/src/subproviders/private_key_wallet_subprovider.ts b/packages/subproviders/src/subproviders/private_key_wallet_subprovider.ts index 005d36f93..f8afab722 100644 --- a/packages/subproviders/src/subproviders/private_key_wallet_subprovider.ts +++ b/packages/subproviders/src/subproviders/private_key_wallet_subprovider.ts @@ -63,15 +63,15 @@ export class PrivateKeyWalletSubprovider extends BaseWalletSubprovider { * @param address Address to sign with * @return Signature hex string (order: rsv) */ - public async signPersonalMessageAsync(dataIfExists: string, address?: string): Promise { - if (_.isUndefined(dataIfExists)) { + public async signPersonalMessageAsync(data: string, address: string): Promise { + if (_.isUndefined(data)) { throw new Error(WalletSubproviderErrors.DataMissingForSignPersonalMessage); } - if (!_.isUndefined(address) && address !== this._address) { - throw new Error(`${WalletSubproviderErrors.AddressNotFound}: ${address}`); + if (_.isUndefined(address) || address !== this._address) { + throw new Error(`${WalletSubproviderErrors.FromAddressMissingOrInvalid}: ${address}`); } - assert.isHexString('data', dataIfExists); - const dataBuff = ethUtil.toBuffer(dataIfExists); + assert.isHexString('data', data); + const dataBuff = ethUtil.toBuffer(data); const msgHashBuff = ethUtil.hashPersonalMessage(dataBuff); const sig = ethUtil.ecsign(msgHashBuff, this._privateKeyBuffer); const rpcSig = ethUtil.toRpcSig(sig.v, sig.r, sig.s); diff --git a/packages/subproviders/src/types.ts b/packages/subproviders/src/types.ts index 9f557731f..140c7c2df 100644 --- a/packages/subproviders/src/types.ts +++ b/packages/subproviders/src/types.ts @@ -80,7 +80,7 @@ export interface PartialTxParams { gasPrice?: string; gas: string; to: string; - from?: string; + from: string; value?: string; data?: string; chainId: number; // EIP 155 chainId - mainnet: 1, ropsten: 3 @@ -101,10 +101,10 @@ export enum WalletSubproviderErrors { AddressNotFound = 'ADDRESS_NOT_FOUND', DataMissingForSignPersonalMessage = 'DATA_MISSING_FOR_SIGN_PERSONAL_MESSAGE', SenderInvalidOrNotSupplied = 'SENDER_INVALID_OR_NOT_SUPPLIED', + FromAddressMissingOrInvalid = 'FROM_ADDRESS_MISSING_OR_INVALID', } export enum LedgerSubproviderErrors { TooOldLedgerFirmware = 'TOO_OLD_LEDGER_FIRMWARE', - FromAddressMissingOrInvalid = 'FROM_ADDRESS_MISSING_OR_INVALID', MultipleOpenConnectionsDisallowed = 'MULTIPLE_OPEN_CONNECTIONS_DISALLOWED', } diff --git a/packages/subproviders/test/integration/ledger_subprovider_test.ts b/packages/subproviders/test/integration/ledger_subprovider_test.ts index 88a3c6db1..4a831af67 100644 --- a/packages/subproviders/test/integration/ledger_subprovider_test.ts +++ b/packages/subproviders/test/integration/ledger_subprovider_test.ts @@ -55,7 +55,10 @@ describe('LedgerSubprovider', () => { }); it('signs a personal message', async () => { const data = ethUtils.bufferToHex(ethUtils.toBuffer(fixtureData.PERSONAL_MESSAGE_STRING)); - const ecSignatureHex = await ledgerSubprovider.signPersonalMessageAsync(data); + const ecSignatureHex = await ledgerSubprovider.signPersonalMessageAsync( + data, + fixtureData.TEST_RPC_ACCOUNT_0, + ); expect(ecSignatureHex.length).to.be.equal(132); expect(ecSignatureHex).to.be.equal(fixtureData.PERSONAL_MESSAGE_SIGNED_RESULT); }); diff --git a/packages/subproviders/test/unit/ledger_subprovider_test.ts b/packages/subproviders/test/unit/ledger_subprovider_test.ts index d3a3fbe26..ad1154831 100644 --- a/packages/subproviders/test/unit/ledger_subprovider_test.ts +++ b/packages/subproviders/test/unit/ledger_subprovider_test.ts @@ -82,7 +82,7 @@ describe('LedgerSubprovider', () => { }); it('signs a personal message', async () => { const data = ethUtils.bufferToHex(ethUtils.toBuffer(fixtureData.PERSONAL_MESSAGE_STRING)); - const ecSignatureHex = await ledgerSubprovider.signPersonalMessageAsync(data); + const ecSignatureHex = await ledgerSubprovider.signPersonalMessageAsync(data, FAKE_ADDRESS); expect(ecSignatureHex).to.be.equal( '0xa6cc284bff14b42bdf5e9286730c152be91719d478605ec46b3bebcd0ae491480652a1a7b742ceb0213d1e744316e285f41f878d8af0b8e632cbca4c279132d001', ); @@ -94,7 +94,7 @@ describe('LedgerSubprovider', () => { return expect( Promise.all([ ledgerSubprovider.getAccountsAsync(), - ledgerSubprovider.signPersonalMessageAsync(data), + ledgerSubprovider.signPersonalMessageAsync(data, FAKE_ADDRESS), ]), ).to.be.rejectedWith(LedgerSubproviderErrors.MultipleOpenConnectionsDisallowed); }); @@ -168,6 +168,7 @@ describe('LedgerSubprovider', () => { gasPrice: '0x00', nonce: '0x00', gas: '0x00', + from: FAKE_ADDRESS, }; const payload = { jsonrpc: '2.0', diff --git a/packages/subproviders/test/unit/mnemonic_wallet_subprovider_test.ts b/packages/subproviders/test/unit/mnemonic_wallet_subprovider_test.ts index 77e5d35ae..a1a5a2296 100644 --- a/packages/subproviders/test/unit/mnemonic_wallet_subprovider_test.ts +++ b/packages/subproviders/test/unit/mnemonic_wallet_subprovider_test.ts @@ -36,7 +36,7 @@ describe('MnemonicWalletSubprovider', () => { }); it('signs a personal message', async () => { const data = ethUtils.bufferToHex(ethUtils.toBuffer(fixtureData.PERSONAL_MESSAGE_STRING)); - const ecSignatureHex = await subprovider.signPersonalMessageAsync(data); + const ecSignatureHex = await subprovider.signPersonalMessageAsync(data, fixtureData.TEST_RPC_ACCOUNT_0); expect(ecSignatureHex).to.be.equal(fixtureData.PERSONAL_MESSAGE_SIGNED_RESULT); }); it('signs a transaction', async () => { diff --git a/packages/subproviders/test/unit/private_key_wallet_subprovider_test.ts b/packages/subproviders/test/unit/private_key_wallet_subprovider_test.ts index 8aaddeaf4..b84aebb18 100644 --- a/packages/subproviders/test/unit/private_key_wallet_subprovider_test.ts +++ b/packages/subproviders/test/unit/private_key_wallet_subprovider_test.ts @@ -32,7 +32,7 @@ describe('PrivateKeyWalletSubprovider', () => { }); it('signs a personal message', async () => { const data = ethUtils.bufferToHex(ethUtils.toBuffer(fixtureData.PERSONAL_MESSAGE_STRING)); - const ecSignatureHex = await subprovider.signPersonalMessageAsync(data); + const ecSignatureHex = await subprovider.signPersonalMessageAsync(data, fixtureData.TEST_RPC_ACCOUNT_0); expect(ecSignatureHex).to.be.equal(fixtureData.PERSONAL_MESSAGE_SIGNED_RESULT); }); it('signs a transaction', async () => { @@ -128,6 +128,23 @@ describe('PrivateKeyWalletSubprovider', () => { }); provider.sendAsync(payload, callback); }); + it('should throw if `address` param is not an address from private key when calling personal_sign', (done: DoneCallback) => { + const nonHexMessage = 'hello world'; + const payload = { + jsonrpc: '2.0', + method: 'personal_sign', + params: [nonHexMessage, fixtureData.TEST_RPC_ACCOUNT_1], + id: 1, + }; + const callback = reportCallbackErrors(done)((err: Error, response: JSONRPCResponsePayload) => { + expect(err).to.not.be.a('null'); + expect(err.message).to.be.equal( + `${WalletSubproviderErrors.FromAddressMissingOrInvalid}: ${fixtureData.TEST_RPC_ACCOUNT_1}`, + ); + done(); + }); + provider.sendAsync(payload, callback); + }); it('should throw if `from` param missing when calling eth_sendTransaction', (done: DoneCallback) => { const tx = { to: '0xafa3f8684e54059998bc3a7b0d2b0da075154d66', @@ -175,7 +192,7 @@ describe('PrivateKeyWalletSubprovider', () => { }; const callback = reportCallbackErrors(done)((err: Error, response: JSONRPCResponsePayload) => { expect(err).to.not.be.a('null'); - expect(err.message).to.be.equal(`${WalletSubproviderErrors.AddressNotFound}: 0x0`); + expect(err.message).to.be.equal(`${WalletSubproviderErrors.FromAddressMissingOrInvalid}: 0x0`); done(); }); provider.sendAsync(payload, callback); -- cgit v1.2.3