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

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

Closed
diracdeltas opened this issue Dec 18, 2020 · 8 comments · Fixed by brave/ethereum-remote-client#208

Comments

@diracdeltas
Copy link
Member

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 diracdeltas added OS/Desktop priority/P3 The next thing for us to work on. It'll ride the trains. labels Dec 18, 2020
@diracdeltas diracdeltas self-assigned this Dec 18, 2020
@diracdeltas diracdeltas added the feature/web3/wallet Integrating Ethereum+ wallet support label Dec 18, 2020
@diracdeltas
Copy link
Member Author

bitgo integration currently uses a key derived from the master seed, but luckily it hasn't shipped yet. #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
Copy link
Contributor

srirambv commented Mar 8, 2021

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
  • Verified test plan from Switch to using metamask-compatible codewords ethereum-remote-client#208
  • Verified creating a new wallet uses 12 word backup phrase
  • Verified able to import 12/24 word wallet on a new wallet created on 1.0.25 component
  • Verified able to upgrade from 1.0.23 to 1.0.25 doesn't corrupt the wallet
  • Verified able to import a 12/24 word wallet on an upgrade profile from 1.0.23 component
  • Verified importing cushion pitch impact album daring marine much annual budget social clarify balance rose almost area busy among bring hidden bind later capable pulp laundry restores 0xea3C17c81E3baC3472d163b2c8b12ddDAa027874 wallet address
  • Verified importing drip caution abandon festival order clown oven regular absorb evidence crew where restores 0x084DCb94038af1715963F149079cE011C4B22961 wallet address
  • Verified adding a new account works
  • Verified able to connect a H/W wallet without issue
  • Verified able to sign transactions successfully using https://danfinlay.github.io/js-eth-personal-sign-examples/
  • Verified able to import private keys and wallet balance shows for imported wallet
  • Verified able to export private keys successfully
  • Verified restoring the same 12/24 word wallet multiple times restores the same wallet address

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
  • Verified test plan from Switch to using metamask-compatible codewords ethereum-remote-client#208
  • Verified creating a new wallet uses 12 word backup phrase
  • Verified able to import 12/24 word wallet on a new wallet created on 1.0.25 component
  • Verified able to upgrade from 1.0.23 to 1.0.25 doesn't corrupt the wallet
  • Verified able to import a 12/24 word wallet on an upgrade profile from 1.0.23 component
  • Verified importing cushion pitch impact album daring marine much annual budget social clarify balance rose almost area busy among bring hidden bind later capable pulp laundry restores 0xea3C17c81E3baC3472d163b2c8b12ddDAa027874 wallet address
  • Verified importing drip caution abandon festival order clown oven regular absorb evidence crew where restores 0x084DCb94038af1715963F149079cE011C4B22961 wallet address
  • Verified adding a new account works
  • Verified able to connect a H/W wallet without issue
  • Verified able to sign transactions successfully using https://danfinlay.github.io/js-eth-personal-sign-examples/
  • Verified able to import private keys and wallet balance shows for imported wallet
  • Verified able to export private keys successfully
  • Verified restoring the same 12/24 word wallet multiple times restores the same wallet address

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
  • Verified test plan from Switch to using metamask-compatible codewords ethereum-remote-client#208
  • Verified creating a new wallet uses 12 word backup phrase
  • Verified able to import 12/24 word wallet on a new wallet created on 1.0.25 component
  • Verified able to upgrade from 1.0.23 to 1.0.25 doesn't corrupt the wallet
  • Verified able to import a 12/24 word wallet on an upgrade profile from 1.0.23 component
  • Verified importing cushion pitch impact album daring marine much annual budget social clarify balance rose almost area busy among bring hidden bind later capable pulp laundry restores 0xea3C17c81E3baC3472d163b2c8b12ddDAa027874 wallet address
  • Verified importing drip caution abandon festival order clown oven regular absorb evidence crew where restores 0x084DCb94038af1715963F149079cE011C4B22961 wallet address
  • Verified adding a new account works
  • Verified able to connect a H/W wallet without issue
  • Verified able to sign transactions successfully using https://danfinlay.github.io/js-eth-personal-sign-examples/
  • Verified able to import private keys and wallet balance shows for imported wallet
  • Verified able to export private keys successfully
  • Verified restoring the same 12/24 word wallet multiple times restores the same wallet address

@aruialmeida
Copy link

aruialmeida commented Apr 27, 2021

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
Copy link
Member Author

@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
Copy link

aruialmeida commented Apr 27, 2021

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
Copy link
Member Author

@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
Copy link

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

@diracdeltas
Copy link
Member Author

@aruialmeida i mean email yan@brave.com

@srirambv srirambv added feature/ethereum-remote-client and removed feature/web3/wallet Integrating Ethereum+ wallet support labels Sep 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment