lukso-network / LIPs

LUKSO Improvement Proposals. Repository for the LUKSO Blockchain Improvement Proposals (LIPs) and LUKSO Standards Process (LSP).
96 stars 44 forks source link

[LSP11] SocialRecovery Improvement #216

Open YamenMerhi opened 1 year ago

YamenMerhi commented 1 year ago

LSP11SocialRecovery Improvement

I would like to open a discussion on the LSP11 standard, which serves as a social recovery mechanism for users to regain access to their Smart Contracts. While this standard is fully functional, there are several aspects I believe could be improved upon to facilitate better adoption and integration with other LSPs.

Critiques

Ideas and Concerns

I can see the features listed above as 'standard', other custom features such as recovering based on specific logic can be implemented on top of the LSP11SocialRecovery contract. For example, a contract that works with Custom logic X for recovering can be added as a guardian for the main LSP11SocialRecovery contract.

While I see that these are lots of ideas and improvements, I don't see a reason to not implement these ideas together If they make sense, instead of standardizing several social recovery contracts with different features and interfaces which will not contribute to the Interoperability.

skimaharvey commented 1 year ago

Looks very nice! Cant agree more on the critiques part:

Concerning the ideas and concerns. Just a few points:

CJ42 commented 1 year ago

The current design of LSP11 fits primarily with an LSP0 owned by an LSPP6.

Yes we can consider this could be one implementation among many, for a specific architecture.

we should aim to design the social recovery system in an owner-agnostic manner too.

Definitely agree

Deployment Costs: As a standard potentially useful to a wide range of users, ideally each account-contract should have an LSP11. However, the current requirement to deploy a unique contract for each account—even when using proxies—could create a significant cost barrier for companies aiming to provide social recovery options to their users

Yes a singleton contract would lower the barrier of entry for projects. Giving them just the need to deploy a singleton contract onchain that acts as the "backbone of the social recovery service". But there might be risk around this to be looked at.

I will have a think in depth about the security concerns it and come back to the discussion.

skimaharvey commented 1 year ago

was rethinking of Secret Word Leak:

Edit: What I personally dont like with the approach above is that people could store all the _value and then "brute-forte" addresses until they find a match to find each one's password. Maybe it is still relevant to have salt + addresOfContractToRecoverFor. Something like this

    function reveal(string memory _value, bytes32 _salt) public returns (bool) {
        bytes32 valueHash = keccak256(abi.encodePacked(addresOfContractToRecoverFor, _value, _salt));
        if (commitments[msg.sender] == valueHash) {
            return true;
        } else {
            return false;
        }
    }
YamenMerhi commented 1 year ago

The discussion that happened yesterday with @NBR2807 covered these points:

skimaharvey commented 1 year ago

Nice points were brought up!

Should the guardians vote for a call data to be executed, or vote for a specific address that can later execute call data? The difference is: When voting for an address, this address needs to execute the transaction itself, and you need to trust this address

IMO it should be the recoverer calling the recovery function (after vote of the guardians). Since he will be the recoverer i don't see any pb having him pass any calldata it wants.

The social recovery is created for cases where the access for this seed phrase or private key is lost, that's why we should not expect that the plainSecret (password used for recovery in LSP11) to be that hard. It could be something specific to the owner such as his dog name or grandmother's name, etc Which could be used as well as a password for other web2 accounts. The idea is to instead of having the contract deal with the plain secret itself and exposing it on the blockchain which may risk the other web2 accounts of the owner, instead of setting the hash of the plain Secret first, you set the hash of the hash, and on the recovery instead of passing the plainSecret, you pass the hash of the plain secret.

Good point, even though it won't improve the security aspect. Makes sense that you may not want to expose the plan secret. Although in my opinion, we would still need to add salt and possibly address the contract we are recovering for to the recovery process to make it more robust. I am pretty sure we could add the view functions that could help with creating the hash (would just need to trust your RPC provider 😛 )

Integration of relay execution.

Why not have the option but not sure it should be part of the standard. Anyway, with LSP17 we might just make some generic social recovery extensions. Can have two different for relay execution or not (with different function signatures). I think we relay execution possibly, we could have different logic where for example we dont even expose the plain secret nor hash but possibly the signature of the recoverer.

Let's see 👍

Edit: I think for the recovery process one should use wether it wants to :

CJ42 commented 1 year ago

@YamenMerhi @skimaharvey

I have looked at the discussion thread and posted all my ideas and comments below.

Firstly, I agree on the fact that LSP11 should be a standard not related directly to LSP6. Because access control could be implemented in any other form depending on the contract or dApp. (I have listed few ideas on top of my head below).

How about passing some calldata to the recoverOwnership function? This calldata could be set in the contract storage for each linked accounts that register to the LSP11 Social Recovery service.

See for instance the two functions at the bottom of this file to see how contracts could register to the Social Recovery service (in this code, this is contract is for installing relayer addresses, but the principle is the same).

https://github.com/PISAresearch/contracts.any.sender/blob/master/versions/0.3.0/contracts/core/Relay.sol

How can we implement an LSP11 that interact directly with the LSP0 setData function? (Or any function really on the account that subscribed to the Social Recovery service).

We could put a social recovery in front of:

Singleton pattern sounds relevant to me. Multiple services could deploy their own social recovery and compete between each other. This would enable users to register / deregister easily, or even subscribe to multiple recovery service at the same time for different purposes.

Privacy of addresses could make sense, as an optional extension. For instance, social recovery of vault that hold large amount of digital value would make sense. As you don’t want the guardians to be exposed, especially if they are UPs and their UP address is publicly known. They could be « tracked » and forced to hand over the secret to recover the account so to give access to malicious users.

Accepting Guardian invitation —> couldn’t we use here LSP14 (the generic standard interface) with the same functions. But the implementation actually is a contract that has multiple owners that are the guardians? So the owners / guardians are addresses that have to accept ownership on invitation. Something like this. This could be maybe an i teresting example of showing how the LSPs are composable, for different implementation (for different use cases), using the same LSP standard interfaces for interroperability.

Relay Execution: maybe the recovery process might not be good to be allowed as a relay execution (although debatable? Why not?) I think it should be possible to allow to vote for guardians using meta transactions. The same way as you can elect someone to go to vote for you on your behalf for the presidential elections. This is actual implemented in existing protocols, such as the Uniswap governance token. User can delegate guardians votes the same way as the function delegateBySig(…) —> this could also be used as a way to implement a one time recovery vote for guardians (instead of allowing guardians to vote infinitely as long as they are guardians)

https://docs.uniswap.org//contracts/v2/reference/Governance/governance-reference

Very good point from @skimaharvey about "pluging-in" a Social Recovery services as a LSP17 Extension for instance. This could be one of many example of how this can be embedded in contracts and dApps. For having it added as an LSP17 Extension, is it so that you can call the UP directly and re-route the call to the LSP11 contract? What would be the benefit? More gas still. Can’t you call the LSP11 contract directly?

The salt is definitely a MUST to add imo. As this would reduce the common types of attacks around password cracking like rainbow tables 🌈. However, we should ensure that salt cannot be reused after a social recovery process completed. Otherwise, the salt becomes publicly known (by looking at the transaction on the explorer), leading attackers to be more able to brute force the secret for the subsequent recovery process.

See some links about rainbow tables:

Having guardians accept the invitation to be guardians will decrease user adoption as guardians need to pay either through a relayer or through gas fees the cost of accepting ownership. Even if subsidized by the owner setting the guardians, it's a cost that is not needed and won't introduce any extra layer of security.

Good counter arguments here.

The interoperability issue of having LSP11 strictly usable by LSP0 X LSP6 MUST be fixed and the LSP11 standard should be generic to support social recovery even for non-LSP0 contracts by having the ability to execute whatever function on the account at the process of recovery.

I cannot agree more on this. This MUST be fixed.

Should the guardians vote for a call data to be executed, or vote for a specific address that can later execute call data?

That’s a good question here to be looked at. We can discuss this further tomorrow in our meeting.

YamenMerhi commented 1 year ago

The discussion that happened yesterday with @NBR2807 , @CJ42 , @skimaharvey and @b00ste covered these points:

YamenMerhi commented 7 months ago

Recent Discussion: