TX: fix TX decoding when some values are actually arrays (#2284)

* tx: fix decoding * Add tests for array inputs * Validate keys are valid tx data keys * tx: add more input value checks and fix the checker * Address feedback Co-authored-by: acolytec3 <17355484+acolytec3@users.noreply.github.com>
ethereumjs · Oct 6, 2022 · 1fbc077 · 1fbc077
1 parent a7bb5ba
commit 1fbc077
Show file tree

Hide file tree

Showing 5 changed files with 150 additions and 2 deletions.
diff --git a/packages/tx/src/baseTransaction.ts b/packages/tx/src/baseTransaction.ts
@@ -451,6 +451,31 @@ export abstract class BaseTransaction<TransactionObject> {
     }
   }
 
+  protected static _validateNotArray(values: { [key: string]: any }) {
+    const txDataKeys = [
+      'nonce',
+      'gasPrice',
+      'gasLimit',
+      'to',
+      'value',
+      'data',
+      'v',
+      'r',
+      's',
+      'type',
+      'baseFee',
+      'maxFeePerGas',
+      'chainId',
+    ]
+    for (const [key, value] of Object.entries(values)) {
+      if (txDataKeys.includes(key)) {
+        if (Array.isArray(value)) {
+          throw new Error(`${key} cannot be an array`)
+        }
+      }
+    }
+  }
+
   /**
    * Return a compact error string representation of the object
    */

diff --git a/packages/tx/src/eip1559Transaction.ts b/packages/tx/src/eip1559Transaction.ts
@@ -117,6 +117,7 @@ export class FeeMarketEIP1559Transaction extends BaseTransaction<FeeMarketEIP155
       s,
     ] = values
 
+    this._validateNotArray({ chainId, v })
     validateNoLeadingZeroes({ nonce, maxPriorityFeePerGas, maxFeePerGas, gasLimit, value, v, r, s })
 
     return new FeeMarketEIP1559Transaction(
@@ -174,6 +175,8 @@ export class FeeMarketEIP1559Transaction extends BaseTransaction<FeeMarketEIP155
       maxPriorityFeePerGas: this.maxPriorityFeePerGas,
     })
 
+    BaseTransaction._validateNotArray(txData)
+
     if (this.gasLimit * this.maxFeePerGas > MAX_INTEGER) {
       const msg = this._errorMsg('gasLimit * maxFeePerGas cannot exceed MAX_INTEGER (2^256-1)')
       throw new Error(msg)

diff --git a/packages/tx/src/eip2930Transaction.ts b/packages/tx/src/eip2930Transaction.ts
@@ -103,6 +103,7 @@ export class AccessListEIP2930Transaction extends BaseTransaction<AccessListEIP2
 
     const [chainId, nonce, gasPrice, gasLimit, to, value, data, accessList, v, r, s] = values
 
+    this._validateNotArray({ chainId, v })
     validateNoLeadingZeroes({ nonce, gasPrice, gasLimit, value, v, r, s })
 
     const emptyAccessList: AccessList = []
@@ -158,6 +159,8 @@ export class AccessListEIP2930Transaction extends BaseTransaction<AccessListEIP2
       gasPrice: this.gasPrice,
     })
 
+    BaseTransaction._validateNotArray(txData)
+
     if (this.gasPrice * this.gasLimit > MAX_INTEGER) {
       const msg = this._errorMsg('gasLimit * gasPrice cannot exceed MAX_INTEGER')
       throw new Error(msg)

diff --git a/packages/tx/src/legacyTransaction.ts b/packages/tx/src/legacyTransaction.ts
@@ -116,6 +116,7 @@ export class Transaction extends BaseTransaction<Transaction> {
       throw new Error(msg)
     }
     this._validateCannotExceedMaxInteger({ gasPrice: this.gasPrice })
+    BaseTransaction._validateNotArray(txData)
 
     if (this.common.gteHardfork('spuriousDragon')) {
       if (!this.isSigned()) {

diff --git a/packages/tx/test/inputValue.spec.ts b/packages/tx/test/inputValue.spec.ts
@@ -2,8 +2,18 @@ import { Chain, Common, Hardfork } from '@ethereumjs/common'
 import { Address, toBuffer } from '@ethereumjs/util'
 import * as tape from 'tape'
 
-import { Transaction } from '../src'
-
+import {
+  AccessListEIP2930Transaction,
+  FeeMarketEIP1559Transaction,
+  Transaction,
+  TransactionFactory,
+} from '../src'
+
+import type {
+  AccessListEIP2930ValuesArray,
+  FeeMarketEIP1559ValuesArray,
+  TxValuesArray,
+} from '../src'
 import type { AddressLike, BigIntLike, BufferLike } from '@ethereumjs/util'
 
 // @returns: Array with subtypes of the AddressLike type for a given address
@@ -136,3 +146,109 @@ tape('[Transaction Input Values]', function (t) {
     st.end()
   })
 })
+
+tape('[Invalid Array Input values]', (t) => {
+  const txTypes = [0x0, 0x1, 0x2]
+  for (const signed of [false, true]) {
+    for (const txType of txTypes) {
+      let tx = TransactionFactory.fromTxData({ type: txType })
+      if (signed) {
+        tx = tx.sign(Buffer.from('42'.repeat(32), 'hex'))
+      }
+      const rawValues = tx.raw()
+      for (let x = 0; x < rawValues.length; x++) {
+        rawValues[x] = <any>[1, 2, 3]
+        switch (txType) {
+          case 0:
+            t.throws(() => Transaction.fromValuesArray(rawValues as TxValuesArray))
+            break
+          case 1:
+            t.throws(() =>
+              AccessListEIP2930Transaction.fromValuesArray(
+                rawValues as AccessListEIP2930ValuesArray
+              )
+            )
+            break
+          case 2:
+            t.throws(() =>
+              FeeMarketEIP1559Transaction.fromValuesArray(rawValues as FeeMarketEIP1559ValuesArray)
+            )
+            break
+        }
+      }
+    }
+  }
+  t.end()
+})
+
+tape('[Invalid Access Lists]', (t) => {
+  const txTypes = [0x1, 0x2]
+  const invalidAccessLists = [
+    [[]], // does not have an address and does not have slots
+    [[[], []]], // the address is an array
+    [['0xde0b295669a9fd93d5f28d9ec85e40f4cb697bae']], // there is no storage slot array
+    [
+      [
+        '0xde0b295669a9fd93d5f28d9ec85e40f4cb697bae',
+        ['0x0000000000000000000000000000000000000000000000000000000000000003', []],
+      ],
+    ], // one of the slots is an array
+    [
+      [
+        '0xde0b295669a9fd93d5f28d9ec85e40f4cb697bae',
+        ['0x0000000000000000000000000000000000000000000000000000000000000003'],
+        '0xab',
+      ],
+    ], // extra field
+    [
+      '0xde0b295669a9fd93d5f28d9ec85e40f4cb697bae',
+      ['0x0000000000000000000000000000000000000000000000000000000000000003'],
+    ], // account/slot needs to be encoded in a deeper array layer
+  ]
+  for (const signed of [false, true]) {
+    for (const txType of txTypes) {
+      for (const invalidAccessListItem of invalidAccessLists) {
+        let tx: any
+        try {
+          tx = TransactionFactory.fromTxData({
+            type: txType,
+            accessList: <any>invalidAccessListItem,
+          })
+          if (signed) {
+            tx = tx.sign(Buffer.from('42'.repeat(32), 'hex'))
+          }
+          t.fail('did not fail on `fromTxData`')
+        } catch (e: any) {
+          t.pass('failed ok on decoding in `fromTxData`')
+          tx = TransactionFactory.fromTxData({ type: txType })
+          if (signed) {
+            tx = tx.sign(Buffer.from('42'.repeat(32), 'hex'))
+          }
+        }
+        const rawValues = tx!.raw()
+
+        if (txType === 1 && rawValues[7].length === 0) {
+          rawValues[7] = invalidAccessListItem
+        } else if (txType === 2 && rawValues[8].length === 0) {
+          rawValues[8] = invalidAccessListItem
+        }
+
+        switch (txType) {
+          case 1:
+            t.throws(() =>
+              AccessListEIP2930Transaction.fromValuesArray(
+                rawValues as AccessListEIP2930ValuesArray
+              )
+            )
+            break
+          case 2:
+            t.throws(() =>
+              FeeMarketEIP1559Transaction.fromValuesArray(rawValues as FeeMarketEIP1559ValuesArray)
+            )
+            break
+        }
+      }
+    }
+  }
+  t.end()
+})