From 712a1ba36ee9f60e56b36533f10e7ad4ce4998e8 Mon Sep 17 00:00:00 2001 From: Fabio Berger Date: Fri, 7 Jul 2017 13:49:02 -0700 Subject: Modify signOrderHashAsync to parse the signatureHex string as V + R + S AND R + S + V and check both for a valid signature in order to fix the issue of different nodes returning it differently --- src/0x.ts | 70 ++++++++++++++++++++++++++++++++---------------------- test/0x.js_test.ts | 12 +++------- 2 files changed, 44 insertions(+), 38 deletions(-) diff --git a/src/0x.ts b/src/0x.ts index 95935c258..837855426 100644 --- a/src/0x.ts +++ b/src/0x.ts @@ -221,39 +221,27 @@ export class ZeroEx { const signature = await this._web3Wrapper.signTransactionAsync(signerAddress, msgHashHex); - let signatureData; - const [nodeVersionNumber] = findVersions(nodeVersion); - // Parity v1.6.6 and earlier returns the signatureData as vrs instead of rsv as Geth does - // Later versions return rsv but for the time being we still want to support version < 1.6.6 - // Date: May 23rd 2017 - const latestParityVersionWithVRS = '1.6.6'; - const isVersionBeforeParityFix = compareVersions(nodeVersionNumber, latestParityVersionWithVRS) <= 0; - if (isParityNode && isVersionBeforeParityFix) { - const signatureBuffer = ethUtil.toBuffer(signature); - let v = signatureBuffer[0]; - if (v < 27) { - v += 27; + // HACK: There is no consensus on whether the signatureHex string should be formatted as + // v + r + s OR r + s + v, and different clients (even different versions of the same client) + // return the signature params in different orders. In order to support all client implementations, + // we parse the signature in both ways, and evaluate if either one is a valid signature. + const ecSignatureVRS = this.parseSignatureHexAsVRS(signature); + if (ecSignatureVRS.v === 27 || ecSignatureVRS.v === 28) { + const isValidVRSSignature = ZeroEx.isValidSignature(orderHash, ecSignatureVRS, signerAddress); + if (isValidVRSSignature) { + return ecSignatureVRS; } - signatureData = { - v, - r: signatureBuffer.slice(1, 33), - s: signatureBuffer.slice(33, 65), - }; - } else { - signatureData = ethUtil.fromRpcSig(signature); } - const {v, r, s} = signatureData; - const ecSignature: ECSignature = { - v, - r: ethUtil.bufferToHex(r), - s: ethUtil.bufferToHex(s), - }; - const isValidSignature = ZeroEx.isValidSignature(orderHash, ecSignature, signerAddress); - if (!isValidSignature) { - throw new Error(ZeroExError.INVALID_SIGNATURE); + const ecSignatureRSV = this.parseSignatureHexAsRSV(signature); + if (ecSignatureRSV.v === 27 || ecSignatureRSV.v === 28) { + const isValidRSVSignature = ZeroEx.isValidSignature(orderHash, ecSignatureRSV, signerAddress); + if (isValidRSVSignature) { + return ecSignatureRSV; + } } - return ecSignature; + + throw new Error(ZeroExError.INVALID_SIGNATURE); } /** * Returns the ethereum addresses of all available exchange contracts @@ -293,4 +281,28 @@ export class ZeroEx { } return proxyAuthorizedExchangeContractAddresses; } + private parseSignatureHexAsVRS(signatureHex: string): ECSignature { + const signatureBuffer = ethUtil.toBuffer(signatureHex); + let v = signatureBuffer[0]; + if (v < 27) { + v += 27; + } + const r = signatureBuffer.slice(1, 33); + const s = signatureBuffer.slice(33, 65); + const ecSignature: ECSignature = { + v, + r: ethUtil.bufferToHex(r), + s: ethUtil.bufferToHex(s), + }; + return ecSignature; + } + private parseSignatureHexAsRSV(signatureHex: string): ECSignature { + const {v, r, s} = ethUtil.fromRpcSig(signatureHex); + const ecSignature: ECSignature = { + v, + r: ethUtil.bufferToHex(r), + s: ethUtil.bufferToHex(s), + }; + return ecSignature; + } } diff --git a/test/0x.js_test.ts b/test/0x.js_test.ts index f25f104bd..0c82c803d 100644 --- a/test/0x.js_test.ts +++ b/test/0x.js_test.ts @@ -163,7 +163,7 @@ describe('ZeroEx library', () => { _.each(stubs, s => s.restore()); stubs = []; }); - it('Should return the correct ECSignature on TestPRC nodeVersion', async () => { + it('Should return the correct ECSignature', async () => { const orderHash = '0x6927e990021d23b1eb7b8789f6a6feaf98fe104bb0cf8259421b79f9a34222b0'; const expectedECSignature = { v: 27, @@ -173,8 +173,7 @@ describe('ZeroEx library', () => { const ecSignature = await zeroEx.signOrderHashAsync(orderHash, makerAddress); expect(ecSignature).to.deep.equal(expectedECSignature); }); - it('should return the correct ECSignature on Parity > V1.6.6', async () => { - const newParityNodeVersion = 'Parity//v1.6.7-beta-e128418-20170518/x86_64-macos/rustc1.17.0'; + it('should return the correct ECSignature for signatureHex concatenated as R + S + V', async () => { const orderHash = '0x34decbedc118904df65f379a175bb39ca18209d6ce41d5ed549d54e6e0a95004'; // tslint:disable-next-line: max-line-length const signature = '0x22109d11d79cb8bf96ed88625e1cd9558800c4073332a9a02857499883ee5ce3050aa3cc1f2c435e67e114cdce54b9527b4f50548342401bc5d2b77adbdacb021b'; @@ -184,8 +183,6 @@ describe('ZeroEx library', () => { s: '0x050aa3cc1f2c435e67e114cdce54b9527b4f50548342401bc5d2b77adbdacb02', }; stubs = [ - Sinon.stub((zeroEx as any)._web3Wrapper, 'getNodeVersionAsync') - .returns(Promise.resolve(newParityNodeVersion)), Sinon.stub((zeroEx as any)._web3Wrapper, 'signTransactionAsync') .returns(Promise.resolve(signature)), Sinon.stub(ZeroEx, 'isValidSignature').returns(true), @@ -194,8 +191,7 @@ describe('ZeroEx library', () => { const ecSignature = await zeroEx.signOrderHashAsync(orderHash, makerAddress); expect(ecSignature).to.deep.equal(expectedECSignature); }); - it('should return the correct ECSignature on Parity < V1.6.6', async () => { - const newParityNodeVersion = 'Parity//v1.6.6-beta-8c6e3f3-20170411/x86_64-macos/rustc1.16.0'; + it('should return the correct ECSignature for signatureHex concatenated as V + R + S', async () => { const orderHash = '0xc793e33ffded933b76f2f48d9aa3339fc090399d5e7f5dec8d3660f5480793f7'; // tslint:disable-next-line: max-line-length const signature = '0x1bc80bedc6756722672753413efdd749b5adbd4fd552595f59c13427407ee9aee02dea66f25a608bbae457e020fb6decb763deb8b7192abab624997242da248960'; @@ -205,8 +201,6 @@ describe('ZeroEx library', () => { s: '0x2dea66f25a608bbae457e020fb6decb763deb8b7192abab624997242da248960', }; stubs = [ - Sinon.stub((zeroEx as any)._web3Wrapper, 'getNodeVersionAsync') - .returns(Promise.resolve(newParityNodeVersion)), Sinon.stub((zeroEx as any)._web3Wrapper, 'signTransactionAsync') .returns(Promise.resolve(signature)), Sinon.stub(ZeroEx, 'isValidSignature').returns(true), -- cgit From bdfbfb829b66b57ecb26a053a2b23665c9fd1549 Mon Sep 17 00:00:00 2001 From: Fabio Berger Date: Fri, 7 Jul 2017 14:20:59 -0700 Subject: Remove duplication of 27, 28 v values --- src/0x.ts | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/0x.ts b/src/0x.ts index 837855426..3ff7c830b 100644 --- a/src/0x.ts +++ b/src/0x.ts @@ -225,16 +225,17 @@ export class ZeroEx { // v + r + s OR r + s + v, and different clients (even different versions of the same client) // return the signature params in different orders. In order to support all client implementations, // we parse the signature in both ways, and evaluate if either one is a valid signature. - const ecSignatureVRS = this.parseSignatureHexAsVRS(signature); - if (ecSignatureVRS.v === 27 || ecSignatureVRS.v === 28) { + const validVParamValues = [27, 28]; + const ecSignatureVRS = signatureUtils.parseSignatureHexAsVRS(signature); + if (_.includes(validVParamValues, ecSignatureVRS.v)) { const isValidVRSSignature = ZeroEx.isValidSignature(orderHash, ecSignatureVRS, signerAddress); if (isValidVRSSignature) { return ecSignatureVRS; } } - const ecSignatureRSV = this.parseSignatureHexAsRSV(signature); - if (ecSignatureRSV.v === 27 || ecSignatureRSV.v === 28) { + const ecSignatureRSV = signatureUtils.parseSignatureHexAsRSV(signature); + if (_.includes(validVParamValues, ecSignatureRSV.v)) { const isValidRSVSignature = ZeroEx.isValidSignature(orderHash, ecSignatureRSV, signerAddress); if (isValidRSVSignature) { return ecSignatureRSV; -- cgit From 68120ad1da1ee72ee11e1286698abc699c80e2cf Mon Sep 17 00:00:00 2001 From: Fabio Berger Date: Fri, 7 Jul 2017 14:21:47 -0700 Subject: Move private helper methods into signatureUtils so that they don't show up in the ZeroEx classes auto-complete list --- src/0x.ts | 25 +------------------------ src/utils/signature_utils.ts | 29 +++++++++++++++++++++++++++++ 2 files changed, 30 insertions(+), 24 deletions(-) create mode 100644 src/utils/signature_utils.ts diff --git a/src/0x.ts b/src/0x.ts index 3ff7c830b..92a892336 100644 --- a/src/0x.ts +++ b/src/0x.ts @@ -9,6 +9,7 @@ import compareVersions = require('compare-versions'); import {Web3Wrapper} from './web3_wrapper'; import {constants} from './utils/constants'; import {utils} from './utils/utils'; +import {signatureUtils} from './utils/signature_utils'; import {assert} from './utils/assert'; import {ExchangeWrapper} from './contract_wrappers/exchange_wrapper'; import {TokenRegistryWrapper} from './contract_wrappers/token_registry_wrapper'; @@ -282,28 +283,4 @@ export class ZeroEx { } return proxyAuthorizedExchangeContractAddresses; } - private parseSignatureHexAsVRS(signatureHex: string): ECSignature { - const signatureBuffer = ethUtil.toBuffer(signatureHex); - let v = signatureBuffer[0]; - if (v < 27) { - v += 27; - } - const r = signatureBuffer.slice(1, 33); - const s = signatureBuffer.slice(33, 65); - const ecSignature: ECSignature = { - v, - r: ethUtil.bufferToHex(r), - s: ethUtil.bufferToHex(s), - }; - return ecSignature; - } - private parseSignatureHexAsRSV(signatureHex: string): ECSignature { - const {v, r, s} = ethUtil.fromRpcSig(signatureHex); - const ecSignature: ECSignature = { - v, - r: ethUtil.bufferToHex(r), - s: ethUtil.bufferToHex(s), - }; - return ecSignature; - } } diff --git a/src/utils/signature_utils.ts b/src/utils/signature_utils.ts new file mode 100644 index 000000000..b312b5554 --- /dev/null +++ b/src/utils/signature_utils.ts @@ -0,0 +1,29 @@ +import * as ethUtil from 'ethereumjs-util'; +import {ECSignature} from '../types'; + +export const signatureUtils = { + parseSignatureHexAsVRS(signatureHex: string): ECSignature { + const signatureBuffer = ethUtil.toBuffer(signatureHex); + let v = signatureBuffer[0]; + if (v < 27) { + v += 27; + } + const r = signatureBuffer.slice(1, 33); + const s = signatureBuffer.slice(33, 65); + const ecSignature: ECSignature = { + v, + r: ethUtil.bufferToHex(r), + s: ethUtil.bufferToHex(s), + }; + return ecSignature; + }, + parseSignatureHexAsRSV(signatureHex: string): ECSignature { + const {v, r, s} = ethUtil.fromRpcSig(signatureHex); + const ecSignature: ECSignature = { + v, + r: ethUtil.bufferToHex(r), + s: ethUtil.bufferToHex(s), + }; + return ecSignature; + }, +}; -- cgit