tx/legacyTransaction: add v guard for non-EIP155 txs

ethereumjs · May 30, 2022 · 98e7f36 · 98e7f36
1 parent f2dbdaa
commit 98e7f36
Show file tree

Hide file tree

Showing 2 changed files with 34 additions and 8 deletions.
diff --git a/packages/tx/src/legacyTransaction.ts b/packages/tx/src/legacyTransaction.ts
@@ -136,6 +136,18 @@ export default class Transaction extends BaseTransaction<Transaction> {
       }
     }
 
+    if (!this.supports(Capability.EIP155ReplayProtection)) {
+      // In case of a signed non-EIP155 tx, `ecrecover` does not get a `chainId` provided
+      // Internally, `ecrecover` will calculate the recovery bit by `v = v - 27`
+      // This recovery bit is either 0 or 1. Thus, any other value than `v = 27` or `v = 28` will
+      // throw upon calling `getSenderAddress` here. Thus throw early in the constructor
+      if (this.v !== undefined) {
+        if (!this.v.eqn(27) && !this.v.eqn(28)) {
+          throw new Error(`Non-EIP155 txs need either v = 27 or v = 28, got v = ${this.v}`)
+        }
+      }
+    }
+
     if (this.common.isActivatedEIP(3860)) {
       checkMaxInitCodeSize(this.common, this.data.length)
     }
@@ -382,7 +394,7 @@ export default class Transaction extends BaseTransaction<Transaction> {
     // No unsigned tx and EIP-155 activated and chain ID included
     if (
       v !== undefined &&
-      !v.eqn(0) &&
+      !v.eqn(0) && // TODO fixme -> why overwrite/return default common when `v` is set to 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,24 @@ tape('[Transaction]', function (t) {
     }
   )
 
+  t.test(
+    'constructor: throw on non-EIP155 transactions which have v != 27 and v != 28',
+    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.doesNotThrow(() => Transaction.fromTxData(getTxData(27)))
+      st.doesNotThrow(() => Transaction.fromTxData(getTxData(28)))
+      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 })