Public commit record
62/100 · Adequate
What the developer wrote
Authored by Ioan Bizău
refactor: stellar confirmations
[no changelog]
(cherry picked from commit a7f5e7200f49b390c65940638900f071258207b4)
✓ Descriptive subject✓ Uses a recognizable type or scope✓ Provides an explanatory body
What changed, and why it matters
This commit refactors how Trezor handles Stellar cryptocurrency transaction confirmations when a 'payment request' is used. Previously, the device might skip confirming individual operations during payment requests. The change ensures that only simple payment operations are allowed in payment requests, rejects invalid operation types, and still requires user confirmation. It also adds a test case for an invalid operation in a payment request. The commit appears to be a defensive hardening change rather than a fix for an active exploit.
Recommended action
Treat as a low-risk hardening/refactor commit. Reviewers should verify that the new error path is reachable from the host protocol and that the payment-request verifier correctly validates the registered output before final signing. No urgent action is indicated by the diff alone.
Security signals we found
01
Restricts payment-request transactions to a single allowed operation type
02
Adds explicit rejection of non-payment operations in payment-request mode
03
Removes per-operation confirmation bypass for non-payment operations in payment-request mode
04
Adds regression test fixture for invalid operation in payment request
05
Cherry-picked from another commit, suggesting backport of a hardening/refactor change
Evidence from the diff
The refactor changes process_operation() in core/src/apps/stellar/operations/__init__.py to accept a payment_request_verifier instead of a boolean confirm flag. When a payment request verifier is present, only StellarPaymentOp is accepted; other operation types raise ValueError("Invalid operation for payment request"). The payment operation is then serialized and registered as an output without showing the normal per-operation confirmation screen. For non-payment-request flows, all operations continue to trigger their normal confirmation UI. In sign_tx.py, the logic that previously tracked output address/asset for payment requests is moved earlier and simplified. A new test fixture is added for a payment request containing an account-merge operation, expecting the error message ‘Invalid operation for payment request’.
Changed components
core/src/apps/stellar/operations/__init__.pycore/src/apps/stellar/sign_tx.pycommon/tests/fixtures/stellar/sign_tx.jsontests/device_tests/stellar/test_stellar.pyInspect captured patch +77 / −44
diff --git a/common/tests/fixtures/stellar/sign_tx.json b/common/tests/fixtures/stellar/sign_tx.json
index 0335d38d..64c745a8 100644
--- a/common/tests/fixtures/stellar/sign_tx.json
+++ b/common/tests/fixtures/stellar/sign_tx.json
@@ -319,6 +319,44 @@
},
"skip_models": ["t1"]
},
+ {
+ "name": "Payment request with invalid op",
+ "parameters": {
+ "xdr": "AAAAAgAAAAAvIrnGLwi3dPPr5t1ufbk8PsLL3gJ5Vho9nFIluMMikgAAAGQAAAAAAAAD6AAAAAEAAAAAG4J3zQAAAABd5CqEAAAAAAAAAAEAAAAAAAAACAAAAABdVWQkZrGFuEMVLp4hkVHbxYkgJ+xAEBpRe+1coDDC4AAAAAAAAAAA",
+ "address_n": "m/44'/148'/0'",
+ "network_passphrase": "Test SDF Network ; September 2015",
+ "tx": {
+ "source_account": "GAXSFOOGF4ELO5HT5PTN23T5XE6D5QWL3YBHSVQ2HWOFEJNYYMRJENBV",
+ "fee": 100,
+ "sequence_number": 1000,
+ "timebounds_start": 461535181,
+ "timebounds_end": 1575234180,
+ "memo_type": "NONE"
+ },
+ "operations": [
+ {
+ "_message_type": "StellarPaymentOp",
+ "destination_account": "GBOVKZBEM2YYLOCDCUXJ4IMRKHN4LCJAE7WEAEA2KF562XFAGDBOB64V",
+ "asset": {
+ "type": "ALPHANUM4",
+ "code": "X",
+ "issuer": "GAUYJFQCYIHFQNS7CI6BFWD2DSSFKDIQZUQ3BLQODDKE4PSW7VVBKENC"
+ },
+ "amount": 200111000
+ },
+ {
+ "_message_type": "StellarAccountMergeOp",
+ "destination_account": "GBOVKZBEM2YYLOCDCUXJ4IMRKHN4LCJAE7WEAEA2KF562XFAGDBOB64V"
+ }
+ ],
+ "payment_request": true
+ },
+ "result": {
+ "error_message": "Invalid operation for payment request"
+ },
+ "skip_models": ["t1"]
+ },
+
{
"name": "StellarAllowTrustOp-allow",
"parameters": {
diff --git a/core/src/apps/stellar/operations/__init__.py b/core/src/apps/stellar/operations/__init__.py
index e69c12ea..2eaa6021 100644
--- a/core/src/apps/stellar/operations/__init__.py
+++ b/core/src/apps/stellar/operations/__init__.py
@@ -4,12 +4,14 @@ if TYPE_CHECKING:
from consts import StellarMessageType
from trezor.utils import Writer
+ from apps.common.payment_request import PaymentRequestVerifier
+
async def process_operation(
w: Writer,
op: StellarMessageType,
output_index: int,
- confirm: bool,
+ payment_request_verifier: PaymentRequestVerifier | None,
) -> None:
# Importing the stuff inside (only) function saves around 100 bytes here
# (probably because the local lookup is more efficient than a global lookup)
@@ -24,62 +26,58 @@ async def process_operation(
await layout.confirm_source_account(op.source_account)
serialize.write_account(w, op.source_account)
writers.write_uint32(w, consts.get_op_code(op))
+
+ if payment_request_verifier is not None:
+ if messages.StellarPaymentOp.is_type_of(op):
+ # will be confirmed as part of payment request confirmation
+ serialize.write_payment_op(w, op)
+ payment_request_verifier.add_output(op.amount, op.destination_account)
+ return
+ else:
+ raise ValueError("Invalid operation for payment request")
+
# NOTE: each branch below has 45 bytes (26 the actions, 19 the condition)
if messages.StellarAccountMergeOp.is_type_of(op):
- if confirm:
- await layout.confirm_account_merge_op(op, output_index)
+ await layout.confirm_account_merge_op(op, output_index)
serialize.write_account_merge_op(w, op)
elif messages.StellarAllowTrustOp.is_type_of(op):
- if confirm:
- await layout.confirm_allow_trust_op(op)
+ await layout.confirm_allow_trust_op(op)
serialize.write_allow_trust_op(w, op)
elif messages.StellarBumpSequenceOp.is_type_of(op):
- if confirm:
- await layout.confirm_bump_sequence_op(op)
+ await layout.confirm_bump_sequence_op(op)
serialize.write_bump_sequence_op(w, op)
elif messages.StellarChangeTrustOp.is_type_of(op):
- if confirm:
- await layout.confirm_change_trust_op(op)
+ await layout.confirm_change_trust_op(op)
serialize.write_change_trust_op(w, op)
elif messages.StellarCreateAccountOp.is_type_of(op):
- if confirm:
- await layout.confirm_create_account_op(op, output_index)
+ await layout.confirm_create_account_op(op, output_index)
serialize.write_create_account_op(w, op)
elif messages.StellarCreatePassiveSellOfferOp.is_type_of(op):
- if confirm:
- await layout.confirm_create_passive_sell_offer_op(op)
+ await layout.confirm_create_passive_sell_offer_op(op)
serialize.write_create_passive_sell_offer_op(w, op)
elif messages.StellarManageDataOp.is_type_of(op):
- if confirm:
- await layout.confirm_manage_data_op(op)
+ await layout.confirm_manage_data_op(op)
serialize.write_manage_data_op(w, op)
elif messages.StellarManageBuyOfferOp.is_type_of(op):
- if confirm:
- await layout.confirm_manage_buy_offer_op(op)
+ await layout.confirm_manage_buy_offer_op(op)
serialize.write_manage_buy_offer_op(w, op)
elif messages.StellarManageSellOfferOp.is_type_of(op):
- if confirm:
- await layout.confirm_manage_sell_offer_op(op)
+ await layout.confirm_manage_sell_offer_op(op)
serialize.write_manage_sell_offer_op(w, op)
elif messages.StellarPathPaymentStrictReceiveOp.is_type_of(op):
- if confirm:
- await layout.confirm_path_payment_strict_receive_op(op, output_index)
+ await layout.confirm_path_payment_strict_receive_op(op, output_index)
serialize.write_path_payment_strict_receive_op(w, op)
elif messages.StellarPathPaymentStrictSendOp.is_type_of(op):
- if confirm:
- await layout.confirm_path_payment_strict_send_op(op, output_index)
+ await layout.confirm_path_payment_strict_send_op(op, output_index)
serialize.write_path_payment_strict_send_op(w, op)
elif messages.StellarPaymentOp.is_type_of(op):
- if confirm:
- await layout.confirm_payment_op(op, output_index)
+ await layout.confirm_payment_op(op, output_index)
serialize.write_payment_op(w, op)
elif messages.StellarSetOptionsOp.is_type_of(op):
- if confirm:
- await layout.confirm_set_options_op(op)
+ await layout.confirm_set_options_op(op)
serialize.write_set_options_op(w, op)
elif messages.StellarClaimClaimableBalanceOp.is_type_of(op):
- if confirm:
- await layout.confirm_claim_claimable_balance_op(op)
+ await layout.confirm_claim_claimable_balance_op(op)
serialize.write_claim_claimable_balance_op(w, op)
else:
raise ValueError("Unknown operation")
diff --git a/core/src/apps/stellar/sign_tx.py b/core/src/apps/stellar/sign_tx.py
index 33faef3f..92103c25 100644
--- a/core/src/apps/stellar/sign_tx.py
+++ b/core/src/apps/stellar/sign_tx.py
@@ -128,9 +128,15 @@ async def sign_tx(msg: StellarSignTx, keychain: Slip21Keychain) -> StellarSigned
progress_obj.report(int(i / num_operations * 900))
op = await call_any(StellarTxOpRequest(), *consts.op_codes.keys())
- # Note: in case of payment requests we don't confirm each operation individually
- # but rather we confirm the whole payment request afterwards
- await process_operation(w, op, current_output_index, confirm=not msg.payment_req) # type: ignore [Argument of type "MessageType" cannot be assigned to parameter "op" of type "StellarMessageType" in function "process_operation"]
+ await process_operation(w, op, current_output_index, verifier) # type: ignore [Argument of type "MessageType" cannot be assigned to parameter "op" of type "StellarMessageType" in function "process_operation"]
+
+ if msg.payment_req:
+ assert verifier is not None
+ if current_output_index != 0:
+ raise ProcessError(
+ "Multiple operations not supported for payment requests"
+ )
+ assert output_address is None and output_asset is None
if op.source_account is not None and op.source_account != address: # type: ignore [Cannot access attribute "source_account" for class "MessageType"]
# if the operation source account does not match the Trezor account
@@ -146,18 +152,9 @@ async def sign_tx(msg: StellarSignTx, keychain: Slip21Keychain) -> StellarSigned
StellarPathPaymentStrictReceiveOp,
]
):
- if msg.payment_req:
- assert verifier is not None
- if current_output_index != 0:
- raise ProcessError(
- "Multiple operations not supported for payment requests"
- )
- if StellarPaymentOp.is_type_of(op):
- verifier.add_output(op.amount, op.destination_account)
- output_address = op.destination_account
- output_asset = op.asset
-
current_output_index += 1
+ if StellarPaymentOp.is_type_of(op):
+ output_address, output_asset = op.destination_account, op.asset
progress_obj.stop()
# ---------------------------------
diff --git a/tests/device_tests/stellar/test_stellar.py b/tests/device_tests/stellar/test_stellar.py
index 8f56231d..437a7642 100644
--- a/tests/device_tests/stellar/test_stellar.py
+++ b/tests/device_tests/stellar/test_stellar.py
@@ -105,7 +105,7 @@ def parameters_to_proto(session, parameters):
slip44=148,
outputs=[
(
- o["amount"],
+ o.get("amount", 0),
o["destination_account"],
)
for o in parameters["operations"]
Risk score
Our methodology →Why this scored 32/100
Human-validated context
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
No validated notes yet.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.