Add transferRole function to AccessRole allowing transferring of roles from role bearers to a new accounts

[lbeder]

In this PR, I've added a new transferRole function to AccessControl, which allows role bearers to transfer their privileges to new accounts in case they are switching to a new account or compromised, without involving the role admin (which in many cases is an involved time-locked governance process).

• Similar to renounceRole, the account has to be the caller itself.
• A successful transfer from account to recipient will result in: 
  • The role will be granted to the recipient.  
  • The role will be revoked from account (the caller)
• A transfer will gracefully ignore and return if: 
  • Both account and recipient are the same accounts.
  • account doesn't bear the target role. 
  • recipient already bears the target role.

I've tried to make the PR as idiosyncratic as possible, but of course, any feedback is highly appreciated and welcome. I've also ensured that all new and existing tests are passing and that the new code is fully covered.

P.S.,
If this PR is accepted and merged, I'd be more than happy to port it to AccessControlUpgradeable.sol as well.

[Amxx]

Hello @lbeder And thank you for raising that.

I'm not sure this is something we want.

The roles are designed to be managed by an admin. This means that if you have a role, you can't necessarily grant it to someone else. I understand that this PR transfers the role, in the sens that it revokes it from the previous owner.

However, you could use that to transfer a role to a smart contract that would allow multiple accounts to operate with it. IMO this would be breaking the assumptions that make AccessControl what it is today.

Basically, I don't think users should be able to create/leverage this kind of contracts without specific approval.

AccessControlProxy is AccessControl {
    bytes32 public constant OPERATOR_ROLE = keccak256("OPERATOR_ROLE");
    
    mapping(address => bool) public protected;

    constructor(address _protected) {
        _grantRole(DEFAULT_ADMIN_ROLE, _msgSender());
    }

    function setProtected(address target, bool value) external onlyRole(DEFAULT_ADMIN_ROLE) {
        protected[target] = value;
    }
    
    function relay(address target, bytes calldata data) external onlyRole(protected[target] ? DEFAULT_ADMIN_ROLE : OPERATOR_ROLE) {
        Address.functionCall(target, data);
    }
}

[lbeder]

Hey @Amxx,

I understand your point but keep in mind that one can't make this assumption today either. For example, an EOA can be shared (even retroactively) between multiple parties either via a threshold ECDSA protocol (like it's done by many custody services) or even the private key itself can be just transferred (not necessarily intentionally) to a third party. I.e., in any case, you can't assume that a role receiver can't grant the role to someone else, while this PR allows it to be handled in a more transparent, observable, and explicit way.

Would you prefer that I'll implement it as an AccessControlTransferrable extension or just shelf it for now?

[nchamo]

Hey everyone!

I'm Nico from Mean Finance 👋 . I came across this PR, and I just wanted to share our experience.

As you say @Amxx , we have an admin that manages all other roles, but we would love to be able to transfer it from one account to the other. We wanted to migrate from EOAs to Multisigs and we had to redeploy 😅
Right now we have a few options to handle this with AccessControl:

1. Make the admin role it's own admin, so we can add a new account and then renounce from the previous account. This is of course not great because we would prefer to have only one admin and we couldn't enforce it
2. Make the admin account a smart contract that handles the transfer on its own, which kind of defeats the purpose of using AccessControl if I need to have another contract that handles access control 😅

That's it, just wanted to give our 2 cents

[heldersepu]

I like this addition, feels like self service, (reset own password, transfer MFA to new device) ...
but maybe it should be an option that the admin can enable/disable, not forced to all consumers of AccessRole by default

[heldersepu]

to require or not to require ...

... maybe all these conditions on this if should be individual require, that way we can explain why it does not get transferred

[lbeder]

Personally, I agree, but I wanted to make this new function as similar and idiosyncratic to the existing code (see the graceful return in _revokeRole).

[frangio]

For example, an EOA can be shared (even retroactively) between multiple parties either via a threshold ECDSA protocol (like it's done by many custody services) or even the private key itself can be just transferred (not necessarily intentionally) to a third party.

These are interesting points to consider but the implications of a transferRole function are different. In particular, sharing the private key with a third party in any way does not give up the ability to renounce the role. If the key is stolen, you can renounce the role without intervention from the admin and this is a really important propery that transferRole invalidates.

If the private key is a threshold key from the start this is simply part of the assumptions when the role is granted.

Personally I don't see reasons to make a role transferable except perhaps for the specific case of the admin as shared by @nchamo. But I think that should be tackled differently, as a special case rather than a general ability to transfer any role. So I think we should shelve this PR and separately discuss a design for that case.

[frangio]

Closing based on the reasoning described in #3593 (comment) and #3593 (comment).

Transferability for the admin role being discussed in #3623.