Add a new APDU PROMPT_UI_DISPLAY to prompt the UI after the addresses…

… checks

Add companion APDUs CHECK_ASSET_IN_NO_DISPLAY and CHECK_REFUND_ADDRESS_NO_DISPLAY
Clarify CHECK_ASSET_IN as the replacement of deprecated CHECK_ASSET_IN_LEGACY for LEGACY flows too
Move FUND_LEGACY and SELL_LEGACY tests to use CHECK_ASSET_IN instead of CHECK_ASSET_IN_LEGACY
Add dedicated tests for CHECK_ASSET_IN_LEGACY
Move lib tests to use PROMPT_UI_DISPLAY
Add dedicated tests for CHECK_ASSET_X_AND_DISPLAY

src/apdu_parser.c

@@ -55,7 +55,7 @@ static uint16_t check_instruction(uint8_t instruction, uint8_t subcommand) {
     // {STATE} if this command is only accepted at a specific state, -1 otherwise
     int check_current_state = -1;
 
-    if (instruction == CHECK_ASSET_IN_AND_DISPLAY &&
+    if ((instruction == CHECK_ASSET_IN_AND_DISPLAY || instruction == CHECK_ASSET_IN_NO_DISPLAY) &&
         (subcommand == SWAP || subcommand == SWAP_NG)) {
         PRINTF("Instruction CHECK_ASSET_IN_AND_DISPLAY is only for SELL and FUND based flows\n");
         return INVALID_INSTRUCTION;
@@ -66,9 +66,10 @@ static uint16_t check_instruction(uint8_t instruction, uint8_t subcommand) {
         return INVALID_INSTRUCTION;
     }
 
-    if (instruction == CHECK_REFUND_ADDRESS_AND_DISPLAY &&
+    if ((instruction == CHECK_REFUND_ADDRESS_AND_DISPLAY ||
+         instruction == CHECK_REFUND_ADDRESS_NO_DISPLAY) &&
         (subcommand != SWAP && subcommand != SWAP_NG)) {
-        PRINTF("Instruction CHECK_REFUND_ADDRESS_AND_DISPLAY is only for SWAP based flows\n");
+        PRINTF("Instruction CHECK_REFUND_ADDRESS_X is only for SWAP based flows\n");
         return INVALID_INSTRUCTION;
     }
 
@@ -104,11 +105,17 @@ static uint16_t check_instruction(uint8_t instruction, uint8_t subcommand) {
             break;
         case CHECK_PAYOUT_ADDRESS:
         case CHECK_ASSET_IN_AND_DISPLAY:
+        case CHECK_ASSET_IN_NO_DISPLAY:
             check_current_state = SIGNATURE_CHECKED;
             check_subcommand_context = true;
             break;
         case CHECK_REFUND_ADDRESS_AND_DISPLAY:
-            check_current_state = TO_ADDR_CHECKED;
+        case CHECK_REFUND_ADDRESS_NO_DISPLAY:
+            check_current_state = PAYOUT_ADDRESS_CHECKED;
+            check_subcommand_context = true;
+            break;
+        case PROMPT_UI_DISPLAY:
+            check_current_state = ALL_ADDRESSES_CHECKED;
             check_subcommand_context = true;
             break;
         case START_SIGNING_TRANSACTION:

src/check_addresses_and_amounts.c

@@ -12,6 +12,7 @@
 #include "menu.h"
 #include "pb_structs.h"
 #include "ticker_normalization.h"
+#include "prompt_ui_display.h"
 
 #include "check_addresses_and_amounts.h"
 
@@ -228,9 +229,8 @@ int check_addresses_and_amounts(const command_t *cmd) {
     }
 
     // Format the fees, except during CHECK_PAYOUT_ADDRESS for SWAP, (it's done in
-    // CHECK_REFUND_ADDRESS_AND_DISPLAY as the fees are in the OUT going currency)
-    if (!((G_swap_ctx.subcommand == SWAP || G_swap_ctx.subcommand == SWAP_NG) &&
-          cmd->ins == CHECK_PAYOUT_ADDRESS)) {
+    // CHECK_REFUND_ADDRESS as the fees are in the OUT going currency)
+    if (cmd->ins != CHECK_PAYOUT_ADDRESS) {
         if (!format_fees(sub_coin_config, application_name)) {
             PRINTF("Error: Failed to format fees amount\n");
             return reply_error(INTERNAL_ERROR);
@@ -250,17 +250,7 @@ int check_addresses_and_amounts(const command_t *cmd) {
         format_account_name();
     }
 
-    // If we are in a SWAP flow at step CHECK_PAYOUT_ADDRESS, we are still waiting for
-    // CHECK_REFUND_ADDRESS_AND_DISPLAY
-    // Otherwise we can trigger the UI to get user validation now
-    if ((G_swap_ctx.subcommand == SWAP || G_swap_ctx.subcommand == SWAP_NG) &&
-        cmd->ins == CHECK_PAYOUT_ADDRESS) {
-        if (reply_success() < 0) {
-            PRINTF("Error: failed to send\n");
-            return -1;
-        }
-        G_swap_ctx.state = TO_ADDR_CHECKED;
-    } else {
+    if (cmd->ins != CHECK_PAYOUT_ADDRESS) {
         // Save the paying coin application_name, we'll need it to start the app during
         // START_SIGNING step
         memcpy(G_swap_ctx.payin_binary_name, application_name, sizeof(application_name));
@@ -270,10 +260,28 @@ int check_addresses_and_amounts(const command_t *cmd) {
         memset(G_swap_ctx.paying_sub_coin_config, 0, sizeof(G_swap_ctx.paying_sub_coin_config));
         memcpy(G_swap_ctx.paying_sub_coin_config, sub_coin_config.bytes, sub_coin_config.size);
 
-        G_swap_ctx.state = WAITING_USER_VALIDATION;
+        // Save the rate. We could have saved it at the first command and then checked at each ŝtep
+        // that it did not change, but it is not certain that the Live sends it right from the
+        // begining and we won't risk regressions for something that is not a security issue.
         G_swap_ctx.rate = cmd->rate;
+    }
 
-        ui_validate_amounts();
+    // Only trigger the UI validation for CHECK_X_AND_DISPLAY
+    if (cmd->ins == CHECK_ASSET_IN_AND_DISPLAY || cmd->ins == CHECK_REFUND_ADDRESS_AND_DISPLAY) {
+        start_ui_display();
+    } else {
+        // No display case, reply the status and update the state machine
+        if (reply_success() < 0) {
+            PRINTF("Error: failed to send\n");
+            return -1;
+        }
+        // If we checked the PAYOUT address (swap flow), we still have the REFUND address to check
+        if (cmd->ins == CHECK_PAYOUT_ADDRESS) {
+            G_swap_ctx.state = PAYOUT_ADDRESS_CHECKED;
+        } else {
+            // Otherwise we are ready to start the display
+            G_swap_ctx.state = ALL_ADDRESSES_CHECKED;
+        }
     }
 
     return 0;

src/command_dispatcher.c

@@ -11,6 +11,7 @@
 #include "check_partner.h"
 #include "start_signing_transaction.h"
 #include "check_addresses_and_amounts.h"
+#include "prompt_ui_display.h"
 
 #include "io.h"
 
@@ -43,14 +44,19 @@ int dispatch_command(const command_t *cmd) {
             break;
         case CHECK_PAYOUT_ADDRESS:
         case CHECK_ASSET_IN_AND_DISPLAY:
+        case CHECK_ASSET_IN_NO_DISPLAY:
         case CHECK_REFUND_ADDRESS_AND_DISPLAY:
+        case CHECK_REFUND_ADDRESS_NO_DISPLAY:
             ret = check_addresses_and_amounts(cmd);
             break;
+        case PROMPT_UI_DISPLAY:
+            ret = prompt_ui_display(cmd);
+            break;
         case START_SIGNING_TRANSACTION:
             ret = start_signing_transaction(cmd);
             break;
         default:
-            __builtin_unreachable();
+            __builtin_trap();
             break;
     }
 

src/commands.h

@@ -14,9 +14,12 @@ typedef enum {
     PROCESS_TRANSACTION_RESPONSE_COMMAND = 0x06,
     CHECK_TRANSACTION_SIGNATURE_COMMAND = 0x07,
     CHECK_PAYOUT_ADDRESS = 0x08,
-    CHECK_ASSET_IN_LEGACY_AND_DISPLAY = 0x08,  // Same ID as CHECK_PAYOUT_ADDRESS
+    CHECK_ASSET_IN_LEGACY_AND_DISPLAY = 0x08,  // Same ID as CHECK_PAYOUT_ADDRESS, deprecated
     CHECK_ASSET_IN_AND_DISPLAY = 0x0B,         // Do note the 0x0B
+    CHECK_ASSET_IN_NO_DISPLAY = 0x0D,          // Do note the 0x0B
     CHECK_REFUND_ADDRESS_AND_DISPLAY = 0x09,
+    CHECK_REFUND_ADDRESS_NO_DISPLAY = 0x0C,
+    PROMPT_UI_DISPLAY = 0x0F,
     START_SIGNING_TRANSACTION = 0x0A,
 } command_e;
 

src/prompt_ui_display.c

@@ -0,0 +1,17 @@
+#include "globals.h"
+#include "commands.h"
+#include "states.h"
+#include "prompt_ui_display.h"
+#include "validate_transaction.h"
+
+void start_ui_display(void) {
+    G_swap_ctx.state = WAITING_USER_VALIDATION;
+    ui_validate_amounts();
+}
+
+int prompt_ui_display(const command_t *cmd) {
+    // We don't care about the command passed as argument
+    UNUSED(cmd);
+    start_ui_display();
+    return 0;
+}

src/prompt_ui_display.h

@@ -0,0 +1,6 @@
+#pragma once
+
+#include "commands.h"
+
+void start_ui_display(void);
+int prompt_ui_display(const command_t *cmd);

src/states.h

@@ -1,15 +1,16 @@
 #pragma once
 
 typedef enum {
-    INITIAL_STATE = 0,
-    WAITING_TRANSACTION = 1,
-    PROVIDER_SET = 2,
-    PROVIDER_CHECKED = 3,
-    TRANSACTION_RECEIVED = 4,
-    SIGNATURE_CHECKED = 5,
-    TO_ADDR_CHECKED = 6,
-    WAITING_USER_VALIDATION = 7,
-    WAITING_SIGNING = 8,
-    SIGN_FINISHED = 9,
+    INITIAL_STATE,
+    WAITING_TRANSACTION,
+    PROVIDER_SET,
+    PROVIDER_CHECKED,
+    TRANSACTION_RECEIVED,
+    SIGNATURE_CHECKED,
+    PAYOUT_ADDRESS_CHECKED,
+    ALL_ADDRESSES_CHECKED,
+    WAITING_USER_VALIDATION,
+    WAITING_SIGNING,
+    SIGN_FINISHED,
     STATE_UPPER_BOUND,
 } state_e;

test/python/apps/exchange.py

@@ -20,13 +20,13 @@ class Command(IntEnum):
     CHECK_PARTNER                     = 0x05
     PROCESS_TRANSACTION_RESPONSE      = 0x06
     CHECK_TRANSACTION_SIGNATURE       = 0x07
-    CHECK_PAYOUT_ADDRESS              = 0x08
     CHECK_ASSET_IN_LEGACY_AND_DISPLAY = 0x08
-    CHECK_ASSET_IN_LEGACY_NO_DISPLAY  = 0x0E
     CHECK_ASSET_IN_AND_DISPLAY        = 0x0B
     CHECK_ASSET_IN_NO_DISPLAY         = 0x0D
+    CHECK_PAYOUT_ADDRESS              = 0x08
     CHECK_REFUND_ADDRESS_AND_DISPLAY  = 0x09
     CHECK_REFUND_ADDRESS_NO_DISPLAY   = 0x0C
+    PROMPT_UI_DISPLAY                 = 0x0F
     START_SIGNING_TRANSACTION         = 0x0A
 
 
@@ -134,20 +134,26 @@ def check_refund_address(self, refund_configuration) -> Generator[None, None, No
         with self._exchange_async(Command.CHECK_REFUND_ADDRESS_AND_DISPLAY, payload=refund_configuration) as response:
             yield response
 
+    def check_refund_address_no_display(self, refund_configuration) -> RAPDU:
+        return self._exchange(Command.CHECK_REFUND_ADDRESS_NO_DISPLAY, payload=refund_configuration)
+
+    @contextmanager
+    def check_asset_in_legacy(self, payout_configuration: bytes) -> Generator[None, None, None]:
+        with self._exchange_async(Command.CHECK_ASSET_IN_LEGACY_AND_DISPLAY, payload=payout_configuration) as response:
+            yield response
+
     @contextmanager
     def check_asset_in(self, payout_configuration: bytes) -> Generator[None, None, None]:
-        if self._subcommand == SubCommand.SELL or self._subcommand == SubCommand.FUND:
-            ins = Command.CHECK_ASSET_IN_LEGACY_AND_DISPLAY
-        else:
-            ins = Command.CHECK_ASSET_IN_AND_DISPLAY
-        with self._exchange_async(ins, payload=payout_configuration) as response:
+        with self._exchange_async(Command.CHECK_ASSET_IN_AND_DISPLAY, payload=payout_configuration) as response:
             yield response
 
-    def get_check_address_response(self) -> RAPDU:
-        if self._premature_error:
-            return self._check_address_result
-        else:
-            return self._client.last_async_response
+    def check_asset_in_no_display(self, payout_configuration: bytes) -> RAPDU:
+        return self._exchange(Command.CHECK_ASSET_IN_NO_DISPLAY, payload=payout_configuration)
+
+    @contextmanager
+    def prompt_ui_display(self) -> Generator[None, None, None]:
+        with self._exchange_async(Command.PROMPT_UI_DISPLAY) as response:
+            yield response
 
     def start_signing_transaction(self) -> RAPDU:
         rapdu = self._exchange(Command.START_SIGNING_TRANSACTION)

test/python/apps/exchange_test_runner.py

@@ -117,7 +117,8 @@ def _perform_valid_exchange(self, subcommand, tx_infos, from_currency_configurat
             ex.check_payout_address(to_configuration)
 
             # Request the final address check and UI approval request on the device
-            with ex.check_refund_address(from_configuration):
+            ex.check_refund_address_no_display(from_configuration)
+            with ex.prompt_ui_display():
                 if ui_validation:
                     self.exchange_navigation_helper.simple_accept()
                 else:
@@ -126,7 +127,8 @@ def _perform_valid_exchange(self, subcommand, tx_infos, from_currency_configurat
                     # As a workaround, we avoid calling the navigation if we want the function to raise
                     pass
         else:
-            with ex.check_asset_in(from_configuration):
+            ex.check_asset_in_no_display(from_configuration)
+            with ex.prompt_ui_display():
                 if ui_validation:
                     self.exchange_navigation_helper.simple_accept()
                 else:

test/python/test_check_address_and_display.py

@@ -0,0 +1,75 @@
+import pytest
+from ragger.utils import prefix_with_len
+
+from .apps.exchange import ExchangeClient, Rate, SubCommand
+from .apps.litecoin import LitecoinClient
+
+from .apps.signing_authority import SigningAuthority, LEDGER_SIGNER
+from .apps.exchange_transaction_builder import get_partner_curve, craft_and_sign_tx, ALL_SUBCOMMANDS, get_credentials
+from .apps import cal as cal
+
+CURRENCY_FROM = cal.ETH_CURRENCY_CONFIGURATION
+CURRENCY_TO = cal.BTC_CURRENCY_CONFIGURATION
+
+SWAP_TX_INFOS = {
+     "payin_address": b"0xd692Cb1346262F584D17B4B470954501f6715a82",
+     "payin_extra_id": b"",
+     "refund_address": b"0xDad77910DbDFdE764fC21FCD4E74D71bBACA6D8D",
+     "refund_extra_id": b"",
+     "payout_address": b"bc1qqtl9jlrwcr3fsfcjj2du7pu6fcgaxl5dsw2vyg",
+     "payout_extra_id": b"",
+     "currency_from": CURRENCY_FROM.ticker,
+     "currency_to": CURRENCY_TO.ticker,
+     "amount_to_provider": bytes.fromhex("013fc3a717fb5000"),
+     "amount_to_wallet": b"\x0b\xeb\xc2\x00",
+}
+FUND_TX_INFOS = {
+    "user_id": "John Wick",
+    "account_name": "Remember Daisy",
+    "in_currency": CURRENCY_FROM.ticker,
+    "in_amount": b"\032\200\250]$T\000",
+    "in_address": "0x252fb4acbe0de4f0bd2409a5ed59a71e4ef1d2bc"
+}
+SELL_TX_INFOS = {
+    "trader_email": "[email protected]",
+    "out_currency": "USD",
+    "out_amount": {"coefficient": b"\x01", "exponent": 3},
+    "in_currency": CURRENCY_FROM.ticker,
+    "in_amount": b"\032\200\250]$T\000",
+    "in_address": "0x252fb4acbe0de4f0bd2409a5ed59a71e4ef1d2bc"
+}
+TX_INFOS = {
+    SubCommand.SWAP: SWAP_TX_INFOS,
+    SubCommand.SWAP_NG: SWAP_TX_INFOS,
+    SubCommand.FUND: FUND_TX_INFOS,
+    SubCommand.FUND_NG: FUND_TX_INFOS,
+    SubCommand.SELL: SELL_TX_INFOS,
+    SubCommand.SELL_NG: SELL_TX_INFOS,
+}
+
+class TestCheckAddressAndDisplay:
+
+    @pytest.mark.parametrize("subcommand", ALL_SUBCOMMANDS)
+    def test_check_address_and_display(self, backend, exchange_navigation_helper, subcommand):
+        suffix = "_" + str(subcommand).split('.')[1].split('_')[0].lower()
+        exchange_navigation_helper.set_test_name_suffix(suffix)
+
+        ex = ExchangeClient(backend, Rate.FIXED, subcommand)
+        partner = SigningAuthority(curve=get_partner_curve(subcommand), name="Default name")
+
+        transaction_id = ex.init_transaction().data
+        credentials = get_credentials(subcommand, partner)
+        ex.set_partner_key(credentials)
+        ex.check_partner_key(LEDGER_SIGNER.sign(credentials))
+
+        tx, tx_signature = craft_and_sign_tx(subcommand, TX_INFOS[subcommand], transaction_id, 339, partner)
+        ex.process_transaction(tx)
+        ex.check_transaction_signature(tx_signature)
+
+        if subcommand == SubCommand.SWAP or subcommand == SubCommand.SWAP_NG:
+            ex.check_payout_address(CURRENCY_TO.get_conf_for_ticker())
+            with ex.check_refund_address(CURRENCY_FROM.get_conf_for_ticker()):
+                exchange_navigation_helper.simple_accept()
+        else:
+            with ex.check_asset_in(CURRENCY_FROM.get_conf_for_ticker()):
+                exchange_navigation_helper.simple_accept()

test/python/test_custom.py

@@ -0,0 +1,23 @@
+# import pytest
+
+# from ragger.utils import RAPDU, prefix_with_len, create_currency_config
+# from ragger.error import ExceptionRAPDU
+
+# from .apps.exchange import ExchangeClient, Rate, SubCommand, Errors, Command, P2_EXTEND, P2_MORE, EXCHANGE_CLASS
+# from .apps.exchange_transaction_builder import get_partner_curve, LEGACY_SUBCOMMANDS, ALL_SUBCOMMANDS, NEW_SUBCOMMANDS, get_credentials, craft_and_sign_tx
+# from .apps.signing_authority import SigningAuthority, LEDGER_SIGNER
+# from .apps import cal as cal
+
+# from time import sleep
+
+# def test_custom(backend):
+#     backend.exchange_raw(bytes.fromhex("e003010000"))
+#     backend.exchange_raw(bytes.fromhex("e00401004b094368616e67656c6c790480d7c0d3a9183597395f58dda05999328da6f18fabd5cda0aff8e8e3fc633436a2dbf48ecb23d40df7c3c7d3e774b77b4b5df0e9f7e08cf1cdf2dba788eb085b"))
+#     backend.exchange_raw(bytes.fromhex("e0050100473045022100e73339e5071b5d232e8cacecbd7c118c919122a43f8abb8b2062d4bfcd58274e022050b11605d8b7e199f791266146227c43fd11d7645b1d881f705a2f8841d21de5"))
+#     backend.exchange_raw(bytes.fromhex("e0060100b4af0a225441347170665658613773584278613673667164347a4c6b4b5741625069333478671a22545046507459724e756636653971573237374e79373447576b77683259376b6d334a2a2a3078386331353633373964636665373631316333346231344238306341373437323837634636413337343a035452584204555344544a1000000000000000000000000024eee965521000000000000000000000000003b9b9eb5a0a46414d59545947595851030f4240"))
+#     backend.exchange_raw(bytes.fromhex("e00701004630440220039c3f8f1bfe9f87a7642d28d26a2a8c46f34e77fc9982dfd6fbf3bdb1577d1c0220021a77b987c63be9562747616fb2cfbf960a58dcdd02092afb6f9cb3a17ecbc7"))
+#     backend.exchange_raw(bytes.fromhex("e00801007315045553445408457468657265756d060455534454063045022100c2eb8f0cfd072affe2aa658eff831c19040542d7d960848ecdedc4f1d4a6f4ef02205ef9b141789f280482de903f8f9aacbea7f1f43db8e5404e7a197fbfeb736c9715058000002c8000003c800000000000000000000000"))
+#     backend.exchange_raw(bytes.fromhex("e00901006d0f035452580454726f6e0503545258003045022100cc308621933e8e94ef6f0a1c05cd6e766a4c4131e0b320cdd8c0f02d7bdb919002206b18b6e92c0c7772be31c61bfbb6879f5e2f2f4e335294184a71e0e1e319c7cb15058000002c800000c3800000000000000000000000"))
+#     backend.exchange_raw(bytes.fromhex("e00a010000"))
+#     backend.exchange_raw(bytes.fromhex("e002000015058000002c800000c3800000000000000000000000"))
+#     backend.exchange_raw(bytes.fromhex("e00410009c058000002c800000c38000000000000000000000000a02e40c220865e37291a9b34ee140e8a8abd0f0315a69080112650a2d747970652e676f6f676c65617069732e636f6d2f70726f746f636f6c2e5472616e73666572436f6e747261637412340a154191a961158bba9a35d8f33e02bf3404147cfd3c101215410110d0f5dc152691ab483dd2c59ef072fb46dad018e5d2bba70270e28f83d0f031"))