Merge pull request #290 from ethereumjs/fix-ecrecover

signature/address: support for high recovery and chain IDs

Author: holgerd77

.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

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"

src/account.ts

@@ -6,7 +6,7 @@ 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,
@@ -137,11 +137,15 @@ 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)
   const address = stripHexPrefix(hexAddress).toLowerCase()
 
-  const prefix = eip1191ChainId !== undefined ? eip1191ChainId.toString() + '0x' : ''
+  let prefix = ''
+  if (eip1191ChainId) {
+    const chainId = toType(eip1191ChainId, TypeOutput.BN)
+    prefix = chainId.toString() + '0x'
+  }
 
   const hash = keccakFromString(prefix + address).toString('hex')
   let ret = '0x'
@@ -164,7 +168,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
 }

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'
 
 /**
@@ -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:
-    | string
-    | 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)
   }

src/signature.ts

@@ -1,41 +1,65 @@
-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, toType, TypeOutput } from './types'
 
 export interface ECDSASignature {
   v: number
   r: Buffer
   s: Buffer
 }
 
+export interface ECDSASignatureBuffer {
+  v: Buffer
+  r: Buffer
+  s: Buffer
+}
+
 /**
  * Returns the ECDSA signature of a message hash.
  */
-export const ecsign = function(
-  msgHash: Buffer,
-  privateKey: Buffer,
-  chainId?: number
-): ECDSASignature {
-  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
+export function ecsign(msgHash: Buffer, privateKey: Buffer, chainId?: number): ECDSASignature
+export function ecsign(msgHash: Buffer, privateKey: Buffer, chainId: BNLike): ECDSASignatureBuffer
+export function ecsign(msgHash: Buffer, privateKey: Buffer, chainId: any): any {
+  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 || 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 }
   }
 
-  return ret
+  const chainIdBN = toType(chainId, TypeOutput.BN)
+  const v = chainIdBN
+    .muln(2)
+    .addn(35)
+    .addn(recovery)
+    .toArrayLike(Buffer)
+  return { r, s, v }
 }
 
-function calculateSigRecovery(v: number, chainId?: number): number {
-  return chainId ? v - (2 * chainId + 35) : v - 27
+function calculateSigRecovery(v: BNLike, chainId?: BNLike): BN {
+  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): 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,25 +68,25 @@ function isValidSigRecovery(recovery: number): boolean {
  */
 export const ecrecover = function(
   msgHash: Buffer,
-  v: number,
+  v: BNLike,
   r: Buffer,
   s: Buffer,
-  chainId?: number
+  chainId?: BNLike
 ): 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))
 }
 
 /**
  * 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 {
   const recovery = calculateSigRecovery(v, chainId)
   if (!isValidSigRecovery(recovery)) {
     throw new Error('Invalid signature v value')
@@ -101,11 +125,11 @@ 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 {
   const SECP256K1_N_DIV_2 = new BN(
     '7fffffffffffffffffffffffffffffff5d576e7357a4501ddfe92f46681b20a0',
@@ -121,8 +145,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

src/types.ts

@@ -1,11 +1,12 @@
 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.
  */
-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 +29,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.
@@ -62,3 +63,58 @@ 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<T extends TypeOutput>(
+  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) {
+    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
+  }
+}

test/account.spec.ts

@@ -602,9 +602,35 @@ 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.throws(
+        () => {
+          toChecksumAddress(addr.toLowerCase(), chainIDNumber)
+        },
+        {
+          message:
+            'The provided number is greater than MAX_SAFE_INTEGER (please use an alternative input type)'
+        }
+      )
+    })
   })
 
   describe('input format', function() {
@@ -613,6 +639,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 +664,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
+          )
         }
       }
     })