MetaMask / metamask-snaps-beta

Fork of MetaMask that supports plugins! Read the Wiki!
https://github.com/MetaMask/metamask-snaps-beta/wiki
MIT License
142 stars 57 forks source link

Maybe: Allow permission to derive key from BIP44 path #104

Closed danfinlay closed 2 years ago

danfinlay commented 4 years ago

Today, if you wanted to make a Bitcoin plugin, for example, you would receive an appKey using our appKey API which would derive a private key unrelated to the slip44 coin derivation path and instead be derived from the source of your plugin.

This is nice for simple use cases, but for well established protocols, many users will want to interact with keys generated in standard ways, and so we will probably want to provide a permission to expose these.

Possible permission label: This site wants to: Manage your keys for the [BITCOIN] protocol.

This could be from a permission that maybe resembles this:

initialPermissions: {
  manageKeys: {
    caveats: [{ type: 'path', value: '0x80000000' }] // this path is bitcoin, and we would know it by slip44
  }
}

This solves an issue of the ability to restore keys generated in well established ways.

danfinlay commented 4 years ago

Not sure this needs to be an MVP-blocker, but depends on how we want to commit to a key derivation strategy for plugins.

Bunjin commented 4 years ago

I agree, there is a lot of potential for this, which would allow MetaMask to become quite easily a multi crypto wallet which could be great for Decentralized exchanges/cross chain swaps for instance. We can either share directly the root private key of depth 2 once the permission is approved m / purpose' / coin_type'

or also include as a parameter (either as a field required and mentioned in permission, or as an option) the other depths of the BIP44 spec: account' / change / address_index'

This would allow to either only give access to one account selectively and/or to allow the apps to use our HD derivation library instead of having to handle this part too.

Bunjin commented 4 years ago

Another question is : Do we allow for value: 0x8000003c which is Eth. This would mean that a website would be able to gain control of an user already potentially funded accounts.

That may be the case too for other cryptocurrency if the user uses the same mnemonic in other crypto (non eth) wallets.

In any case, we need a HUGE red flag for any of these permissions and probably to present these huge security risks permissions to the user individually, and not bundled.

danfinlay commented 4 years ago

Do we allow for value: 0x8000003c which is Eth.

I think at least initially we would disallow this, but I know that over time advanced users will request basically every possible permission, so I think it ultimately will come down to whether we can make the confirmation descriptive enough to endow the permission with the correct gravity.

danfinlay commented 4 years ago

As for the derivation paths, it would be great to only expose a single key, but realistically, providing UI to intelligently show what key they are exposing, and what it puts at risk may be outside the scope of the feature: The feature exists largely so we can externalize the responsibility of representing protocols that other teams may be more expert on, and if we were able to tell you how valuable exposing your zcash keys are, for example, we would have built a sort of zcash wallet already just for that sake.

So my initial MVP implementation would probably be:

Over time we can start to break these up into more fine-grained pieces. It may be nice to pre-ship with value-checkers for different services, so we can warn users when they are putting valuable keys at risk.

rekmarks commented 2 years ago

We did this elsewhere: https://github.com/MetaMask/snaps-skunkworks/blob/32843a7f58e3274f70abb58c4ef284fc53e146e0/packages/rpc-methods/src/restricted/getBip44Entropy.ts