From 32833b7301ede19b3b80d95df32c0565efdd583a Mon Sep 17 00:00:00 2001 From: Fabio Berger Date: Wed, 30 May 2018 14:08:43 -0700 Subject: Fix order-utils tests --- packages/order-utils/src/index.ts | 4 +- packages/order-utils/src/order_factory.ts | 31 ++++++--- packages/order-utils/src/order_hash.ts | 4 +- packages/order-utils/src/signature_utils.ts | 82 +++++++++++++---------- packages/order-utils/src/types.ts | 22 ++++++ packages/order-utils/test/order_hash_test.ts | 2 +- packages/order-utils/test/signature_utils_test.ts | 59 ++++++++-------- 7 files changed, 123 insertions(+), 81 deletions(-) (limited to 'packages') diff --git a/packages/order-utils/src/index.ts b/packages/order-utils/src/index.ts index 5b4e20119..b844fbfcb 100644 --- a/packages/order-utils/src/index.ts +++ b/packages/order-utils/src/index.ts @@ -1,10 +1,10 @@ export { orderHashUtils } from './order_hash'; -export { isValidSignatureAsync, ecSignOrderHashAsync } from './signature_utils'; +export { isValidSignatureAsync, ecSignOrderHashAsync, addSignedMessagePrefix } from './signature_utils'; export { orderFactory } from './order_factory'; export { constants } from './constants'; export { crypto } from './crypto'; export { generatePseudoRandomSalt } from './salt'; -export { OrderError } from './types'; +export { OrderError, MessagePrefixType, MessagePrefixOpts } from './types'; export { AbstractBalanceAndProxyAllowanceFetcher } from './abstract/abstract_balance_and_proxy_allowance_fetcher'; export { AbstractOrderFilledCancelledFetcher } from './abstract/abstract_order_filled_cancelled_fetcher'; export { RemainingFillableCalculator } from './remaining_fillable_calculator'; diff --git a/packages/order-utils/src/order_factory.ts b/packages/order-utils/src/order_factory.ts index fe341b845..3f3dc524c 100644 --- a/packages/order-utils/src/order_factory.ts +++ b/packages/order-utils/src/order_factory.ts @@ -1,12 +1,12 @@ -import { Provider, SignedOrder } from '@0xproject/types'; +import { ECSignature, Provider, SignedOrder } from '@0xproject/types'; import { BigNumber } from '@0xproject/utils'; +import * as ethUtil from 'ethereumjs-util'; import * as _ from 'lodash'; import { orderHashUtils } from './order_hash'; import { generatePseudoRandomSalt } from './salt'; -import { ecSignOrderHashAsync, getVRSHexString } from './signature_utils'; - -const SHOULD_ADD_PERSONAL_MESSAGE_PREFIX = false; +import { ecSignOrderHashAsync } from './signature_utils'; +import { MessagePrefixType } from './types'; export const orderFactory = { async createSignedOrderAsync( @@ -44,14 +44,25 @@ export const orderFactory = { expirationTimeSeconds, }; const orderHash = orderHashUtils.getOrderHashHex(order); - const ecSignature = await ecSignOrderHashAsync( - provider, - orderHash, - makerAddress, - SHOULD_ADD_PERSONAL_MESSAGE_PREFIX, - ); + const messagePrefixOpts = { + prefixType: MessagePrefixType.EthSign, + shouldAddPrefixBeforeCallingEthSign: false, + }; + const ecSignature = await ecSignOrderHashAsync(provider, orderHash, makerAddress, messagePrefixOpts); const signature = getVRSHexString(ecSignature); const signedOrder: SignedOrder = _.assign(order, { signature }); return signedOrder; }, }; + +function getVRSHexString(ecSignature: ECSignature): string { + const vrs = `0x${intToHex(ecSignature.v)}${ethUtil.stripHexPrefix(ecSignature.r)}${ethUtil.stripHexPrefix( + ecSignature.s, + )}`; + return vrs; +} + +function intToHex(i: number): string { + const hex = ethUtil.bufferToHex(ethUtil.toBuffer(i)); + return hex; +} diff --git a/packages/order-utils/src/order_hash.ts b/packages/order-utils/src/order_hash.ts index 3d0db5b0c..a4e36ab89 100644 --- a/packages/order-utils/src/order_hash.ts +++ b/packages/order-utils/src/order_hash.ts @@ -9,7 +9,7 @@ import * as _ from 'lodash'; import { assert } from './assert'; import { crypto } from './crypto'; -const INVALID_TAKER_FORMAT = 'instance.taker is not of a type(s) string'; +const INVALID_TAKER_FORMAT = 'instance.takerAddress is not of a type(s) string'; export const orderHashUtils = { /** @@ -34,7 +34,7 @@ export const orderHashUtils = { */ getOrderHashHex(order: SignedOrder | Order): string { try { - assert.doesConformToSchema('order', order, schemas.orderSchema); + assert.doesConformToSchema('order', order, schemas.orderSchema, [schemas.hexSchema]); } catch (error) { if (_.includes(error.message, INVALID_TAKER_FORMAT)) { const errMsg = diff --git a/packages/order-utils/src/signature_utils.ts b/packages/order-utils/src/signature_utils.ts index 106bbf4e8..8cd4264ab 100644 --- a/packages/order-utils/src/signature_utils.ts +++ b/packages/order-utils/src/signature_utils.ts @@ -8,12 +8,13 @@ import { artifacts } from './artifacts'; import { assert } from './assert'; import { ExchangeContract } from './generated_contract_wrappers/exchange'; import { ISignerContract } from './generated_contract_wrappers/i_signer'; -import { OrderError } from './types'; +import { MessagePrefixOpts, MessagePrefixType, OrderError } from './types'; /** * Verifies that the provided signature is valid according to the 0x Protocol smart contracts * @param data The hex encoded data signed by the supplied signature. - * @param signature An object containing the elliptic curve signature parameters. + * @param signature A hex encoded 0x Protocol signature made up of: [SignatureType][TypeSpecificData]. + * E.g [SignatureType.EIP712][vrs] * @param signerAddress The hex encoded address that signed the data, producing the supplied signature. * @return Whether the signature is valid for the supplied signerAddress and data. */ @@ -41,10 +42,8 @@ export async function isValidSignatureAsync( // types use ECRecover... case SignatureType.Ecrecover: { const ecSignature = parseECSignature(signature); - const dataBuff = ethUtil.toBuffer(data); - const msgHashBuff = ethUtil.hashPersonalMessage(dataBuff); - const msgHash = ethUtil.bufferToHex(msgHashBuff); - return isValidECSignature(msgHash, ecSignature, signerAddress); + const prefixedMessageHex = addSignedMessagePrefix(data, MessagePrefixType.EthSign); + return isValidECSignature(prefixedMessageHex, ecSignature, signerAddress); } case SignatureType.EIP712: { @@ -53,11 +52,9 @@ export async function isValidSignatureAsync( } case SignatureType.Trezor: { - const dataBuff = ethUtil.toBuffer(data); - const msgHashBuff = hashTrezorPersonalMessage(dataBuff); - const msgHash = ethUtil.bufferToHex(msgHashBuff); + const prefixedMessageHex = addSignedMessagePrefix(data, MessagePrefixType.Trezor); const ecSignature = parseECSignature(signature); - return isValidECSignature(msgHash, ecSignature, signerAddress); + return isValidECSignature(prefixedMessageHex, ecSignature, signerAddress); } // TODO: Rename Contract -> Wallet @@ -78,13 +75,14 @@ export async function isValidSignatureAsync( } } -export function getVRSHexString(ecSignature: ECSignature): string { - const vrs = `0x${intToHex(ecSignature.v)}${ethUtil.stripHexPrefix(ecSignature.r)}${ethUtil.stripHexPrefix( - ecSignature.s, - )}`; - return vrs; -} - +/** + * Checks if the supplied elliptic curve signature corresponds to signing `data` with + * the private key corresponding to `signerAddress` + * @param data The hex encoded data signed by the supplied signature. + * @param signature An object containing the elliptic curve signature parameters. + * @param signerAddress The hex encoded address that signed the data, producing the supplied signature. + * @return Whether the ECSignature is valid. + */ export function isValidECSignature(data: string, signature: ECSignature, signerAddress: string): boolean { assert.isHexString('data', data); assert.doesConformToSchema('signature', signature, schemas.ecSignatureSchema); @@ -112,17 +110,16 @@ export function isValidECSignature(data: string, signature: ECSignature, signerA * @param orderHash Hex encoded orderHash to sign. * @param signerAddress The hex encoded Ethereum address you wish to sign it with. This address * must be available via the Provider supplied to 0x.js. - * @param shouldAddPersonalMessagePrefix Some signers add the personal message prefix `\x19Ethereum Signed Message` - * themselves (e.g Parity Signer, Ledger, TestRPC) and others expect it to already be done by the client - * (e.g Metamask). Depending on which signer this request is going to, decide on whether to add the prefix - * before sending the request. + * @param hashPrefixOpts Different signers add/require different prefixes be appended to the message being signed. + * Since we cannot know ahead of time which signer you are using, you must supply both a prefixType and + * whether it must be added before calling `eth_sign` (some signers add it themselves) * @return An object containing the Elliptic curve signature parameters generated by signing the orderHash. */ export async function ecSignOrderHashAsync( provider: Provider, orderHash: string, signerAddress: string, - shouldAddPersonalMessagePrefix: boolean, + messagePrefixOpts: MessagePrefixOpts, ): Promise { assert.isHexString('orderHash', orderHash); const web3Wrapper = new Web3Wrapper(provider); @@ -130,12 +127,10 @@ export async function ecSignOrderHashAsync( const normalizedSignerAddress = signerAddress.toLowerCase(); let msgHashHex = orderHash; - if (shouldAddPersonalMessagePrefix) { - const orderHashBuff = ethUtil.toBuffer(orderHash); - const msgHashBuff = ethUtil.hashPersonalMessage(orderHashBuff); - msgHashHex = ethUtil.bufferToHex(msgHashBuff); + const prefixedMsgHashHex = addSignedMessagePrefix(orderHash, messagePrefixOpts.prefixType); + if (messagePrefixOpts.shouldAddPrefixBeforeCallingEthSign) { + msgHashHex = prefixedMsgHashHex; } - const signature = await web3Wrapper.signMessageAsync(normalizedSignerAddress, msgHashHex); // HACK: There is no consensus on whether the signatureHex string should be formatted as @@ -146,7 +141,7 @@ export async function ecSignOrderHashAsync( const validVParamValues = [27, 28]; const ecSignatureVRS = parseSignatureHexAsVRS(signature); if (_.includes(validVParamValues, ecSignatureVRS.v)) { - const isValidVRSSignature = isValidECSignature(orderHash, ecSignatureVRS, normalizedSignerAddress); + const isValidVRSSignature = isValidECSignature(prefixedMsgHashHex, ecSignatureVRS, normalizedSignerAddress); if (isValidVRSSignature) { return ecSignatureVRS; } @@ -154,7 +149,7 @@ export async function ecSignOrderHashAsync( const ecSignatureRSV = parseSignatureHexAsRSV(signature); if (_.includes(validVParamValues, ecSignatureRSV.v)) { - const isValidRSVSignature = isValidECSignature(orderHash, ecSignatureRSV, normalizedSignerAddress); + const isValidRSVSignature = isValidECSignature(prefixedMsgHashHex, ecSignatureRSV, normalizedSignerAddress); if (isValidRSVSignature) { return ecSignatureRSV; } @@ -163,6 +158,30 @@ export async function ecSignOrderHashAsync( throw new Error(OrderError.InvalidSignature); } +export function addSignedMessagePrefix(message: string, messagePrefixType: MessagePrefixType): string { + switch (messagePrefixType) { + case MessagePrefixType.None: + return message; + + case MessagePrefixType.EthSign: { + const msgBuff = ethUtil.toBuffer(message); + const prefixedMsgBuff = ethUtil.hashPersonalMessage(msgBuff); + const prefixedMsgHex = ethUtil.bufferToHex(prefixedMsgBuff); + return prefixedMsgHex; + } + + case MessagePrefixType.Trezor: { + const msgBuff = ethUtil.toBuffer(message); + const prefixedMsgBuff = hashTrezorPersonalMessage(msgBuff); + const prefixedMsgHex = ethUtil.bufferToHex(prefixedMsgBuff); + return prefixedMsgHex; + } + + default: + throw new Error(`Unrecognized MessagePrefixType: ${messagePrefixType}`); + } +} + function hashTrezorPersonalMessage(message: Buffer): Buffer { const prefix = ethUtil.toBuffer('\x19Ethereum Signed Message:\n' + String.fromCharCode(message.length)); return ethUtil.sha3(Buffer.concat([prefix, message])); @@ -183,11 +202,6 @@ function parseECSignature(signature: string): ECSignature { return ecSignature; } -function intToHex(i: number): string { - const hex = ethUtil.bufferToHex(ethUtil.toBuffer(i)); - return hex; -} - function getSignatureTypeIndexIfExists(signature: string): number { const unprefixedSignature = ethUtil.stripHexPrefix(signature); const signatureTypeHex = unprefixedSignature.substr(0, 2); diff --git a/packages/order-utils/src/types.ts b/packages/order-utils/src/types.ts index f79d52359..db0bfb249 100644 --- a/packages/order-utils/src/types.ts +++ b/packages/order-utils/src/types.ts @@ -1,3 +1,25 @@ export enum OrderError { InvalidSignature = 'INVALID_SIGNATURE', } + +/** + * The requisite message prefix (is any) to add to an `eth_sign` request. + */ +export enum MessagePrefixType { + None = 'NONE', + EthSign = 'ETH_SIGN', + Trezor = 'TREZOR', +} + +/** + * Options related to message prefixing of messages sent to `eth_sign` + * Some signers prepend a message prefix (e.g Parity Signer, Ledger, TestRPC), while + * others require it already be prepended (e.g Metamask). In addition, different signers + * expect slightly different prefixes (See: https://github.com/ethereum/go-ethereum/issues/14794). + * Depending on the signer that will receive your signing request, you must specify the + * desired prefix and whether it should be added before making the `eth_sign` request. + */ +export interface MessagePrefixOpts { + prefixType: MessagePrefixType; + shouldAddPrefixBeforeCallingEthSign: boolean; +} diff --git a/packages/order-utils/test/order_hash_test.ts b/packages/order-utils/test/order_hash_test.ts index 7cb87c8ab..21ae22589 100644 --- a/packages/order-utils/test/order_hash_test.ts +++ b/packages/order-utils/test/order_hash_test.ts @@ -39,7 +39,7 @@ describe('Order hashing', () => { it('throws a readable error message if taker format is invalid', async () => { const orderWithInvalidtakerFormat = { ...order, - taker: (null as any) as string, + takerAddress: (null as any) as string, }; const expectedErrorMessage = 'Order taker must be of type string. If you want anyone to be able to fill an order - pass ZeroEx.NULL_ADDRESS'; diff --git a/packages/order-utils/test/signature_utils_test.ts b/packages/order-utils/test/signature_utils_test.ts index 4babd5582..773a32a56 100644 --- a/packages/order-utils/test/signature_utils_test.ts +++ b/packages/order-utils/test/signature_utils_test.ts @@ -7,7 +7,7 @@ import 'make-promises-safe'; import 'mocha'; import * as Sinon from 'sinon'; -import { ecSignOrderHashAsync, generatePseudoRandomSalt, orderHashUtils } from '../src'; +import { ecSignOrderHashAsync, generatePseudoRandomSalt, MessagePrefixType, orderHashUtils } from '../src'; import { isValidECSignature, isValidSignatureAsync } from '../src/signature_utils'; import { chaiSetup } from './utils/chai_setup'; @@ -16,8 +16,6 @@ import { provider, web3Wrapper } from './utils/web3_wrapper'; chaiSetup.configure(); const expect = chai.expect; -const SHOULD_ADD_PERSONAL_MESSAGE_PREFIX = false; - describe('Signature utils', () => { describe('#isValidSignature', () => { let dataHex = '0x6927e990021d23b1eb7b8789f6a6feaf98fe104bb0cf8259421b79f9a34222b0'; @@ -75,28 +73,28 @@ describe('Signature utils', () => { // TODO: remaining sigs }); describe('#isValidECSignature', () => { - // The Exchange smart contract `isValidECSignature` method only validates orderHashes and assumes - // the length of the data is exactly 32 bytes. Thus for these tests, we use data of this size. - const dataHex = '0x6927e990021d23b1eb7b8789f6a6feaf98fe104bb0cf8259421b79f9a34222b0'; + // TODO: Replace this with a vanilla signature without ANY prefix or modification const signature = { v: 27, - r: '0x61a3ed31b43c8780e905a260a35faefcc527be7516aa11c0256729b5b351bc33', - s: '0x40349190569279751135161d22529dc25add4f6069af05be04cacbda2ace2254', + r: '0xaca7da997ad177f040240cdccf6905b71ab16b74434388c3a72f34fd25d64393', + s: '0x46b2bac274ff29b48b3ea6e2d04c1336eaceafda3c53ab483fc3ff12fac3ebf2', }; - const address = '0x5409ed021d9299bf6814279a6a1411a7e866a631'; + const data = '0x47173285a8d7341e5e972fc677286384f802f8ef42a5ec5f03bbfa254cb01fad'; + const address = '0x0e5cb767cce09a7f3ca594df118aa519be5e2b5a'; + it("should return false if the data doesn't pertain to the signature & address", async () => { expect(isValidECSignature('0x0', signature, address)).to.be.false(); }); it("should return false if the address doesn't pertain to the signature & data", async () => { const validUnrelatedAddress = '0x8b0292b11a196601ed2ce54b665cafeca0347d42'; - expect(isValidECSignature(dataHex, signature, validUnrelatedAddress)).to.be.false(); + expect(isValidECSignature(data, signature, validUnrelatedAddress)).to.be.false(); }); - it("should return false if the signature doesn't pertain to the dataHex & address", async () => { + it("should return false if the signature doesn't pertain to the data & address", async () => { const wrongSignature = _.assign({}, signature, { v: 28 }); - expect(isValidECSignature(dataHex, wrongSignature, address)).to.be.false(); + expect(isValidECSignature(data, wrongSignature, address)).to.be.false(); }); - it('should return true if the signature does pertain to the dataHex & address', async () => { - const isValidSignatureLocal = isValidECSignature(dataHex, signature, address); + it('should return true if the signature does pertain to the data & address', async () => { + const isValidSignatureLocal = isValidECSignature(data, signature, address); expect(isValidSignatureLocal).to.be.true(); }); }); @@ -147,12 +145,11 @@ describe('Signature utils', () => { r: '0x61a3ed31b43c8780e905a260a35faefcc527be7516aa11c0256729b5b351bc33', s: '0x40349190569279751135161d22529dc25add4f6069af05be04cacbda2ace2254', }; - const ecSignature = await ecSignOrderHashAsync( - provider, - orderHash, - makerAddress, - SHOULD_ADD_PERSONAL_MESSAGE_PREFIX, - ); + const messagePrefixOpts = { + prefixType: MessagePrefixType.EthSign, + shouldAddPrefixBeforeCallingEthSign: false, + }; + const ecSignature = await ecSignOrderHashAsync(provider, orderHash, makerAddress, messagePrefixOpts); expect(ecSignature).to.deep.equal(expectedECSignature); }); it('should return the correct ECSignature for signatureHex concatenated as R + S + V', async () => { @@ -181,12 +178,11 @@ describe('Signature utils', () => { }, }; - const ecSignature = await ecSignOrderHashAsync( - fakeProvider, - orderHash, - makerAddress, - SHOULD_ADD_PERSONAL_MESSAGE_PREFIX, - ); + const messagePrefixOpts = { + prefixType: MessagePrefixType.EthSign, + shouldAddPrefixBeforeCallingEthSign: false, + }; + const ecSignature = await ecSignOrderHashAsync(fakeProvider, orderHash, makerAddress, messagePrefixOpts); expect(ecSignature).to.deep.equal(expectedECSignature); }); it('should return the correct ECSignature for signatureHex concatenated as V + R + S', async () => { @@ -212,12 +208,11 @@ describe('Signature utils', () => { }, }; - const ecSignature = await ecSignOrderHashAsync( - fakeProvider, - orderHash, - makerAddress, - SHOULD_ADD_PERSONAL_MESSAGE_PREFIX, - ); + const messagePrefixOpts = { + prefixType: MessagePrefixType.EthSign, + shouldAddPrefixBeforeCallingEthSign: false, + }; + const ecSignature = await ecSignOrderHashAsync(fakeProvider, orderHash, makerAddress, messagePrefixOpts); expect(ecSignature).to.deep.equal(expectedECSignature); }); }); -- cgit v1.2.3