Brave-browser: crypto wallet: auto-generated mnemonic is not standard compatible

[SCBuergel]

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]

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]

@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]

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

[diracdeltas]

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]

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]

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]

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]

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]

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]

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

[diracdeltas]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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

• Verified private key is all in lowercase

---

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

• Verified private key is all in lowercase

---

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

• Verified private key is all in lowercase

[sebastianzolg]

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]

@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]

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.