fluidkey / stealth-safe

15 stars 1 forks source link

Robust private view key sharing between Safe owners #4

Closed mozrt2 closed 1 year ago

mozrt2 commented 1 year ago

In the POC, we created an onchain registry which stores encrypted versions of the shared private view keys used to scan for transfer events to a Stealth Safe by the owners of the master Safe. We use eth-crypto’s encryptWithPublicKey() to encrypt the same private key N times using the public key of each owner. A master Safe of N owners is therefore added to the onchain registry with N encrypted keys, one for each owner.

The drawback with this method is that one compromised private key within all N owners would enable an attacker to get access to the shared view key and find all transactions privately sent to the Safe.

More research needs to be done to find a good alternative. One proposed technique to look into is a Diffie-Hellman key exchange with multiple parties. Implementations using Shamir’s secret sharing and / or the use of another layer of encryption (e.g. password) should also be considered.

mozrt2 commented 1 year ago

Suggested improvements for v1, @70nyIT let me know what you think:

The encryption and decryption functions would then look as follows:

Encryption

encryptViewingPrivateKeySa (
    viewingPublicKeyArray: string[], 
    viewingPrivateKeySa: string,  
    requiredShares?: number, 
    salt?: string 
) {
    return viewingPrivateKeySaEncryptedArray
}

Decryption

decryptViewingPrivateKeySa (
    viewingPrivateKeyArray: string[],
    viewingPrivateKeySaEncryptedArray: string,  
    salt?: string 
) {
    return viewingPrivateKeySa
}

This should give sufficient security options for most use cases. Users could of course also choose not to store the keys onchain at all and/or use a different key management (and generation) solution.

Later on, we could look into Distributed Key Generation schemes. From my research, there doesn't seem to be any ready to use js libraries for this right now, so would keep things simple for now.

70nyIT commented 1 year ago

I have difficulties to see how a SSS can work practically (not theoretically, that is a great point!). So far, if I'm a Safe owner, I need the viewingPrivateKeySa to decrypt the Entropy number that's encrypted in the event emitted. As a user, I get the viewingPrivateKeySaEncrypted, decrypt it and use it to scan the events.

If an SSS schema is introduced, then I can get just a part of the key, then wait for another user (in case of 2/3) that decrypts his part. He then has to send me his portion (decrypted) and I have to send him mine.

While technically is possible, practically if we don't want a centralized service to rely on, we need to set up a P2P encryption channel, with e2e encryption.

I agree that storing the viewingPrivateKeySa on chain should be optional (while this cannot be for the viewingPubKeySa). Of course each user will need to input the key in the UI manually.

The salt to encrypt the viewingPrivateKeySa should be the salt to generate the viewingKeyPairOg, that can be a simple PIN to avoid a malicious website can force me to sign the same message. This though should be at Umbra level, or at least agree with them on how to introduce it, unless we want to use a different message to sign to generate the keys needed to encryp/decrypt the viewingPrivateKeySa. The salt PIN (or password) can be added at the end of the message.

In addition to your points, I'd add that the message signed to generate the viewingKeyPairSa should be a random message, and not a fixed message. This avoids it can be recreated anytime by a malicious attacker. Since it's just a viewing key, setting up a Distributed Key Generation can bring advantages, but maybe not as much as for situations where the key is a spending key, but I agree can be something we could add to the protocol

mozrt2 commented 1 year ago

Thanks for reviewing this - good points!

SSS: My thinking was that some users might prefer some centralization in exchange for more security. Example below:

But this might be overcomplicating it right now so I'm also fine with leaving it out and just having the salt as additional safety. Let me know what you think.

Very good point regarding the random message - we can generate the message to be signed with some unique data (such as the date & a salt). What this means however is that the viewingPrivateKeySa cannot be regenerated if it somehow gets lost wherever it is stored.

70nyIT commented 1 year ago

Reading your points I think that, for the scope of our tool, we should say that the viewingPrivateKeySa is optional when registering the corresponding public key. This opens up a lot of possible extensions that can be done and integrated within our solution. We should have, instead, an optional string parameter in the SC function, when storing the Sa keys. This can be used to store any data that might be useful to remember the service used to encrypt the viewingPrivateKeySa, like a "post-it note". Anyone should be able to use it at their convenience.

Yes, if the ability to decrypt the viewingPrivateKeySa is lost, then no recovery if the key has been generated by signing a random string. But if we add a salt ... who records (or remember) the salt? Maybe an option like Sarcophagus ( https://sarcophagus.io/ ) can be integrated, but I think this goes into the case above: should be an extension while we focus on the core.

70nyIT commented 1 year ago

instead, we should maybe versioning the viewingKeyPairSa, so that if something changes on the master Sa, a new key can be added, without breaking previous ownership. I know this might be expensive in terms of gas-cost, but it's also a one-time call and I don't think our solution will ever be used on mainnet.

Or, let me see the other way around, if you use it on mainnet, I bet you drive a Ferrari

mozrt2 commented 1 year ago

Fully agree with the direction of keeping the onchain Safe Stealth Key Registry flexible for different implementations of viewingPrivateKeySa storage and encryption (or not).

So now we also should review what JS functions are needed as extensions of umbra-js. The encryption functions in my second post in this thread are probably out of scope then. viewingKeyPairSa could be generated based on a signature or any other method. So I guess the conclusion is that this can be defined on a per-service basis and the issue can be closed here?

70nyIT commented 1 year ago

Yes. This also connects a lot with what I'm doing in #1 , as I'll take the function definitions in the Umbra.js class, reporting which need to be extended, and which need to be added. Then we can proceed to write the implementations.

70nyIT commented 1 year ago

I also think that the Robust scope of this issue stays also in the generation of a key by signing random data, and not a static message

mozrt2 commented 1 year ago

Perfect, I will then close this issue

mozrt2 commented 1 year ago

Actually one more thing that just came to mind. If we go the route of letting the specific service decide how to manage the viewingPrivateKeySa our Stealth Key Registry contract is not so different from the Umbra one anymore. Once we are clear on the architecture / requirements, we should discuss if we can make the Umbra Stealth Key Registry compatible with smart accounts instead of having a separate one.

70nyIT commented 1 year ago

Makes sense. In this case we need the core team to add the possibilities to:

So even our "native" service for sharing PrivateKeys can be plugged into the existing solution (as an "on-chain private keys sharing service").

The hard part I see, is that the current registry is non-upgradable (that's positive as it guarantees security) - see for example https://etherscan.io/address/0x31fe56609c65cd0c510e7125f051d440424d38f3#code

In this scenario a new contract needs to be deployed, but I'm not sure how they'll handle old deposits vs new deposits. With a new contract

This is something that maybe with the EIP coming out might be solved

70nyIT commented 1 year ago

well, we could actually use the current contract. The workaround is to set spendingPubKey to a very low number (ex: 1, 2, 3, etc, almost impossible that this can be a secure privateKey), but since it's open, anyone can write a number thus it will be pretty confusing.

mozrt2 commented 1 year ago

Yes, also was thinking through using the contract as it is, but agree this wouldn't be an elegant solution.

Let me ask if they plan to upload a new Stealth Key Registry when updating to be compliant with the EIPs. If so, we can suggest our changes until then.