From 5d316ced4dd6f4c4238d8c1506ed4cc8cb518fbf Mon Sep 17 00:00:00 2001 From: Dan Miller Date: Fri, 4 Jan 2019 12:43:23 -0330 Subject: [PATCH 1/3] Fix jsdoc param type of v for documentation of isValidSignature --- index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/index.js b/index.js index e4eb5203..c2688246 100644 --- a/index.js +++ b/index.js @@ -571,7 +571,7 @@ exports.addHexPrefix = function (str) { /** * Validate ECDSA signature * @method isValidSignature - * @param {Buffer} v + * @param {Number} v * @param {Buffer} r * @param {Buffer} s * @param {Boolean} [homestead=true] From 7fb0a30c2450120b3d0ffd1e103878143d1e7b6c Mon Sep 17 00:00:00 2001 From: Dan Miller Date: Fri, 4 Jan 2019 12:44:25 -0330 Subject: [PATCH 2/3] Fix validation of s size in isValidSignature so that it applies to the correct hardforks --- index.js | 2 +- test/index.js | 18 ++++++++++++++++++ 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/index.js b/index.js index c2688246..453e152e 100644 --- a/index.js +++ b/index.js @@ -598,7 +598,7 @@ exports.isValidSignature = function (v, r, s, homestead, chainId) { return false } - if ((homestead === false) && (new BN(s).cmp(SECP256K1_N_DIV_2) === 1)) { + if (homestead && (new BN(s).cmp(SECP256K1_N_DIV_2) === 1)) { return false } diff --git a/test/index.js b/test/index.js index fc5d1e91..6242c0c7 100644 --- a/test/index.js +++ b/test/index.js @@ -558,6 +558,24 @@ describe('isValidSignature', function () { const s = Buffer.from('129ff05af364204442bdb53ab6f18a99ab48acc9326fa689f228040429e3ca66', 'hex') assert.equal(ethUtils.isValidSignature(29, r, s), false) }) + it('should fail when on homestead and s > secp256k1n/2', function () { + const SECP256K1_N_DIV_2 = new BN('7fffffffffffffffffffffffffffffff5d576e7357a4501ddfe92f46681b20a0', 16) + + const r = Buffer.from('99e71a99cb2270b8cac5254f9e99b6210c6c10224a1579cf389ef88b20a1abe9', 'hex') + const s = Buffer.from(SECP256K1_N_DIV_2.add(new BN('1', 16)).toString(16), 'hex') + + const v = 27 + assert.equal(ethUtils.isValidSignature(v, r, s), true) + }) + it('should not fail when not on homestead but s > secp256k1n/2', function () { + const SECP256K1_N_DIV_2 = new BN('7fffffffffffffffffffffffffffffff5d576e7357a4501ddfe92f46681b20a0', 16) + + const r = Buffer.from('99e71a99cb2270b8cac5254f9e99b6210c6c10224a1579cf389ef88b20a1abe9', 'hex') + const s = Buffer.from(SECP256K1_N_DIV_2.add(new BN('1', 16)).toString(16), 'hex') + + const v = 27 + assert.equal(ethUtils.isValidSignature(v, r, s), true) + }) it('should work otherwise', function () { const r = Buffer.from('99e71a99cb2270b8cac5254f9e99b6210c6c10224a1579cf389ef88b20a1abe9', 'hex') const s = Buffer.from('129ff05af364204442bdb53ab6f18a99ab48acc9326fa689f228040429e3ca66', 'hex') From 777241f3d76cf5809eb19afa4e6985fe7cde9047 Mon Sep 17 00:00:00 2001 From: Dan Miller Date: Fri, 1 Feb 2019 14:42:23 -0330 Subject: [PATCH 3/3] Improve readibility and defaults of homestead or greater checks in isValidSignature --- index.js | 7 ++++--- test/index.js | 4 ++-- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/index.js b/index.js index 453e152e..9bd6a2a6 100644 --- a/index.js +++ b/index.js @@ -574,12 +574,13 @@ exports.addHexPrefix = function (str) { * @param {Number} v * @param {Buffer} r * @param {Buffer} s - * @param {Boolean} [homestead=true] + * @param {Boolean} [homesteadOrLater=true] Indicates whether this is being used on either the homestead hardfork or a later one * @param {Number} [chainId] * @return {Boolean} */ -exports.isValidSignature = function (v, r, s, homestead, chainId) { +exports.isValidSignature = function (v, r, s, homesteadOrLater, chainId) { + homesteadOrLater = homesteadOrLater === undefined ? true : homesteadOrLater const SECP256K1_N_DIV_2 = new BN('7fffffffffffffffffffffffffffffff5d576e7357a4501ddfe92f46681b20a0', 16) const SECP256K1_N = new BN('fffffffffffffffffffffffffffffffebaaedce6af48a03bbfd25e8cd0364141', 16) @@ -598,7 +599,7 @@ exports.isValidSignature = function (v, r, s, homestead, chainId) { return false } - if (homestead && (new BN(s).cmp(SECP256K1_N_DIV_2) === 1)) { + if (homesteadOrLater && (new BN(s).cmp(SECP256K1_N_DIV_2) === 1)) { return false } diff --git a/test/index.js b/test/index.js index 6242c0c7..8f1b1c17 100644 --- a/test/index.js +++ b/test/index.js @@ -565,7 +565,7 @@ describe('isValidSignature', function () { const s = Buffer.from(SECP256K1_N_DIV_2.add(new BN('1', 16)).toString(16), 'hex') const v = 27 - assert.equal(ethUtils.isValidSignature(v, r, s), true) + assert.equal(ethUtils.isValidSignature(v, r, s, true), false) }) it('should not fail when not on homestead but s > secp256k1n/2', function () { const SECP256K1_N_DIV_2 = new BN('7fffffffffffffffffffffffffffffff5d576e7357a4501ddfe92f46681b20a0', 16) @@ -574,7 +574,7 @@ describe('isValidSignature', function () { const s = Buffer.from(SECP256K1_N_DIV_2.add(new BN('1', 16)).toString(16), 'hex') const v = 27 - assert.equal(ethUtils.isValidSignature(v, r, s), true) + assert.equal(ethUtils.isValidSignature(v, r, s, false), true) }) it('should work otherwise', function () { const r = Buffer.from('99e71a99cb2270b8cac5254f9e99b6210c6c10224a1579cf389ef88b20a1abe9', 'hex')