Merge pull request #5 from functori/merge-latest-ledgerhq-app-exchange

Merge latest ledgerhq app exchange

.codespellignore

@@ -0,0 +1 @@
+nin

.github/workflows/ci-workflow.yml

@@ -13,3 +13,5 @@ jobs:
     uses: ./.github/workflows/reusable_swap_functional_tests.yml
     with:
       branch_for_exchange: ${{ github.ref }}
+      run_for_devices: '["nanos", "nanosp"]'
+      test_filter: '"tezos_new"'

.github/workflows/misspellings_checks.yml

@@ -0,0 +1,30 @@
+name: Misspellings checks
+
+# This workflow performs some misspelling checks on the repository
+# It is there to help us maintain a level of quality in our codebase and does not have to be kept on forked
+# applications.
+
+on:
+  workflow_dispatch:
+  push:
+    branches:
+      - master
+      - main
+      - develop
+  pull_request:
+
+jobs:
+  misspell:
+    name: Check misspellings
+    runs-on: ubuntu-latest
+    steps:
+    - name: Clone
+      uses: actions/checkout@v3
+
+    - name: Check misspellings
+      uses: codespell-project/actions-codespell@v1
+      with:
+        builtin: clear,rare
+        check_filenames: true
+        ignore_words_file: .codespellignore
+        path: src/,test/python/,protocol.md,Makefile,README.md

.github/workflows/reusable_swap_functional_tests.yml

@@ -5,7 +5,7 @@ on:
     inputs:
       repo_for_exchange:
         required: false
-        default: 'LedgerHQ/app-exchange'
+        default: 'functori/app-exchange'
         type: string
       branch_for_exchange:
         required: false

Makefile

@@ -32,8 +32,8 @@ APP_LOAD_PARAMS += $(COMMON_LOAD_PARAMS)
 
 APPNAME      = "Exchange"
 APPVERSION_M = 3
-APPVERSION_N = 1
-APPVERSION_P = 0
+APPVERSION_N = 3
+APPVERSION_P = 2
 APPVERSION   = $(APPVERSION_M).$(APPVERSION_N).$(APPVERSION_P)-dev
 
 ifdef TESTING
@@ -70,12 +70,11 @@ DEFINES   += LEDGER_MAJOR_VERSION=$(APPVERSION_M) LEDGER_MINOR_VERSION=$(APPVERS
 DEFINES   += OS_IO_SEPROXYHAL
 DEFINES   += HAVE_SPRINTF
 DEFINES   += HAVE_IO_USB HAVE_L4_USBLIB IO_USB_MAX_ENDPOINTS=4 IO_HID_EP_LENGTH=64 HAVE_USB_APDU
+DEFINES   += HAVE_WEBUSB WEBUSB_URL_SIZE_B=0 WEBUSB_URL=""
 
 DEFINES   += USB_SEGMENT_SIZE=64
 DEFINES   += BLE_SEGMENT_SIZE=32 #max MTU, min 20
 
-DEFINES   += UNUSED\(x\)=\(void\)x
-
 ifeq ($(TARGET_NAME),$(filter $(TARGET_NAME),TARGET_NANOX TARGET_STAX))
     DEFINES += HAVE_BLE BLE_COMMAND_TIMEOUT_MS=2000 HAVE_BLE_APDU
 endif

ledger_app.toml

@@ -0,0 +1,7 @@
+[app]
+build_directory = "./"
+sdk = "C"
+devices = ["nanos", "nanox", "nanos+", "stax"]
+
+[tests]
+pytest_directory = "./test/python/"

protocol.md

@@ -1,6 +1,6 @@
 # Protocol to speak with EXCHANGE application
 
-Communication is done through a serie of request-response exchanges (APDU / RAPDU).
+Communication is done through a series of request-response exchanges (APDU / RAPDU).
 
 
 ## Request:
@@ -19,7 +19,7 @@ Communication is done through a serie of request-response exchanges (APDU / RAPD
 
 | Name                         | Value | Description                                                                           |
 | ---------------------------- | ----- | ------------------------------------------------------------------------------------- |
-| GET_VERSION                  | 0x02  | Get application version. This APDU can be sent independantly of the current app state |
+| GET_VERSION                  | 0x02  | Get application version. This APDU can be sent independently of the current app state |
 | START_NEW_TRANSACTION        | 0x03  | Start new EXCHANGE transaction. This APDU resets the app state                        |
 | SET_PARTNER_KEY              | 0x04  | Set the credentials of the exchange partner                                           |
 | CHECK_PARTNER                | 0x05  | Check that the credentials of the exchange partner are signed by the Ledger key       |
@@ -111,12 +111,25 @@ No data expected.
 
 #### SET_PARTNER_KEY
 
+##### For all LEGACY TYPES, the data for this command is:
+
 | Bytes              | Description                          |
 | ------------------ | ------------------------------------ |
 | 1 byte             | Length N of the encoded partner name |
 | N bytes            | Partner name encoded with utf-8      |
 | LC - (1 + N) bytes | Partner public key                   |
 
+##### For all UNIFIED TYPES, the data for this command is:
+
+| Bytes              | Description                          |
+| ------------------ | ------------------------------------ |
+| 1 byte             | Length N of the encoded partner name |
+| N bytes            | Partner name encoded with utf-8      |
+| 1 byte             | Curve used by the partner            |
+| LC - (2 + N) bytes | Partner public key                   |
+
+With the possible values for the curve being 0x00 for SECP256K1, and 0x01 for SECP256R1.
+
 #### CHECK_PARTNER
 
 | Bytes    | Description                                                                             |
@@ -125,31 +138,56 @@ No data expected.
 
 #### PROCESS_TRANSACTION_RESPONSE
 
-| Bytes        | Description                                                                                     |
-| ------------ | ----------------------------------------------------------------------------------------------- |
-| 1 or 2 bytes | Length N of the encoded transaction proposal. 1 bytes on Legacy TYPES, 2 bytes on new TYPES     |
-| N bytes      | Transaction proposal. No encoding for SWAP_LEGACY, URLsafe base 64 encoding for all other TYPES |
-| 1 byte       | Length M of the transaction fees                                                                |
-| M bytes      | Transaction fees                                                                                |
-
 Please refer to the src/protobuf files for the actual transaction proposal content.
 
+##### For all LEGACY TYPES, the data for this command is:
+
+| Bytes   | Description                                                                                                 |
+| ------- | ----------------------------------------------------------------------------------------------------------- |
+| 1 byte  | Length N of the encoded transaction proposal                                                                |
+| N bytes | Transaction proposal. Bytes array for SWAP_LEGACY, URLsafe base 64 encoding for SELL_LEGACY and FUND_LEGACY |
+| 1 byte  | Length M of the transaction fees                                                                            |
+| M bytes | Transaction fees                                                                                            |
+
+##### For all UNIFIED TYPES, the data for this command is:
+
+| Bytes   | Description                                  |
+| ------- | -------------------------------------------- |
+| 1 byte  | Format used for the transaction encoding     |
+| 2 bytes | Length N of the encoded transaction proposal |
+| N bytes | Encoded transaction proposal                 |
+| 1 byte  | Length M of the transaction fees             |
+| M bytes | Transaction fees                             |
+
+With the possible values for the format being 0x00 for Bytes Array (no encoding), and 0x01 for Base 64 Url encoding.
+
 The DATA of this command may exceed the capacity of a single APDU (255 bytes), in this case use the EXTENSION feature.
 
 #### CHECK_TRANSACTION_SIGNATURE
 
-| Bytes    | Description                                                                                                |
-| -------- | ---------------------------------------------------------------------------------------------------------- |
-| LC bytes | Signature of the proposed transaction by the partner.                                                      |
+##### For all LEGACY TYPES, the data for this command is:
+
+| Bytes    | Description                                                   |
+| -------- | ------------------------------------------------------------- |
+| LC bytes | Signature of the computed transaction proposed by the partner |
+
+For SWAP_LEGACY TYPE, the signature is computed on the transaction proposal. \
+For SELL_LEGACY and FUND_LEGACY the signature is computed on the transaction proposal prefixed with a DOT ('.').
+
+For SWAP_LEGACY and FUND_LEGACY, the signature is in DER format. \
+For SELL_LEGACY the signature is in (R,S) format.
+
+##### For all UNIFIED TYPES, the data for this command is:
 
-For SWAP_LEGACY TYPE, the signature is computed on the sha256 hash of the transaction. \
-For all other TYPES the signature is computed on the sha256 hash of the transaction prefixed with a DOT ('.').
+| Bytes        | Description                                                   |
+| ------------ | ------------------------------------------------------------- |
+| 1 byte       | If the signature is computed on a prefixed transaction        |
+| 1 byte       | Format of the signature itself                                |
+| LC - 2 bytes | Signature of the computed transaction proposed by the partner |
 
-For SWAP_LEGACY and FUND_LEGACY TYPES, the signature is in DER format. \
-For all other TYPES the signature is in (R,S) format.
+With the possible values for the format of the transaction used for signing being 0x01 if it was DOT ('.') prefixed, 0x00 otherwise.
 
-For SWAP_LEGACY TYPE, the signature is computed on the secp256k1 curve. \
-For all other TYPES the signature is computed on the secp256r1 curve.
+With the possible values for the format of the signature itself being 0x00 for DER format, and 0x01 for (R,S) format.
 
 #### CHECK_ASSET_IN_LEGACY
 

src/apdu_parser.c

@@ -36,9 +36,9 @@ typedef struct apdu_s {
     uint8_t rate;
     uint8_t subcommand;
     uint16_t data_length;
-    // We could have put the recontructed apdu buffer here but it would increase the RAM usage by
+    // We could have put the reconstructed apdu buffer here but it would increase the RAM usage by
     // 512 bytes which is a lot on NANOS
-    // Instead the recontructed apdu buffer is G_swap_ctx.raw_transaction
+    // Instead the reconstructed apdu buffer is G_swap_ctx.raw_transaction
     // It is unionized with the decoded protobuf transaction requests
     // Pro: less memory usage
     // Cons: use cautiously and only during the command PROCESS_TRANSACTION_RESPONSE_COMMAND
@@ -48,8 +48,11 @@ apdu_t G_received_apdu;
 
 // Dedicated function for instruction checking as it's self contained
 static uint16_t check_instruction(uint8_t instruction, uint8_t subcommand) {
+    // True if the instruction is part of a flow and the context must be checked
     bool check_subcommand_context = false;
+    // True if the instruction can be received between user approval and child lib start
     bool allowed_during_waiting_for_signing = false;
+    // {STATE} if this command is only accepted at a specific state, -1 otherwise
     int check_current_state = -1;
 
     if (instruction == CHECK_ASSET_IN && (subcommand == SWAP || subcommand == SWAP_NG)) {
@@ -62,11 +65,16 @@ static uint16_t check_instruction(uint8_t instruction, uint8_t subcommand) {
         return INVALID_INSTRUCTION;
     }
 
+    if (instruction == CHECK_REFUND_ADDRESS && subcommand != SWAP && subcommand != SWAP_NG) {
+        PRINTF("Instruction CHECK_REFUND_ADDRESS is only for SWAP based flows\n");
+        return INVALID_INSTRUCTION;
+    }
+
     switch (instruction) {
         case GET_VERSION_COMMAND:
             // We ignore the current context for this command as it doesn't modify anything
             check_subcommand_context = false;
-            // No strict dependancy on the current state as long as it is not a protected state
+            // No strict dependency on the current state as long as it is not a protected state
             // (WAITING_USER_VALIDATION and WAITING_SIGNING)
             check_current_state = -1;
             break;

src/buffer.c

@@ -2,37 +2,37 @@
 #include "os.h"
 
 // Parse a buffer at a given offset to read a buf_t, the offset is incremented accordingly
-// param[in]        <in_buffer>  the total buffer to parse
-// param[in]        <in_size>   the total size of the buffer to parse
-// param[out]       <out>       the buf_t read from in_buffer at offset, can by 0 sized
-// param[in/out]    <offset>    the current offset at wich we are in <in_buffer>
+// param[in]        <in_buffer>            the total buffer to parse
+// param[in]        <in_size>              the total size of the buffer to parse
+// param[in]        <size_of_length_field> the size of the length field in the header
+// param[out]       <out>                  the buf_t read from in_buffer at offset, can by 0 sized
+// param[in/out]    <offset>               the current offset at which we are in <in_buffer>
 bool parse_to_sized_buffer(uint8_t *in_buffer,
                            uint16_t in_size,
-                           uint8_t size_of_lenght_field,
+                           uint8_t size_of_length_field,
                            buf_t *out,
                            uint16_t *offset) {
-    if (*offset + size_of_lenght_field > in_size) {
+    if (*offset + size_of_length_field > in_size) {
         // We can't even read the size
         PRINTF("Failed to read the header sized %d, only %d bytes available\n",
-               size_of_lenght_field,
+               size_of_length_field,
                in_size);
         return false;
     }
 
     // Read the size
-    if (size_of_lenght_field == 1) {
+    if (size_of_length_field == 1) {
         out->size = in_buffer[*offset];
-    } else if (size_of_lenght_field == 2) {
+    } else if (size_of_length_field == 2) {
         out->size = U2BE(in_buffer, *offset);
-    } else if (size_of_lenght_field == 4) {
-        out->size = U4BE(in_buffer, *offset);
     } else {
-        PRINTF("Unable to read a %d sized header\n", size_of_lenght_field);
+        PRINTF("Unable to read a %d sized header\n", size_of_length_field);
         return false;
     }
-    *offset += size_of_lenght_field;
+    *offset += size_of_length_field;
 
-    if (*offset + out->size > in_size) {
+    // Cast before comparison to avoid potential int overflow
+    if (((uint32_t) *offset) + out->size > in_size) {
         PRINTF("Not enough remaining bytes to read. Total %d, offset %d, claims %d bytes\n",
                in_size,
                *offset,
@@ -50,3 +50,19 @@ bool parse_to_sized_buffer(uint8_t *in_buffer,
 
     return true;
 }
+
+// Parse a buffer at a given offset to read a buf_t, the offset is incremented accordingly
+// param[in]        <in_buffer>  the total buffer to parse
+// param[in]        <in_size>   the total size of the buffer to parse
+// param[out]       <out>       the int read from in_buffer at offset
+// param[in/out]    <offset>    the current offset at which we are in <in_buffer>
+bool pop_uint8_from_buffer(uint8_t *in_buffer, uint16_t in_size, uint8_t *out, uint16_t *offset) {
+    if (*offset + 1 > in_size) {
+        PRINTF("Failed to read the uint, only %d bytes available\n", in_size);
+        return false;
+    }
+    *out = in_buffer[*offset];
+    *offset += 1;
+
+    return true;
+}