Limited ecsign v return value to Buffer on extended input types, additional input value checks

Author: holgerd77

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)) {

test/signature.spec.ts

@@ -56,33 +56,61 @@ 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')
     })
   })
 })
 
+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(() => {