From 0f8741660ac99cbc61b9d0d4fe42d0b715338bdb Mon Sep 17 00:00:00 2001 From: Jochem Brouwer Date: Sat, 27 Feb 2021 14:47:16 +0100 Subject: [PATCH 01/11] signature: add support for very high recovery IDs --- src/signature.ts | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/src/signature.ts b/src/signature.ts index c0235af3..b6e67e6f 100644 --- a/src/signature.ts +++ b/src/signature.ts @@ -30,12 +30,15 @@ export const ecsign = function( return ret } -function calculateSigRecovery(v: number, chainId?: number): number { - return chainId ? v - (2 * chainId + 35) : v - 27 +function calculateSigRecovery(v: number | BN | Buffer, chainId?: number | BN | Buffer): BN { + const vBN = new BN(toBuffer(v)) + const chainIdBN = chainId ? new BN(toBuffer(chainId)) : undefined + return chainIdBN ? vBN.sub(chainIdBN.muln(2).addn(35)) : vBN.subn(27) } -function isValidSigRecovery(recovery: number): boolean { - return recovery === 0 || recovery === 1 +function isValidSigRecovery(recovery: number | BN): boolean { + const rec = new BN(recovery) + return rec.eqn(0) || rec.eqn(1) } /** @@ -44,17 +47,17 @@ function isValidSigRecovery(recovery: number): boolean { */ export const ecrecover = function( msgHash: Buffer, - v: number, + v: number | BN | Buffer, r: Buffer, s: Buffer, - chainId?: number + chainId?: number | BN | Buffer ): Buffer { const signature = Buffer.concat([setLengthLeft(r, 32), setLengthLeft(s, 32)], 64) const recovery = calculateSigRecovery(v, chainId) if (!isValidSigRecovery(recovery)) { throw new Error('Invalid signature v value') } - const senderPubKey = ecdsaRecover(signature, recovery, msgHash) + const senderPubKey = ecdsaRecover(signature, recovery.toNumber(), msgHash) return Buffer.from(publicKeyConvert(senderPubKey, false).slice(1)) } From db8d73673a3365ba13a55c672bb55b2202281a9a Mon Sep 17 00:00:00 2001 From: Jochem Brouwer Date: Sat, 27 Feb 2021 17:05:29 +0100 Subject: [PATCH 02/11] signature: add test of failing tx on YoloV3 --- test/signature.spec.ts | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/test/signature.spec.ts b/test/signature.spec.ts index 52b8a0f3..0fba18db 100644 --- a/test/signature.spec.ts +++ b/test/signature.spec.ts @@ -106,6 +106,43 @@ describe('ecrecover', function() { ecrecover(echash, 27, s, r) }) }) + it('should return the right sender when using very high chain id / v values', function() { + // This data is from a transaction of the YoloV3 network, block 77, txhash c6121a23ca17b8ff70d4706c7d134920c1da43c8329444c96b4c63a55af1c760 + /* + { + nonce: '0x8', + gasPrice: '0x3b9aca00', + gasLimit: '0x1a965', + to: undefined, + value: '0x0', + data: '0x608060405234801561001057600080fd5b50610101806100206000396000f3fe608060405260043610601f5760003560e01c8063776d1a0114603b576020565b5b6000543660008037600080366000845af43d6000803e3d6000f35b348015604657600080fd5b50608660048036036020811015605b57600080fd5b81019080803573ffffffffffffffffffffffffffffffffffffffff1690602001909291905050506088565b005b806000806101000a81548173ffffffffffffffffffffffffffffffffffffffff021916908373ffffffffffffffffffffffffffffffffffffffff1602179055505056fea26469706673582212206d3160e3f009c6ebac579877e529c0a1ca8313678f08fe311659d440067d26ea64736f6c63430007040033', + v: '0xf2ded8deec6714', + r: '0xec212841e0b7aaffc3b3e33a08adf32fa07159e856ef23db85175a4f6d71dc0f', + s: '0x4b8e02b96b94064a5aa2f8d72bd0040616ba8e482a5dd96422e38c9a4611f8d5' + } + */ + const senderPubKey = Buffer.from( + '78988201fbceed086cfca7b64e382d08d0bd776898731443d2907c097745b7324c54f522087f5964412cddba019f192de0fd57a0ffa63f098c2b200e53594b15', + 'hex' + ) + const msgHash = Buffer.from( + '8ae8cb685a7a9f29494b07b287c3f6a103b73fa178419d10d1184861a40f6afe', + 'hex' + ) + const v = Buffer.from('f2ded8deec6714', 'hex') + const r = Buffer.from('ec212841e0b7aaffc3b3e33a08adf32fa07159e856ef23db85175a4f6d71dc0f', 'hex') + const s = Buffer.from('4b8e02b96b94064a5aa2f8d72bd0040616ba8e482a5dd96422e38c9a4611f8d5', 'hex') + const chainID = Buffer.from('796f6c6f763378', 'hex') + const sender = ecrecover(msgHash, v, r, s, chainID) + assert.ok(sender.equals(senderPubKey), 'sender pubkey correct') + const chainIDNumber = parseInt(chainID.toString('hex'), 16) + const vNumber = parseInt(v.toString('hex'), 16) + assert.throws(() => { + // If we would use numbers for the `v` and `chainId` parameters, then it should throw. + // (The numbers are too high to perform arithmetic on) + ecrecover(msgHash, vNumber, r, s, chainIDNumber) + }) + }) }) describe('hashPersonalMessage', function() { From 1c9d742fd75ab6b0e73d418fe9b4e2270ac26f80 Mon Sep 17 00:00:00 2001 From: holgerd77 Date: Tue, 2 Mar 2021 09:55:23 +0100 Subject: [PATCH 03/11] Added Buffer to BNLike type, PrefixedHexString in types, use BNLike for ecrecover v and chainId input, added type tests --- src/bytes.ts | 4 ++-- src/signature.ts | 7 ++++--- src/types.ts | 4 ++-- test/signature.spec.ts | 25 +++++++++++++++++++------ 4 files changed, 27 insertions(+), 13 deletions(-) diff --git a/src/bytes.ts b/src/bytes.ts index 7594771f..a9d13a42 100644 --- a/src/bytes.ts +++ b/src/bytes.ts @@ -1,6 +1,6 @@ import BN from 'bn.js' import { intToBuffer, stripHexPrefix, padToEven, isHexString, isHexPrefixed } from 'ethjs-util' -import { TransformableToArray, TransformableToBuffer } from './types' +import { PrefixedHexString, TransformableToArray, TransformableToBuffer } from './types' import { assertIsBuffer, assertIsArray, assertIsHexString } from './helpers' /** @@ -112,7 +112,7 @@ export const unpadHexString = function(a: string): string { */ export const toBuffer = function( v: - | string + | PrefixedHexString | number | BN | Buffer diff --git a/src/signature.ts b/src/signature.ts index b6e67e6f..14743912 100644 --- a/src/signature.ts +++ b/src/signature.ts @@ -3,6 +3,7 @@ import BN from 'bn.js' import { toBuffer, setLengthLeft, bufferToHex, bufferToInt } from './bytes' import { keccak } from './hash' import { assertIsBuffer } from './helpers' +import { BNLike } from './types' export interface ECDSASignature { v: number @@ -30,7 +31,7 @@ export const ecsign = function( return ret } -function calculateSigRecovery(v: number | BN | Buffer, chainId?: number | BN | Buffer): BN { +function calculateSigRecovery(v: BNLike, chainId?: BNLike): BN { const vBN = new BN(toBuffer(v)) const chainIdBN = chainId ? new BN(toBuffer(chainId)) : undefined return chainIdBN ? vBN.sub(chainIdBN.muln(2).addn(35)) : vBN.subn(27) @@ -47,10 +48,10 @@ function isValidSigRecovery(recovery: number | BN): boolean { */ export const ecrecover = function( msgHash: Buffer, - v: number | BN | Buffer, + v: BNLike, r: Buffer, s: Buffer, - chainId?: number | BN | Buffer + chainId?: BNLike ): Buffer { const signature = Buffer.concat([setLengthLeft(r, 32), setLengthLeft(s, 32)], 64) const recovery = calculateSigRecovery(v, chainId) diff --git a/src/types.ts b/src/types.ts index 7b3d7725..3afa856a 100644 --- a/src/types.ts +++ b/src/types.ts @@ -5,7 +5,7 @@ import { unpadBuffer } from './bytes' /* * A type that represents a BNLike input that can be converted to a BN. */ -export type BNLike = BN | string | number +export type BNLike = BN | PrefixedHexString | number | Buffer /* * A type that represents a BufferLike input that can be converted to a Buffer. @@ -28,7 +28,7 @@ export type PrefixedHexString = string * A type that represents an Address-like value. * To convert to address, use `new Address(toBuffer(value))` */ -export type AddressLike = Address | Buffer | string +export type AddressLike = Address | Buffer | PrefixedHexString /* * A type that represents an object that has a `toArray()` method. diff --git a/test/signature.spec.ts b/test/signature.spec.ts index 0fba18db..30c7542c 100644 --- a/test/signature.spec.ts +++ b/test/signature.spec.ts @@ -129,14 +129,27 @@ describe('ecrecover', function() { '8ae8cb685a7a9f29494b07b287c3f6a103b73fa178419d10d1184861a40f6afe', 'hex' ) - const v = Buffer.from('f2ded8deec6714', 'hex') + const r = Buffer.from('ec212841e0b7aaffc3b3e33a08adf32fa07159e856ef23db85175a4f6d71dc0f', 'hex') const s = Buffer.from('4b8e02b96b94064a5aa2f8d72bd0040616ba8e482a5dd96422e38c9a4611f8d5', 'hex') - const chainID = Buffer.from('796f6c6f763378', 'hex') - const sender = ecrecover(msgHash, v, r, s, chainID) - assert.ok(sender.equals(senderPubKey), 'sender pubkey correct') - const chainIDNumber = parseInt(chainID.toString('hex'), 16) - const vNumber = parseInt(v.toString('hex'), 16) + + const vBuffer = Buffer.from('f2ded8deec6714', 'hex') + const chainIDBuffer = Buffer.from('796f6c6f763378', 'hex') + let sender = ecrecover(msgHash, vBuffer, r, s, chainIDBuffer) + assert.ok(sender.equals(senderPubKey), 'sender pubkey correct (Buffer)') + + const vBN = new BN(vBuffer) + const chainIDBN = new BN(chainIDBuffer) + sender = ecrecover(msgHash, vBN, r, s, chainIDBN) + assert.ok(sender.equals(senderPubKey), 'sender pubkey correct (BN)') + + const vHexString = '0xf2ded8deec6714' + const chainIDHexString = '0x796f6c6f763378' + sender = ecrecover(msgHash, vHexString, r, s, chainIDHexString) + assert.ok(sender.equals(senderPubKey), 'sender pubkey correct (HexString)') + + const chainIDNumber = parseInt(chainIDBuffer.toString('hex'), 16) + const vNumber = parseInt(vBuffer.toString('hex'), 16) assert.throws(() => { // If we would use numbers for the `v` and `chainId` parameters, then it should throw. // (The numbers are too high to perform arithmetic on) From 5c80ce87fd21fba61b68e4ab3b80a80ecaf5e53c Mon Sep 17 00:00:00 2001 From: holgerd77 Date: Tue, 2 Mar 2021 12:04:23 +0100 Subject: [PATCH 04/11] Added BNLike chainId input and return types for ecsign, added tests --- src/signature.ts | 85 +++++++++++++++++++++++++++++++++++++----- test/signature.spec.ts | 39 ++++++++++++++----- 2 files changed, 106 insertions(+), 18 deletions(-) diff --git a/src/signature.ts b/src/signature.ts index 14743912..50dfea21 100644 --- a/src/signature.ts +++ b/src/signature.ts @@ -1,9 +1,10 @@ -const { ecdsaSign, ecdsaRecover, publicKeyConvert } = require('ethereum-cryptography/secp256k1') +import { ecdsaSign, ecdsaRecover, publicKeyConvert } from 'ethereum-cryptography/secp256k1' import BN from 'bn.js' import { toBuffer, setLengthLeft, bufferToHex, bufferToInt } from './bytes' import { keccak } from './hash' import { assertIsBuffer } from './helpers' -import { BNLike } from './types' +import { BNLike, PrefixedHexString } from './types' +import { isHexString } from '.' export interface ECDSASignature { v: number @@ -11,21 +12,87 @@ export interface ECDSASignature { s: Buffer } +export interface ECDSASignatureBN { + v: BN + r: Buffer + s: Buffer +} + +export interface ECDSASignatureBuffer { + v: Buffer + r: Buffer + s: Buffer +} + +export interface ECDSASignatureHexString { + v: PrefixedHexString + r: Buffer + s: Buffer +} + /** * Returns the ECDSA signature of a message hash. */ -export const ecsign = function( +export function ecsign(msgHash: Buffer, privateKey: Buffer, chainId?: number): ECDSASignature +export function ecsign(msgHash: Buffer, privateKey: Buffer, chainId: BN): ECDSASignatureBN +export function ecsign(msgHash: Buffer, privateKey: Buffer, chainId: Buffer): ECDSASignatureBuffer +export function ecsign( msgHash: Buffer, privateKey: Buffer, - chainId?: number -): ECDSASignature { + chainId: PrefixedHexString +): ECDSASignatureHexString +export function ecsign(msgHash: Buffer, privateKey: Buffer, chainId: any): any { const sig = ecdsaSign(msgHash, privateKey) const recovery: number = sig.recid - const ret = { - r: Buffer.from(sig.signature.slice(0, 32)), - s: Buffer.from(sig.signature.slice(32, 64)), - v: chainId ? recovery + (chainId * 2 + 35) : recovery + 27 + let ret + if (typeof chainId === 'number') { + return { + r: Buffer.from(sig.signature.slice(0, 32)), + s: Buffer.from(sig.signature.slice(32, 64)), + v: recovery + (chainId * 2 + 35) + } + } else if (BN.isBN(chainId)) { + ret = { + r: Buffer.from(sig.signature.slice(0, 32)), + s: Buffer.from(sig.signature.slice(32, 64)), + v: (chainId as BN) + .muln(2) + .addn(35) + .addn(recovery) + } + } else if (Buffer.isBuffer(chainId)) { + ret = { + r: Buffer.from(sig.signature.slice(0, 32)), + s: Buffer.from(sig.signature.slice(32, 64)), + v: toBuffer( + new BN(chainId) + .muln(2) + .addn(35) + .addn(recovery) + ) + } + } else if (typeof chainId === 'string') { + if (!isHexString(chainId)) { + throw new Error(`A chainId string must be provided with a 0x-prefix, given: ${chainId}`) + } + ret = { + r: Buffer.from(sig.signature.slice(0, 32)), + s: Buffer.from(sig.signature.slice(32, 64)), + v: + '0x' + + new BN(toBuffer(chainId)) + .muln(2) + .addn(35) + .addn(recovery) + .toString('hex') + } + } else { + ret = { + r: Buffer.from(sig.signature.slice(0, 32)), + s: Buffer.from(sig.signature.slice(32, 64)), + v: recovery + 27 + } } return ret diff --git a/test/signature.spec.ts b/test/signature.spec.ts index 30c7542c..6033880b 100644 --- a/test/signature.spec.ts +++ b/test/signature.spec.ts @@ -48,17 +48,38 @@ describe('ecsign', function() { }) it('should produce a signature for chainId=150', function() { - const chainId = 150 - const sig = ecsign(echash, ecprivkey, chainId) - assert.deepEqual( - sig.r, - Buffer.from('99e71a99cb2270b8cac5254f9e99b6210c6c10224a1579cf389ef88b20a1abe9', 'hex') + const expectedSigR = Buffer.from( + '99e71a99cb2270b8cac5254f9e99b6210c6c10224a1579cf389ef88b20a1abe9', + 'hex' ) - assert.deepEqual( - sig.s, - Buffer.from('129ff05af364204442bdb53ab6f18a99ab48acc9326fa689f228040429e3ca66', 'hex') + const expectedSigS = Buffer.from( + '129ff05af364204442bdb53ab6f18a99ab48acc9326fa689f228040429e3ca66', + 'hex' ) - assert.equal(sig.v, chainId * 2 + 35) + + const sig = ecsign(echash, ecprivkey, 150) + assert.deepEqual(sig.r, expectedSigR) + assert.deepEqual(sig.s, expectedSigS) + assert.equal(sig.v, 150 * 2 + 35) + + const sigBN = ecsign(echash, ecprivkey, new BN(150)) + assert.deepEqual(sigBN.r, expectedSigR) + assert.deepEqual(sigBN.s, expectedSigS) + assert.deepEqual(sigBN.v, new BN(150).muln(2).addn(35)) + + const sigBuffer = ecsign(echash, ecprivkey, Buffer.from([150])) + assert.deepEqual(sigBuffer.r, expectedSigR) + assert.deepEqual(sigBuffer.s, expectedSigS) + assert.deepEqual(sigBuffer.v, Buffer.from('014f', 'hex')) + + const sigHexString = ecsign(echash, ecprivkey, '0x96') + assert.deepEqual(sigHexString.r, expectedSigR) + assert.deepEqual(sigHexString.s, expectedSigS) + assert.equal(sigHexString.v, '0x14f') + + assert.throws(function() { + ecsign(echash, ecprivkey, '96') + }) }) }) From 24279bf839eb697a8f8e2b643662f6d2986c266b Mon Sep 17 00:00:00 2001 From: holgerd77 Date: Tue, 2 Mar 2021 13:22:40 +0100 Subject: [PATCH 05/11] Limited ecsign v return value to Buffer on extended input types, additional input value checks --- src/signature.ts | 92 +++++++++++++++++------------------------- test/signature.spec.ts | 57 +++++++++++++++++++++----- 2 files changed, 84 insertions(+), 65 deletions(-) diff --git a/src/signature.ts b/src/signature.ts index 50dfea21..2c7b14f4 100644 --- a/src/signature.ts +++ b/src/signature.ts @@ -3,7 +3,7 @@ import BN from 'bn.js' import { toBuffer, setLengthLeft, bufferToHex, bufferToInt } from './bytes' import { keccak } from './hash' import { assertIsBuffer } from './helpers' -import { BNLike, PrefixedHexString } from './types' +import { BNLike } from './types' import { isHexString } from '.' export interface ECDSASignature { @@ -12,86 +12,53 @@ export interface ECDSASignature { s: Buffer } -export interface ECDSASignatureBN { - v: BN - r: Buffer - s: Buffer -} - export interface ECDSASignatureBuffer { v: Buffer r: Buffer s: Buffer } -export interface ECDSASignatureHexString { - v: PrefixedHexString - r: Buffer - s: Buffer -} - /** * Returns the ECDSA signature of a message hash. */ export function ecsign(msgHash: Buffer, privateKey: Buffer, chainId?: number): ECDSASignature -export function ecsign(msgHash: Buffer, privateKey: Buffer, chainId: BN): ECDSASignatureBN -export function ecsign(msgHash: Buffer, privateKey: Buffer, chainId: Buffer): ECDSASignatureBuffer export function ecsign( msgHash: Buffer, privateKey: Buffer, - chainId: PrefixedHexString -): ECDSASignatureHexString + chainId: BN | string | Buffer +): ECDSASignatureBuffer export function ecsign(msgHash: Buffer, privateKey: Buffer, chainId: any): any { const sig = ecdsaSign(msgHash, privateKey) const recovery: number = sig.recid let ret - if (typeof chainId === 'number') { - return { - r: Buffer.from(sig.signature.slice(0, 32)), - s: Buffer.from(sig.signature.slice(32, 64)), - v: recovery + (chainId * 2 + 35) - } - } else if (BN.isBN(chainId)) { - ret = { - r: Buffer.from(sig.signature.slice(0, 32)), - s: Buffer.from(sig.signature.slice(32, 64)), - v: (chainId as BN) - .muln(2) - .addn(35) - .addn(recovery) - } - } else if (Buffer.isBuffer(chainId)) { - ret = { - r: Buffer.from(sig.signature.slice(0, 32)), - s: Buffer.from(sig.signature.slice(32, 64)), - v: toBuffer( - new BN(chainId) - .muln(2) - .addn(35) - .addn(recovery) + const r = Buffer.from(sig.signature.slice(0, 32)) + const s = Buffer.from(sig.signature.slice(32, 64)) + if (!chainId || typeof chainId === 'number') { + if (chainId && !Number.isSafeInteger(chainId)) { + throw new Error( + 'The provided chainId is greater than MAX_SAFE_INTEGER (please use an alternative input type)' ) } - } else if (typeof chainId === 'string') { - if (!isHexString(chainId)) { + return { + r, + s, + v: chainId ? recovery + (chainId * 2 + 35) : recovery + 27 + } + } else { + // BN, string, Buffer + if (typeof chainId === 'string' && !isHexString(chainId)) { throw new Error(`A chainId string must be provided with a 0x-prefix, given: ${chainId}`) } ret = { - r: Buffer.from(sig.signature.slice(0, 32)), - s: Buffer.from(sig.signature.slice(32, 64)), - v: - '0x' + + r, + s, + v: toBuffer( new BN(toBuffer(chainId)) .muln(2) .addn(35) .addn(recovery) - .toString('hex') - } - } else { - ret = { - r: Buffer.from(sig.signature.slice(0, 32)), - s: Buffer.from(sig.signature.slice(32, 64)), - v: recovery + 27 + ) } } @@ -120,6 +87,23 @@ export const ecrecover = function( s: Buffer, chainId?: BNLike ): Buffer { + if (typeof v === 'string' && !isHexString(v)) { + throw new Error(`A v value string must be provided with a 0x-prefix, given: ${v}`) + } + if (typeof chainId === 'string' && !isHexString(chainId)) { + throw new Error(`A chainId string must be provided with a 0x-prefix, given: ${chainId}`) + } + if (typeof v === 'number' && !Number.isSafeInteger(v)) { + throw new Error( + 'The provided v is greater than MAX_SAFE_INTEGER (please use an alternative input type)' + ) + } + if (typeof chainId === 'number' && !Number.isSafeInteger(chainId)) { + throw new Error( + 'The provided chainId is greater than MAX_SAFE_INTEGER (please use an alternative input type)' + ) + } + const signature = Buffer.concat([setLengthLeft(r, 32), setLengthLeft(s, 32)], 64) const recovery = calculateSigRecovery(v, chainId) if (!isValidSigRecovery(recovery)) { diff --git a/test/signature.spec.ts b/test/signature.spec.ts index 6033880b..6a897fed 100644 --- a/test/signature.spec.ts +++ b/test/signature.spec.ts @@ -56,26 +56,23 @@ describe('ecsign', function() { '129ff05af364204442bdb53ab6f18a99ab48acc9326fa689f228040429e3ca66', 'hex' ) + const expectedSigV = Buffer.from('014f', 'hex') const sig = ecsign(echash, ecprivkey, 150) assert.deepEqual(sig.r, expectedSigR) assert.deepEqual(sig.s, expectedSigS) assert.equal(sig.v, 150 * 2 + 35) - const sigBN = ecsign(echash, ecprivkey, new BN(150)) - assert.deepEqual(sigBN.r, expectedSigR) - assert.deepEqual(sigBN.s, expectedSigS) - assert.deepEqual(sigBN.v, new BN(150).muln(2).addn(35)) - - const sigBuffer = ecsign(echash, ecprivkey, Buffer.from([150])) + let sigBuffer = ecsign(echash, ecprivkey, new BN(150)) assert.deepEqual(sigBuffer.r, expectedSigR) assert.deepEqual(sigBuffer.s, expectedSigS) - assert.deepEqual(sigBuffer.v, Buffer.from('014f', 'hex')) + assert.deepEqual(sigBuffer.v, expectedSigV) + + sigBuffer = ecsign(echash, ecprivkey, Buffer.from([150])) + assert.deepEqual(sigBuffer.v, expectedSigV) - const sigHexString = ecsign(echash, ecprivkey, '0x96') - assert.deepEqual(sigHexString.r, expectedSigR) - assert.deepEqual(sigHexString.s, expectedSigS) - assert.equal(sigHexString.v, '0x14f') + sigBuffer = ecsign(echash, ecprivkey, '0x96') + assert.deepEqual(sigBuffer.v, expectedSigV) assert.throws(function() { ecsign(echash, ecprivkey, '96') @@ -83,6 +80,37 @@ describe('ecsign', function() { }) }) +it('should produce a signature for a high number chainId greater than MAX_SAFE_INTEGER', function() { + const chainIDBuffer = Buffer.from('796f6c6f763378', 'hex') + const expectedSigR = Buffer.from( + '99e71a99cb2270b8cac5254f9e99b6210c6c10224a1579cf389ef88b20a1abe9', + 'hex' + ) + const expectedSigS = Buffer.from( + '129ff05af364204442bdb53ab6f18a99ab48acc9326fa689f228040429e3ca66', + 'hex' + ) + const expectedSigV = Buffer.from('f2ded8deec6713', 'hex') + + let sigBuffer = ecsign(echash, ecprivkey, new BN(chainIDBuffer)) + assert.deepEqual(sigBuffer.r, expectedSigR) + assert.deepEqual(sigBuffer.s, expectedSigS) + assert.deepEqual(sigBuffer.v, expectedSigV) + + sigBuffer = ecsign(echash, ecprivkey, chainIDBuffer) + assert.deepEqual(sigBuffer.v, expectedSigV) + + sigBuffer = ecsign(echash, ecprivkey, '0x' + chainIDBuffer.toString('hex')) + assert.deepEqual(sigBuffer.v, expectedSigV) + + const chainIDNumber = parseInt(chainIDBuffer.toString('hex'), 16) + assert.throws(() => { + // If we would use a number for the `chainId` parameter then it should throw. + // (The numbers are too high to perform arithmetic on) + ecsign(echash, ecprivkey, chainIDNumber) + }) +}) + describe('ecrecover', function() { it('should recover a public key', function() { const r = Buffer.from('99e71a99cb2270b8cac5254f9e99b6210c6c10224a1579cf389ef88b20a1abe9', 'hex') @@ -169,6 +197,13 @@ describe('ecrecover', function() { sender = ecrecover(msgHash, vHexString, r, s, chainIDHexString) assert.ok(sender.equals(senderPubKey), 'sender pubkey correct (HexString)') + assert.throws(function() { + ecrecover(msgHash, 'f2ded8deec6714', r, s, chainIDHexString) + }) + assert.throws(function() { + ecrecover(msgHash, vHexString, r, s, '796f6c6f763378') + }) + const chainIDNumber = parseInt(chainIDBuffer.toString('hex'), 16) const vNumber = parseInt(vBuffer.toString('hex'), 16) assert.throws(() => { From abe0aff8230f80aef73d9fec8c1617cee7f228b1 Mon Sep 17 00:00:00 2001 From: holgerd77 Date: Tue, 2 Mar 2021 18:36:56 +0100 Subject: [PATCH 06/11] Added BNLike input type for chain IDs in toChecksumAddress() and isValidChecksumAddress(), expanded tests --- src/account.ts | 20 ++++++++++++++++---- test/account.spec.ts | 29 +++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 4 deletions(-) diff --git a/src/account.ts b/src/account.ts index e6895064..90236ca0 100644 --- a/src/account.ts +++ b/src/account.ts @@ -1,7 +1,7 @@ import assert from 'assert' import BN from 'bn.js' import * as rlp from 'rlp' -import { stripHexPrefix } from 'ethjs-util' +import { isHexString, stripHexPrefix } from 'ethjs-util' import { KECCAK256_RLP, KECCAK256_NULL } from './constants' import { zeros, bufferToHex, toBuffer } from './bytes' import { keccak, keccak256, keccakFromString, rlphash } from './hash' @@ -137,11 +137,23 @@ export const isValidAddress = function(hexAddress: string): boolean { * WARNING: Checksums with and without the chainId will differ. As of 2019-06-26, the most commonly * used variation in Ethereum was without the chainId. This may change in the future. */ -export const toChecksumAddress = function(hexAddress: string, eip1191ChainId?: number): string { +export const toChecksumAddress = function(hexAddress: string, eip1191ChainId?: BNLike): string { assertIsHexString(hexAddress) + if (typeof eip1191ChainId === 'string' && !isHexString(eip1191ChainId)) { + throw new Error(`A chainId string must be provided with a 0x-prefix, given: ${eip1191ChainId}`) + } const address = stripHexPrefix(hexAddress).toLowerCase() - const prefix = eip1191ChainId !== undefined ? eip1191ChainId.toString() + '0x' : '' + let prefix = '' + if (eip1191ChainId) { + // Performance optimization + if (typeof eip1191ChainId === 'number' && Number.isSafeInteger(eip1191ChainId)) { + prefix = eip1191ChainId.toString() + } else { + prefix = new BN(toBuffer(eip1191ChainId)).toString() + } + prefix += '0x' + } const hash = keccakFromString(prefix + address).toString('hex') let ret = '0x' @@ -164,7 +176,7 @@ export const toChecksumAddress = function(hexAddress: string, eip1191ChainId?: n */ export const isValidChecksumAddress = function( hexAddress: string, - eip1191ChainId?: number + eip1191ChainId?: BNLike ): boolean { return isValidAddress(hexAddress) && toChecksumAddress(hexAddress, eip1191ChainId) === hexAddress } diff --git a/test/account.spec.ts b/test/account.spec.ts index ce9e22a0..a5e81aa0 100644 --- a/test/account.spec.ts +++ b/test/account.spec.ts @@ -602,9 +602,27 @@ describe('.toChecksumAddress()', function() { for (const [chainId, addresses] of Object.entries(eip1191ChecksummAddresses)) { for (const addr of addresses) { assert.equal(toChecksumAddress(addr.toLowerCase(), Number(chainId)), addr) + assert.equal(toChecksumAddress(addr.toLowerCase(), Buffer.from([chainId])), addr) + assert.equal(toChecksumAddress(addr.toLowerCase(), new BN(chainId)), addr) + assert.equal( + toChecksumAddress(addr.toLowerCase(), '0x' + Buffer.from([chainId]).toString('hex')), + addr + ) } } }) + it('Should encode large chain ids greater than MAX_INTEGER correctly', function() { + const addr = '0x88021160C5C792225E4E5452585947470010289D' + const chainIDBuffer = Buffer.from('796f6c6f763378', 'hex') + assert.equal(toChecksumAddress(addr.toLowerCase(), chainIDBuffer), addr) + assert.equal(toChecksumAddress(addr.toLowerCase(), new BN(chainIDBuffer)), addr) + assert.equal( + toChecksumAddress(addr.toLowerCase(), '0x' + chainIDBuffer.toString('hex')), + addr + ) + const chainIDNumber = parseInt(chainIDBuffer.toString('hex'), 16) + assert.equal(toChecksumAddress(addr.toLowerCase(), chainIDNumber), addr) + }) }) describe('input format', function() { @@ -613,6 +631,11 @@ describe('.toChecksumAddress()', function() { toChecksumAddress('52908400098527886E0F7030069857D2E4169EE7'.toLowerCase()) }) }) + it('Should throw when the chainId is not hex-prefixed', function() { + assert.throws(function() { + toChecksumAddress('0xde709f2102306220921060314715629080e2fb77', '1234') + }) + }) }) }) @@ -633,6 +656,12 @@ describe('.isValidChecksumAddress()', function() { for (const [chainId, addresses] of Object.entries(eip1191ChecksummAddresses)) { for (const addr of addresses) { assert.equal(isValidChecksumAddress(addr, Number(chainId)), true) + assert.equal(isValidChecksumAddress(addr, Buffer.from([chainId])), true) + assert.equal(isValidChecksumAddress(addr, new BN(chainId)), true) + assert.equal( + isValidChecksumAddress(addr, '0x' + Buffer.from([chainId]).toString('hex')), + true + ) } } }) From b3ba3eec78c30cee2c8ea81e257b19d39d30739c Mon Sep 17 00:00:00 2001 From: holgerd77 Date: Tue, 2 Mar 2021 19:14:56 +0100 Subject: [PATCH 07/11] Added BNLike input type for v and chain IDs in toRpcSig() and isValidSignature(), expanded tests --- src/signature.ts | 34 +++++++----- test/signature.spec.ts | 118 +++++++++++++++++++++++++++++------------ 2 files changed, 103 insertions(+), 49 deletions(-) diff --git a/src/signature.ts b/src/signature.ts index 2c7b14f4..96dfa3d6 100644 --- a/src/signature.ts +++ b/src/signature.ts @@ -76,17 +76,7 @@ function isValidSigRecovery(recovery: number | BN): boolean { return rec.eqn(0) || rec.eqn(1) } -/** - * ECDSA public key recovery from signature. - * @returns Recovered public key - */ -export const ecrecover = function( - msgHash: Buffer, - v: BNLike, - r: Buffer, - s: Buffer, - chainId?: BNLike -): Buffer { +function vAndChainIdTypeChecks(v: BNLike, chainId?: BNLike) { if (typeof v === 'string' && !isHexString(v)) { throw new Error(`A v value string must be provided with a 0x-prefix, given: ${v}`) } @@ -103,6 +93,20 @@ export const ecrecover = function( 'The provided chainId is greater than MAX_SAFE_INTEGER (please use an alternative input type)' ) } +} + +/** + * ECDSA public key recovery from signature. + * @returns Recovered public key + */ +export const ecrecover = function( + msgHash: Buffer, + v: BNLike, + r: Buffer, + s: Buffer, + chainId?: BNLike +): Buffer { + vAndChainIdTypeChecks(v, chainId) const signature = Buffer.concat([setLengthLeft(r, 32), setLengthLeft(s, 32)], 64) const recovery = calculateSigRecovery(v, chainId) @@ -117,7 +121,8 @@ export const ecrecover = function( * Convert signature parameters into the format of `eth_sign` RPC method. * @returns Signature */ -export const toRpcSig = function(v: number, r: Buffer, s: Buffer, chainId?: number): string { +export const toRpcSig = function(v: BNLike, r: Buffer, s: Buffer, chainId?: BNLike): string { + vAndChainIdTypeChecks(v, chainId) const recovery = calculateSigRecovery(v, chainId) if (!isValidSigRecovery(recovery)) { throw new Error('Invalid signature v value') @@ -156,12 +161,13 @@ export const fromRpcSig = function(sig: string): ECDSASignature { * @param homesteadOrLater Indicates whether this is being used on either the homestead hardfork or a later one */ export const isValidSignature = function( - v: number, + v: BNLike, r: Buffer, s: Buffer, homesteadOrLater: boolean = true, - chainId?: number + chainId?: BNLike ): boolean { + vAndChainIdTypeChecks(v, chainId) const SECP256K1_N_DIV_2 = new BN( '7fffffffffffffffffffffffffffffff5d576e7357a4501ddfe92f46681b20a0', 16 diff --git a/test/signature.spec.ts b/test/signature.spec.ts index 6a897fed..972856f5 100644 --- a/test/signature.spec.ts +++ b/test/signature.spec.ts @@ -7,7 +7,8 @@ import { hashPersonalMessage, isValidSignature, fromRpcSig, - toRpcSig + toRpcSig, + intToBuffer } from '../src' const echash = Buffer.from( @@ -294,6 +295,45 @@ describe('isValidSignature', function() { const s = Buffer.from('129ff05af364204442bdb53ab6f18a99ab48acc9326fa689f228040429e3ca66', 'hex') const v = chainId * 2 + 35 assert.equal(isValidSignature(v, r, s, false, chainId), true) + assert.equal(isValidSignature(intToBuffer(v), r, s, false, intToBuffer(chainId)), true) + assert.equal(isValidSignature(new BN(v), r, s, false, new BN(chainId)), true) + assert.equal( + isValidSignature( + '0x' + intToBuffer(v).toString('hex'), + r, + s, + false, + '0x' + intToBuffer(chainId).toString('hex') + ), + true + ) + }) + it('should work otherwise(chainId larger than MAX_INTEGER)', function() { + const r = Buffer.from('ec212841e0b7aaffc3b3e33a08adf32fa07159e856ef23db85175a4f6d71dc0f', 'hex') + const s = Buffer.from('4b8e02b96b94064a5aa2f8d72bd0040616ba8e482a5dd96422e38c9a4611f8d5', 'hex') + + const vBuffer = Buffer.from('f2ded8deec6714', 'hex') + const chainIDBuffer = Buffer.from('796f6c6f763378', 'hex') + assert.equal(isValidSignature(vBuffer, r, s, false, chainIDBuffer), true) + assert.equal(isValidSignature(new BN(vBuffer), r, s, false, new BN(chainIDBuffer)), true) + assert.equal( + isValidSignature( + '0x' + vBuffer.toString('hex'), + r, + s, + false, + '0x' + chainIDBuffer.toString('hex') + ), + true + ) + + const chainIDNumber = parseInt(chainIDBuffer.toString('hex'), 16) + const vNumber = parseInt(vBuffer.toString('hex'), 16) + assert.throws(() => { + // If we would use numbers for the `v` and `chainId` parameters, then it should throw. + // (The numbers are too high to perform arithmetic on) + isValidSignature(vNumber, r, s, false, chainIDNumber) + }) }) // FIXME: add homestead test }) @@ -303,49 +343,57 @@ describe('message sig', function() { const s = Buffer.from('129ff05af364204442bdb53ab6f18a99ab48acc9326fa689f228040429e3ca66', 'hex') it('should return hex strings that the RPC can use', function() { - assert.equal( - toRpcSig(27, r, s), + const sig = '0x99e71a99cb2270b8cac5254f9e99b6210c6c10224a1579cf389ef88b20a1abe9129ff05af364204442bdb53ab6f18a99ab48acc9326fa689f228040429e3ca661b' - ) - assert.deepEqual( - fromRpcSig( - '0x99e71a99cb2270b8cac5254f9e99b6210c6c10224a1579cf389ef88b20a1abe9129ff05af364204442bdb53ab6f18a99ab48acc9326fa689f228040429e3ca661b' - ), - { - v: 27, - r: r, - s: s - } - ) - assert.deepEqual( - fromRpcSig( - '0x99e71a99cb2270b8cac5254f9e99b6210c6c10224a1579cf389ef88b20a1abe9129ff05af364204442bdb53ab6f18a99ab48acc9326fa689f228040429e3ca6600' - ), - { - v: 27, - r: r, - s: s - } - ) + assert.equal(toRpcSig(27, r, s), sig) + assert.deepEqual(fromRpcSig(sig), { + v: 27, + r: r, + s: s + }) }) it('should return hex strings that the RPC can use (chainId=150)', function() { + const sig = + '0x99e71a99cb2270b8cac5254f9e99b6210c6c10224a1579cf389ef88b20a1abe9129ff05af364204442bdb53ab6f18a99ab48acc9326fa689f228040429e3ca66014f' const chainId = 150 const v = chainId * 2 + 35 + assert.equal(toRpcSig(v, r, s, chainId), sig) + assert.equal(toRpcSig(intToBuffer(v), r, s, intToBuffer(chainId)), sig) + assert.equal(toRpcSig(new BN(v), r, s, new BN(chainId)), sig) assert.equal( - toRpcSig(v, r, s, chainId), - '0x99e71a99cb2270b8cac5254f9e99b6210c6c10224a1579cf389ef88b20a1abe9129ff05af364204442bdb53ab6f18a99ab48acc9326fa689f228040429e3ca66014f' - ) - assert.deepEqual( - fromRpcSig( - '0x99e71a99cb2270b8cac5254f9e99b6210c6c10224a1579cf389ef88b20a1abe9129ff05af364204442bdb53ab6f18a99ab48acc9326fa689f228040429e3ca66014f' + toRpcSig( + '0x' + intToBuffer(v).toString('hex'), + r, + s, + '0x' + intToBuffer(chainId).toString('hex') ), - { - v, - r: r, - s: s - } + sig ) + assert.deepEqual(fromRpcSig(sig), { + v, + r: r, + s: s + }) + }) + + it('should return hex strings that the RPC can use (chainId larger than MAX_SAFE_INTEGER)', function() { + const sig = + '0x99e71a99cb2270b8cac5254f9e99b6210c6c10224a1579cf389ef88b20a1abe9129ff05af364204442bdb53ab6f18a99ab48acc9326fa689f228040429e3ca66f2ded8deec6714' + const chainIDBuffer = Buffer.from('796f6c6f763378', 'hex') + const vBuffer = Buffer.from('f2ded8deec6714', 'hex') + assert.equal(toRpcSig(vBuffer, r, s, chainIDBuffer), sig) + assert.equal(toRpcSig(new BN(vBuffer), r, s, new BN(chainIDBuffer)), sig) + assert.equal( + toRpcSig('0x' + vBuffer.toString('hex'), r, s, '0x' + chainIDBuffer.toString('hex')), + sig + ) + + const chainIDNumber = parseInt(chainIDBuffer.toString('hex'), 16) + const vNumber = parseInt(vBuffer.toString('hex'), 16) + assert.throws(function() { + toRpcSig(vNumber, r, s, chainIDNumber) + }) }) it('should throw on shorter length', function() { From 243fd4776d789c7b8c37dc22d0249465d1a0766c Mon Sep 17 00:00:00 2001 From: Ryan Ghods Date: Tue, 2 Mar 2021 22:35:08 -0800 Subject: [PATCH 08/11] add `toType` helper with 0x-prefix and MAX_SAFE_INTEGER checks --- src/account.ts | 16 ++------ src/bytes.ts | 26 ++++++------ src/signature.ts | 92 +++++++++++------------------------------- src/types.ts | 51 ++++++++++++++++++++++- test/account.spec.ts | 10 ++++- test/signature.spec.ts | 8 ++-- 6 files changed, 104 insertions(+), 99 deletions(-) diff --git a/src/account.ts b/src/account.ts index 90236ca0..fb8d46ba 100644 --- a/src/account.ts +++ b/src/account.ts @@ -1,12 +1,12 @@ import assert from 'assert' import BN from 'bn.js' import * as rlp from 'rlp' -import { isHexString, stripHexPrefix } from 'ethjs-util' +import { stripHexPrefix } from 'ethjs-util' import { KECCAK256_RLP, KECCAK256_NULL } from './constants' import { zeros, bufferToHex, toBuffer } from './bytes' import { keccak, keccak256, keccakFromString, rlphash } from './hash' import { assertIsHexString, assertIsBuffer } from './helpers' -import { BNLike, BufferLike, bnToRlp } from './types' +import { BNLike, BufferLike, bnToRlp, toType, TypeOutput } from './types' const { privateKeyVerify, @@ -139,20 +139,12 @@ export const isValidAddress = function(hexAddress: string): boolean { */ export const toChecksumAddress = function(hexAddress: string, eip1191ChainId?: BNLike): string { assertIsHexString(hexAddress) - if (typeof eip1191ChainId === 'string' && !isHexString(eip1191ChainId)) { - throw new Error(`A chainId string must be provided with a 0x-prefix, given: ${eip1191ChainId}`) - } const address = stripHexPrefix(hexAddress).toLowerCase() let prefix = '' if (eip1191ChainId) { - // Performance optimization - if (typeof eip1191ChainId === 'number' && Number.isSafeInteger(eip1191ChainId)) { - prefix = eip1191ChainId.toString() - } else { - prefix = new BN(toBuffer(eip1191ChainId)).toString() - } - prefix += '0x' + const chainId = toType(eip1191ChainId, TypeOutput.BN) + prefix = chainId.toString() + '0x' } const hash = keccakFromString(prefix + address).toString('hex') diff --git a/src/bytes.ts b/src/bytes.ts index a9d13a42..f2229e3b 100644 --- a/src/bytes.ts +++ b/src/bytes.ts @@ -105,24 +105,24 @@ export const unpadHexString = function(a: string): string { return stripZeros(a) as string } +export type ToBufferInputTypes = + | PrefixedHexString + | number + | BN + | Buffer + | Uint8Array + | number[] + | TransformableToArray + | TransformableToBuffer + | null + | undefined + /** * Attempts to turn a value into a `Buffer`. * Inputs supported: `Buffer`, `String`, `Number`, null/undefined, `BN` and other objects with a `toArray()` or `toBuffer()` method. * @param v the value */ -export const toBuffer = function( - v: - | PrefixedHexString - | number - | BN - | Buffer - | Uint8Array - | number[] - | TransformableToArray - | TransformableToBuffer - | null - | undefined -): Buffer { +export const toBuffer = function(v: ToBufferInputTypes): Buffer { if (v === null || v === undefined) { return Buffer.allocUnsafe(0) } diff --git a/src/signature.ts b/src/signature.ts index 96dfa3d6..9083f104 100644 --- a/src/signature.ts +++ b/src/signature.ts @@ -3,8 +3,7 @@ import BN from 'bn.js' import { toBuffer, setLengthLeft, bufferToHex, bufferToInt } from './bytes' import { keccak } from './hash' import { assertIsBuffer } from './helpers' -import { BNLike } from './types' -import { isHexString } from '.' +import { BNLike, toType, TypeOutput } from './types' export interface ECDSASignature { v: number @@ -22,53 +21,33 @@ export interface ECDSASignatureBuffer { * Returns the ECDSA signature of a message hash. */ export function ecsign(msgHash: Buffer, privateKey: Buffer, chainId?: number): ECDSASignature -export function ecsign( - msgHash: Buffer, - privateKey: Buffer, - chainId: BN | string | Buffer -): ECDSASignatureBuffer +export function ecsign(msgHash: Buffer, privateKey: Buffer, chainId: BNLike): ECDSASignatureBuffer export function ecsign(msgHash: Buffer, privateKey: Buffer, chainId: any): any { - const sig = ecdsaSign(msgHash, privateKey) - const recovery: number = sig.recid - - let ret - const r = Buffer.from(sig.signature.slice(0, 32)) - const s = Buffer.from(sig.signature.slice(32, 64)) - if (!chainId || typeof chainId === 'number') { - if (chainId && !Number.isSafeInteger(chainId)) { - throw new Error( - 'The provided chainId is greater than MAX_SAFE_INTEGER (please use an alternative input type)' - ) - } - return { - r, - s, - v: chainId ? recovery + (chainId * 2 + 35) : recovery + 27 - } + const { signature, recid: recovery } = ecdsaSign(msgHash, privateKey) + + const r = Buffer.from(signature.slice(0, 32)) + const s = Buffer.from(signature.slice(32, 64)) + + if (!chainId) { + return { r, s, v: recovery + 27 } } else { - // BN, string, Buffer - if (typeof chainId === 'string' && !isHexString(chainId)) { - throw new Error(`A chainId string must be provided with a 0x-prefix, given: ${chainId}`) - } - ret = { - r, - s, - v: toBuffer( - new BN(toBuffer(chainId)) - .muln(2) - .addn(35) - .addn(recovery) - ) - } + const chainIdBN = toType(chainId, TypeOutput.BN) + const vValue = chainIdBN + .muln(2) + .addn(35) + .addn(recovery) + const v = typeof chainId === 'number' ? vValue.toNumber() : vValue.toArrayLike(Buffer) + return { r, s, v } } - - return ret } function calculateSigRecovery(v: BNLike, chainId?: BNLike): BN { - const vBN = new BN(toBuffer(v)) - const chainIdBN = chainId ? new BN(toBuffer(chainId)) : undefined - return chainIdBN ? vBN.sub(chainIdBN.muln(2).addn(35)) : vBN.subn(27) + const vBN = toType(v, TypeOutput.BN) + if (!chainId) { + return vBN.subn(27) + } + const chainIdBN = toType(chainId, TypeOutput.BN) + return vBN.sub(chainIdBN.muln(2).addn(35)) } function isValidSigRecovery(recovery: number | BN): boolean { @@ -76,25 +55,6 @@ function isValidSigRecovery(recovery: number | BN): boolean { return rec.eqn(0) || rec.eqn(1) } -function vAndChainIdTypeChecks(v: BNLike, chainId?: BNLike) { - if (typeof v === 'string' && !isHexString(v)) { - throw new Error(`A v value string must be provided with a 0x-prefix, given: ${v}`) - } - if (typeof chainId === 'string' && !isHexString(chainId)) { - throw new Error(`A chainId string must be provided with a 0x-prefix, given: ${chainId}`) - } - if (typeof v === 'number' && !Number.isSafeInteger(v)) { - throw new Error( - 'The provided v is greater than MAX_SAFE_INTEGER (please use an alternative input type)' - ) - } - if (typeof chainId === 'number' && !Number.isSafeInteger(chainId)) { - throw new Error( - 'The provided chainId is greater than MAX_SAFE_INTEGER (please use an alternative input type)' - ) - } -} - /** * ECDSA public key recovery from signature. * @returns Recovered public key @@ -106,8 +66,6 @@ export const ecrecover = function( s: Buffer, chainId?: BNLike ): Buffer { - vAndChainIdTypeChecks(v, chainId) - const signature = Buffer.concat([setLengthLeft(r, 32), setLengthLeft(s, 32)], 64) const recovery = calculateSigRecovery(v, chainId) if (!isValidSigRecovery(recovery)) { @@ -122,7 +80,6 @@ export const ecrecover = function( * @returns Signature */ export const toRpcSig = function(v: BNLike, r: Buffer, s: Buffer, chainId?: BNLike): string { - vAndChainIdTypeChecks(v, chainId) const recovery = calculateSigRecovery(v, chainId) if (!isValidSigRecovery(recovery)) { throw new Error('Invalid signature v value') @@ -167,7 +124,6 @@ export const isValidSignature = function( homesteadOrLater: boolean = true, chainId?: BNLike ): boolean { - vAndChainIdTypeChecks(v, chainId) const SECP256K1_N_DIV_2 = new BN( '7fffffffffffffffffffffffffffffff5d576e7357a4501ddfe92f46681b20a0', 16 @@ -182,8 +138,8 @@ export const isValidSignature = function( return false } - const rBN: BN = new BN(r) - const sBN: BN = new BN(s) + const rBN = new BN(r) + const sBN = new BN(s) if (rBN.isZero() || rBN.gt(SECP256K1_N) || sBN.isZero() || sBN.gt(SECP256K1_N)) { return false diff --git a/src/types.ts b/src/types.ts index 3afa856a..511b7e48 100644 --- a/src/types.ts +++ b/src/types.ts @@ -1,6 +1,7 @@ import BN from 'bn.js' +import { isHexString } from 'ethjs-util' import { Address } from './address' -import { unpadBuffer } from './bytes' +import { unpadBuffer, toBuffer, ToBufferInputTypes } from './bytes' /* * A type that represents a BNLike input that can be converted to a BN. @@ -62,3 +63,51 @@ export function bnToRlp(value: BN): Buffer { // for compatibility with browserify and similar tools return unpadBuffer(value.toArrayLike(Buffer)) } + +/** + * Type output options + */ +export enum TypeOutput { + Number, + BN, + Buffer, + PrefixedHexString +} + +export type TypeOutputReturnType = { + [TypeOutput.Number]: Number + [TypeOutput.BN]: BN + [TypeOutput.Buffer]: Buffer + [TypeOutput.PrefixedHexString]: PrefixedHexString +} + +/** + * Convert an input to a specified type + * @param input value to convert + * @param outputType type to output + */ +export function toType( + input: ToBufferInputTypes, + outputType: T +): TypeOutputReturnType[T] { + if (typeof input === 'string' && !isHexString(input)) { + throw new Error(`A string must be provided with a 0x-prefix, given: ${input}`) + } else if (typeof input === 'number' && !Number.isSafeInteger(input)) { + throw new Error( + 'The provided number is greater than MAX_SAFE_INTEGER (please use an alternative input type)' + ) + } + + input = toBuffer(input) + + if (outputType === TypeOutput.Buffer) { + return input as any + } else if (outputType === TypeOutput.BN) { + return new BN(input) as any + } else if (outputType === TypeOutput.Number) { + return new BN(input).toNumber() as any + } else { + // outputType === TypeOutput.PrefixedHexString + return `0x${input.toString('hex')}` as any + } +} diff --git a/test/account.spec.ts b/test/account.spec.ts index a5e81aa0..469ef129 100644 --- a/test/account.spec.ts +++ b/test/account.spec.ts @@ -621,7 +621,15 @@ describe('.toChecksumAddress()', function() { addr ) const chainIDNumber = parseInt(chainIDBuffer.toString('hex'), 16) - assert.equal(toChecksumAddress(addr.toLowerCase(), chainIDNumber), addr) + assert.throws( + () => { + toChecksumAddress(addr.toLowerCase(), chainIDNumber) + }, + { + message: + 'The provided number is greater than MAX_SAFE_INTEGER (please use an alternative input type)' + } + ) }) }) diff --git a/test/signature.spec.ts b/test/signature.spec.ts index 972856f5..09dd25d8 100644 --- a/test/signature.spec.ts +++ b/test/signature.spec.ts @@ -348,8 +348,8 @@ describe('message sig', function() { assert.equal(toRpcSig(27, r, s), sig) assert.deepEqual(fromRpcSig(sig), { v: 27, - r: r, - s: s + r, + s }) }) @@ -372,8 +372,8 @@ describe('message sig', function() { ) assert.deepEqual(fromRpcSig(sig), { v, - r: r, - s: s + r, + s }) }) From 059ce3b12d4130a790b198e507f681298e209e49 Mon Sep 17 00:00:00 2001 From: Ryan Ghods Date: Wed, 3 Mar 2021 13:08:31 -0800 Subject: [PATCH 09/11] add tests for `toType` add error if output number is greater than MAX_SAFE_INTEGER --- src/types.ts | 11 ++++- test/types.spec.ts | 112 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 121 insertions(+), 2 deletions(-) create mode 100644 test/types.spec.ts diff --git a/src/types.ts b/src/types.ts index 511b7e48..bdca4baf 100644 --- a/src/types.ts +++ b/src/types.ts @@ -75,7 +75,7 @@ export enum TypeOutput { } export type TypeOutputReturnType = { - [TypeOutput.Number]: Number + [TypeOutput.Number]: number [TypeOutput.BN]: BN [TypeOutput.Buffer]: Buffer [TypeOutput.PrefixedHexString]: PrefixedHexString @@ -105,7 +105,14 @@ export function toType( } else if (outputType === TypeOutput.BN) { return new BN(input) as any } else if (outputType === TypeOutput.Number) { - return new BN(input).toNumber() as any + const bn = new BN(input) + const max = new BN(Number.MAX_SAFE_INTEGER.toString()) + if (bn.gt(max)) { + throw new Error( + 'The provided number is greater than MAX_SAFE_INTEGER (please use an alternative output type)' + ) + } + return bn.toNumber() as any } else { // outputType === TypeOutput.PrefixedHexString return `0x${input.toString('hex')}` as any diff --git a/test/types.spec.ts b/test/types.spec.ts new file mode 100644 index 00000000..16ae5f31 --- /dev/null +++ b/test/types.spec.ts @@ -0,0 +1,112 @@ +import assert from 'assert' +import BN from 'bn.js' +import { toType, TypeOutput, intToBuffer, bufferToHex, intToHex, bnToHex, toBuffer } from '../src' + +describe('toType', function() { + describe('from Number', function() { + const num = 1000 + it('should convert to Number', function() { + const result = toType(num, TypeOutput.Number) + assert.strictEqual(result, num) + }) + it('should convert to BN', function() { + const result = toType(num, TypeOutput.BN) + assert.ok(result.eq(new BN(num))) + }) + it('should convert to Buffer', function() { + const result = toType(num, TypeOutput.Buffer) + assert.ok(result.equals(intToBuffer(num))) + }) + it('should convert to PrefixedHexString', function() { + const result = toType(num, TypeOutput.PrefixedHexString) + assert.strictEqual(result, bufferToHex(new BN(num).toArrayLike(Buffer))) + }) + it('should throw an error if greater than MAX_SAFE_INTEGER', function() { + assert.throws( + () => { + const num = Number.MAX_SAFE_INTEGER + 1 + toType(num, TypeOutput.BN) + }, + { + message: + 'The provided number is greater than MAX_SAFE_INTEGER (please use an alternative input type)' + } + ) + }) + }) + describe('from BN', function() { + const num = new BN(1000) + it('should convert to Number', function() { + const result = toType(num, TypeOutput.Number) + assert.strictEqual(result, num.toNumber()) + }) + it('should convert to BN', function() { + const result = toType(num, TypeOutput.BN) + assert.ok(result.eq(num)) + }) + it('should convert to Buffer', function() { + const result = toType(num, TypeOutput.Buffer) + assert.ok(result.equals(num.toArrayLike(Buffer))) + }) + it('should convert to PrefixedHexString', function() { + const result = toType(num, TypeOutput.PrefixedHexString) + assert.strictEqual(result, bufferToHex(num.toArrayLike(Buffer))) + }) + it('should throw an error if converting to Number and greater than MAX_SAFE_INTEGER', function() { + const num = new BN(Number.MAX_SAFE_INTEGER).addn(1) + assert.throws( + () => { + toType(num, TypeOutput.Number) + }, + { + message: + 'The provided number is greater than MAX_SAFE_INTEGER (please use an alternative output type)' + } + ) + }) + }) + describe('from Buffer', function() { + const num = intToBuffer(1000) + it('should convert to Number', function() { + const result = toType(num, TypeOutput.Number) + assert.ok(intToBuffer(result).equals(num)) + }) + it('should convert to BN', function() { + const result = toType(num, TypeOutput.BN) + assert.ok(result.eq(new BN(num))) + }) + it('should convert to Buffer', function() { + const result = toType(num, TypeOutput.Buffer) + assert.ok(result.equals(num)) + }) + it('should convert to PrefixedHexString', function() { + const result = toType(num, TypeOutput.PrefixedHexString) + assert.strictEqual(result, bufferToHex(num)) + }) + }) + describe('from HexPrefixedString', function() { + const num = intToHex(1000) + it('should convert to Number', function() { + const result = toType(num, TypeOutput.Number) + assert.strictEqual(intToHex(result), num) + }) + it('should convert to BN', function() { + const result = toType(num, TypeOutput.BN) + assert.strictEqual(bnToHex(result), num) + }) + it('should convert to Buffer', function() { + const result = toType(num, TypeOutput.Buffer) + assert.ok(result.equals(toBuffer(num))) + }) + it('should throw an error if is not 0x-prefixed', function() { + assert.throws( + () => { + toType('1', TypeOutput.Number) + }, + { + message: 'A string must be provided with a 0x-prefix, given: 1' + } + ) + }) + }) +}) From db4ee779a50adc8173fb202708c22f278bad1206 Mon Sep 17 00:00:00 2001 From: Ryan Ghods Date: Wed, 3 Mar 2021 13:08:39 -0800 Subject: [PATCH 10/11] improve ecsign logic --- src/signature.ts | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/src/signature.ts b/src/signature.ts index 9083f104..31424a36 100644 --- a/src/signature.ts +++ b/src/signature.ts @@ -28,17 +28,24 @@ export function ecsign(msgHash: Buffer, privateKey: Buffer, chainId: any): any { const r = Buffer.from(signature.slice(0, 32)) const s = Buffer.from(signature.slice(32, 64)) - if (!chainId) { - return { r, s, v: recovery + 27 } - } else { - const chainIdBN = toType(chainId, TypeOutput.BN) - const vValue = chainIdBN - .muln(2) - .addn(35) - .addn(recovery) - const v = typeof chainId === 'number' ? vValue.toNumber() : vValue.toArrayLike(Buffer) + if (!chainId || typeof chainId === 'number') { + // return legacy type ECDSASignature (deprecated in favor of ECDSASignatureBuffer to handle large chainIds) + if (chainId && !Number.isSafeInteger(chainId)) { + throw new Error( + 'The provided number is greater than MAX_SAFE_INTEGER (please use an alternative input type)' + ) + } + const v = chainId ? recovery + (chainId * 2 + 35) : recovery + 27 return { r, s, v } } + + const chainIdBN = toType(chainId, TypeOutput.BN) + const v = chainIdBN + .muln(2) + .addn(35) + .addn(recovery) + .toArrayLike(Buffer) + return { r, s, v } } function calculateSigRecovery(v: BNLike, chainId?: BNLike): BN { From 46415c1e43d3f51989412055292a32915e7e2173 Mon Sep 17 00:00:00 2001 From: Ryan Ghods Date: Wed, 3 Mar 2021 13:08:57 -0800 Subject: [PATCH 11/11] dev ex: remove lint from `npm run test` and add as a separate step to ci --- .github/workflows/build.yml | 2 ++ package.json | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 1f5ce9e0..98993c3c 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -22,6 +22,8 @@ jobs: - uses: actions/checkout@v2 - run: npm install + + - run: npm run lint - run: npm run test - uses: codecov/codecov-action@v1 diff --git a/package.json b/package.json index 6b8f355a..1692b692 100644 --- a/package.json +++ b/package.json @@ -16,7 +16,7 @@ "docs:build": "npx typedoc --options typedoc.js", "lint": "ethereumjs-config-lint", "lint:fix": "ethereumjs-config-lint-fix", - "test": "npm run lint && npm run test:node && npm run test:browser", + "test": "npm run test:node && npm run test:browser", "test:browser": "karma start karma.conf.js", "test:node": "nyc --reporter=lcov mocha --require ts-node/register 'test/*.spec.ts'", "tsc": "ethereumjs-config-tsc"