normalize transaction chain id to handle non-standard chain ids and r… (#1411)

Co-authored-by: goosewobbler <432005+goosewobbler@users.noreply.github.com>

Author: mholtzman

main/api/http.ts

@@ -1,5 +1,6 @@
 import http, { IncomingMessage, ServerResponse } from 'http'
 import log from 'electron-log'
+import { isHexString } from '@ethereumjs/util'
 
 import provider from '../provider'
 import accounts from '../accounts'
@@ -86,10 +87,20 @@ const handler = (req: IncomingMessage, res: ServerResponse) => {
           )
 
         const origin = parseOrigin(req.headers.origin)
-        const { payload } = updateOrigin(rawPayload, origin)
+        const { payload, chainId } = updateOrigin(rawPayload, origin)
 
         extendSession(payload._origin)
 
+        if (!isHexString(chainId)) {
+          const error = {
+            message: `Invalid chain id (${rawPayload.chainId}), chain id must be hex-prefixed string`,
+            code: -1
+          }
+
+          res.writeHead(401, { 'Content-Type': 'application/json' })
+          return res.end(JSON.stringify({ id: payload.id, jsonrpc: payload.jsonrpc, error }))
+        }
+
         if (protectedMethods.indexOf(payload.method) > -1 && !(await isTrusted(payload))) {
           let error = { message: `Permission denied, approve ${origin} in Frame to continue`, code: 4001 }
           // Review

main/api/ws.ts

@@ -1,6 +1,8 @@
+import { IncomingMessage, Server } from 'http'
 import WebSocket from 'ws'
 import { v4 as uuid } from 'uuid'
 import log from 'electron-log'
+import { isHexString } from '@ethereumjs/util'
 
 import store from '../store'
 import provider from '../provider'
@@ -17,7 +19,6 @@ import {
 } from './origins'
 import validPayload from './validPayload'
 import protectedMethods from './protectedMethods'
-import { IncomingMessage, Server } from 'http'
 
 const logTraffic = process.env.LOG_TRAFFIC
 
@@ -98,6 +99,14 @@ const handler = (socket: FrameWebSocket, req: IncomingMessage) => {
 
     const { payload, chainId } = updateOrigin(rawPayload, origin, rawPayload.__extensionConnecting)
 
+    if (!isHexString(chainId)) {
+      const error = {
+        message: `Invalid chain id (${rawPayload.chainId}), chain id must be hex-prefixed string`,
+        code: -1
+      }
+      return res({ id: rawPayload.id, jsonrpc: rawPayload.jsonrpc, error })
+    }
+
     if (!rawPayload.__extensionConnecting) {
       extendSession(payload._origin)
     }

main/provider/index.ts

@@ -19,7 +19,7 @@ import accounts, {
 } from '../accounts'
 import Chains, { Chain } from '../chains'
 import { getSignerType, Type as SignerType } from '../../resources/domain/signer'
-import { TransactionData } from '../../resources/domain/transaction'
+import { normalizeChainId, TransactionData } from '../../resources/domain/transaction'
 import { populate as populateTransaction, maxFee, classifyTransaction } from '../transaction'
 import FrameAccount from '../accounts/Account'
 import { capitalize } from '../../resources/utils'
@@ -449,14 +449,23 @@ export class Provider extends EventEmitter {
   }
 
   async fillTransaction(newTx: RPC.SendTransaction.TxParams, cb: Callback<TransactionMetadata>) {
-    if (!newTx) return cb(new Error('No transaction data'))
+    if (!newTx) {
+      return cb(new Error('No transaction data'))
+    }
+
+    const connection = this.connection.connections['ethereum'][parseInt(newTx.chainId, 16)]
+    const chainConnected = connection && (connection.primary?.connected || connection.secondary?.connected)
+
+    if (!chainConnected) {
+      return cb(new Error(`Chain ${newTx.chainId} not connected`))
+    }
 
     try {
       const approvals: RequiredApproval[] = []
       const accountId = (accounts.current() || {}).id
       const rawTx = getRawTx(newTx, accountId)
       const gas = gasFees(rawTx)
-      const chainConfig = this.connection.connections['ethereum'][parseInt(rawTx.chainId)].chainConfig
+      const { chainConfig } = connection
 
       const estimateGasLimit = async () => {
         try {
@@ -497,69 +506,65 @@ export class Provider extends EventEmitter {
   }
 
   sendTransaction(payload: RPC.SendTransaction.Request, res: RPCRequestCallback, targetChain: Chain) {
-    const txParams = payload.params[0]
-    const payloadChain = payload.chainId
-    const txChain = txParams.chainId
-
-    if (payloadChain && txChain && parseInt(payloadChain, 16) !== parseInt(txChain, 16)) {
-      return resError(
-        `Chain for transaction (${txChain}) does not match request target chain (${payloadChain})`,
-        payload,
-        res
-      )
-    }
+    try {
+      const txParams = payload.params[0]
+      const payloadChain = payload.chainId
 
-    const newTx = {
-      ...txParams,
-      chainId: txChain || payloadChain || addHexPrefix(targetChain.id.toString(16))
-    }
+      const normalizedTx = normalizeChainId(txParams, payloadChain ? parseInt(payloadChain, 16) : undefined)
+      const tx = {
+        ...normalizedTx,
+        chainId: normalizedTx.chainId || payloadChain || addHexPrefix(targetChain.id.toString(16))
+      }
 
-    const currentAccount = accounts.current()
+      const currentAccount = accounts.current()
 
-    log.verbose(`sendTransaction(${JSON.stringify(newTx)}`)
+      log.verbose(`sendTransaction(${JSON.stringify(tx)}`)
 
-    this.fillTransaction(newTx, (err, transactionMetadata) => {
-      if (err) {
-        resError(err, payload, res)
-      } else {
-        const txMetadata = transactionMetadata as TransactionMetadata
-        const from = txMetadata.tx.from
+      this.fillTransaction(tx, (err, transactionMetadata) => {
+        if (err) {
+          resError(err, payload, res)
+        } else {
+          const txMetadata = transactionMetadata as TransactionMetadata
+          const from = txMetadata.tx.from
 
-        if (!currentAccount || !hasAddress(currentAccount, from)) {
-          return resError('Transaction is not from currently selected account', payload, res)
-        }
+          if (!currentAccount || !hasAddress(currentAccount, from)) {
+            return resError('Transaction is not from currently selected account', payload, res)
+          }
 
-        const handlerId = this.addRequestHandler(res)
+          const handlerId = this.addRequestHandler(res)
 
-        const { feesUpdated, recipientType, ...data } = txMetadata.tx
+          const { feesUpdated, recipientType, ...data } = txMetadata.tx
 
-        const unclassifiedReq = {
-          handlerId,
-          type: 'transaction',
-          data,
-          payload,
-          account: (currentAccount as FrameAccount).id,
-          origin: payload._origin,
-          approvals: [],
-          feesUpdatedByUser: false,
-          recipientType,
-          recognizedActions: []
-        } as Omit<TransactionRequest, 'classification'>
+          const unclassifiedReq = {
+            handlerId,
+            type: 'transaction',
+            data,
+            payload,
+            account: (currentAccount as FrameAccount).id,
+            origin: payload._origin,
+            approvals: [],
+            feesUpdatedByUser: false,
+            recipientType,
+            recognizedActions: []
+          } as Omit<TransactionRequest, 'classification'>
 
-        const classification = classifyTransaction(unclassifiedReq)
+          const classification = classifyTransaction(unclassifiedReq)
 
-        const req = {
-          ...unclassifiedReq,
-          classification
-        }
+          const req = {
+            ...unclassifiedReq,
+            classification
+          }
 
-        accounts.addRequest(req, res)
+          accounts.addRequest(req, res)
 
-        txMetadata.approvals.forEach((approval) => {
-          currentAccount?.addRequiredApproval(req, approval.type, approval.data)
-        })
-      }
-    })
+          txMetadata.approvals.forEach((approval) => {
+            currentAccount?.addRequiredApproval(req, approval.type, approval.data)
+          })
+        }
+      })
+    } catch (e) {
+      resError((e as Error).message, payload, res)
+    }
   }
 
   getTransactionByHash(payload: RPCRequestPayload, cb: RPCRequestCallback, targetChain: Chain) {

resources/domain/transaction/index.ts

@@ -1,4 +1,5 @@
 import { JsonTx } from '@ethereumjs/tx'
+import { addHexPrefix, isHexString } from '@ethereumjs/util'
 
 export enum GasFeesSource {
   Dapp = 'Dapp',
@@ -23,3 +24,33 @@ export function typeSupportsBaseFee(type: string) {
 export function usesBaseFee(rawTx: TransactionData) {
   return typeSupportsBaseFee(rawTx.type)
 }
+
+function parseChainId(chainId: string) {
+  if (isHexString(chainId)) {
+    return parseInt(chainId, 16)
+  }
+
+  return Number(chainId)
+}
+
+// TODO: move this into requests parsing module
+export function normalizeChainId(tx: RPC.SendTransaction.TxParams, targetChain?: number) {
+  if (!tx.chainId) return tx
+
+  const chainId = parseChainId(tx.chainId)
+
+  if (!chainId) {
+    throw new Error(`Chain for transaction (${tx.chainId}) is not a hex-prefixed string`)
+  }
+
+  if (targetChain && targetChain !== chainId) {
+    throw new Error(
+      `Chain for transaction (${tx.chainId}) does not match request target chain (${targetChain})`
+    )
+  }
+
+  return {
+    ...tx,
+    chainId: addHexPrefix(chainId.toString(16))
+  }
+}

test/main/provider/index.test.js

@@ -807,6 +807,9 @@ describe('#send', () => {
         })
 
         connection.connections.ethereum[chainId] = {
+          primary: {
+            connected: true
+          },
           chainConfig: chainConfig(chainId, chainId === 1 ? 'london' : 'istanbul')
         }
       })
@@ -886,27 +889,6 @@ describe('#send', () => {
     })
 
     describe('replacing gas fees', () => {
-      beforeEach(() => {
-        const chainIds = [1, 137]
-
-        chainIds.forEach((chainId) => {
-          store.set('main.networksMeta.ethereum', chainId, 'gas', {
-            price: {
-              selected: 'standard',
-              levels: { slow: '', standard: '', fast: gweiToHex(30), asap: '', custom: '' },
-              fees: {
-                maxPriorityFeePerGas: gweiToHex(1),
-                maxBaseFeePerGas: gweiToHex(8)
-              }
-            }
-          })
-
-          connection.connections.ethereum[chainId] = {
-            chainConfig: chainConfig(chainId, chainId === 1 ? 'london' : 'istanbul')
-          }
-        })
-      })
-
       it('adds a 10% gas buffer when replacing a legacy transaction', (done) => {
         tx.type = '0x0'
         tx.chainId = addHexPrefix((137).toString(16))

test/resources/domain/transaction/index.test.js

@@ -1,4 +1,9 @@
-import { typeSupportsBaseFee, usesBaseFee } from '../../../../resources/domain/transaction'
+import {
+  normalizeChainId,
+  parseChainId,
+  typeSupportsBaseFee,
+  usesBaseFee
+} from '../../../../resources/domain/transaction'
 
 describe('#typeSupportsBaseFee', () => {
   it('does not support a base fee for type 0', () => {
@@ -39,3 +44,43 @@ describe('#usesBaseFee', () => {
     expect(usesBaseFee(tx)).toBe(true)
   })
 })
+
+describe('#normalizeChainId', () => {
+  it('does not modify a transaction with no chain id', () => {
+    const tx = { to: '0xframe' }
+
+    expect(normalizeChainId(tx)).toStrictEqual(tx)
+  })
+
+  it('normalizes a hex-prefixed chain id', () => {
+    const tx = { to: '0xframe', chainId: '0xa' }
+
+    expect(normalizeChainId(tx)).toStrictEqual({ to: '0xframe', chainId: '0xa' })
+  })
+
+  it('does not handle a hex chain id with no prefix', () => {
+    const tx = { to: '0xframe', chainId: 'a' }
+
+    expect(() => normalizeChainId(tx)).toThrowError(/chain for transaction.*is not a hex-prefixed string/i)
+  })
+
+  it('normalizes a numeric chain id', () => {
+    const tx = { to: '0xframe', chainId: 14 }
+
+    expect(normalizeChainId(tx)).toStrictEqual({ to: '0xframe', chainId: '0xe' })
+  })
+
+  it('normalizes a numeric string chain id', () => {
+    const tx = { to: '0xframe', chainId: '100' }
+
+    expect(normalizeChainId(tx)).toStrictEqual({ to: '0xframe', chainId: '0x64' })
+  })
+
+  it('does not allow a chain id that does not match the target chain', () => {
+    const tx = { to: '0xframe', chainId: '0xa' }
+
+    expect(() => normalizeChainId(tx, 11)).toThrowError(
+      /chain for transaction.*does not match request target chain/i
+    )
+  })
+})