From b0890bb72db904cf2d1eb1238f4e9253ac487485 Mon Sep 17 00:00:00 2001 From: Alois Klink Date: Thu, 27 Jan 2022 23:07:24 +0000 Subject: [PATCH 1/4] refactor(ethSignTypedData)!: Improve docs & types BREAKING CHANGE: EthereumSignTypedData now requires passing Trezor T variables. Trezor Model 1 parameters are still optional, as blind signing requires using an extra dependency on an external library, and is less secure. --- CHANGELOG.md | 2 + docs/methods/ethereumSignTypedData.md | 89 +++++++++++++------- src/js/core/methods/EthereumSignTypedData.js | 39 +++++---- src/js/types/__tests__/ethereum.js | 27 ++++++ src/js/types/api.js | 6 +- src/js/types/networks/ethereum.js | 22 +++-- src/ts/types/__tests__/ethereum.ts | 22 +++++ src/ts/types/api.d.ts | 11 ++- src/ts/types/networks/ethereum.d.ts | 11 +++ 9 files changed, 170 insertions(+), 59 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2e8f39872..52359ac3c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,7 +7,9 @@ - Some methods not throwing `ui-device_firmware_unsupported` when the current device firmware didn't support the method. ### Changed + - @trezor/blockchain-link 2.0.0 use workers as commonjs modules in nodejs and react-native env. +- Ethereum: EthereumSignTypedData must always have at least Trezor T parameters. # 8.2.6 diff --git a/docs/methods/ethereumSignTypedData.md b/docs/methods/ethereumSignTypedData.md index 7f1f91d31..0795f0ab5 100644 --- a/docs/methods/ethereumSignTypedData.md +++ b/docs/methods/ethereumSignTypedData.md @@ -19,52 +19,81 @@ TrezorConnect.ethereumSignTypedData(params).then(function(result) { ``` > :warning: **Supported only by Trezor T with Firmware 2.4.3 or higher!** +> :warning: **Blind signing is supported only on Trezor Model 1 with Firmware 1.10.5 or higher!** ### Params [****Optional common params****](commonParams.md) -###### [flowtype](../../src/js/types/networks/ethereum.js#L102-105) +###### [flowtype](../../src/js/types/networks/ethereum.js#104-116) * `path` — *required* `string | Array` minimum length is `3`. [read more](path.md) * `data` - *required* `Object` type of [`EthereumSignTypedDataMessage`](../../src/js/types/networks/ethereum.js#L90)`. A JSON Schema definition can be found in the [EIP-712 spec]([EIP-712](https://eips.ethereum.org/EIPS/eip-712)). * `metamask_v4_compat` - *required* `boolean` set to `true` for compatibility with [MetaMask's signTypedData_v4](https://docs.metamask.io/guide/signing-data.html#sign-typed-data-v4). +#### Blind signing (optional addition for Trezor Model 1 compatibility) + +The Trezor Model 1 firmware currently does not support constructing EIP-712 +hashes. + +However, it supports signing pre-constructed hashes. + +EIP-712 hashes can be constructed with the plugin function at +["trezor-connect/lib/plugins/ethereum/typedData.js"](../../src/js/plugins/ethereum/typedData.js) +(included as a plugin due to a depedency on `@metamask/eth-sig-utils`). + +You may also wish to contruct your own hashes using a different library. + +###### [flowtype](../../src/js/types/networks/ethereum.js#L114-121) + +* `domain_separator_hash` - *required* `string` hex-encoded 32-byte hash of the EIP-712 domain. +* `message_hash` - *required* `string` hex-encoded 32-byte hash of the EIP-712 message. + ### Example ```javascript +const eip712Data = { + types: { + EIP712Domain: [ + { + name: 'name', + type: 'string', + }, + ], + Message: [ + { + name: "Best Wallet", + type: "string" + }, + { + name: "Number", + type: "uint64" + } + ] + }, + primaryType: 'Message', + domain: { + name: 'example.trezor.io', + }, + message: { + "Best Wallet": "Trezor Model T", + // be careful with JavaScript numbers: MAX_SAFE_INTEGER is quite low + "Number": `${2n ** 55n}`, + }, +}; + +// This functionality is separate from trezor-connect, as it requires @metamask/eth-sig-utils, +// which is a large JavaScript dependency +const transformTypedDataPlugin = require("trezor-connect/lib/plugins/ethereum/typedData.js"); +const {domain_separator_hash, message_hash} = transformTypedDataPlugin(eip712Data, true); + TrezorConnect.ethereumSignTypedData({ path: "m/44'/60'/0'", - data: { - types: { - EIP712Domain: [ - { - name: 'name', - type: 'string', - }, - ], - Message: [ - { - name: "Best Wallet", - type: "string" - }, - { - name: "Number", - type: "uint64" - } - ] - }, - primaryType: 'Message', - domain: { - name: 'example.trezor.io', - }, - message: { - "Best Wallet": "Trezor Model T", - // be careful with JavaScript numbers: MAX_SAFE_INTEGER is quite low - "Number": `${2n ** 55n}`, - }, - }, + data: eip712Data, metamask_v4_compat: true, + // These are optional, but required for Trezor Model 1 compatibility + domain_separator_hash, + message_hash, }); ``` diff --git a/src/js/core/methods/EthereumSignTypedData.js b/src/js/core/methods/EthereumSignTypedData.js index c9e2cb2ec..97720a83f 100644 --- a/src/js/core/methods/EthereumSignTypedData.js +++ b/src/js/core/methods/EthereumSignTypedData.js @@ -9,15 +9,17 @@ import type { MessageResponse, EthereumTypedDataStructAck } from '../../types/tr import { ERRORS } from '../../constants'; import type { EthereumSignTypedData as EthereumSignTypedDataParams, - EthereumSignTypedHash as EthereumSignTypedHashParams, + EthereumSignTypedHashAndData as EthereumSignTypedHashAndDataParams, } from '../../types/networks/ethereum'; import { getFieldType, parseArrayType, encodeData } from './helpers/ethereumSignTypedData'; -type Params = { - address_n: number[], - ...$Exact, - ...$Exact, -}; +type Params = $Diff< + { + address_n: number[], + ...$Exact | $Exact, + }, + { path: any }, // removes the "path" variable from this.params +>; export default class EthereumSignTypedData extends AbstractMethod<'ethereumSignTypedData'> { params: Params; @@ -30,10 +32,10 @@ export default class EthereumSignTypedData extends AbstractMethod<'ethereumSignT // validate incoming parameters validateParams(payload, [ { name: 'path', required: true }, - { name: 'metamask_v4_compat', type: 'boolean' }, // model T - { name: 'data', type: 'object' }, - // model One + { name: 'metamask_v4_compat', type: 'boolean', required: true }, + { name: 'data', type: 'object', required: true }, + // model One (optional params) { name: 'domain_separator_hash', type: 'string' }, { name: 'message_hash', type: 'string' }, ]); @@ -45,13 +47,18 @@ export default class EthereumSignTypedData extends AbstractMethod<'ethereumSignT this.info = getNetworkLabel('Sign #NETWORK typed data', network); this.params = { - path, address_n: path, metamask_v4_compat: payload.metamask_v4_compat, - domain_separator_hash: payload.domain_separator_hash || '', - message_hash: payload.message_hash || '', - data: payload.data || undefined, + data: payload.data, }; + + if (payload.message_hash) { + this.params = { + ...this.params, + domain_separator_hash: payload.domain_separator_hash, + message_hash: payload.message_hash, + }; + } } async run() { @@ -64,7 +71,9 @@ export default class EthereumSignTypedData extends AbstractMethod<'ethereumSignT { name: 'message_hash', type: 'string', required: true }, ]); - const { domain_separator_hash, message_hash } = this.params; + const { domain_separator_hash, message_hash } = + // $FlowIssue validateParams() confirms that these hashes exist + (this.params: EthereumSignTypedHashAndDataParams); // For Model 1 we use EthereumSignTypedHash const response = await cmd.typedCall( @@ -84,9 +93,7 @@ export default class EthereumSignTypedData extends AbstractMethod<'ethereumSignT }; } - validateParams(this.params, [{ name: 'data', type: 'object', required: true }]); const { data, metamask_v4_compat } = this.params; - // $FlowIssue const { types, primaryType, domain, message } = data; // For Model T we use EthereumSignTypedData diff --git a/src/js/types/__tests__/ethereum.js b/src/js/types/__tests__/ethereum.js index 327fd5ba2..ac3808901 100644 --- a/src/js/types/__tests__/ethereum.js +++ b/src/js/types/__tests__/ethereum.js @@ -242,7 +242,34 @@ export const signTypedData = async () => { await TrezorConnect.ethereumSignTypedData({ path: 'm/44', + data: { + types: { + EIP712Domain: [], + EmptyMessage: [], + }, + primaryType: 'EmptyMessage', + domain: {}, + message: {}, + }, message_hash: '0x', domain_separator_hash: '0x', + metamask_v4_compat: true, + }); + + // $FlowExpectedError `message_hash` is given, but it's an invalid type. + await TrezorConnect.ethereumSignTypedData({ + path: 'm/44', + data: { + types: { + EIP712Domain: [], + EmptyMessage: [], + }, + primaryType: 'EmptyMessage', + domain: {}, + message: {}, + }, + message_hash: 123456, + domain_separator_hash: '0x1234', + metamask_v4_compat: true, }); }; diff --git a/src/js/types/api.js b/src/js/types/api.js index 7ec0f4d0d..d514daa2e 100644 --- a/src/js/types/api.js +++ b/src/js/types/api.js @@ -254,10 +254,8 @@ export type API = { ethereumGetPublicKey: Bundled, ethereumSignTransaction: Method, ethereumSignMessage: Method, - ethereumSignTypedData: Mixed< - Ethereum.EthereumSignTypedData, - Ethereum.EthereumSignTypedHash, - Protobuf.EthereumTypedDataSignature, + ethereumSignTypedData: Method< + Ethereum.EthereumSignTypedData | Ethereum.EthereumSignTypedHashAndData, Protobuf.EthereumTypedDataSignature, >, diff --git a/src/js/types/networks/ethereum.js b/src/js/types/networks/ethereum.js index 0511d7cef..fe6ee4b7f 100644 --- a/src/js/types/networks/ethereum.js +++ b/src/js/types/networks/ethereum.js @@ -101,22 +101,34 @@ type EthereumSignTypedDataMessage = { message: { [fieldName: string]: any }, }; +/** + * Used for full EIP-712 signing + * (currently only supported on Trezor Model T) + */ export type EthereumSignTypedData = { path: string | number[], metamask_v4_compat: boolean, - data: EthereumSignTypedDataMessage, - domain_separator_hash?: typeof undefined, - message_hash?: typeof undefined, + data: EthereumSignTypedDataMessage, }; +/** + * Used for EIP-712 blind signing on Trezor Model 1 only + */ export type EthereumSignTypedHash = { path: string | number[], - metamask_v4_compat?: typeof undefined, - data?: typeof undefined, domain_separator_hash: string, message_hash: string, }; +/** + * Used for full EIP-712 signing or blind signing. + * Supports both Trezor Model T and Trezor Model 1 + */ +export type EthereumSignTypedHashAndData = { + ...$Exact, + ...$Exact, +}; + // verify message export type EthereumVerifyMessage = { diff --git a/src/ts/types/__tests__/ethereum.ts b/src/ts/types/__tests__/ethereum.ts index 74fe6168c..019a75635 100644 --- a/src/ts/types/__tests__/ethereum.ts +++ b/src/ts/types/__tests__/ethereum.ts @@ -235,7 +235,29 @@ export const signTypedData = async () => { await TrezorConnect.ethereumSignTypedData({ path: 'm/44', + metamask_v4_compat: true, + data: { + types: { EIP712Domain: [] }, + primaryType: 'EIP712Domain', + domain: {}, + message: {}, + }, message_hash: '0x', domain_separator_hash: '0x', }); + + await TrezorConnect.ethereumSignTypedData({ + path: 'm/44', + metamask_v4_compat: true, + data: { + types: { EIP712Domain: [] }, + // @ts-expect-error: primaryType not in `types` + primaryType: 'UnknownType', + domain: {}, + message: {}, + }, + // @ts-expect-error: incorrect type for message_hash + message_hash: 12345, + domain_separator_hash: '0x', + }); }; diff --git a/src/ts/types/api.d.ts b/src/ts/types/api.d.ts index fce8f6df9..5f3d7071c 100644 --- a/src/ts/types/api.d.ts +++ b/src/ts/types/api.d.ts @@ -303,11 +303,14 @@ export namespace TrezorConnect { function ethereumSignMessage( params: P.CommonParams & Ethereum.EthereumSignMessage, ): P.Response; + /** + * @param params Passing: + * - {@link Ethereum.EthereumSignTypedData} is required for Trezor T + * - {@link Ethereum.EthereumSignTypedHash} is required for Trezor 1 compatability + */ function ethereumSignTypedData( - params: P.CommonParams & Ethereum.EthereumSignTypedData, - ): P.Response; - function ethereumSignTypedData( - params: P.CommonParams & Ethereum.EthereumSignTypedHash, + params: P.CommonParams & + (Ethereum.EthereumSignTypedData | Ethereum.EthereumSignTypedHashAndData), ): P.Response; function ethereumVerifyMessage( params: P.CommonParams & Ethereum.EthereumVerifyMessage, diff --git a/src/ts/types/networks/ethereum.d.ts b/src/ts/types/networks/ethereum.d.ts index fbb829157..c4965c611 100644 --- a/src/ts/types/networks/ethereum.d.ts +++ b/src/ts/types/networks/ethereum.d.ts @@ -105,12 +105,23 @@ export interface EthereumSignTypedData { metamask_v4_compat: boolean; } +/** + * The Trezor Model 1 cannot currently calculate EIP-712 hashes by itself, + * so we have to precalculate them. + */ export interface EthereumSignTypedHash { path: string | number[]; domain_separator_hash: string; message_hash: string; } +/** + * Used for full EIP-712 signing or blind signing. + * Supports both Trezor Model T and Trezor Model 1. + */ +export type EthereumSignTypedHashAndData = + EthereumSignTypedData & EthereumSignTypedHash; + // verify message export interface EthereumVerifyMessage { From e7b58b8afa39075550218d1c0bfa01641a04a023 Mon Sep 17 00:00:00 2001 From: Alois Klink Date: Fri, 4 Feb 2022 03:43:19 +0000 Subject: [PATCH 2/4] fix(ethSignTypedData): Support domain-only signs Add support for signing domain-only data in ethereumSignTypedData, e.g. when primaryType = EIP712Domain. On Trezor T, this behaviour previously gave incorrect signatures or threw errors. On Trezor Model 1, not passing the "message_hash" parameter caused an error. --- CHANGELOG.md | 1 + docs/methods/ethereumSignTypedData.md | 7 +++- src/data/config.json | 5 +++ src/js/core/methods/EthereumSignTypedData.js | 39 +++++++++++++++++-- src/js/types/networks/ethereum.js | 3 +- .../__tests__/deviceFeaturesUtils.test.js | 2 + src/ts/types/networks/ethereum.d.ts | 3 +- tests/__fixtures__/ethereumSignTypedData.js | 39 ++++++++++--------- 8 files changed, 73 insertions(+), 26 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 52359ac3c..a8348e29a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ ### Fixed - Some methods not throwing `ui-device_firmware_unsupported` when the current device firmware didn't support the method. +- Ethereum: EthereumSignTypedData now supports signing domain-only data, i.e. when `primaryType` is equal to `EIP712Domain`. ### Changed diff --git a/docs/methods/ethereumSignTypedData.md b/docs/methods/ethereumSignTypedData.md index 0795f0ab5..1dddd6c44 100644 --- a/docs/methods/ethereumSignTypedData.md +++ b/docs/methods/ethereumSignTypedData.md @@ -27,6 +27,8 @@ TrezorConnect.ethereumSignTypedData(params).then(function(result) { ###### [flowtype](../../src/js/types/networks/ethereum.js#104-116) +> :warning: **Domain-only signing (`data.primaryType` = `"EIP712Domain"`) is supported only on Trezor T with Firmware 2.4.4 or higher!** + * `path` — *required* `string | Array` minimum length is `3`. [read more](path.md) * `data` - *required* `Object` type of [`EthereumSignTypedDataMessage`](../../src/js/types/networks/ethereum.js#L90)`. A JSON Schema definition can be found in the [EIP-712 spec]([EIP-712](https://eips.ethereum.org/EIPS/eip-712)). * `metamask_v4_compat` - *required* `boolean` set to `true` for compatibility with [MetaMask's signTypedData_v4](https://docs.metamask.io/guide/signing-data.html#sign-typed-data-v4). @@ -46,8 +48,11 @@ You may also wish to contruct your own hashes using a different library. ###### [flowtype](../../src/js/types/networks/ethereum.js#L114-121) +> :warning: **Domain-only signing (empty `message_hash`) is supported only on Trezor Model 1 with Firmware 1.10.6 or higher!** + * `domain_separator_hash` - *required* `string` hex-encoded 32-byte hash of the EIP-712 domain. -* `message_hash` - *required* `string` hex-encoded 32-byte hash of the EIP-712 message. +* `message_hash` - *optional* `string` hex-encoded 32-byte hash of the EIP-712 message. + This is optional for the domain-only hashes where `primaryType` is `EIP712Domain`. ### Example diff --git a/src/data/config.json b/src/data/config.json index fdf444d05..a63a6a283 100644 --- a/src/data/config.json +++ b/src/data/config.json @@ -170,6 +170,11 @@ "methods": ["ethereumSignTypedData"], "min": ["1.10.5", "2.4.3"], "comment": ["EIP-712 typed signing support added in 1.10.5/2.4.3"] + }, + { + "capabilities": ["eip712-domain-only"], + "min": ["1.10.6", "2.4.4"], + "comment": ["EIP-712 domain-only signing, when primaryType=EIP712Domain"] } ] } diff --git a/src/js/core/methods/EthereumSignTypedData.js b/src/js/core/methods/EthereumSignTypedData.js index 97720a83f..5e80d60e2 100644 --- a/src/js/core/methods/EthereumSignTypedData.js +++ b/src/js/core/methods/EthereumSignTypedData.js @@ -12,6 +12,7 @@ import type { EthereumSignTypedHashAndData as EthereumSignTypedHashAndDataParams, } from '../../types/networks/ethereum'; import { getFieldType, parseArrayType, encodeData } from './helpers/ethereumSignTypedData'; +import { messageToHex } from '../../utils/formatUtils'; type Params = $Diff< { @@ -52,12 +53,42 @@ export default class EthereumSignTypedData extends AbstractMethod<'ethereumSignT data: payload.data, }; - if (payload.message_hash) { + if (payload.domain_separator_hash) { this.params = { ...this.params, - domain_separator_hash: payload.domain_separator_hash, - message_hash: payload.message_hash, + // leading `0x` in hash-strings causes issues + domain_separator_hash: messageToHex(payload.domain_separator_hash), }; + + if (payload.message_hash) { + this.params = { + ...this.params, + // leading `0x` in hash-strings causes issues + message_hash: messageToHex(payload.message_hash), + }; + } else if (this.params.data.primaryType !== 'EIP712Domain') { + throw ERRORS.TypedError( + 'Method_InvalidParameter', + 'message_hash should only be empty when data.primaryType=EIP712Domain', + ); + } + } + + if (this.params.data.primaryType === 'EIP712Domain') { + // Only newer firmwares support this feature + // Older firmwares will give wrong results / throw errors + this.firmwareRange = getFirmwareRange( + 'eip712-domain-only', + network, + this.firmwareRange, + ); + + if (this.params.message_hash) { + throw ERRORS.TypedError( + 'Method_InvalidParameter', + 'message_hash should be empty when data.primaryType=EIP712Domain', + ); + } } } @@ -68,7 +99,7 @@ export default class EthereumSignTypedData extends AbstractMethod<'ethereumSignT if (this.device.features.model === '1') { validateParams(this.params, [ { name: 'domain_separator_hash', type: 'string', required: true }, - { name: 'message_hash', type: 'string', required: true }, + { name: 'message_hash', type: 'string' }, ]); const { domain_separator_hash, message_hash } = diff --git a/src/js/types/networks/ethereum.js b/src/js/types/networks/ethereum.js index fe6ee4b7f..9a65d49fb 100644 --- a/src/js/types/networks/ethereum.js +++ b/src/js/types/networks/ethereum.js @@ -117,7 +117,8 @@ export type EthereumSignTypedData = { export type EthereumSignTypedHash = { path: string | number[], domain_separator_hash: string, - message_hash: string, + /** Optional for domain-only hashes (when EIP712Domain is the primaryType) */ + message_hash?: string, }; /** diff --git a/src/js/utils/__tests__/deviceFeaturesUtils.test.js b/src/js/utils/__tests__/deviceFeaturesUtils.test.js index 6ff6de562..48482e33b 100644 --- a/src/js/utils/__tests__/deviceFeaturesUtils.test.js +++ b/src/js/utils/__tests__/deviceFeaturesUtils.test.js @@ -119,6 +119,7 @@ describe('utils/deviceFeaturesUtils', () => { replaceTransaction: 'update-required', decreaseOutput: 'update-required', eip1559: 'update-required', + 'eip712-domain-only': 'update-required', taproot: 'update-required', signMessageNoScriptType: 'update-required', }); @@ -136,6 +137,7 @@ describe('utils/deviceFeaturesUtils', () => { replaceTransaction: 'update-required', decreaseOutput: 'update-required', eip1559: 'update-required', + 'eip712-domain-only': 'update-required', taproot: 'update-required', signMessageNoScriptType: 'update-required', }); diff --git a/src/ts/types/networks/ethereum.d.ts b/src/ts/types/networks/ethereum.d.ts index c4965c611..ffa92776b 100644 --- a/src/ts/types/networks/ethereum.d.ts +++ b/src/ts/types/networks/ethereum.d.ts @@ -112,7 +112,8 @@ export interface EthereumSignTypedData { export interface EthereumSignTypedHash { path: string | number[]; domain_separator_hash: string; - message_hash: string; + /** Not required for domain-only signing, when EIP712Domain is the `primaryType` */ + message_hash?: string; } /** diff --git a/tests/__fixtures__/ethereumSignTypedData.js b/tests/__fixtures__/ethereumSignTypedData.js index 221ea8d44..222a2babc 100644 --- a/tests/__fixtures__/ethereumSignTypedData.js +++ b/tests/__fixtures__/ethereumSignTypedData.js @@ -1,40 +1,41 @@ import commonFixtures from '../../submodules/trezor-common/tests/fixtures/ethereum/sign_typed_data.json'; const fixtures = commonFixtures.tests.flatMap(({ name, parameters, result }) => { - const fixture = { - setup: { - firmware: [['2.4.2', '2-master']], + let legacyResults = [ + { + // ethereumSignTypedData support was only added in 2.4.3/1.10.5 + rules: ['<2.4.3', '<1.10.5'], + success: false, }, + ]; + if (parameters.data.primaryType === 'EIP712Domain') { + legacyResults = [ + { + // domain-only signTypedData not supported before this + rules: ['<2.4.4', '<1.10.6'], + success: false, + }, + ]; + } + + const fixture = { description: `${name} ${parameters.comment ?? ''}`, name, params: parameters, + legacyResults, result: { address: result.address, signature: result.sig, }, }; + return fixture; }); -const t1Fixtures = fixtures - .filter(f => f.params.metamask_v4_compat) - .map(f => { - const fixture = { - ...f, - description: `t1: ${f.description}`, - setup: { - ...f.setup, - firmware: [['1.10.6', '1-master']], - }, - }; - - return fixture; - }); - export default { method: 'ethereumSignTypedData', setup: { mnemonic: commonFixtures.setup.mnemonic, }, - tests: [...fixtures, ...t1Fixtures], + tests: fixtures, }; From 2ad21b03bdfd63e54c5ca2427bfbe12cb036c1f3 Mon Sep 17 00:00:00 2001 From: Alois Klink Date: Fri, 4 Feb 2022 14:46:45 +0000 Subject: [PATCH 3/4] fix(plugins/ethereum): Support domain-only EIP712 Adds support for domain-only EIP-712 hashes to plugins/ethereum/typedData.js --- src/js/plugins/ethereum/typedData.js | 27 ++++++++++++++++++----- src/js/plugins/ethereum/typedData.test.js | 26 +++++++++++++++++----- 2 files changed, 42 insertions(+), 11 deletions(-) diff --git a/src/js/plugins/ethereum/typedData.js b/src/js/plugins/ethereum/typedData.js index 4b67368fd..663424eec 100644 --- a/src/js/plugins/ethereum/typedData.js +++ b/src/js/plugins/ethereum/typedData.js @@ -20,6 +20,17 @@ function sanitizeData(data) { } } +/** + * Calculates the domain_separator_hash and message_hash from an EIP-712 Typed Data object. + * + * The Trezor Model 1 does not currently support constructing the hash on the device, + * so this function pre-computes them. + * + * @template {sigUtil.TypedMessage} T + * @param {T} data - The EIP-712 Typed Data object. + * @param {boolean} metamask_v4_compat - Set to `true` for compatibility with Metamask's signTypedData_v4 function. + * @returns {{domain_separator_hash: string, message_hash?: string} & T} The hashes. + */ const transformTypedData = (data, metamask_v4_compat) => { if (!metamask_v4_compat) { throw new Error('Trezor: Only version 4 of typed data signing is supported'); @@ -36,12 +47,16 @@ const transformTypedData = (data, metamask_v4_compat) => { version, ).toString('hex'); - const messageHash = sigUtil.TypedDataUtils.hashStruct( - primaryType, - sanitizeData(message), - types, - version, - ).toString('hex'); + let messageHash = null; + + if (primaryType !== 'EIP712Domain') { + messageHash = sigUtil.TypedDataUtils.hashStruct( + primaryType, + sanitizeData(message), + types, + version, + ).toString('hex'); + } return { domain_separator_hash: domainSeparatorHash, diff --git a/src/js/plugins/ethereum/typedData.test.js b/src/js/plugins/ethereum/typedData.test.js index 6e5beafe7..d6d664907 100644 --- a/src/js/plugins/ethereum/typedData.test.js +++ b/src/js/plugins/ethereum/typedData.test.js @@ -1,17 +1,33 @@ const commonFixtures = require('../../../../submodules/trezor-common/tests/fixtures/ethereum/sign_typed_data.json'); const typedData = require('./typedData'); +// Adds 0x to a string if it doesn't start with one +// fixtures sometimes start with 0x, sometimes not +function messageToHex(string) { + return string.startsWith('0x') ? string : `0x${string}`; +} + describe('typedData', () => { commonFixtures.tests .filter(test => test.parameters.metamask_v4_compat) .forEach(test => { it('typedData to message_hash and domain_separator_hash', () => { - const transformed = typedData(test.parameters.data, true); - // todo: fixtures in firmware-repo not unified, probably bug - const { domain_separator_hash /* , message_hash */ } = transformed; + const transformed = typedData( + test.parameters.data, + test.parameters.metamask_v4_compat, + ); + const { domain_separator_hash, message_hash } = transformed; - expect(`0x${domain_separator_hash}`).toEqual(test.parameters.domain_separator_hash); - // expect(`0x${message_hash}`).toEqual(test.parameters.message_hash); + expect(messageToHex(domain_separator_hash)).toEqual( + messageToHex(test.parameters.domain_separator_hash), + ); + if (message_hash) { + expect(messageToHex(message_hash)).toEqual( + messageToHex(test.parameters.message_hash), + ); + } else { + expect(null).toEqual(test.parameters.message_hash); + } }); }); }); From 5530f409b923bb9209e4858f7c0e2b25105abd79 Mon Sep 17 00:00:00 2001 From: Alois Klink Date: Wed, 9 Feb 2022 02:57:26 +0000 Subject: [PATCH 4/4] ci(ethereum): Enable ethereumSignTypedData in CI ethereumSignTypedData now passes on tests, so we can add it to CI. --- .github/workflows/karma.yml | 2 +- .github/workflows/tests.yml | 2 +- .gitlab-ci.yml | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/.github/workflows/karma.yml b/.github/workflows/karma.yml index 7ce61f327..458cc3dd2 100644 --- a/.github/workflows/karma.yml +++ b/.github/workflows/karma.yml @@ -190,7 +190,7 @@ jobs: path: build # xvfb is required to run karma - run: sudo apt-get install xvfb - - run: xvfb-run --auto-servernum ./tests/run.sh -s 'yarn test:karma:production' -i ethereumGetAddress,ethereumGetPublicKey,ethereumSignMessage,ethereumSignTransaction,ethereumVerifyMessage + - run: xvfb-run --auto-servernum ./tests/run.sh -s 'yarn test:karma:production' -i ethereumGetAddress,ethereumGetPublicKey,ethereumSignMessage,ethereumSignTransaction,ethereumVerifyMessage,ethereumSignTypedData nem: needs: check diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 8383258a1..298d9833f 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -135,7 +135,7 @@ jobs: with: path: node_modules key: ${{ runner.os }}-yarn-${{ hashFiles('**/yarn.lock') }} - - run: ./tests/run.sh -i ethereumGetAddress,ethereumGetPublicKey,ethereumSignMessage,ethereumSignTransaction,ethereumVerifyMessage + - run: ./tests/run.sh -i ethereumGetAddress,ethereumGetPublicKey,ethereumSignMessage,ethereumSignTransaction,ethereumVerifyMessage,ethereumSignTypedData nem: needs: check diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index c3c859e49..76cfb37f5 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -159,7 +159,7 @@ publish beta release to npm: - TESTS_INCLUDED_METHODS: "stellarGetAddress,stellarSignTransaction" - TESTS_INCLUDED_METHODS: "cardanoGetAddress,cardanoGetNativeScriptHash,cardanoGetPublicKey,cardanoSignTransaction" - TESTS_INCLUDED_METHODS: "eosGetPublicKey,eosSignTransaction" - - TESTS_INCLUDED_METHODS: "ethereumGetAddress,ethereumGetPublicKey,ethereumSignMessage,ethereumSignTransaction,ethereumVerifyMessage" + - TESTS_INCLUDED_METHODS: "ethereumGetAddress,ethereumGetPublicKey,ethereumSignMessage,ethereumSignTransaction,ethereumVerifyMessage,ethereumSignTypedData" - TESTS_INCLUDED_METHODS: "nemGetAddress,nemSignTransaction" - TESTS_INCLUDED_METHODS: "rippleGetAddress,rippleSignTransaction" - TESTS_INCLUDED_METHODS: "tezosGetAddress,tezosGetPublicKey" @@ -171,7 +171,7 @@ publish beta release to npm: - TESTS_INCLUDED_METHODS: "applySettings,applyFlags,getFeatures" - TESTS_INCLUDED_METHODS: "signTransaction" - TESTS_INCLUDED_METHODS: "getAccountInfo,getAddress,getPublicKey,signMessage,verifyMessage,composeTransaction" - - TESTS_INCLUDED_METHODS: "ethereumGetAddress,ethereumGetPublicKey,ethereumSignMessage,ethereumSignTransaction,ethereumVerifyMessage" + - TESTS_INCLUDED_METHODS: "ethereumGetAddress,ethereumGetPublicKey,ethereumSignMessage,ethereumSignTransaction,ethereumVerifyMessage,ethereumSignTypedData" .test: extends: .jobs