LedgerHQ / app-monero

Monero wallet application for Ledger Nano S & X
Apache License 2.0
273 stars 101 forks source link

Remove account feature (or add warning) #77

Open selsta opened 4 years ago

selsta commented 4 years ago

Since Ledger monero app v1.5.1 there is the following feature:

Up to 9 independent wallets: master seed is derived with BIP32 following the BIP44 path recommendation: m/purpose'/coin_type'/account'/change/address_index. The numerical path m/44'/128'/0'/0/0 was the only used until now. In this last release the account' can be selected between 0 and 9, allowing up to 9 distinct wallets.

We get a lot of support requests from people changing their account and having all kinds of huge issues.

There is also a monero feature called "account" adding extra confusion what the account setting inside Ledger is.

I would suggest to remove this feature again, or rename + add a huge warning when changing account.

johnalanwoods commented 4 years ago

@selsta completely agree.

pricead commented 4 years ago

Makes sense to remove this. If someone really wants multiple distinct accounts, they can just use the device's additional passphrase / PIN feature.

selsta commented 4 years ago

Considering that some people already use this feature, removing is probably not going to work :/ I would still like renaming + adding an extensive warning.

sanderfoobar commented 4 years ago

The additional passphrase / PIN feature is similar to accounts in that it grants you access to a completely different wallet.

As example, when you're in a wallet using say, Bitcoin Electrum + ledger, and you let the ledger device go to sleep, then wake it up, you'll be granted with a passphrase/PIN prompt - you now have 2 options:

  1. Enter wallet passphrase
  2. Enter global PIN

Depending on which passhprase/PIN you enter, you open a different wallet on the Ledger. However, Electrum still thinks you're on the old one - there will be weird wallet behavior. This is also observed over at Monero, both GUI and CLI.

Anyway, this issue is about the naming, or removal of accounts - I'm in favor of any of those two options.

grydz commented 4 years ago

I agree that we should rename the feature in the Monero application.

Any suggestions?

typh289 commented 4 years ago

I have a question that might fit here, the Seed Recovery is possible for accounts different than n0 as well with the blue-app-monero tools? i second that this should be either renamed or better explained btw, it's quite confusing currently

cyanlink commented 4 years ago

Using the derivation path can be good practice in other coins but NOT Monero. Since GUI Wallet, or other wallet interface does not provide you that option directly, recovering such wallets can be super hard -- you will need to generate priv/pub keys using other tools that takes derivation path. (too much tech details for an average user to figure out!) Monero's official "subaddress" solution is hierarchical, but it is not about BIP derivation path also but it is using its own (account/address) derivation. Also, this feature is not well documented, look at this: I googled "ledger monero doc" and the first result official result tells me nothing about this menu entry. I suggest we rename the "account" here to "wallet" and prompt user in ledger app that it is a legacy feature. (don't forget to update docs too! IMO Ledger Support always has the best user-accessibility and should definitely document this.)

cyanlink commented 4 years ago

@typh289 I'm afraid not, look at this:Source Code, the devs hard-coded the derivation path u"m/44'/128'/0'/0/0" so it will only generate words of XMR/0.

cyanlink commented 4 years ago

@typh289 however if you use online mode to show 25 word seed on your device, the 25 word seed is account number specific (every 'account' has their own 25 word seed). since the code on ledger uses master seed of monero-app which is generated with different derivation path (different account' value) on initialization.

WaxedCandy commented 2 years ago

I suspect that the reason this was added to is to allow users to mitigate for vulnerabilities linked below without having to set a new master Ledger seed. Renaming from "Account" to "Wallet" as shown in selsta's pull request #96 would be ideal.

CVE-2020-6861: Ledger Monero App Spend key Extraction

Ledger Security Bulletin 007

Ledger Security Bulletin 008