fix(core,legacy): Fix domain-only ethTypedData

When doing Ethereum signTypedData, and the primaryType="EIP712Domain", we completely ignore the "message" part and only sign the domain. According to the community, this is technically allowed by the spec, and may be used by ETH smart contracts to save on gas. Test case generated by @MetaMask/eth-sig-util's library. See: https://ethereum-magicians.org/t/eip-712-standards-clarification-primarytype-as-domaintype/3286
trezor · Jan 14, 2022 · 5c4703c · 5c4703c
1 parent b2136e3
commit 5c4703c
Show file tree

Hide file tree

Showing 15 changed files with 163 additions and 45 deletions.
diff --git a/common/protob/messages-ethereum.proto b/common/protob/messages-ethereum.proto
@@ -159,7 +159,7 @@ message EthereumVerifyMessage {
 message EthereumSignTypedHash {
     repeated uint32 address_n = 1;              // BIP-32 path to derive the key from master node
     required bytes domain_separator_hash = 2;   // Hash of domainSeparator of typed data to be signed
-    required bytes message_hash = 3;               // Hash of the data of typed data to be signed
+    optional bytes message_hash = 3;            // Hash of the data of typed data to be signed (empty if domain-only data)
 }
 
 /**

diff --git a/common/tests/fixtures/ethereum/sign_typed_data.json b/common/tests/fixtures/ethereum/sign_typed_data.json
@@ -4,6 +4,77 @@
         "passphrase": ""
     },
     "tests": [
+        {
+            "name": "bare_minimum",
+            "comment": "Bare minimum EIP-712 message (domain only)",
+            "parameters": {
+                "path": "m/44'/60'/0'/0/0",
+                "metamask_v4_compat": true,
+                "data": {
+                    "types": {
+                        "EIP712Domain": []
+                    },
+                    "primaryType": "EIP712Domain",
+                    "message": {},
+                    "domain": {}
+                },
+                "message_hash": null,
+                "domain_separator_hash": "0x6192106f129ce05c9075d319c1fa6ea9b3ae37cbd0c1ef92e2be7137bb07baa1"
+            },
+            "result": {
+                "address": "0x73d0385F4d8E00C5e6504C6030F47BF6212736A8",
+                "sig": "0x18aaea9abed7cd88d3763a9a420e2e7b71a9f991685fbc62d74da86326cffa680644862d459d1973e422777a3933bc74190b1cae9a5418ddaea645a7d7630dd91c"
+            }
+        },
+        {
+            "name": "full_domain_empty_message",
+            "comment": "Domain only EIP-712 message",
+            "parameters": {
+                "path": "m/44'/60'/0'/0/0",
+                "metamask_v4_compat": true,
+                "data": {
+                    "types": {
+                        "EIP712Domain": [
+                            {
+                                "name": "name",
+                                "type": "string"
+                            },
+                            {
+                                "name": "version",
+                                "type": "string"
+                            },
+                            {
+                                "name": "chainId",
+                                "type": "uint256"
+                            },
+                            {
+                                "name": "verifyingContract",
+                                "type": "address"
+                            },
+                            {
+                                "name": "salt",
+                                "type": "bytes32"
+                            }
+                        ]
+                    },
+                    "primaryType": "EIP712Domain",
+                    "message": {},
+                    "domain": {
+                        "name": "Trezor",
+                        "version": "Test v0.0.0",
+                        "chainId": 1,
+                        "verifyingContract": "0xCcCCccccCCCCcCCCCCCcCcCccCcCCCcCcccccccC",
+                        "salt": "0x0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"
+                    }
+                },
+                "message_hash": null,
+                "domain_separator_hash": "0xf85aaf157e9a36dc6e12643fff450fdf8d98fd0d0e41c5b42bb1f7aae6c83388"
+            },
+            "result": {
+                "address": "0x73d0385F4d8E00C5e6504C6030F47BF6212736A8",
+                "sig": "0x98a3e66f738002da98c70b976ef131c11ed8b94aad872140574ed2a2d4a2bac53a9350e284994274f0a7ce1191cf79bf13f2f0d0a862dcf0dd86ad8141eb90dc1c"
+            }
+        },
         {
             "name": "basic_data",
             "parameters": {

diff --git a/core/.changelog.d/2036.fixed b/core/.changelog.d/2036.fixed
@@ -0,0 +1 @@
+Fix domain-only EIP-712 hashes (i.e. when `primaryType`=`EIP712Domain`)
diff --git a/core/src/apps/ethereum/layout.py b/core/src/apps/ethereum/layout.py
@@ -6,6 +6,7 @@
 from trezor.messages import EthereumFieldType, EthereumStructMember
 from trezor.strings import format_amount, format_plural
 from trezor.ui.layouts import (
+    confirm_action,
     confirm_address,
     confirm_amount,
     confirm_blob,
@@ -111,16 +112,27 @@ def require_confirm_data(ctx: Context, data: bytes, data_total: int) -> Awaitabl
     )
 
 
-async def confirm_hash(ctx: Context, message_hash: bytes) -> None:
-    await confirm_blob(
+async def confirm_typed_data_final(ctx: Context) -> None:
+    await confirm_action(
         ctx,
-        "confirm_hash",
-        title="Confirm hash",
-        data="0x" + hexlify(message_hash).decode(),
+        "confirm_typed_data_final",
+        title="Confirm typed data",
+        action="Really sign EIP-712 typed data?",
+        verb="Hold to confirm",
         hold=True,
     )
 
 
+def confirm_empty_typed_message(ctx: Context) -> Awaitable[None]:
+    return confirm_text(
+        ctx,
+        "confirm_empty_typed_message",
+        title="Confirm message",
+        data="",
+        description="No message field",
+    )
+
+
 async def should_show_domain(ctx: Context, name: bytes, version: bytes) -> bool:
     domain_name = decode_typed_data(name, "string")
     domain_version = decode_typed_data(version, "string")

diff --git a/core/src/apps/ethereum/sign_typed_data.py b/core/src/apps/ethereum/sign_typed_data.py
@@ -20,7 +20,8 @@
 from .helpers import address_from_bytes, get_type_name
 from .keychain import PATTERNS_ADDRESS, with_keychain_from_path
 from .layout import (
-    confirm_hash,
+    confirm_empty_typed_message,
+    confirm_typed_data_final,
     confirm_typed_value,
     should_show_array,
     should_show_domain,
@@ -82,23 +83,30 @@ async def generate_typed_data_hash(
         parent_objects=["EIP712Domain"],
     )
 
-    show_message = await should_show_struct(
-        ctx,
-        description=primary_type,
-        data_members=typed_data_envelope.types[primary_type].members,
-        title="Confirm message",
-        button_text="Show full message",
-    )
-    message_hash = await typed_data_envelope.hash_struct(
-        primary_type=primary_type,
-        member_path=[1],
-        show_data=show_message,
-        parent_objects=[primary_type],
-    )
+    # Setting the primary_type to "EIP712Domain" is technically in spec
+    # In this case, we ignore the "message" part and only use the "domain" part
+    # https://ethereum-magicians.org/t/eip-712-standards-clarification-primarytype-as-domaintype/3286
+    if primary_type == "EIP712Domain":
+        await confirm_empty_typed_message(ctx)
+        message_hash = b""
+    else:
+        show_message = await should_show_struct(
+            ctx,
+            description=primary_type,
+            data_members=typed_data_envelope.types[primary_type].members,
+            title="Confirm message",
+            button_text="Show full message",
+        )
+        message_hash = await typed_data_envelope.hash_struct(
+            primary_type=primary_type,
+            member_path=[1],
+            show_data=show_message,
+            parent_objects=[primary_type],
+        )
 
-    await confirm_hash(ctx, message_hash)
+    await confirm_typed_data_final(ctx)
 
-    return keccak256(b"\x19" + b"\x01" + domain_separator + message_hash)
+    return keccak256(b"\x19\x01" + domain_separator + message_hash)
 
 
 def get_hash_writer() -> HashWriter:

diff --git a/core/src/trezor/messages.py b/core/src/trezor/messages.py
@@ -3234,14 +3234,14 @@ def is_type_of(cls, msg: protobuf.MessageType) -> TypeGuard["EthereumVerifyMessa
     class EthereumSignTypedHash(protobuf.MessageType):
         address_n: "list[int]"
         domain_separator_hash: "bytes"
-        message_hash: "bytes"
+        message_hash: "bytes | None"
 
         def __init__(
             self,
             *,
             domain_separator_hash: "bytes",
-            message_hash: "bytes",
             address_n: "list[int] | None" = None,
+            message_hash: "bytes | None" = None,
         ) -> None:
             pass
 

diff --git a/legacy/firmware/.changelog.d/2036.fixed b/legacy/firmware/.changelog.d/2036.fixed
@@ -0,0 +1 @@
+Fix domain-only EIP-712 hashes (i.e. when `primaryType`=`EIP712Domain`)
diff --git a/legacy/firmware/ethereum.c b/legacy/firmware/ethereum.c
@@ -976,23 +976,30 @@ int ethereum_message_verify(const EthereumVerifyMessage *msg) {
   return 0;
 }
 
+/*
+ * EIP-712 hashes might have no message_hash if primaryType="EIP712Domain".
+ * In this case, set has_message_hash=false.
+ */
 static void ethereum_typed_hash(const uint8_t domain_separator_hash[32],
                                 const uint8_t message_hash[32],
-                                uint8_t hash[32]) {
+                                bool has_message_hash, uint8_t hash[32]) {
   struct SHA3_CTX ctx = {0};
   sha3_256_Init(&ctx);
   sha3_Update(&ctx, (const uint8_t *)"\x19\x01", 2);
   sha3_Update(&ctx, domain_separator_hash, 32);
-  sha3_Update(&ctx, message_hash, 32);
+  if (has_message_hash) {
+    sha3_Update(&ctx, message_hash, 32);
+  }
   keccak_Final(&ctx, hash);
 }
 
 void ethereum_typed_hash_sign(const EthereumSignTypedHash *msg,
                               const HDNode *node,
                               EthereumTypedDataSignature *resp) {
   uint8_t hash[32] = {0};
+
   ethereum_typed_hash(msg->domain_separator_hash.bytes, msg->message_hash.bytes,
-                      hash);
+                      msg->has_message_hash, hash);
 
   uint8_t v = 0;
   if (ecdsa_sign_digest(&secp256k1, node->private_key, hash,

diff --git a/legacy/firmware/fsm_msg_ethereum.h b/legacy/firmware/fsm_msg_ethereum.h
@@ -216,7 +216,8 @@ void fsm_msgEthereumSignTypedHash(const EthereumSignTypedHash *msg) {
 
   CHECK_PIN
 
-  if (msg->domain_separator_hash.size != 32 || msg->message_hash.size != 32) {
+  if (msg->domain_separator_hash.size != 32 ||
+      (msg->has_message_hash && msg->message_hash.size != 32)) {
     fsm_sendFailure(FailureType_Failure_DataError, _("Invalid hash length"));
     return;
   }
@@ -256,12 +257,17 @@ void fsm_msgEthereumSignTypedHash(const EthereumSignTypedHash *msg) {
     layoutHome();
     return;
   }
-  layoutConfirmHash(&bmp_icon_warning, _("EIP-712 message hash"),
-                    msg->message_hash.bytes, 32);
-  if (!protectButton(ButtonRequestType_ButtonRequest_Other, false)) {
-    fsm_sendFailure(FailureType_Failure_ActionCancelled, NULL);
-    layoutHome();
-    return;
+
+  // No message hash when setting primaryType="EIP712Domain"
+  // https://ethereum-magicians.org/t/eip-712-standards-clarification-primarytype-as-domaintype/3286
+  if (msg->has_message_hash) {
+    layoutConfirmHash(&bmp_icon_warning, _("EIP-712 message hash"),
+                      msg->message_hash.bytes, 32);
+    if (!protectButton(ButtonRequestType_ButtonRequest_Other, false)) {
+      fsm_sendFailure(FailureType_Failure_ActionCancelled, NULL);
+      layoutHome();
+      return;
+    }
   }
 
   ethereum_typed_hash_sign(msg, node, resp);

diff --git a/python/.changelog.d/2036.fixed b/python/.changelog.d/2036.fixed
@@ -0,0 +1,2 @@
+Allow passing empty `message_hash` for domain-only EIP-712 hashes
+for Trezor T1 (i.e. when `primaryType`=`EIP712Domain`)
diff --git a/python/src/trezorlib/cli/ethereum.py b/python/src/trezorlib/cli/ethereum.py
@@ -455,10 +455,12 @@ def sign_typed_data_hash(
     Sign hash of typed data (EIP-712) with Ethereum address.
 
     For T1 backward compatibility.
+
+    MESSAGE_HASH_HEX can be set to an empty string '' for domain-only hashes.
     """
     address_n = tools.parse_path(address)
     domain_hash = ethereum.decode_hex(domain_hash_hex)
-    message_hash = ethereum.decode_hex(message_hash_hex)
+    message_hash = ethereum.decode_hex(message_hash_hex) if message_hash_hex else None
     ret = ethereum.sign_typed_data_hash(client, address_n, domain_hash, message_hash)
     output = {
         "domain_hash": domain_hash_hex,

diff --git a/python/src/trezorlib/ethereum.py b/python/src/trezorlib/ethereum.py
@@ -361,7 +361,10 @@ def verify_message(
 
 @expect(messages.EthereumTypedDataSignature)
 def sign_typed_data_hash(
-    client: "TrezorClient", n: "Address", domain_hash: bytes, message_hash: bytes
+    client: "TrezorClient",
+    n: "Address",
+    domain_hash: bytes,
+    message_hash: Optional[bytes],
 ) -> "MessageType":
     return client.call(
         messages.EthereumSignTypedHash(

diff --git a/python/src/trezorlib/messages.py b/python/src/trezorlib/messages.py
@@ -4742,15 +4742,15 @@ class EthereumSignTypedHash(protobuf.MessageType):
     FIELDS = {
         1: protobuf.Field("address_n", "uint32", repeated=True, required=False),
         2: protobuf.Field("domain_separator_hash", "bytes", repeated=False, required=True),
-        3: protobuf.Field("message_hash", "bytes", repeated=False, required=True),
+        3: protobuf.Field("message_hash", "bytes", repeated=False, required=False),
     }
 
     def __init__(
         self,
         *,
         domain_separator_hash: "bytes",
-        message_hash: "bytes",
         address_n: Optional[Sequence["int"]] = None,
+        message_hash: Optional["bytes"] = None,
     ) -> None:
         self.address_n: Sequence["int"] = address_n if address_n is not None else []
         self.domain_separator_hash = domain_separator_hash

diff --git a/tests/device_tests/ethereum/test_sign_typed_data.py b/tests/device_tests/ethereum/test_sign_typed_data.py
@@ -50,7 +50,10 @@ def test_ethereum_sign_typed_data_blind(client, parameters, result):
             client,
             address_n,
             ethereum.decode_hex(parameters["domain_separator_hash"]),
-            ethereum.decode_hex(parameters["message_hash"]),
+            # message hash is empty for domain-only hashes
+            ethereum.decode_hex(parameters["message_hash"])
+            if parameters["message_hash"]
+            else None,
         )
         assert ret.address == result["address"]
         assert f"0x{ret.signature.hex()}" == result["sig"]

diff --git a/tests/ui_tests/fixtures.json b/tests/ui_tests/fixtures.json
@@ -602,13 +602,15 @@
 "ethereum-test_getpublickey.py::test_ethereum_getpublickey[parameters0-result0]": "095af81ec79e9b510c90d9fa34fed343f3840807190c67bc237af885695ae687",
 "ethereum-test_getpublickey.py::test_ethereum_getpublickey[parameters1-result1]": "095af81ec79e9b510c90d9fa34fed343f3840807190c67bc237af885695ae687",
 "ethereum-test_getpublickey.py::test_ethereum_getpublickey[parameters2-result2]": "095af81ec79e9b510c90d9fa34fed343f3840807190c67bc237af885695ae687",
-"ethereum-test_sign_typed_data.py::test_ethereum_sign_typed_data[basic_data]": "4f512d6beb0222079aaa878a2bcd2c41ac389fdd47edad3e0b0b5779677d8697",
-"ethereum-test_sign_typed_data.py::test_ethereum_sign_typed_data[complex_data]": "275a8630a795c966419f6fc6de834bb576bfc3dbc16a1fd7605aa2b8ceae666e",
-"ethereum-test_sign_typed_data.py::test_ethereum_sign_typed_data[struct_list_non_v4]": "7dd23b14bd273b937836a24cf056c745e7b3461139f895caefd4624e0d5545f5",
-"ethereum-test_sign_typed_data.py::test_ethereum_sign_typed_data[struct_list_v4]": "b7e3475d4906942bc0e8d62203ae91a13ea0d702c3a7a53b9777bea670c4a7f7",
-"ethereum-test_sign_typed_data.py::test_ethereum_sign_typed_data[structs_arrays_v4]": "31cc5b5c1d9d94f0761208f5dc6423d283b9118b12b87cf878d20fa144f4f252",
+"ethereum-test_sign_typed_data.py::test_ethereum_sign_typed_data[bare_minimum]": "e13b237f58a9977c6edf1917730748b881749acecc3d040b1fd9d794e6e02299",
+"ethereum-test_sign_typed_data.py::test_ethereum_sign_typed_data[basic_data]": "84f1d23e031919fedb4ef6b3f3dc1cb667836b9efcd0c5a0da586010e38e1fd6",
+"ethereum-test_sign_typed_data.py::test_ethereum_sign_typed_data[complex_data]": "27ee23894c19e5704d02ae684d50a0f0d14678d3f74a95e67fc4975818a1faa8",
+"ethereum-test_sign_typed_data.py::test_ethereum_sign_typed_data[full_domain_empty_message]": "c3c299087e9cac4d7554314abc8637d041da69f0c0ee2863c5526f1f89448ae2",
+"ethereum-test_sign_typed_data.py::test_ethereum_sign_typed_data[struct_list_non_v4]": "84f1d23e031919fedb4ef6b3f3dc1cb667836b9efcd0c5a0da586010e38e1fd6",
+"ethereum-test_sign_typed_data.py::test_ethereum_sign_typed_data[struct_list_v4]": "84f1d23e031919fedb4ef6b3f3dc1cb667836b9efcd0c5a0da586010e38e1fd6",
+"ethereum-test_sign_typed_data.py::test_ethereum_sign_typed_data[structs_arrays_v4]": "84f1d23e031919fedb4ef6b3f3dc1cb667836b9efcd0c5a0da586010e38e1fd6",
 "ethereum-test_sign_typed_data.py::test_ethereum_sign_typed_data_cancel": "08712efae2d007610289bbfb3a8fe6800547e884636c83c5bf0e25f33728789e",
-"ethereum-test_sign_typed_data.py::test_ethereum_sign_typed_data_show_more_button": "1adbea797586685ce09aae58b0a2b89e1617e4eaad23a8c1ac6fc10b041e57a5",
+"ethereum-test_sign_typed_data.py::test_ethereum_sign_typed_data_show_more_button": "72959d95278c136cb2071f6452b2a61782b9ac3e6946d8fb5f8db8f153171e6f",
 "ethereum-test_sign_verify_message.py::test_signmessage[parameters0-result0]": "7aa14b29e5005d8fdc0a8b497ed5d3ebea15c7017f9c457d09214f2d05fbc532",
 "ethereum-test_sign_verify_message.py::test_signmessage[parameters1-result1]": "c5fb9393267c3d9b9bf5839aab6c641d3931286411f291cd1d8e937cb224ae2d",
 "ethereum-test_sign_verify_message.py::test_signmessage[parameters2-result2]": "8499b87474becc06010e9b4356398a3e29ee2ef152e04653f15dcc227fc486d6",
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1 @@
		Fix domain-only EIP-712 hashes (i.e. when `primaryType`=`EIP712Domain`)
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1,2 @@
		Allow passing empty `message_hash` for domain-only EIP-712 hashes
		for Trezor T1 (i.e. when `primaryType`=`EIP712Domain`)