search

brave / brave-browser

Brave browser for Android, iOS, Linux, macOS, Windows.
https://brave.com
Mozilla Public License 2.0
17.68k stars 2.31k forks source link

crypto wallet: auto-generated mnemonic is not standard compatible #9837

Closed SCBuergel closed 4 years ago

SCBuergel commented 4 years ago

Description

Brave does not correctly derive the Ethereum address for 24-word mnemonics. The crypto wallet automatically creates 24 seed words upon first start which are also not yielding the correct address. Entering those seed words in another tool such as MyEtherWallet or MyCrypto derives another address.

As a consequence all funds that are sent to a Brave address cannot be recovered and might be lost!

Steps to Reproduce

  1. Create a new profile on brave (to have a blank wallet setup)
  2. Open the crypto wallet and follow the setup instructions
  3. The setup instructions will lead to a 24-word mnemonic, copy that one (in my case it was toward around purity endorse style assume seed discover beyond mosquito lend auto cattle shed smile pretty push slot grid hotel eagle govern defense element
    1. Note the address that is shown in the brave wallet - in my case it was 0xC5443F1948FF4273222E04E93A9365780990Bc1D
    2. Open e.g. MyEtherWallet (same result using MyCrypto desktop app) or any other standard Ethereum wallet and restore the address from the mnemonic.
    3. The resulting Ethereum address is a different one, in my case 0x815B52687Ff57Ae9a58B4894e66ad476f0ba224D

Actual result:

The Ethereum address generated in Brave does not match those generated with standard tools.

Expected result:

The Ethereum address generated in Brave should match those generated with standard tools.

Reproduces how often:

Easily reproduced

Brave version (brave://version info)

on Ubuntu 18.04

Brave | 1.8.96 Chromium: 81.0.4044.138 (Official Build) (64-bit)
-- | --
Revision | 8c6c7ba89cc9453625af54f11fd83179e23450fa-refs/branch-heads/4044@{#999}
OS | Linux
JavaScript | V8 8.1.307.32

Miscellaneous Information:

Generally, Brave seems to be unable to import 24-word mnemonics correctly. E.g. the test vector zoo zoo zoo zoo zoo zoo zoo zoo zoo zoo zoo zoo zoo zoo zoo zoo zoo zoo zoo zoo zoo zoo zoo vote yields the address 0x63D1ec5Cb2D2D2d3168266E88DA0F08c7455e6D9 (should be 0x1959f5f4979c5Cd87D5CB75c678c770515cb5E0E).

12-word mnemonics on the other hand do work fine. The test vector zoo zoo zoo zoo zoo zoo zoo zoo zoo zoo zoo wrong yields the correct address 0xfc2077CA7F403cBECA41B1B0F62D91B5EA631B5E

karalabe commented 4 years ago

After a quick skim of the code (way over my head here), is this line correct? https://github.com/brave/brave-core/blob/cd5a08090094fc1a67033e2760c3a067b27e001d/components/brave_sync/crypto/crypto.cc#L148

bool PassphraseToBytes32(const std::string& passphrase,
                         std::vector<uint8_t>* bytes) {
  DCHECK(bytes);
  size_t written;
  bytes->resize(DEFAULT_SEED_SIZE);
  if (bip39_mnemonic_to_bytes(nullptr, passphrase.c_str(), bytes->data(),
                              bytes->size(), &written) != WALLY_OK) {
    LOG(ERROR) << "bip39_mnemonic_to_bytes failed";
    return false;
  }
  return true;
}

bip39_mnemonic_to_bytes only converts the mnemonic into a binary representation, but it does not actually derive the BIP39 seed. That is done by bip39_mnemonic_to_seed, which doesn't seem to be used anywhere in the code. Maybe someone mistook the result of PassphraseToBytes32 as the actual seed to derive keys with?

SCBuergel commented 4 years ago

@karalabe I don't get why that would work for 12 word mnemonics but not for 24 word mnemonics though. Since I haven't been digging into C for well over a decade I won't be able to comment much further.

bbondy commented 4 years ago

@diracdeltas can you take a look? Is this also a problem with MetaMask?

diracdeltas commented 4 years ago

I don't get why that would work for 12 word mnemonics but not for 24 word mnemonics though.

we use different code paths for 12 vs 24. 12-word does the same thing as metamask (needed for legacy compatibility with metamask imports) whereas 24 uses our own derivation method, described here: https://github.com/brave/brave-browser/wiki/Brave-Ethereum-Remote-Client-Wallet-Seed-Information

unfortunately when we designed this, we didn't have compatibility with other wallet providers as a goal, so it's not surprising to me that this is happening :(

@karalabe that code is used for sync, not crypto wallets. i think you want to look here: https://github.com/brave/eth-hd-keyring/blob/master/index.js#L188-L199

diracdeltas commented 4 years ago

this is where it calls the metamask-compatible code for 12-word phrases btw: https://github.com/brave/eth-hd-keyring/blob/master/index.js#L180

diracdeltas commented 4 years ago

so at a glance, i think this is because we are calling https://github.com/brave/eth-hd-keyring/blob/master/index.js#L176, which doesn't actually derive the bip39 seed (the same issue @karalabe found, but in js not c++). this can pretty easily be replaced by just calling bip39.mnemonicToSeed(mnemonic). however, the tricky part is migrating existing wallet users.

  1. new users get wallets that are derived using bip39 so they are compatible with other wallet providers
  2. for users who already have a wallet, we call bip39.entropyToMnemonic to generate their "correct" code words, assuming this method works as expected. we show an alert or notifcation to update their code words when they update to the new version.

open to thoughts here

diracdeltas commented 4 years ago

i did some digging; findings here

how brave does wallet derivation/restoration

  1. generate 32 bytes of entropy and runs this through HKDF to generate a "seed" (see step 7 of https://github.com/brave/brave-browser/wiki/Brave-Ethereum-Remote-Client-Wallet-Seed-Information#protocol)
  2. calls bip39.entropyToMnemonic to encode it as a mnemonic
  3. calls hdkey.fromMasterSeed directly on the 32-byte seed (https://github.com/bitcoin/bips/blob/master/bip-0032.mediawiki#master-key-generation)
  4. upon restoration, we call bip39.mnemonicToEntropy on the mnemonic to convert it back into the seed

how metamask does wallet derivation/restoration

as far as i understand:

  1. calls bip39.generateMnemonic() to generate 16 bytes of entropy encoded as a mnemonic. this follows https://github.com/bitcoin/bips/blob/master/bip-0039.mediawiki#generating-the-mnemonic.
  2. calls mnemonicToSeed, which does some PBKDF2-HMAC-SHA512 on the mnemonic to generate a 64-byte seed: https://github.com/bitcoin/bips/blob/master/bip-0039.mediawiki#from-mnemonic-to-seed
  3. calls hdkey.fromMasterSeed with the 64-byte seed
  4. upon restoration, calls bip39.mnemonicToSeed

unfortunately, i think this will make it difficult for us to encode the brave wallet codewords as other-wallet-compatible code words, because we would need to find the PBKDF2 preimage. if other wallets are willing to take the seed directly as input, that could work.

markrtgh commented 4 years ago

Why does this not have a P1 and bug label?

Have confirmed this issue. A short-term workaround would be for a user to export and save private keys for each active account in the wallet. However this results in a new problem as the exported private key does not work in the MyCrypto desktop app as the key has all uppercase letters. Indicates the exported private key format is also nonstandard. For a successful import, a user first needs to change the private key to all lowercase letters before importing into MyCrypto and I doubt many users would know this. FYI Metamask displays private key as uppercase but exports as lowercase. Suggest Brave change the exported private key to lowercase.

There appears to be a number of options.

  1. Do nothing. This is not an option as more newbies start to use the wallet there will be a whole lot of future pain, confusion and loss of crypto funds.
  2. Change private key export to lowercase.
  3. Change private key export to lowercase, depreciate the current wallet with a fixed end of life date displayed in the current wallet and instructions on how to export private keys. After this date replace the wallet with a new one that conforms to BIP39 and provide a tool on github/within Brave/or a link to a site to allow users who miss the cutoff date to export private keys.
  4. As for 3 but for a fixed time have two crypto wallets in Brave, the depreciated one and the new one that conforms to BIP39.
diracdeltas commented 4 years ago

I didn't realize you could import the private key directly into other wallets. In that case, seems like the easy one-line fix is to change the export to lowercase. Is everyone happy with that?

bbondy commented 4 years ago

sounds good to me, are there any downsides to this?

diracdeltas commented 4 years ago

dan finlay from metamask doesn't exactly remember why they chose uppercase; it's possible some other wallets expect uppercase as well? in which case i think it's safest to just offer a button in the private key export screen that converts between uppercase and lowercase so users can do either.

SCBuergel commented 4 years ago

Please keep in mind that exporting/importing private keys is really only a last resort temporary solution.

Users expect mnemonics to work across wallets in a consistent fashion, i.e. the same addresses get generated from the same mnemonic. If the Brave wallet does not follow established standards then the number of issues you will get here with people claiming loss-of-fund situations will explode.

diracdeltas commented 4 years ago

Users expect mnemonics to work across wallets in a consistent fashion, i.e. the same addresses get generated from the same mnemonic. If the Brave wallet does not follow established standards then the number of issues you will get here with people claiming loss-of-fund situations will explode.

AFAICT we've only had 2 complaints of this so far, probably because this bug only occurs if you are going from Brave to another wallet implementation. I agree it would be ideal to be standards-compatible because that's what users expect, but either way the funds are fully recoverable.

I think the 2 options are:

  1. Add lowercase key export option. In the codewords screen, add a disclaimer that restoring wallets from the codewords requires using Brave; for exporting to other wallets, use the private key export. This is my preferred solution since it's easier.

  2. Add lowercase key export option, but also change the wallet derivation process to be standard-compatible; then on wallet restoration in Brave, we ask whether their wallet codewords are v1 or v2 and derive accordingly. This might cause some user confusion if someone doesn't know whether they are using v1 or v2 codewords, which can also lead to perceived loss of funds.

diracdeltas commented 4 years ago

update - metamask has an upstream PR to stop upper-casing the private key, which we should pull once it's merged. https://github.com/MetaMask/metamask-extension/pull/8850

lentils79 commented 4 years ago

Okay, this can be the 3rd complaint. I expect most users of Brave Crypto Wallet would not realise the issue exists. If they had known of the issue they probably wouldn't have choosen the Brave Crypto Wallet in the first place.

Is there a guide for deriving seed words that will restore the crypto wallet addresses to other wallets?

AFAICT we've only had 2 complaints of this so far, probably because this bug only occurs if you are going from Brave to another wallet implementation. I agree it would be ideal to be standards-compatible because that's what users expect, but either way the funds are fully recoverable.

I think the 2 options are:

  1. Add lowercase key export option. In the codewords screen, add a disclaimer that restoring wallets from the codewords requires using Brave; for exporting to other wallets, use the private key export. This is my preferred solution since it's easier.
  2. Add lowercase key export option, but also change the wallet derivation process to be standard-compatible; then on wallet restoration in Brave, we ask whether their wallet codewords are v1 or v2 and derive accordingly. This might cause some user confusion if someone doesn't know whether they are using v1 or v2 codewords, which can also lead to perceived loss of funds.
diracdeltas commented 4 years ago

Is there a guide for deriving seed words that will restore the crypto wallet addresses to other wallets?

As I explained in https://github.com/brave/brave-browser/issues/9837#issuecomment-644477534, it is not computationally feasible to derive other-wallet seedwords from brave seedwords AFAICT. So given that this option isn't on the table, I'm leaning toward option 1.

lentils79 commented 4 years ago

So are you saving the Brave seed words won't even restore a Brave crypto wallet? Are these seed words completely useless then?

Is there a guide for deriving seed words that will restore the crypto wallet addresses to other wallets?

As I explained in https://github.com/brave/brave-browser/issues/9837#issuecomment-644477534, it is not computationally feasible to derive other-wallet seedwords from brave seedwords AFAICT. So given that this option isn't on the table, I'm leaning toward option 1.

diracdeltas commented 4 years ago

So are you saving the Brave seed words won't even restore a Brave crypto wallet? Are these seed words completely useless then?

Where did you get this from? I explicitly said above that it will work to restore Brave wallets, but not other wallets, because we designed it without cross-compatibility as a goal.

lentils79 commented 4 years ago

So is the only option currently to export private keys of each used account with the Brave wallet? Is there currently an export feature, or are we just waiting for metamask to provide the lowercase private key export feature?

So are you saving the Brave seed words won't even restore a Brave crypto wallet? Are these seed words completely useless then?

Where did you get this from? I explicitly said above that it will work to restore Brave wallets, but not other wallets, because we designed it without cross-compatibility as a goal.

diracdeltas commented 4 years ago

Is there currently an export feature, or are we just waiting for metamask to provide the lowercase private key export feature?

just waiting on metamask to convert to lowercase

srirambv commented 4 years ago

Verification passed on

Brave 1.12.114 Chromium: 84.0.4147.135 (Official Build) (64-bit)
Revision c42bd09b3f24da1698d71d3b4f57402137163566-refs/branch-heads/4147@{#1102}
OS Windows 10 OS Version 1809 (Build 17763.1397)
Component 0.1.69

Verification passed on

Brave 1.13.75 Chromium: 85.0.4183.69 (Official Build) beta (64-bit)
Revision 4554ea1a1171bd8d06951a4b7d9336afe6c59967-refs/branch-heads/4183@{#1426}
OS Linux
Component 0.1.69

Verification passed on

Brave 1.14.55 Chromium: 85.0.4183.69 (Official Build) nightly (64-bit)
Revision 4554ea1a1171bd8d06951a4b7d9336afe6c59967-refs/branch-heads/4183@{#1426}
OS macOS Version 10.15.5 (Build 19F101)
Component 0.1.69
sebastianzolg commented 3 years ago

Hello,

it looks to me that this issue was originally tracking two different problems:

1) Export key in lower case 2) Have compatibility with other wallet providers as mentioned by @diracdeltas

I've created a wallet using MEW. When restoring with my seed (from MEW) in Brave's Crypto Wallets, it restores another account with a different public address. I think this is related to point number 2.

I guess this issue only solved point number 1 and left point number 2 out. Are there still plans to have better compatibility?

Thanks —Sebastian

diracdeltas commented 3 years ago

@sebastianzolg We can't easily support importing words from Brave into another wallet because our derivation algorithm is different, but we could potentially support importing words from other wallets into Brave if that's a thing that people would find useful.

Alternatively, if MEW supports exporting private keys, we could add an option to restore from private key instead of mnemonic.

sebastianzolg commented 3 years ago

Thanks, @diracdeltas, for the clarification! My scenario is importing the seed from another Wallet into Brave. Given that key management is still a nightmare in this distributed ledger world. The behavior should at least be explained when importing in Brave, so newbies are not that confused.

Thanks for the clarification anyway. I would vote for such an import scenario.

diracdeltas commented 3 years ago

@sebastianzolg i think it is technically feasible for Brave wallets to support import from other wallets (just not the other way around). i'll open an issue for that.

tauren commented 3 years ago

Chiming in here as well, as this issue bit my ass and wasted a bunch of time. My bad assuming Brave wallets were compatible with all the others.

I'm developing a smart contract and using HDWalletProvider which takes a mnemonic as input. It does NOT work with the 24 words generated by Brave. I assumed it did, and was pulling out my hair trying to figure out why I couldn't deploy. So essentially, ethereum/ERC-20/smart contract developers cannot use Brave to generate an HD wallet for smart contract use. I have to use MetaMask or some other standards compliant wallet to generate the words.

Personally I think Brave should seriously consider evolving to be compatible. I realize this will be difficult since you need to maintain support for all existing wallets, but alienating developers is probably not in your best interest.

diracdeltas commented 3 years ago

@tauren i already have a PR open for this. please see https://github.com/brave/brave-browser/issues/13245

diracdeltas commented 3 years ago

^ sorry meant to reply to a comment from someone else that was apparently deleted

MicahZoltu commented 3 years ago

Can this be re-opened, as it still is an issue? It is still not fixed as I have a 24-word phrase that cannot be used in Brave because Brave doesn't follow the standard HD Wallet algorithm for key derivation from it.

IIUC, #13245 only fixed 12-word phrases, but 24-word phrases are still non-standards compliant. While I understand that this is a huge pain to fix, the pain of Brave mnemonics being standards non-compliant is only going to get worse with additional crypto adoption over time and the longer we wait to start on the road to standards compliance in brave the more painful this will be.

My recommendation on long-term migration strategy:

  1. Update the recover UI to have an option for the user to choose between "Externally Created Seed Phrase" and "Brave Created Seed Phrase" (exact wording and UX TBD).
  2. For new users start generating standards compliant 24-word seed phrases and at the same time change the recovery UI to prompt for "Externally Created Seed Phrase or Brave Created Seed Phrase after date X" and "Brave Created Seed Phrase before date X" (exact wording and UX TBD).
  3. Eventually (years from now) hide the option to recover from a legacy Brave seed phrase somewhere so most users don't have to see/deal with it but users with ancient seed phrases can still derive.

Another option would be that when the user tries to recover a 24 word phrase, check to see which accounts have assets on the current network so we can helpfully hint to the user which one they probably have.

bbondy commented 3 years ago

Can this be re-opened, as it still is an issue? It is still not fixed as I have a 24-word phrase that cannot be used in Brave because Brave doesn't follow the standard HD Wallet algorithm for key derivation from it.

Can you restore that on MetaMask and have it work? I believe it's matching MetaMask at least with our Crypto Wallets extension.

cc @diracdeltas @darkdh

MicahZoltu commented 3 years ago

According to #13245, you cannot, and my experience agrees with this:

for users who are restoring a wallet, restore using the standard way if their mnemonic is 12 words, but restore using the legacy way if it's 24.

If I have a 24-word phrase it is interpreted one way, but if I have a 12-word phrase it is interpreted a different way. The 12-word interpretation is standards compliant, while the 24-word interpretation is non-compliant (something custom for Brave Crypto Wallets).

As a test case,

zoo zoo zoo zoo zoo zoo zoo zoo zoo zoo zoo zoo zoo zoo zoo zoo zoo zoo zoo zoo zoo zoo zoo vote

This mnemonic restores to address 0x1959f5f4979c5Cd87D5CB75c678c770515cb5E0E in MetaMask and https://github.com/Zoltu/ethereum-crypto (when given derivation path m/44'/60'/0'/0/0) and it restores to 0x63D1ec5Cb2D2D2d3168266E88DA0F08c7455e6D9 in Brave Crypto Wallets.

zoo zoo zoo zoo zoo zoo zoo zoo zoo zoo zoo wrong

This seed phrase on the other hand resolves to 0xfc2077CA7F403cBECA41B1B0F62D91B5EA631B5E in MetaMask, Brave, and https://github.com/Zoltu/ethereum-crypto

bbondy commented 3 years ago

got it, I understand now, thanks.

Note that we're doing an entire rewrite of the wallet now so this old extension won't be maintained. We'll be sure to at least support both of these ways with the new wallet though.

diracdeltas commented 3 years ago

It is still not fixed as I have a 24-word phrase that cannot be used in Brave because Brave doesn't follow the standard HD Wallet algorithm for key derivation from it.

that's a good point; if the phrase is 24 words, there's no way for us to automatically tell if it's a metamask-style phrase or a legacy brave phrase.

we could let the user pick which derivation path to use in this case, or just support importing from the raw private key.

MicahZoltu commented 3 years ago

I think importing from private keys is not the right solution. Users have been trained to understand recovery words and that they are secret, but "big hexadecimal numbers" look like account addresses, transaction hashes, etc. to users and they don't know to keep them secret. Also, a user with multiple wallets shouldn't have to import/export every key individually. Part of the point of HD Wallets is to make it so you can carry around a single value and it is portable between different tools.

aruialmeida commented 3 years ago

Last month i create i brave crypto account in "crypto wallets", it give me 24 seed phrase, and now i go there, want to open the account with the 24 seed phrase and it ask me for 12 seed phrase... need some help. thanks i already read the above answers, but need help anyway.

bbondy commented 3 years ago

it accepts both 12 and 24 words

aruialmeida commented 3 years ago

Thanks for the answer bbondy.

I create the wallet in the "create" button in the image, and it gave me 24 words phrase. And now i restore it in another computer in the "restore" button (in the image) and it accept the 24 word phrase but open a different address account!!!

appreciate some help.

yt1

igloohead commented 3 years ago

I also need help with this. My 24-word phrase provided by Brave Crypto Wallets does not open the same address in MetaMask that Brave had generated within its built-in browser.

What if I want to import the seed into Trezor or Ledger?

Does it require a 12-word seed? What is the 12-word seed compared to the 24-word?

diracdeltas commented 3 years ago

@igloohead it's not possible for the legacy 24-word phrases to be metamask compatible. if you wish to export your wallet from brave for import into metamask, you have to do it via the private key export option.

Jules-Win commented 2 years ago

Hi all... I found this page trying to solve a problem I have with restoring my Brave wallet with my seed phrase. I had multiple accounts in my wallet, one of which is very important to me. Unfortunately for me, I didn't back up the private key to that address, and the seed phrase loaded Account 1 (which I used before the one I need, and can't find right now) with the complete tnxn history, my contacts and custom networks. The way I understand the derivation path (I have no clue about coding/programming etc.) should load my addresses always in the same order when I click "Add account". Why is my address not showing up then? It was created in my Brave wallet, so it must have been derived from the seed derivation path... I made an account here for the sole purpose of trying to resolve this issue. If someone knows a solution to my problem, some help would be greatly appreciated. Thank you in advance.

SCBuergel commented 2 years ago

@Jules-Win I'm not affiliated with Brave in any way but your story sux, so just to clarify:

  1. You created a wallet that was most likely affected by the above mentioned bug, correct?
  2. A seed phrase can be used to derive an arbitrary number of addresses, the addresses are deterministic - so account number 15 is always going to be the same address. Do I understand correctly that you derived the same address for Account 1 but the next Account is different from what you expected?
  3. Did you try installing the old version of Brave that you used in the past ( and restore your wallet from there? The one in which I identified the bug was version 1.8.96 and you can find the release here.
Jules-Win commented 2 years ago

Thank you for the response, sir. I actually derived (add account) more addresses by mistake while I was using the Flare Finance beta, while on the Coston testnet. A couple months after that, I connected one of the derived addresses to the app I used to create a node. I reset the wallet recently, and the first address derived was the one I used before setting up secondary ones. The tnxs history was there, all the custom networks I added were there as well, but when I clicked on "Add account", the address I needed wasn't showing up. Went frantic and added 400+ addresses, but it never showed. I can't see the derivation path, because of the difference in code, I guess. When I enter my seed in the MEW wallet, it derives a diff address for Account 1. I'll try installing this version and see if I can derive the right address. Thank you again for the feedback, regardless of how it goes. Will post what happened.

Jules-Win commented 2 years ago

No luck, unfortunately... Derived 60. Not showing up. Any other ideas maybe. Thanks again regardless. :)

BICE-BN commented 2 years ago

Man - the one time I used an seed to generate multiple addresses. I never double checked what brave was doing (which I should have done). Now trying to migrate and it is just not possible, it does not matter which hardware wallet or software wallet I use they all generate the same public addresses from the seed - but none of these addresses are the ones created by brave.

So yes - was an super fan of brave from day 1 - but these are the kind of mistakes that break trust in an un-restorable way. Will have to spent a lot of gas migrating all my brave software wallets - to a new wallet NOT generated by brave.

I will keep brave (probably) as browser maybe, but will NEVER let it have access to my crypto in anyway. Good luck with your company.

bbondy commented 2 years ago

@BICE-BN Simply go to restore in the wallet, enter your seed phrase and check on the option for Import from legacy option.

Screen Shot 2022-01-06 at 9 03 07 AM