Util: Allow v to be 0 or 1 for EIP1559 transactions (#1905)

* Util: Allow v to be `0` or `1` for EIP1559 transactions * vm: fix ecrecover precompile for v=0 and v=1 * tx/legacyTransaction: add `v` guard for non-EIP155 txs * tx: switch to earlier v validation and throwing before v common/EIP-155 determination logic * tx: remove v==0 check which always defaults to the default common Co-authored-by: Jochem Brouwer <[email protected]> Co-authored-by: Holger Drewes <[email protected]>
ethereumjs · May 31, 2022 · 30ca12e · 30ca12e
1 parent 2f42dcf
commit 30ca12e
Show file tree

Hide file tree

Showing 5 changed files with 103 additions and 9 deletions.
diff --git a/packages/tx/src/legacyTransaction.ts b/packages/tx/src/legacyTransaction.ts
@@ -378,11 +378,21 @@ export default class Transaction extends BaseTransaction<Transaction> {
    * Validates tx's `v` value
    */
   private _validateTxV(v?: BN, common?: Common): Common {
+    // Check for valid v values in the scope of a signed legacy tx
+    if (v !== undefined) {
+      // v is 1. not matching the EIP-155 chainId included case and...
+      // v is 2. not matching the classic v=27 or v=28 case
+      if (v.ltn(37) && !v.eqn(27) && !v.eqn(28)) {
+        throw new Error(
+          `Legacy txs need either v = 27/28 or v >= 37 (EIP-155 replay protection), got v = ${v}`
+        )
+      }
+    }
+
     let chainIdBN
     // No unsigned tx and EIP-155 activated and chain ID included
     if (
       v !== undefined &&
-      !v.eqn(0) &&
       (!common || common.gteHardfork('spuriousDragon')) &&
       !v.eqn(27) &&
       !v.eqn(28)

diff --git a/packages/tx/test/legacy.spec.ts b/packages/tx/test/legacy.spec.ts
@@ -389,8 +389,6 @@ tape('[Transaction]', function (t) {
         'hex'
       )
 
-      const fixtureTxSignedWithEIP155 = Transaction.fromTxData(txData).sign(privateKey)
-
       const common = new Common({
         chain: Chain.Mainnet,
         hardfork: Hardfork.TangerineWhistle,
@@ -400,9 +398,7 @@ tape('[Transaction]', function (t) {
         common,
       }).sign(privateKey)
 
-      let signedWithEIP155 = Transaction.fromTxData(<any>fixtureTxSignedWithEIP155.toJSON()).sign(
-        privateKey
-      )
+      let signedWithEIP155 = Transaction.fromTxData(<any>txData).sign(privateKey)
 
       st.true(signedWithEIP155.verifySignature())
       st.notEqual(signedWithEIP155.v?.toString('hex'), '1c')
@@ -416,7 +412,7 @@ tape('[Transaction]', function (t) {
       st.notEqual(signedWithEIP155.v?.toString('hex'), '1c')
       st.notEqual(signedWithEIP155.v?.toString('hex'), '1b')
 
-      let signedWithoutEIP155 = Transaction.fromTxData(<any>fixtureTxSignedWithEIP155.toJSON(), {
+      let signedWithoutEIP155 = Transaction.fromTxData(<any>txData, {
         common,
       }).sign(privateKey)
 
@@ -427,7 +423,7 @@ tape('[Transaction]', function (t) {
         "v shouldn't be EIP155 encoded"
       )
 
-      signedWithoutEIP155 = Transaction.fromTxData(<any>fixtureTxSignedWithoutEIP155.toJSON(), {
+      signedWithoutEIP155 = Transaction.fromTxData(<any>txData, {
         common,
       }).sign(privateKey)
 
@@ -442,6 +438,27 @@ tape('[Transaction]', function (t) {
     }
   )
 
+  t.test(
+    'constructor: throw on legacy transactions which have v != 27 and v != 28 and v < 37',
+    function (st) {
+      function getTxData(v: number) {
+        return {
+          v,
+        }
+      }
+      for (let n = 0; n < 27; n++) {
+        st.throws(() => Transaction.fromTxData(getTxData(n)))
+      }
+      st.throws(() => Transaction.fromTxData(getTxData(29)))
+      st.throws(() => Transaction.fromTxData(getTxData(36)))
+
+      st.doesNotThrow(() => Transaction.fromTxData(getTxData(27)))
+      st.doesNotThrow(() => Transaction.fromTxData(getTxData(28)))
+      st.doesNotThrow(() => Transaction.fromTxData(getTxData(37)))
+      st.end()
+    }
+  )
+
   t.test('sign(), verifySignature(): sign tx with chainId specified in params', function (st) {
     const common = new Common({ chain: 42, hardfork: Hardfork.Petersburg })
     let tx = Transaction.fromTxData({}, { common })

diff --git a/packages/util/src/signature.ts b/packages/util/src/signature.ts
@@ -46,6 +46,9 @@ export function ecsign(msgHash: Buffer, privateKey: Buffer, chainId: any): any {
 
 function calculateSigRecovery(v: BNLike, chainId?: BNLike): BN {
   const vBN = toType(v, TypeOutput.BN)
+
+  if (vBN.eqn(0) || vBN.eqn(1)) return toType(v, TypeOutput.BN)
+
   if (!chainId) {
     return vBN.subn(27)
   }
@@ -60,6 +63,7 @@ function isValidSigRecovery(recovery: number | BN): boolean {
 
 /**
  * ECDSA public key recovery from signature.
+ * NOTE: Accepts `v == 0 | v == 1` for EIP1559 transactions
  * @returns Recovered public key
  */
 export const ecrecover = function (
@@ -80,6 +84,7 @@ export const ecrecover = function (
 
 /**
  * Convert signature parameters into the format of `eth_sign` RPC method.
+ * NOTE: Accepts `v == 0 | v == 1` for EIP1559 transactions
  * @returns Signature
  */
 export const toRpcSig = function (v: BNLike, r: Buffer, s: Buffer, chainId?: BNLike): string {
@@ -94,6 +99,7 @@ export const toRpcSig = function (v: BNLike, r: Buffer, s: Buffer, chainId?: BNL
 
 /**
  * Convert signature parameters into the format of Compact Signature Representation (EIP-2098).
+ * NOTE: Accepts `v == 0 | v == 1` for EIP1559 transactions
  * @returns Signature
  */
 export const toCompactSig = function (v: BNLike, r: Buffer, s: Buffer, chainId?: BNLike): string {
@@ -115,6 +121,8 @@ export const toCompactSig = function (v: BNLike, r: Buffer, s: Buffer, chainId?:
 /**
  * Convert signature format of the `eth_sign` RPC method to signature parameters
  * NOTE: all because of a bug in geth: https://github.com/ethereum/go-ethereum/issues/2053
+ * NOTE: After EIP1559, `v` could be `0` or `1` but this function assumes
+ * it's a signed message (EIP-191 or EIP-712) adding `27` at the end. Remove if needed.
  */
 export const fromRpcSig = function (sig: string): ECDSASignature {
   const buf: Buffer = toBuffer(sig)
@@ -150,6 +158,7 @@ export const fromRpcSig = function (sig: string): ECDSASignature {
 
 /**
  * Validate a ECDSA signature.
+ * NOTE: Accepts `v == 0 | v == 1` for EIP1559 transactions
  * @param homesteadOrLater Indicates whether this is being used on either the homestead hardfork or a later one
  */
 export const isValidSignature = function (

diff --git a/packages/util/test/signature.spec.ts b/packages/util/test/signature.spec.ts
@@ -150,6 +150,14 @@ tape('ecrecover', function (t) {
     st.ok(pubkey.equals(privateToPublic(ecprivkey)))
     st.end()
   })
+  t.test('should recover a public key (v = 0)', function (st) {
+    const r = Buffer.from('99e71a99cb2270b8cac5254f9e99b6210c6c10224a1579cf389ef88b20a1abe9', 'hex')
+    const s = Buffer.from('129ff05af364204442bdb53ab6f18a99ab48acc9326fa689f228040429e3ca66', 'hex')
+    const v = 0
+    const pubkey = ecrecover(echash, v, r, s)
+    st.ok(pubkey.equals(privateToPublic(ecprivkey)))
+    st.end()
+  })
   t.test('should fail on an invalid signature (v = 21)', function (st) {
     const r = Buffer.from('99e71a99cb2270b8cac5254f9e99b6210c6c10224a1579cf389ef88b20a1abe9', 'hex')
     const s = Buffer.from('129ff05af364204442bdb53ab6f18a99ab48acc9326fa689f228040429e3ca66', 'hex')
@@ -312,6 +320,20 @@ tape('isValidSignature', function (t) {
     st.ok(isValidSignature(v, r, s))
     st.end()
   })
+  t.test('should work otherwise (v=0)', function (st) {
+    const r = Buffer.from('99e71a99cb2270b8cac5254f9e99b6210c6c10224a1579cf389ef88b20a1abe9', 'hex')
+    const s = Buffer.from('129ff05af364204442bdb53ab6f18a99ab48acc9326fa689f228040429e3ca66', 'hex')
+    const v = 0
+    st.ok(isValidSignature(v, r, s))
+    st.end()
+  })
+  t.test('should work otherwise (v=1)', function (st) {
+    const r = Buffer.from('99e71a99cb2270b8cac5254f9e99b6210c6c10224a1579cf389ef88b20a1abe9', 'hex')
+    const s = Buffer.from('129ff05af364204442bdb53ab6f18a99ab48acc9326fa689f228040429e3ca66', 'hex')
+    const v = 1
+    st.ok(isValidSignature(v, r, s))
+    st.end()
+  })
   t.test('should work otherwise (chainId=3)', function (st) {
     const r = Buffer.from('99e71a99cb2270b8cac5254f9e99b6210c6c10224a1579cf389ef88b20a1abe9', 'hex')
     const s = Buffer.from('129ff05af364204442bdb53ab6f18a99ab48acc9326fa689f228040429e3ca66', 'hex')
@@ -396,6 +418,18 @@ tape('message sig', function (t) {
     st.end()
   })
 
+  t.test('should support compact signature representation (EIP-2098) (v=0)', function (st) {
+    const sig =
+      '0x99e71a99cb2270b8cac5254f9e99b6210c6c10224a1579cf389ef88b20a1abe9129ff05af364204442bdb53ab6f18a99ab48acc9326fa689f228040429e3ca66'
+    st.equal(toCompactSig(0, r, s), sig)
+    st.deepEqual(fromRpcSig(sig), {
+      v: 27,
+      r,
+      s,
+    })
+    st.end()
+  })
+
   t.test('should support compact signature representation 2 (EIP-2098)', function (st) {
     const sig =
       '0x99e71a99cb2270b8cac5254f9e99b6210c6c10224a1579cf389ef88b20a1abe9929ff05af364204442bdb53ab6f18a99ab48acc9326fa689f228040429e3ca66'
@@ -408,6 +442,18 @@ tape('message sig', function (t) {
     st.end()
   })
 
+  t.test('should support compact signature representation 2 (EIP-2098) (v=1)', function (st) {
+    const sig =
+      '0x99e71a99cb2270b8cac5254f9e99b6210c6c10224a1579cf389ef88b20a1abe9929ff05af364204442bdb53ab6f18a99ab48acc9326fa689f228040429e3ca66'
+    st.equal(toCompactSig(1, r, s), sig)
+    st.deepEqual(fromRpcSig(sig), {
+      v: 28,
+      r,
+      s,
+    })
+    st.end()
+  })
+
   t.test('should return hex strings that the RPC can use (chainId=150)', function (st) {
     const sig =
       '0x99e71a99cb2270b8cac5254f9e99b6210c6c10224a1579cf389ef88b20a1abe9129ff05af364204442bdb53ab6f18a99ab48acc9326fa689f228040429e3ca66014f'
@@ -478,7 +524,7 @@ tape('message sig', function (t) {
 
   t.test('should throw on invalid v value', function (st) {
     st.throws(function () {
-      toRpcSig(1, r, s)
+      toRpcSig(2, r, s)
     })
     st.end()
   })

diff --git a/packages/vm/src/evm/precompiles/01-ecrecover.ts b/packages/vm/src/evm/precompiles/01-ecrecover.ts
@@ -16,6 +16,18 @@ export default function (opts: PrecompileInput): ExecResult {
 
   const msgHash = data.slice(0, 32)
   const v = data.slice(32, 64)
+  const vBN = new BN(v)
+
+  // Guard against util's `ecrecover`: without providing chainId this will return
+  // a signature in most of the cases in the cases that `v=0` or `v=1`
+  // However, this should throw, only 27 and 28 is allowed as input
+  if (!vBN.eqn(27) && !vBN.eqn(28)) {
+    return {
+      gasUsed,
+      returnValue: Buffer.alloc(0),
+    }
+  }
+
   const r = data.slice(64, 96)
   const s = data.slice(96, 128)