From a06a2afd272772ed29673ed45b63e2ee848bf548 Mon Sep 17 00:00:00 2001 From: Matt Holtzman Date: Tue, 7 Feb 2023 15:37:39 -0500 Subject: [PATCH 1/4] normalize transaction chain id to handle non-standard chain ids and respond with proper errors --- main/api/http.ts | 13 +- main/api/ws.ts | 9 ++ main/provider/index.ts | 113 +++++++++--------- resources/domain/transaction/index.ts | 31 +++++ test/main/provider/index.test.js | 24 +--- .../domain/transaction/index.test.js | 47 +++++++- 6 files changed, 160 insertions(+), 77 deletions(-) diff --git a/main/api/http.ts b/main/api/http.ts index 72e05d356..14329a2cb 100644 --- a/main/api/http.ts +++ b/main/api/http.ts @@ -8,6 +8,7 @@ import store from '../store' import { updateOrigin, isTrusted, parseOrigin } from './origins' import validPayload from './validPayload' import protectedMethods from './protectedMethods' +import { isHexString } from '@ethereumjs/util' const logTraffic = process.env.LOG_TRAFFIC @@ -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 diff --git a/main/api/ws.ts b/main/api/ws.ts index 3c5763feb..2adcb7df2 100644 --- a/main/api/ws.ts +++ b/main/api/ws.ts @@ -18,6 +18,7 @@ import { import validPayload from './validPayload' import protectedMethods from './protectedMethods' import { IncomingMessage, Server } from 'http' +import { isHexString } from '@ethereumjs/util' 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) } diff --git a/main/provider/index.ts b/main/provider/index.ts index f7831d037..90eacf690 100644 --- a/main/provider/index.ts +++ b/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' @@ -441,14 +441,23 @@ export class Provider extends EventEmitter { } async fillTransaction(newTx: RPC.SendTransaction.TxParams, cb: Callback) { - 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 { @@ -489,69 +498,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 + const unclassifiedReq = { + handlerId, + type: 'transaction', + data, + payload, + account: (currentAccount as FrameAccount).id, + origin: payload._origin, + approvals: [], + feesUpdatedByUser: false, + recipientType, + recognizedActions: [] + } as Omit - 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) { diff --git a/resources/domain/transaction/index.ts b/resources/domain/transaction/index.ts index 16bc38618..39b550e6e 100644 --- a/resources/domain/transaction/index.ts +++ b/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)) + } +} diff --git a/test/main/provider/index.test.js b/test/main/provider/index.test.js index 43adb5ef4..72dc6e2a8 100644 --- a/test/main/provider/index.test.js +++ b/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)) diff --git a/test/resources/domain/transaction/index.test.js b/test/resources/domain/transaction/index.test.js index d96cbe412..001396cc4 100644 --- a/test/resources/domain/transaction/index.test.js +++ b/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 + ) + }) +}) From a3703459a954b71007e7f835043d8b05589104c2 Mon Sep 17 00:00:00 2001 From: Matt Holtzman Date: Tue, 14 Feb 2023 14:09:13 -0500 Subject: [PATCH 2/4] update imports --- main/api/http.ts | 2 +- main/api/ws.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/main/api/http.ts b/main/api/http.ts index 14329a2cb..13444c95a 100644 --- a/main/api/http.ts +++ b/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' @@ -8,7 +9,6 @@ import store from '../store' import { updateOrigin, isTrusted, parseOrigin } from './origins' import validPayload from './validPayload' import protectedMethods from './protectedMethods' -import { isHexString } from '@ethereumjs/util' const logTraffic = process.env.LOG_TRAFFIC diff --git a/main/api/ws.ts b/main/api/ws.ts index 2adcb7df2..6cbe6052e 100644 --- a/main/api/ws.ts +++ b/main/api/ws.ts @@ -1,6 +1,7 @@ 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' @@ -18,7 +19,6 @@ import { import validPayload from './validPayload' import protectedMethods from './protectedMethods' import { IncomingMessage, Server } from 'http' -import { isHexString } from '@ethereumjs/util' const logTraffic = process.env.LOG_TRAFFIC From 997d7a028c14d8d33385f7744e5070a1fabff037 Mon Sep 17 00:00:00 2001 From: goosewobbler <432005+goosewobbler@users.noreply.github.com> Date: Tue, 14 Feb 2023 19:16:19 +0000 Subject: [PATCH 3/4] Update ws.ts --- main/api/ws.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/main/api/ws.ts b/main/api/ws.ts index 6cbe6052e..3d6e13fdf 100644 --- a/main/api/ws.ts +++ b/main/api/ws.ts @@ -1,3 +1,4 @@ +import { IncomingMessage, Server } from 'http' import WebSocket from 'ws' import { v4 as uuid } from 'uuid' import log from 'electron-log' @@ -18,7 +19,7 @@ import { } from './origins' import validPayload from './validPayload' import protectedMethods from './protectedMethods' -import { IncomingMessage, Server } from 'http' + const logTraffic = process.env.LOG_TRAFFIC From 35a5a0f7700d8c357c5787bfbd0866e080489af2 Mon Sep 17 00:00:00 2001 From: goosewobbler <432005+goosewobbler@users.noreply.github.com> Date: Tue, 14 Feb 2023 19:16:58 +0000 Subject: [PATCH 4/4] Update ws.ts --- main/api/ws.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/main/api/ws.ts b/main/api/ws.ts index 3d6e13fdf..10a14a0e9 100644 --- a/main/api/ws.ts +++ b/main/api/ws.ts @@ -20,7 +20,6 @@ import { import validPayload from './validPayload' import protectedMethods from './protectedMethods' - const logTraffic = process.env.LOG_TRAFFIC const subs: Record = {}