EIP-712 empty array handling fix

client/CHANGELOG.md

@@ -5,13 +5,17 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
-## [0.4.1] - 2024-04-05
+## [0.4.1] - 2024-04-15
 
 ### Added
 
 - Add new function `send_raw`, allowing to send a raw payload APDU
 - Add new error code definition
 
+### Fixed
+
+- Encoding of EIP-712 bytes elements
+
 ## [0.4.0] - 2024-04-03
 
 ### Added

client/src/ledger_app_clients/ethereum/eip712/InputData.py

@@ -4,7 +4,8 @@
 import signal
 import sys
 import copy
-from typing import Any, Callable, Optional
+from typing import Any, Callable, Optional, Union
+import struct
 
 from client import keychain
 from client.client import EthAppClient, EIP712FieldType
@@ -118,69 +119,63 @@ def send_struct_def_field(typename, keyname):
     return (typename, type_enum, typesize, array_lvls)
 
 
-def encode_integer(value, typesize):
-    data = bytearray()
-
+def encode_integer(value: Union[str | int], typesize: int) -> bytes:
     # Some are already represented as integers in the JSON, but most as strings
     if isinstance(value, str):
-        base = 10
-        if value.startswith("0x"):
-            base = 16
-        value = int(value, base)
+        value = int(value, 0)
 
     if value == 0:
-        data.append(0)
+        data = b'\x00'
     else:
-        if value < 0:  # negative number, send it as unsigned
-            mask = 0
-            for i in range(typesize):  # make a mask as big as the typesize
-                mask = (mask << 8) | 0xff
-            value &= mask
-        while value > 0:
-            data.append(value & 0xff)
-            value >>= 8
-        data.reverse()
+        # biggest uint type accepted by struct.pack
+        uint64_mask = 0xffffffffffffffff
+        data = struct.pack(">QQQQ",
+                           (value >> 192) & uint64_mask,
+                           (value >> 128) & uint64_mask,
+                           (value >> 64) & uint64_mask,
+                           value & uint64_mask)
+        data = data[len(data) - typesize:]
+        data = data.lstrip(b'\x00')
     return data
 
 
-def encode_int(value, typesize):
+def encode_int(value: str, typesize: int) -> bytes:
     return encode_integer(value, typesize)
 
 
-def encode_uint(value, typesize):
+def encode_uint(value: str, typesize: int) -> bytes:
     return encode_integer(value, typesize)
 
 
-def encode_hex_string(value, size):
-    data = bytearray()
-    value = value[2:]  # skip 0x
-    byte_idx = 0
-    while byte_idx < size:
-        data.append(int(value[(byte_idx * 2):(byte_idx * 2 + 2)], 16))
-        byte_idx += 1
-    return data
+def encode_hex_string(value: str, size: int) -> bytes:
+    assert value.startswith("0x")
+    value = value[2:]
+    if len(value) < (size * 2):
+        value = value.rjust(size * 2, "0")
+    assert len(value) == (size * 2)
+    return bytes.fromhex(value)
 
 
-def encode_address(value, typesize):
+def encode_address(value: str, typesize: int) -> bytes:
     return encode_hex_string(value, 20)
 
 
-def encode_bool(value, typesize):
-    return encode_integer(value, typesize)
+def encode_bool(value: str, typesize: int) -> bytes:
+    return encode_integer(value, 1)
 
 
-def encode_string(value, typesize):
+def encode_string(value: str, typesize: int) -> bytes:
     data = bytearray()
     for char in value:
         data.append(ord(char))
     return data
 
 
-def encode_bytes_fix(value, typesize):
+def encode_bytes_fix(value: str, typesize: int) -> bytes:
     return encode_hex_string(value, typesize)
 
 
-def encode_bytes_dyn(value, typesize):
+def encode_bytes_dyn(value: str, typesize: int) -> bytes:
     # length of the value string
     # - the length of 0x (2)
     # / by the length of one byte in a hex string (2)

src_features/signMessageEIP712/field_hash.c

@@ -228,7 +228,7 @@ static bool field_hash_finalize(const void *const field_ptr,
             return false;
         }
     }
-    path_advance();
+    path_advance(true);
     fh->state = FHS_IDLE;
     ui_712_finalize_field();
     return true;

src_features/signMessageEIP712/path.c

@@ -255,6 +255,7 @@ static bool array_depth_list_push(uint8_t path_idx, uint8_t size) {
     arr = &path_struct->array_depths[path_struct->array_depth_count];
     arr->path_index = path_idx;
     arr->size = size;
+    arr->index = 0;
     path_struct->array_depth_count += 1;
     return true;
 }
@@ -285,11 +286,14 @@ static bool array_depth_list_pop(void) {
  * Updates the path so that it doesn't point to a struct-type field, but rather
  * only to actual fields.
  *
+ * @param[in] skip_if_array skip if path is already pointing at an array
+ * @param[in] stop_at_array stop at the first downstream array
  * @return whether the path update worked or not
  */
-static bool path_update(void) {
+static bool path_update(bool skip_if_array, bool stop_at_array) {
     uint8_t fields_count;
     const void *struct_ptr;
+    const void *starting_field_ptr;
     const void *field_ptr;
     const char *typename;
     uint8_t typename_len;
@@ -298,10 +302,20 @@ static bool path_update(void) {
     if (path_struct == NULL) {
         return false;
     }
-    if ((field_ptr = get_field(NULL)) == NULL) {
+    if ((starting_field_ptr = get_field(NULL)) == NULL) {
         return false;
     }
+    field_ptr = starting_field_ptr;
     while (struct_field_type(field_ptr) == TYPE_CUSTOM) {
+        // check if we meet one of the given conditions
+        if (((field_ptr == starting_field_ptr) && skip_if_array) ||
+            ((field_ptr != starting_field_ptr) && stop_at_array)) {
+            // only if it is the first iteration of that array depth
+            if ((path_struct->array_depths[path_struct->array_depth_count - 1].index == 0) &&
+                struct_field_is_array(field_ptr)) {
+                break;
+            }
+        }
         typename = get_struct_field_typename(field_ptr, &typename_len);
         if ((struct_ptr = get_structn(typename, typename_len)) == NULL) {
             return false;
@@ -313,11 +327,16 @@ static bool path_update(void) {
         if (push_new_hash_depth(true) == false) {
             return false;
         }
-        // get the struct typehash
-        if (type_hash(typename, typename_len, hash) == false) {
-            return false;
+
+        // The only times they are both at false is if we are traversing an empty array,
+        // don't do a typehash in that case
+        if ((skip_if_array != false) || (stop_at_array != false)) {
+            // get the struct typehash
+            if (type_hash(typename, typename_len, hash) == false) {
+                return false;
+            }
+            feed_last_hash_depth(hash);
         }
-        feed_last_hash_depth(hash);
 
         // TODO: Find a better way to show inner structs in verbose mode when it might be
         //       an empty array of structs in which case we don't want to show it but the
@@ -381,7 +400,7 @@ bool path_set_root(const char *const struct_name, uint8_t name_length) {
     struct_state = DEFINED;
 
     // because the first field could be a struct type
-    path_update();
+    path_update(true, true);
     return true;
 }
 
@@ -449,6 +468,9 @@ bool path_new_array_depth(const uint8_t *const data, uint8_t length) {
     }
 
     array_size = *data;
+    if (!path_update(false, array_size > 0)) {
+        return false;
+    }
     array_depth_count_bak = path_struct->array_depth_count;
     for (pidx = 0; pidx < path_struct->depth_count; ++pidx) {
         if ((field_ptr = get_nth_field(NULL, pidx + 1)) == NULL) {
@@ -492,8 +514,8 @@ bool path_new_array_depth(const uint8_t *const data, uint8_t length) {
     }
     if (array_size == 0) {
         do {
-            path_advance();
-        } while (path_struct->array_depth_count != array_depth_count_bak);
+            path_advance(false);
+        } while (path_struct->array_depth_count > array_depth_count_bak);
     }
 
     return true;
@@ -546,8 +568,8 @@ static bool path_advance_in_array(void) {
 
         if ((path_struct->array_depth_count > 0) &&
             (arr_depth->path_index == (path_struct->depth_count - 1))) {
-            if (arr_depth->size > 0) arr_depth->size -= 1;
-            if (arr_depth->size == 0) {
+            arr_depth->index += 1;
+            if (arr_depth->index == arr_depth->size) {
                 array_depth_list_pop();
                 end_reached = true;
             } else {
@@ -563,7 +585,7 @@ static bool path_advance_in_array(void) {
  *
  * @return whether the advancement was successful or not
  */
-bool path_advance(void) {
+bool path_advance(bool array_check) {
     bool end_reached;
 
     do {
@@ -573,8 +595,7 @@ bool path_advance(void) {
             end_reached = false;
         }
     } while (end_reached);
-    path_update();
-    return true;
+    return path_update(array_check, array_check);
 }
 
 /**

src_features/signMessageEIP712/path.h

@@ -12,6 +12,7 @@
 typedef struct {
     uint8_t path_index;
     uint8_t size;
+    uint8_t index;
 } s_array_depth;
 
 typedef enum { ROOT_DOMAIN, ROOT_MESSAGE } e_root_type;
@@ -27,7 +28,7 @@ typedef struct {
 
 bool path_set_root(const char *const struct_name, uint8_t length);
 const void *path_get_field(void);
-bool path_advance(void);
+bool path_advance(bool array_check);
 bool path_init(void);
 void path_deinit(void);
 bool path_new_array_depth(const uint8_t *const data, uint8_t length);

tests/ragger/eip712_input_files/02-recipients_array_mail-data.json

@@ -38,10 +38,6 @@
             { "name": "chainId", "type": "uint256" },
             { "name": "verifyingContract", "type": "address" }
         ],
-        "Group": [
-          { "name": "name", "type": "string" },
-          { "name": "members", "type": "Person[]" }
-        ],
         "Mail": [
             { "name": "from", "type": "Person" },
             { "name": "to", "type": "Person[]" },

tests/ragger/eip712_input_files/03-long_string-data.json

@@ -33,10 +33,6 @@
             { "name": "chainId", "type": "uint256" },
             { "name": "verifyingContract", "type": "address" }
         ],
-        "Group": [
-          { "name": "name", "type": "string" },
-          { "name": "members", "type": "Person[]" }
-        ],
         "Mail": [
             { "name": "from", "type": "Person" },
             { "name": "to", "type": "Person[]" },

tests/ragger/eip712_input_files/04-long_bytes-data.json

@@ -33,10 +33,6 @@
             { "name": "chainId", "type": "uint256" },
             { "name": "verifyingContract", "type": "address" }
         ],
-        "Group": [
-          { "name": "name", "type": "string" },
-          { "name": "members", "type": "Person[]" }
-        ],
         "Mail": [
             { "name": "from", "type": "Person" },
             { "name": "to", "type": "Person[]" },