Merge pull request #196 from LedgerHQ/feewarning

Warn if fees are above 10% of total amount

src/handler/sign_psbt.c

@@ -1328,6 +1328,15 @@ confirm_transaction(dispatcher_context_t *dc, sign_psbt_state_t *st) {
             return false;
         }
     } else {
+        // if the value of fees is 10% or more of the amount, and it's more than 10000
+        if (10 * fee >= st->inputs_total_amount && st->inputs_total_amount > 10000) {
+            if (!ui_warn_high_fee(dc)) {
+                SEND_SW(dc, SW_DENY);
+                ui_post_processing_confirm_transaction(dc, false);
+                return false;
+            }
+        }
+
         // Show final user validation UI
         bool is_self_transfer = st->outputs.n_external == 0;
         if (!ui_validate_transaction(dc, COIN_COINID_SHORT, fee, is_self_transfer)) {

src/ui/display.c

@@ -199,6 +199,12 @@ bool ui_validate_output(dispatcher_context_t *context,
     return io_ui_process(context, true);
 }
 
+bool ui_warn_high_fee(dispatcher_context_t *context) {
+    ui_warn_high_fee_flow();
+
+    return io_ui_process(context, true);
+}
+
 bool ui_validate_transaction(dispatcher_context_t *context,
                              const char *coin_name,
                              uint64_t fee,

src/ui/display.h

@@ -132,6 +132,8 @@ bool ui_validate_output(dispatcher_context_t *context,
                         const char *coin_name,
                         uint64_t amount);
 
+bool ui_warn_high_fee(dispatcher_context_t *context);
+
 bool ui_validate_transaction(dispatcher_context_t *context,
                              const char *coin_name,
                              uint64_t fee,
@@ -165,6 +167,8 @@ void ui_display_output_address_amount_flow(int index);
 
 void ui_display_output_address_amount_no_index_flow(int index);
 
+void ui_warn_high_fee_flow(void);
+
 void ui_accept_transaction_flow(bool is_self_transfer);
 
 void ui_display_transaction_prompt(const int external_outputs_total_count);

src/ui/display_bagl.c

@@ -167,6 +167,25 @@ UX_STEP_NOCB(ux_validate_address_step,
                  .text = g_ui_state.validate_output.address_or_description,
              });
 
+// Step with eye icon and a "high fees" warning
+#ifdef TARGET_NANOS
+UX_STEP_NOCB(ux_high_fee_step,
+             pnn,
+             {
+                 &C_icon_eye,
+                 "Fees over 10%",
+                 "of total value!",
+             });
+#else
+UX_STEP_NOCB(ux_high_fee_step,
+             pnn,
+             {
+                 &C_icon_eye,
+                 "Fees are above 10%",
+                 "of total amount!",
+             });
+#endif
+
 UX_STEP_NOCB(ux_confirm_transaction_step, pnn, {&C_icon_eye, "Confirm", "transaction"});
 UX_STEP_NOCB(ux_confirm_selftransfer_step, pnn, {&C_icon_eye, "Confirm", "self-transfer"});
 UX_STEP_NOCB(ux_confirm_transaction_fees_step,
@@ -386,6 +405,16 @@ UX_FLOW(ux_display_output_address_amount_flow,
         &ux_display_approve_step,
         &ux_display_reject_step);
 
+// Finalize see the transaction fees and finally accept signing
+// #1 screen: eye icon + "Confirm transaction"
+// #2 screen: fee amount
+// #3 screen: "Continue", with approve button
+// #4 screen: reject button
+UX_FLOW(ux_warn_high_fee_flow,
+        &ux_high_fee_step,
+        &ux_display_continue_step,
+        &ux_display_reject_step);
+
 // Finalize see the transaction fees and finally accept signing
 // #1 screen: eye icon + "Confirm transaction"
 // #2 screen: fee amount
@@ -466,6 +495,10 @@ void ui_display_output_address_amount_no_index_flow(int index) {
     ui_display_output_address_amount_flow(index);
 }
 
+void ui_warn_high_fee_flow(void) {
+    ux_flow_init(0, ux_warn_high_fee_flow, NULL);
+}
+
 void ui_accept_transaction_flow(bool is_self_transfer) {
     ux_flow_init(0,
                  is_self_transfer ? ux_accept_selftransfer_flow : ux_accept_transaction_flow,

src/ui/display_nbgl.c

@@ -469,6 +469,15 @@ void ui_display_default_wallet_address_flow(void) {
 }
 
 // Warning Flows
+void ui_warn_high_fee_flow(void) {
+    nbgl_useCaseChoice(&C_round_warning_64px,
+                       "Warning",
+                       "Fees are above 10%\n of total amount",
+                       "Continue",
+                       "Reject",
+                       ux_flow_response);
+}
+
 void ui_display_warning_external_inputs_flow(void) {
     nbgl_useCaseChoice(&C_round_warning_64px,
                        "Warning",

tests/automations/sign_message_accept.json

@@ -28,7 +28,7 @@
       ]
     },
     {
-      "regexp": "[S]?ign",
+      "regexp": "Sign",
       "conditions": [
         [ "seen", false ]
       ],
@@ -39,7 +39,7 @@
       ]
     },
     {
-      "regexp": "[S]?ign",
+      "regexp": "Sign",
       "conditions": [
         [ "seen", true ]
       ],

tests/automations/sign_message_reject.json

@@ -9,7 +9,7 @@
       ]
     },
     {
-      "regexp": "[S]?ign|Path|Message hash|Approve",
+      "regexp": "Sign|Path|Message hash|Approve",
       "actions": [
         ["button", 2, true],
         ["button", 2, false],

tests/automations/sign_with_default_wallet_accept_highfee.json

@@ -0,0 +1,37 @@
+{
+  "version": 1,
+  "rules": [
+    {
+      "regexp": "Tap to continue|Fees are above 10%",
+      "actions": [
+        ["button", 2, true],
+        ["button", 2, false],
+        ["finger", 55, 550, true],
+        ["finger", 55, 550, false]
+      ]
+    },
+    {
+      "regexp": "Approve|Accept|Hold to sign|Continue",
+      "actions": [
+        [ "button", 1, true ],
+        [ "button", 2, true ],
+        [ "button", 2, false ],
+        [ "button", 1, false ],
+        [ "finger", 55, 550, true]
+      ]
+    },
+    {
+      "regexp": "Review|Amount|Address|Confirm|Fees",
+      "actions": [
+        ["button", 2, true],
+        ["button", 2, false]
+      ]
+    },
+    {
+      "regexp": "SIGNED",
+      "actions": [
+        ["finger", 55, 550, false]
+      ]
+    }
+  ]
+}

tests/automations/sign_with_wallet_accept.json

@@ -2,7 +2,7 @@
   "version": 1,
   "rules": [
     {
-      "regexp": "[S]?pend from|Wallet name|Review|Amount|Address|Confirm|Fees",
+      "regexp": "Spend from|Wallet name|Review|Amount|Address|Confirm|Fees",
       "actions": [
         ["button", 2, true],
         ["button", 2, false]

tests/automations/sign_with_wallet_external_inputs_accept.json

@@ -8,7 +8,7 @@
       ]
     },
     {
-      "regexp": "[S]?pend from|Wallet name|There are|Reject if you['-]re|Review|Amount|Address|Confirm|Fees",
+      "regexp": "Spend from|Wallet name|There are|Reject if you['-]re|Review|Amount|Address|Confirm|Fees",
       "actions": [
         ["button", 2, true],
         ["button", 2, false]

tests/automations/sign_with_wallet_missing_nonwitnessutxo_accept.json

@@ -35,7 +35,7 @@
       ]
     },
     {
-      "regexp": "Unverified|Update|or third party|[S]?pend from|Wallet name|Review|Amount|Address|Confirm|Fees",
+      "regexp": "Unverified|Update|or third party|Spend from|Wallet name|Review|Amount|Address|Confirm|Fees",
       "actions": [
         ["button", 2, true],
         ["button", 2, false]

tests/test_get_extended_pubkey.py

@@ -165,11 +165,7 @@ def ux_thread():
         comm.press_and_release("right")
         comm.wait_for_text_event("Confirm public key")
         comm.press_and_release("right")
-        # Temporary fix for broken OCR
-        if (model == "nanox"):
-            comm.wait_for_text_event("111-/222-/333-")
-        else:
-            comm.wait_for_text_event("111'/222'/333'")
+        comm.wait_for_text_event("111'/222'/333'")
 
         comm.press_and_release("right")
         comm.wait_for_text_event("not sure")  # second line of "Reject if you're not sure"

tests/test_sign_psbt.py

@@ -27,15 +27,13 @@
 
 
 CURRENCY_TICKER = "TEST"
-# For nano X/S+ OCR used in speculos misreads 'S'. See caveats.txt
-CURRENCY_TICKER_ALT = "TET"
 
 
 def format_amount(ticker: str, amount: int) -> str:
     """Formats an amounts in sats as shown in the app: divided by 10_000_000, with no trailing zeroes."""
     assert amount >= 0
-
-    return f"{ticker} {str(Decimal(amount) / 100_000_000)}"
+    btc_amount = f"{(amount/100_000_000):.8f}".rstrip('0').rstrip('.')
+    return f"{ticker} {btc_amount}"
 
 
 def should_go_right(event: dict):
@@ -128,7 +126,7 @@ def parse_signing_events(events: List[dict]) -> dict:
             if len(ret["addresses"]) == 0:
                 ret["addresses"].append("")
 
-            ret["addresses"][-1] += ev["text"].strip().replace("O", "0")  # OCR misreads O for 0
+            ret["addresses"][-1] += ev["text"].strip()
 
         elif next_step.startswith("Fees"):
             ret["fees"] += ev["text"].strip()
@@ -212,6 +210,34 @@ def test_sign_psbt_singlesig_sh_wpkh_1to2(client: Client):
     )]
 
 
+@has_automation("automations/sign_with_default_wallet_accept_highfee.json")
+def test_sign_psbt_highfee(client: Client):
+    # Transactions with fees higher than 10% of total amount
+    # An aditional warning is shown.
+
+    # PSBT for a wrapped segwit 1-input 2-output spend (1 change address)
+    psbt = open_psbt_from_file(f"{tests_root}/psbt/singlesig/sh-wpkh-1to2.psbt")
+
+    # Make sure that the fees are at least 10% of the total amount
+    for out in psbt.tx.vout:
+        out.nValue = int(out.nValue * 0.9)
+
+    # the test is only interesting if the total amount is at least 10000 sats
+    assert sum(input.witness_utxo.nValue for input in psbt.inputs) >= 10000
+
+    wallet = WalletPolicy(
+        "",
+        "sh(wpkh(@0/**))",
+        [
+            "[f5acc2fd/49'/1'/0']tpubDC871vGLAiKPcwAw22EjhKVLk5L98UGXBEcGR8gpcigLQVDDfgcYW24QBEyTHTSFEjgJgbaHU8CdRi9vmG4cPm1kPLmZhJEP17FMBdNheh3"
+        ],
+    )
+
+    result = client.sign_psbt(psbt, wallet, None)
+
+    assert len(result) == 1
+
+
 @has_automation("automations/sign_with_default_wallet_accept.json")
 def test_sign_psbt_singlesig_wpkh_1to2(client: Client):
 
@@ -586,7 +612,8 @@ def test_sign_psbt_singlesig_wpkh_4to3(client: Client, comm: SpeculosClient, is_
     n_outs = 3
 
     in_amounts = [10000 + 10000 * i for i in range(n_ins)]
-    out_amounts = [9999 + 9999 * i for i in range(n_outs)]
+    total_in = sum(in_amounts)
+    out_amounts = [total_in // n_outs - i for i in range(n_outs)]
 
     change_index = 1
 
@@ -619,18 +646,15 @@ def test_sign_psbt_singlesig_wpkh_4to3(client: Client, comm: SpeculosClient, is_
 
     parsed_events = parse_signing_events(all_events)
 
-    assert ((parsed_events["fees"] == format_amount(CURRENCY_TICKER, fees_amount)) or
-            (parsed_events["fees"] == format_amount(CURRENCY_TICKER_ALT, fees_amount)))
+    assert parsed_events["fees"] == format_amount(CURRENCY_TICKER, fees_amount)
 
     shown_out_idx = 0
     for out_idx in range(n_outs):
         if out_idx != change_index:
             out_amt = psbt.tx.vout[out_idx].nValue
-            assert ((parsed_events["amounts"][shown_out_idx] == format_amount(CURRENCY_TICKER, out_amt)) or
-                    (parsed_events["amounts"][shown_out_idx] == format_amount(CURRENCY_TICKER_ALT, out_amt)))
+            assert parsed_events["amounts"][shown_out_idx] == format_amount(CURRENCY_TICKER, out_amt)
 
-            out_addr = Script(psbt.tx.vout[out_idx].scriptPubKey).address(
-                network=NETWORKS["test"]).replace('O', '0')  # OCR misreads O for 0
+            out_addr = Script(psbt.tx.vout[out_idx].scriptPubKey).address(network=NETWORKS["test"])
             assert parsed_events["addresses"][shown_out_idx] == out_addr
 
             shown_out_idx += 1
@@ -677,12 +701,10 @@ def test_sign_psbt_singlesig_large_amount(client: Client, comm: SpeculosClient,
 
     parsed_events = parse_signing_events(all_events)
 
-    assert ((parsed_events["fees"] == format_amount(CURRENCY_TICKER, fees_amount)) or
-            (parsed_events["fees"] == format_amount(CURRENCY_TICKER_ALT, fees_amount)))
+    assert parsed_events["fees"] == format_amount(CURRENCY_TICKER, fees_amount)
 
     out_amt = psbt.tx.vout[0].nValue
-    assert ((parsed_events["amounts"][0] == format_amount(CURRENCY_TICKER, out_amt)) or
-            (parsed_events["amounts"][0] == format_amount(CURRENCY_TICKER_ALT, out_amt)))
+    assert parsed_events["amounts"][0] == format_amount(CURRENCY_TICKER, out_amt)
 
 
 @pytest.mark.timeout(0)  # disable timeout