feat(core): Make allowance check nicer (implements #2923)

Properly displays recipient address and allowance amount for the `approve` function, similar to ERC20 token transfers.

[no changelog]

common/tests/fixtures/ethereum/sign_tx.json

@@ -24,6 +24,26 @@
                 "sig_s": "633a74429eb6d3aeec4ed797542236a85daab3cab15e37736b87a45697541d7a"
             }
         },
+        {
+            "name": "erc20_approve",
+            "parameters": {
+                "comment": "Calling approve function for ERC 20 token",
+                "data": "095ea7b300000000000000000000000012345678901234567890123456789012345678900000000000000000000000000000000000000000000000056bc75e2d63100000",
+                "path": "m/44'/60'/0'/0/1",
+                "to_address": "0xfc6b5d6af8a13258f7cbd0d39e11b35e01a32f93",
+                "chain_id": 1,
+                "nonce": "0x0",
+                "gas_price": "0x14",
+                "gas_limit": "0x14",
+                "tx_type": null,
+                "value": "0x0"
+            },
+            "result": {
+                "sig_v": 38,
+                "sig_r": "44386ce8c59780b2c7674f347fc891bcb22bd5eddf5b17bf9bc3fc71f06e0561",
+                "sig_s": "1467aa6e6887f4bef4278caad3499f06c1f8730967d988f760a4f536db7796f3"
+            }
+        },
         {
             "name": "wanchain",
             "parameters": {

core/src/apps/ethereum/layout.py

@@ -44,8 +44,20 @@ def require_confirm_tx(
     )
 
 
+async def confirm_max_fee(
+    gas_price: int, gas_limit: int, network: EthereumNetworkInfo
+) -> Any:
+    max_fee = format_ethereum_amount(gas_price * gas_limit, None, network)
+    await confirm_text(
+        "confirm_max_fee",
+        "Total",
+        data="",  # TODO Would like to have None!
+        description=f"Maximum fee: {max_fee}",
+    )
+
+
 async def require_confirm_fee(
-    spending: int,
+    spending: int | None,
     gas_price: int,
     gas_limit: int,
     network: EthereumNetworkInfo,
@@ -56,16 +68,23 @@ async def require_confirm_fee(
         description="Gas price:",
         amount=format_ethereum_amount(gas_price, None, network),
     )
-    await confirm_total(
+    if spending is None:
-        total_amount=format_ethereum_amount(spending, token, network),
+        await confirm_max_fee(
-        fee_amount=format_ethereum_amount(gas_price * gas_limit, None, network),
+            gas_price,
-        total_label="Amount sent:",
+            gas_limit,
-        fee_label="Maximum fee:",
+            network,
-    )
+        )
+    else:
+        await confirm_total(
+            total_amount=format_ethereum_amount(spending, token, network),
+            fee_amount=format_ethereum_amount(gas_price * gas_limit, None, network),
+            total_label="Amount sent:",
+            fee_label="Maximum fee:",
+        )
 
 
 async def require_confirm_eip1559_fee(
-    spending: int,
+    spending: int | None,
     max_priority_fee: int,
     max_gas_fee: int,
     gas_limit: int,
@@ -82,12 +101,19 @@ async def require_confirm_eip1559_fee(
         format_ethereum_amount(max_priority_fee, None, network),
         "Priority fee per gas",
     )
-    await confirm_total(
+    if spending is None:
-        format_ethereum_amount(spending, token, network),
+        await confirm_max_fee(
-        format_ethereum_amount(max_gas_fee * gas_limit, None, network),
+            gas_price,
-        total_label="Amount sent:",
+            gas_limit,
-        fee_label="Maximum fee:",
+            network,
-    )
+        )
+    else:
+        await confirm_total(
+            format_ethereum_amount(spending, token, network),
+            format_ethereum_amount(max_gas_fee * gas_limit, None, network),
+            total_label="Amount sent:",
+            fee_label="Maximum fee:",
+        )
 
 
 def require_confirm_unknown_token(address_bytes: bytes) -> Awaitable[None]:
@@ -139,6 +165,26 @@ async def confirm_typed_data_final() -> None:
     )
 
 
+def require_confirm_approve_tx(
+    address_bytes: bytes,
+    allowance: int,
+    network: EthereumNetworkInfo,
+    token: EthereumTokenInfo,
+) -> Awaitable[None]:
+    from trezor.ui.layouts import confirm_action
+    from ubinascii import hexlify
+
+    address_hex = "0x" + hexlify(address_bytes).decode()
+
+    await confirm_action(
+        "confirm_approve",
+        "Confirm approve",
+        f"Allow {address_hex} to withdraw up to {format_ethereum_amount(allowance, token, network)}",
+        verb="Hold to confirm",
+        hold=True,
+    )
+
+
 def confirm_empty_typed_message() -> Awaitable[None]:
     return confirm_text(
         "confirm_empty_typed_message",

core/src/apps/ethereum/sign_tx.py

@@ -29,31 +29,18 @@ async def sign_tx(
 ) -> EthereumTxRequest:
     from trezor.utils import HashWriter
     from trezor.crypto.hashlib import sha3_256
-    from apps.common import paths
+    from .layout import require_confirm_fee
-    from .layout import (
-        require_confirm_data,
-        require_confirm_fee,
-        require_confirm_tx,
-    )
 
     # check
     if msg.tx_type not in [1, 6, None]:
         raise DataError("tx_type out of bounds")
     if len(msg.gas_price) + len(msg.gas_limit) > 30:
         raise DataError("Fee overflow")
-    check_common_fields(msg)
 
-    await paths.validate_path(keychain, msg.address_n)
+    token, address_bytes, recipient, value = await sign_tx_inner(msg, keychain, defs)
-
-    # Handle ERC20s
-    token, address_bytes, recipient, value = await handle_erc20(msg, defs)
 
     data_total = msg.data_length
 
-    await require_confirm_tx(recipient, value, defs.network, token)
-    if token is None and msg.data_length > 0:
-        await require_confirm_data(msg.data_initial_chunk, data_total)
-
     await require_confirm_fee(
         value,
         int.from_bytes(msg.gas_price, "big"),
@@ -99,31 +86,66 @@ async def sign_tx(
     return result
 
 
-async def handle_erc20(
+async def sign_tx_inner(
     msg: MsgInSignTx,
-    definitions: Definitions,
+    keychain: Keychain,
+    defs: Definitions,
 ) -> tuple[EthereumTokenInfo | None, bytes, bytes, int]:
-    from .layout import require_confirm_unknown_token
     from . import tokens
+    from .layout import (
+        require_confirm_approve_tx,
+        require_confirm_data,
+        require_confirm_fee,
+        require_confirm_tx,
+        require_confirm_unknown_token,
+    )
+    from apps.common import paths
+
+    check_common_fields(msg)
+    await paths.validate_path(keychain, msg.address_n)
 
     data_initial_chunk = msg.data_initial_chunk  # local_cache_attribute
     token = None
     address_bytes = recipient = bytes_from_address(msg.to)
     value = int.from_bytes(msg.value, "big")
+    run_confirm_tx = True
     if (
         len(msg.to) in (40, 42)
         and len(msg.value) == 0
         and msg.data_length == 68
         and len(data_initial_chunk) == 68
-        and data_initial_chunk[:16]
-        == b"\xa9\x05\x9c\xbb\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
     ):
-        token = definitions.get_token(address_bytes)
+        initial_prefix = data_initial_chunk[:16]
-        recipient = data_initial_chunk[16:36]
+        # See https://github.com/ethereum-lists/4bytes/blob/master/signatures/a9059cbb etc
-        value = int.from_bytes(data_initial_chunk[36:68], "big")
+        ETH_ERC20_TRANSFER = (
+            b"\xa9\x05\x9c\xbb\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
+        )
+        ETH_ERC20_APPROVE = (
+            b"\x09\x5e\xa7\xb3\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
+        )
 
-        if token is tokens.UNKNOWN_TOKEN:
+        if initial_prefix == ETH_ERC20_TRANSFER:
-            await require_confirm_unknown_token(address_bytes)
+            token = defs.get_token(address_bytes)
+            recipient = data_initial_chunk[16:36]
+            value = int.from_bytes(data_initial_chunk[36:68], "big")
+
+            if token is tokens.UNKNOWN_TOKEN:
+                await require_confirm_unknown_token(address_bytes)
+        elif initial_prefix == ETH_ERC20_APPROVE:
+            token = defs.get_token(address_bytes)
+            recipient = data_initial_chunk[16:36]
+            # We are not transferring any value, just setting an allowance.
+            value = None
+            allowance = int.from_bytes(data_initial_chunk[36:68], "big")
+
+            await require_confirm_approve_tx(recipient, allowance, defs.network, token)
+            run_confirm_tx = False
+
+    if run_confirm_tx:
+        await require_confirm_tx(recipient, value, defs.network, token)
+
+    if token is None and msg.data_length > 0:
+        await require_confirm_data(msg.data_initial_chunk, msg.data_length)
 
     return token, address_bytes, recipient, value
 

core/src/apps/ethereum/sign_tx_eip1559.py

@@ -43,7 +43,7 @@ async def sign_tx_eip1559(
         require_confirm_eip1559_fee,
         require_confirm_tx,
     )
-    from .sign_tx import handle_erc20, send_request_chunk, check_common_fields
+    from .sign_tx import sign_tx_inner, send_request_chunk
 
     gas_limit = msg.gas_limit  # local_cache_attribute
 
@@ -52,19 +52,11 @@ async def sign_tx_eip1559(
         raise wire.DataError("Fee overflow")
     if len(msg.max_priority_fee) + len(gas_limit) > 30:
         raise wire.DataError("Fee overflow")
-    check_common_fields(msg)
 
-    await paths.validate_path(keychain, msg.address_n)
+    token, address_bytes, recipient, value = await sign_tx_inner(msg, keychain, defs)
-
-    # Handle ERC20s
-    token, address_bytes, recipient, value = await handle_erc20(msg, defs)
 
     data_total = msg.data_length
 
-    await require_confirm_tx(recipient, value, defs.network, token)
-    if token is None and msg.data_length > 0:
-        await require_confirm_data(msg.data_initial_chunk, data_total)
-
     await require_confirm_eip1559_fee(
         value,
         int.from_bytes(msg.max_priority_fee, "big"),

tests/ui_tests/fixtures.json

@@ -2757,6 +2757,7 @@
 "TT_ethereum-test_signtx.py::test_signtx[data_1]": "863146c0494b76d4d8a9773a406994c3a35fb8c0cb989f372214aaa80b63b73c",
 "TT_ethereum-test_signtx.py::test_signtx[data_2_bigdata]": "05aa081d00a78c3a432904b19c21df1512c7660f4be3edaec75c9bc1aad83ae8",
 "TT_ethereum-test_signtx.py::test_signtx[erc20_token]": "d51cf4119a784bac447971e8e80be44985c28a9e5f14917efb7c2064d7f21752",
+"TT_ethereum-test_signtx.py::test_signtx[erc20_approve]": "a171451f6f0ba9da5d77790a7600766f639c5e4845c47b7992c430ce49f5260a",
 "TT_ethereum-test_signtx.py::test_signtx[max_chain_id]": "3723ceb0436e8d7f4669edcf36e72cb1fe65d891f910886fa79e9e406c5e9479",
 "TT_ethereum-test_signtx.py::test_signtx[max_chain_plus_one]": "3723ceb0436e8d7f4669edcf36e72cb1fe65d891f910886fa79e9e406c5e9479",
 "TT_ethereum-test_signtx.py::test_signtx[max_uint64]": "3723ceb0436e8d7f4669edcf36e72cb1fe65d891f910886fa79e9e406c5e9479",