chore: enable SLIP-24 for ERC-20 `transfer`
What the developer wrote
Authored by Ioan Bizău
chore: enable SLIP-24 for ERC-20 `transfer`
[no changelog]
What changed, and why it matters
This commit enables Trezor hardware wallets to use SLIP-24 payment requests when signing ERC-20 token transfers. Previously, payment requests were rejected for any contract interaction, including ERC-20 transfers. The change adds a new code path that verifies the token transfer amount and recipient against a signed payment request before showing the confirmation screen. It is a feature-enablement change rather than a fix for an active vulnerability, but it touches security-critical signing flow code.
Review the new payment-request path for ERC-20 transfers to ensure `payment_request_verifier.add_output()` receives the correct raw amount and recipient, that `verify()` failures are handled safely, and that the UI flow cannot be bypassed. Verify that removing the blanket contract-interaction rejection does not open other contract-call types to payment-request misuse. Consider whether `assert data_length == 0` is appropriate for production security assumptions or should be an explicit error.
Security signals we found
Feature enablement for SLIP-24 payment requests on ERC-20 `transfer` calls
Removal of blanket rejection of payment requests for contract interactions
Addition of payment-request verification path inside ERC-20 transfer handler
Use of `assert` for control-flow assumptions (`assert data_length == 0`)
Change to security-critical transaction confirmation flow
Evidence from the diff
The patch modifies Ethereum transaction signing in trezor-firmware. In clear_signing.py, _handle_transfer now accepts a payment_request_verifier. When present, it adds the raw ERC-20 transfer amount and recipient address to the verifier, calls verify(), and uses require_confirm_payment_request instead of the standard require_confirm_tx. In sign_tx.py, the blanket DataError('Payment Requests don't support contract interactions') is removed; instead, payment-request verification is delegated to the clear-signing approver for ERC-20 transfers, while plain ETH transfers keep their existing payment-request path. The change also removes a TODO comment about SLIP-24 and tokens.
Changed components
core/src/apps/ethereum/clear_signing.pycore/src/apps/ethereum/sign_tx.pyEthereum ERC-20 token transfer signingSLIP-24 payment request verificationInspect captured patch +48 / −18
diff --git a/core/src/apps/ethereum/clear_signing.py b/core/src/apps/ethereum/clear_signing.py
index 5f2b6c3a..dc3ccd25 100644
--- a/core/src/apps/ethereum/clear_signing.py
+++ b/core/src/apps/ethereum/clear_signing.py
@@ -14,6 +14,8 @@ if TYPE_CHECKING:
from trezor.messages import EthereumTokenInfo
from trezor.ui.layouts import StrPropertyType
+ from apps.common.payment_request import PaymentRequestVerifier
+
from .definitions import Definitions
from .helpers import ConfirmDataFn
from .keychain import MsgInSignTx
@@ -636,6 +638,7 @@ def get_approver(
value: int,
maximum_fee: str,
fee_items: Iterable[StrPropertyType],
+ payment_request_verifier: PaymentRequestVerifier | None,
) -> tuple[ConfirmDataFn, Coroutine[Any, Any, None]] | None:
from .clear_signing_definitions import ALL_DISPLAY_FORMATS
@@ -677,6 +680,7 @@ def get_approver(
token,
maximum_fee,
fee_items,
+ payment_request_verifier,
)
@@ -702,6 +706,7 @@ def _get_summary_handler(
token: EthereumTokenInfo,
maximum_fee: str,
fee_items: Iterable[StrPropertyType],
+ payment_request_verifier: PaymentRequestVerifier | None,
) -> Coroutine[Any, Any, None]:
from .clear_signing_definitions import (
APPROVE_DISPLAY_FORMAT,
@@ -729,6 +734,7 @@ def _get_summary_handler(
token,
maximum_fee,
fee_items,
+ payment_request_verifier,
)
# generic UI for any function that has a `DisplayFormat`
@@ -799,8 +805,9 @@ async def _handle_transfer(
token: EthereumTokenInfo,
maximum_fee: str,
fee_items: Iterable[StrPropertyType],
+ payment_request_verifier: PaymentRequestVerifier | None,
) -> None:
- from .layout import require_confirm_tx
+ from .layout import require_confirm_payment_request, require_confirm_tx
args, fields = context.get_parameters_and_fields(
msg.address_n, msg.value, definitions, token
@@ -813,21 +820,42 @@ async def _handle_transfer(
assert arg0_name == "To"
assert isinstance(recipient_addr, str)
+ arg1_raw_value = args[1]
+ assert isinstance(arg1_raw_value, int)
((arg1_name, value, _), _, _) = fields[1]
assert arg1_name == "Amount"
assert isinstance(value, str)
- await require_confirm_tx(
- recipient_addr,
- value,
- address_bytes,
- msg.address_n,
- maximum_fee,
- fee_items,
- token,
- is_send=True,
- chunkify=bool(msg.chunkify),
- )
+ if payment_request_verifier:
+ # SLIP-24 payment requests for ERC-20 token transfers
+
+ assert msg.payment_req is not None
+
+ payment_request_verifier.add_output(arg1_raw_value, recipient_addr)
+ payment_request_verifier.verify()
+ await require_confirm_payment_request(
+ recipient_addr,
+ msg.payment_req,
+ msg.address_n,
+ maximum_fee,
+ fee_items,
+ msg.chain_id,
+ definitions.network,
+ token,
+ address_from_bytes(address_bytes, definitions.network),
+ )
+ else:
+ await require_confirm_tx(
+ recipient_addr,
+ value,
+ address_bytes,
+ msg.address_n,
+ maximum_fee,
+ fee_items,
+ token,
+ is_send=True,
+ chunkify=bool(msg.chunkify),
+ )
async def _handle_generic_ui(
diff --git a/core/src/apps/ethereum/sign_tx.py b/core/src/apps/ethereum/sign_tx.py
index 1426f0cf..6cf57b3b 100644
--- a/core/src/apps/ethereum/sign_tx.py
+++ b/core/src/apps/ethereum/sign_tx.py
@@ -199,11 +199,15 @@ async def confirm_tx_data(
value = int.from_bytes(msg.value, "big")
clear_signing_approver = clear_signing.get_approver(
- msg, defs, address_bytes, value, maximum_fee, fee_items
+ msg,
+ defs,
+ address_bytes,
+ value,
+ maximum_fee,
+ fee_items,
+ payment_request_verifier,
)
if clear_signing_approver is not None:
- if payment_request_verifier is not None:
- raise DataError("Payment Requests don't support contract interactions")
return clear_signing_approver
recipient_str = (
@@ -211,8 +215,7 @@ async def confirm_tx_data(
)
if payment_request_verifier is not None:
- if data_length > 0:
- raise DataError("Payment Requests don't support contract interactions")
+ assert data_length == 0
# If a payment_request_verifier is provided, then msg.payment_req must have been set.
assert msg.payment_req is not None
@@ -228,7 +231,6 @@ async def confirm_tx_data(
fee_items,
msg.chain_id,
network,
- # TODO: SLIP-24 cannot deal with tokens? So we should get rid of these?
None,
None,
)
Why this scored 30/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.