From c181c8caca45da42a7b8a5c4f40a601c9026de7c Mon Sep 17 00:00:00 2001 From: Christopher Bergqvist Date: Tue, 25 Jul 2023 10:01:06 +0200 Subject: [PATCH] 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 | 20 ++++++ core/src/apps/ethereum/layout.py | 74 +++++++++++++++++---- core/src/apps/ethereum/sign_tx.py | 72 +++++++++++++------- core/src/apps/ethereum/sign_tx_eip1559.py | 12 +--- tests/ui_tests/fixtures.json | 1 + 5 files changed, 130 insertions(+), 49 deletions(-) diff --git a/common/tests/fixtures/ethereum/sign_tx.json b/common/tests/fixtures/ethereum/sign_tx.json index e5cf5cb08..013f2fad6 100644 --- a/common/tests/fixtures/ethereum/sign_tx.json +++ b/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": { diff --git a/core/src/apps/ethereum/layout.py b/core/src/apps/ethereum/layout.py index e3ddd63a1..7cd4dd716 100644 --- a/core/src/apps/ethereum/layout.py +++ b/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( - 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:", - ) + if spending is None: + await confirm_max_fee( + gas_price, + gas_limit, + 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( - format_ethereum_amount(spending, token, network), - format_ethereum_amount(max_gas_fee * gas_limit, None, network), - total_label="Amount sent:", - fee_label="Maximum fee:", - ) + if spending is None: + await confirm_max_fee( + gas_price, + gas_limit, + 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", diff --git a/core/src/apps/ethereum/sign_tx.py b/core/src/apps/ethereum/sign_tx.py index e782d1f91..aef421a49 100644 --- a/core/src/apps/ethereum/sign_tx.py +++ b/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_data, - require_confirm_fee, - require_confirm_tx, - ) + from .layout import require_confirm_fee # 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) - # Handle ERC20s - token, address_bytes, recipient, value = await handle_erc20(msg, defs) + token, address_bytes, recipient, value = await sign_tx_inner(msg, keychain, 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) - recipient = data_initial_chunk[16:36] - value = int.from_bytes(data_initial_chunk[36:68], "big") + initial_prefix = data_initial_chunk[:16] + # See https://github.com/ethereum-lists/4bytes/blob/master/signatures/a9059cbb etc + 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 initial_prefix == ETH_ERC20_TRANSFER: + 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 tokens.UNKNOWN_TOKEN: - await require_confirm_unknown_token(address_bytes) + 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 diff --git a/core/src/apps/ethereum/sign_tx_eip1559.py b/core/src/apps/ethereum/sign_tx_eip1559.py index 127180716..f4e901301 100644 --- a/core/src/apps/ethereum/sign_tx_eip1559.py +++ b/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) - - # Handle ERC20s - token, address_bytes, recipient, value = await handle_erc20(msg, defs) + token, address_bytes, recipient, value = await sign_tx_inner(msg, keychain, 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"), diff --git a/tests/ui_tests/fixtures.json b/tests/ui_tests/fixtures.json index 1ee2bc546..087029f53 100644 --- a/tests/ui_tests/fixtures.json +++ b/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",