Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Show source account path when signing #2151

Closed
andrewkozlik opened this issue Mar 3, 2022 · 5 comments
Closed

Show source account path when signing #2151

andrewkozlik opened this issue Mar 3, 2022 · 5 comments
Assignees
Labels
bitcoin Bitcoin related core Trezor Core firmware. Runs on Trezor Model T and T2B1. feature Product related issue visible for end user T1B1 legacy Trezor One
Milestone

Comments

@andrewkozlik
Copy link
Contributor

andrewkozlik commented Mar 3, 2022

When signing a transaction Trezor should show the path of the account from which the transaction inputs originate. For example:

  • more technical Spending from m/86'/0'/0', or
  • more user friendly: Spending from BTC Taproot account #1.

We need to think about the UI details. Presumably it requires an extra screen. I am not sure where the screen should be located in the signing flow. At first glance it makes sense at the beginning, but if there are inputs from multiple accounts, then it makes more sense at the end so that we can show the amounts being spent from each account. It would also be good if it fit together nicely with any warnings that we display during signing.

I propose we show the screen at the beginning and ignore the multi-account use-case (just display Spending from multiple accounts in that case).

Weakly related to #1244.

@andrewkozlik andrewkozlik added bitcoin Bitcoin related feature Product related issue visible for end user labels Mar 3, 2022
@andrewkozlik andrewkozlik added core Trezor Core firmware. Runs on Trezor Model T and T2B1. T1B1 legacy Trezor One labels Mar 3, 2022
@hynek-jina hynek-jina removed the MEDIUM label May 6, 2022
@PaperHandedPuppy
Copy link

Not only would it be informational to see which account is doing the signing, not knowing which account is signing is a security risk. A corrupted host computer could send the Trezor a request to sign something on a different derivation path than what the host is telling you it's signing for.

If you restricted yourself to only doing safe (easily verifiable) Eth transfers for m/44'/60'/0'/0/0 which holds the majority of your funds, but allow yourself to sign/send riskier arbitrary (not easily decipherable without a decompiler) transactions for m/44'/60'/0'/0/1 which only ever sees a much smaller amount. You'd think you're signing an arbitrary transaction for m/44'/60'/0'/0/1 because that's what your corrupt host is showing you, but you're actually signing for m/44'/60'/0'/0/0 when you don't want to

Workaround: use passphrases, instead of derivation paths, to safely keep "accounts" separate from one another. the passphrase needn't be difficult to remember, it could be 'a' for account a, 'b' for account b, etc

@sime
Copy link
Contributor

sime commented May 31, 2022

Designs

Model T

Source

Single account Multiple accounts
tt-From From

Model 1

Source

Single account Multiple accounts
t1-From t1-From-1

@andrewkozlik
Copy link
Contributor Author

Note: If we implement #2353, then the dialog should reference not only the account but also the passphrase.

@marnova marnova moved this from 🎯 To do to 🏃‍♀️ In progress in Firmware Nov 24, 2022
@marnova marnova moved this from 🏃‍♀️ In progress to 🎯 To do in Firmware Dec 5, 2022
@marnova marnova assigned marnova and unassigned marnova Dec 5, 2022
@marnova marnova removed their assignment Dec 14, 2022
@Hannsek Hannsek modified the milestones: 🖼️ UI2, 🎨 T2B1 Jan 10, 2023
@andrewkozlik andrewkozlik self-assigned this Jan 12, 2023
@Hannsek
Copy link
Contributor

Hannsek commented Jan 24, 2023

@matejcik
Copy link
Contributor

Seems the new designs take this into consideration: account info is not in front, but it's under the "(i)" at the last confirmation screen.

We might want to iterate on that but we should do that in a separate issue; I would close this when #2680 goes in

@Hannsek Hannsek closed this as completed Apr 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bitcoin Bitcoin related core Trezor Core firmware. Runs on Trezor Model T and T2B1. feature Product related issue visible for end user T1B1 legacy Trezor One
Projects
Archived in project
Development

No branches or pull requests

7 participants