Openzeppelin-contracts: Superuser: Upgradeable security for Smart Contracts

[federicobond]

Scenario: you create a small crowdsale for your widget and it becomes really popular very quickly. Now there is a million dollar price tag on the poorly secured private key you used to deploy your contract and you start sleeping badly at night. Nobody worth their salt would secure a million dollars in a hot wallet.

Luckily, you remember that your contract inherits from Superuser, so next morning you generate a key in cold storage and take note of its address. You then use your owner account to configureSuperuser(address) and your sleep improves again because any sensitive method is using the requireSuperuser modifier. This modifier ensures that msg.sender is the superuser or (in case no superuser is configured, the owner). BOOM! Upgradeable security and peace of mind.

I will have a working implementation later today. Feel free to leave any feedback.

UPDATE: As was pointed out in the Slack channel, this is not a good example, as you can just transfer the ownership of the contract. A better example is when you want to keep a hot key for performing managerial actions but have a cold storage key as superuser in case the hot key is compromised.

[maraoz]

:+1: nice idea. Maybe the example is not the best, true, but I see the value

[ajsantander]

@federicobond would you like to make a PR to explore this idea?

[federicobond]

I am not sure if the use case is general enough to warrant its inclusion in Zeppelin, but here are my updated thoughts on it:

A Superuser contract would inherit from Ownable. The superuser variable in this contract points to an address whose key should be kept in a cold wallet and never used for any other purpose. If there is a security breach and someone gets access to the owner key, this can be used as a last resort to regain control of the contract, transferring the ownership back to the original owner.

contract Superuser is Ownable {
    address public superuser;
    function setOwner(); // can only be called by superuser
    function transferSuperuser(); // can be called by superuser or owner if superuser is not defined
}

This could be useful if owner key must be used on a regular basis for administrative purposes.

[shrugs]

This could/should be implemented with RBAC.sol now that it's available :)

[elopio]

@federicobond still sounds relevant to me. Having two owners instead of one, good deal.
As @shrugs, now this is a lot easier. Maybe you can make a PR and then we discuss there if it makes sense to add it to openzeppelin, or we should just make a post to document how to do it. Are you still interested in helping with this?

[pmosse]

Hey,

If we implement the Superuser contract with RBAC, instead of having a single superuser we would have an unlimited number of them, by creating the superuser role. Do you think this is okay in a practical scenario? Besides that, we should make sure that superusers can only be set once (either in the constructor or with a setSuperusers function). Once they are set, the owner shouldn't be able to modify them as the owner's account could get compromised in the future.
This could be implemented by following a similar idea to the Whitelist contract and creating the setSuperusers (onlyOwner and onlyIfSuperusersUndefined) and transferOwnership (onlySuperuser) functions.

If we only want a single superuser, then maybe a contract inheriting just from Ownable should be enough, following @federicobond's idea.

Let me know your thoughts and I can work in a PR.

(You should know I am doing my first steps in OZ and Blokchain in general :) )

Thanks!

[shrugs]

You can have the only function allowed to change superusers be transferSuperuser() which will unset the previous super user and then assign the new super user.

contract Superuser is Ownable, RBAC {
  ROLE_SUPERUSER = "superuser";

  modifier onlySuperuser() {
    checkRole(msg.sender, ROLE_SUPERUSER);
    _;
  }

  constructor () {
    addRole(msg.sender, ROLE_SUPERUSER);
  }

  function transferSuperuser(address _newSuperuser) onlySuperuser() {
    removeRole(msg.sender, ROLE_SUPERUSER);
    addRole(_newSuperuser, ROLE_SUPERUSER);
  }
}

that's pseudo code, but it seems to be the gist of it. This is pretty much how I implemented RBACOwnable in a pending PR to OZ.

[shrugs]

Implementing single-address roles in rbac is relatively straightforward. the only difference between RBAC with "only one person can have this role" and something like Ownable is that the user's address isn't directly accessible in the contract; the callee must know an address in order to call hasRole and see if that address actually does have that role.