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

Wallet recovery, ETH derivation path from June 2017 #15

Open
JasonCoombs opened this issue Jan 24, 2022 · 15 comments
Open

Wallet recovery, ETH derivation path from June 2017 #15

JasonCoombs opened this issue Jan 24, 2022 · 15 comments

Comments

@JasonCoombs
Copy link

JasonCoombs commented Jan 24, 2022

I'm recovering a wallet created in June 2017 containing ETH.

The user forgot which App they used to create the wallet, but we know they used Bread Wallet also and this wallet might have been created with Bread Wallet. We successfully recovered the BTC balance in the wallet with the seed words using the current version of Bread Wallet, but it appears the ETH derivation path might have changed since June 2017.

e.g. this commit on September 24, 2017: breadwallet/breadwallet-core@8256769#diff-804fa0e5b81c52e1473cf06c4ba455b69add800fd3ef56f728dc7b39221bd484L163

shows the old derivation path (apparently also for ETH) was m/0H/chain/index rather than relying on BIP44.

Q: What was the ETH derivation path in June 2017?

https://github.com/breadwallet/breadwallet-core

JasonCoombs referenced this issue in breadwallet/breadwallet-core Jan 24, 2022
@JasonCoombs
Copy link
Author

@voisine
Copy link
Member

voisine commented Jan 25, 2022

The commit you linked doesn't appear to have changed any derivation paths, it's just a refactor to remove code duplication. I don't believe we ever changed the derivation path. That would break recovering old wallets, including our own internal test wallets, so that's something we were careful to avoid.

@JasonCoombs
Copy link
Author

JasonCoombs commented Jan 25, 2022

BIP44 appears to have been added later. I understand that making such a change would have broken wallet recovery, but that's exactly what it seems might have happened here, which is why I am searching for the answer to the derivation path question so the inaccessible ETH balance can be recovered.

See:
here: breadwallet/breadwallet-core#41

// The BIP32 privateKey for m/44'/60'/0'/0/index
    BRBIP32PrivKeyPath(&privateKey, &seed, sizeof(UInt512), 5,
                       44 | BIP32_HARD,          // purpose  : BIP-44
                       60 | BIP32_HARD,          // coin_type: Ethereum
                       0  | BIP32_HARD,          // account  : <n/a>
                       0,                        // change   : not change
                       index);                   // index    :

The commit referenced breadwallet/breadwallet-core@8256769#diff-804fa0e5b81c52e1473cf06c4ba455b69add800fd3ef56f728dc7b39221bd484L163 shows Bread Wallet using only m/0H/chain/index for ETH derivation, not m/44H/60H/0H/index

Can you point me to the source code from 2017 that derived the Ethereum keys using either BIP44 as it does today or passing the "chain" code parameter to the BRBIP32PrivKeyList() function that Bread Wallet used for deriving Ethereum keys back in 2017?

@JasonCoombs
Copy link
Author

JasonCoombs commented Jan 26, 2022

Note the change made in September 2017 to the function that derives the key.

Prior to breadwallet/breadwallet-core@8256769#diff-804fa0e5b81c52e1473cf06c4ba455b69add800fd3ef56f728dc7b39221bd484L163 it appears that Bread Wallet derived the Ethereum key (just one, correct? not HD multiple keys like some other wallets did back then) by calling the same function BRBIP32PrivKeyList() as for Bitcoin derivation. Where do we see exactly what argument values were being passed for Ethereum key derivation to the parameters of that old function in the old code pre-September 2017?

@JasonCoombs
Copy link
Author

JasonCoombs commented Jan 26, 2022

The commit you linked doesn't appear to have changed any derivation paths, it's just a refactor to remove code duplication. I don't believe we ever changed the derivation path. That would break recovering old wallets, including our own internal test wallets, so that's something we were careful to avoid.

Am I missing something simple here? Maybe Bread Wallet did not support Ethereum yet back in June of 2017?

breadwallet-ios imported breadwallet-core @ d0dbad3 August 17, 2017 https://github.com/breadwallet/breadwallet-ios/tree/4c7bc055038c074914ca9ae5e35466aeb6bd9485/Modules

breadwallet-ios @ 4c7bc05 August 17, 2017 https://github.com/breadwallet/breadwallet-ios/tree/4c7bc055038c074914ca9ae5e35466aeb6bd9485

https://github.com/breadwallet/breadwallet-core/tree/d0dbad3b7b6691e54ae9fca5659c13726654bf5a

I just don't see Ethereum derivation anywhere. What I see, in breadwallet-core, is Bitcoin derivation. And I see BIP39 seed words passed from the iOS swift code to the c code in brcore. What am I missing and where? Thanks.

@JasonCoombs
Copy link
Author

According to this line in BRWallet.c back in 2017 Bread Wallet only supported two "chain" values during key derivation, internal or external, with internal used for change addresses and external used for receive addresses.

https://github.com/breadwallet/breadwallet-core/blob/f4aec7f538f42ffc93de5d009d529baad28bede6/BRWallet.c#L325

This strongly suggests that from April - September 2017 Bread Wallet only supported Bitcoin. Is this correct?

@voisine
Copy link
Member

voisine commented Jan 28, 2022

I need to verify, but this sounds right. That was around the time we added support for eth. That’s probably why I refactored that code to make it more general purpose.

@shivangigandhi
Copy link
Contributor

FWIW - As I recall Ethereum support was added around March/April 2018.

@taoeffect
Copy link

I'm rather extremely disappointed with the BRD developers - they've given their users no way to self-recover their funds without going through coinbase.

They could have allowed their app to continue functioning and simply let users point them to their own nodes, but instead they hijacked their funds to what I'm assuming is a KYC-only operation.

How does one recover ETH without going through coinbase? BTC? Any of the other tokens?

There are no instructions.

@voisine
Copy link
Member

voisine commented Jun 30, 2022

@taoeffect Coinbase Wallet is a self custody app just like BRD, and is separate from the Coinbase retail app. Coinbase Wallet does not require KYC to recover your funds.

@taoeffect
Copy link

@taoeffect Coinbase Wallet is a self custody app just like BRD, and is separate from the Coinbase retail app. Coinbase Wallet does not require KYC to recover your funds.

This seems to be contradicted by some of the users in the BRD subreddit. As one user said:

I open the app, I click on Bitcoin, I click on send, I enter an amount, and click next. The Send To screen only allows me to enter in a Coinbase user name. I don't see anyway to enter a bitcoin address.

Please help.

@voisine
Copy link
Member

voisine commented Jun 30, 2022

@taoeffect sounds like user error, you can scan bitcoin addresses with CB Wallet

@taoeffect
Copy link

taoeffect commented Jul 2, 2022

@voisine OK, I looked into this more and can confirm what you're saying, it seems like CB Wallet does let users send to addresses.

I still think there should be an option for users who don't want to use CB Wallet (for whatever reason). I don't think users should be forced into that funnel, especially given Coinbase's history of tracking user funds, stealing funds, sharing of personal data with government agencies, etc.

BRD could have, at the very least, written a guide on how to use the seed to migrate to various other wallet options. It really wouldn't be that hard. And again, they could have handed over the app to its open source community, allowed users to point it at their own nodes, etc.

If I recall correctly, these were even features that existed in previous versions. Instead, it seems BRD went out of its way to remove those features, and do additional work to sabotage and undermine their own app so that it would be completely unusable. I am guessing they did this so that they could get $$$ from CB. What I don't understand is why. If BRD needed $ to fund development, they could've introduced an optional fee that's on by default within the app to support development. Instead, they destroyed their hard work, destroyed the good will they'd built up with the community, and in the process a lot of users seem to have lost their funds too, at least according to r/BRDapp. I just don't get it.

😒

@taoeffect
Copy link

taoeffect commented Jul 2, 2022

BTW, here are the steps on how to migrate ETH to Metamask for those wondering:

  1. Install MetaMask: https://metamask.io/
  2. Use https://iancoleman.io/bip39/ - or better yet, get it via the releases page and run with your wifi off: https://github.com/iancoleman/bip39/releases/tag/0.5.4
  3. Enter your seed where it says "BIP39 Mnemonic"
  4. Choose "ETH - Ethereum" where it says "Coin"
  5. Make sure BIP44 Derivation Path is chosen (it is by default)
  6. In the table below where it says "Derived Addresses", copy the value in the first row under "Private Key"
  7. Back in Metamask, after setting it up, click the icon in the top-right of the Metamask window to open a menu, and select "Import Account"
  8. Paste that private key and click "Import"

For BTC, just use BlueWallet to import your seed.

@voisine
Copy link
Member

voisine commented Jul 2, 2022

@taoeffect BRD has always been open source, anyone is welcome to fork it.

Also we had a team of 40+ people and a burn rate of $1M/mo. Our revenues were significantly more than what an optional developer support fee would have generated. Joining with coinbase was the best option for our users, investors and team. All the issues you mentioned with coinbase are the result of US financial regulations for licensed custody services, and don’t apply to their self-custody product.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants