brave / brave-browser

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

revert to using metamask's bip39 key derivation method in cryptowallets #13245

Closed diracdeltas closed 3 years ago

diracdeltas commented 3 years ago

the 24-word phrase that users are told to backup in crypto-wallets is actually a master seed which is supposed to be used to derive various private keys (including the wallet private key), but users generally assume it's just the wallet private key. this causes confusion when they try to import/export their wallet to other applications. although we offer the option to export the private key in hex encoding, which should be compatible with other apps, a lot of people expect us to support the bip39 standard.

the original justification for our non-standard design (https://github.com/brave/brave-browser/wiki/Brave-Ethereum-Remote-Client-Wallet-Seed-Information) was this:

  1. various applications in brave require a private key which only the user knows (cryptowallets, rewards, the encryption key for Brave Sync, etc.). for usability, it would be ideal if the user only had to backup a single phrase from which all of these keys could be derived using a one-way function. we call this the master key.
  2. compromise of one application's private key should NOT compromise other applications. for instance, if your wallet private key gets leaked, the attacker should not be able to use it to derive your sync encryption key.

we cannot satisfy both these requirements if the backup phrase is the mnemonic for the cryptowallet private key itself.

unfortunately, it's also not possible to derive the bip39 mnemonic phrase from the wallet private key, because the wallet seed is derived from the mnemonic using PBKDF2. https://github.com/bitcoin/bips/blob/master/bip-0039.mediawiki

the only solution i can think of is to revert to metamask's way of key derivation, which is compatible with other wallets. luckily we haven't yet implemented key consolidation for rewards/sync/etc., so the migration process is not too bad:

  1. new users will get the standard-compatible mnemonic
  2. improve the messaging for existing users to make it clear that their mnemonic does NOT correspond to the wallet private key, but they can create a new wallet and transfer the funds over to get one that is.
  3. 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.
diracdeltas commented 3 years ago

bitgo integration currently uses a key derived from the master seed, but luckily it hasn't shipped yet. https://github.com/brave/brave-browser/issues/12296

unless we want users to have to write down a separate mnemonic for their bitgo multisig private key, we will need to change that implementation to use a key derived from the metamask-compatible cryptowallet private key. looks like https://github.com/bitcoin/bips/blob/master/bip-0032.mediawiki can be used for this.

cc @ryanml @marshall

srirambv commented 3 years ago

Verification passed on

Brave 1.22.49 Chromium: 89.0.4389.72 (Official Build) beta (64-bit)
Revision 3f345f156bfd157bd1bea06310e55f3fb2490359-refs/branch-heads/4389@{#1393}
OS Linux
Component 1.0.25 Dev

Verification passed on

Brave 1.21.74 Chromium: 89.0.4389.72 (Official Build) (64-bit)
Revision 3f345f156bfd157bd1bea06310e55f3fb2490359-refs/branch-heads/4389@{#1393}
OS Windows 10 OS Version 2009 (Build 19042.804)
Component 1.0.25 Dev

Verification passed on

Brave 1.23.21 Chromium: 89.0.4389.72 (Official Build) nightly (x86_64)
Revision 3f345f156bfd157bd1bea06310e55f3fb2490359-refs/branch-heads/4389@{#1393}
OS macOS Version 10.15.7 (Build 19H114)
Component 1.0.25 Dev

aruialmeida commented 3 years ago

Hi, 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!!!

Above says "but restore using the legacy way if it's 24", but don´t understand.

appreciate some help. yt1 create restore

diracdeltas commented 3 years ago

@aruialmeida seems like a bug. what version of Crypto Wallets is on each of the two computers? you can check in brave://extensions/?id=odbfpeeihdkbihmopkbjmoonfanlbfcl.

the latest version should create 12 word phrases, not 24

aruialmeida commented 3 years ago

thanks for the reply diracdeltas.

if i create a new account now it create with 12 words, yes. but i create some months ago, and in that time it gave me 24 words. and now i am trying to restore the account without success. It open in another address i don´t recognize.

both brave versions are up-to-date as can see in the image. brave version

diracdeltas commented 3 years ago

@aruialmeida you mentioned in your other comment that it worked in metamask. can you send me:

  1. the original wallet address you created in Brave (with the 24-word phrase)
  2. the wallet address that was restored when you imported the phrase into metamask
  3. the wallet address that was restored when you imported the phrase into Brave on another computer

feel free to email yan at brave.com, thanks!

aruialmeida commented 3 years ago

ok, i am in brave.com, i am seeing Yan Zhu (Chief Information Security Officer), but i am not seeing the e-mail.

diracdeltas commented 3 years ago

@aruialmeida i mean email yan@brave.com