5

Since I am developing a smart contract which requires multiple owners, I modified the Ownable class from OpenZeppelin like this

contract Ownable {
    mapping(address => bool) public owner;
    event AddedOwner(address newOwner);
    event RemovedOwner(address removedOwner);

    /**
    * @dev The Ownable constructor sets the original `owner` of the contract to the sender
    * account.
    */
    function Ownable() public {
        owner[msg.sender] = true;
    }

    /**
    * @dev Throws if called by any account other than the owner.
    */
    modifier onlyOwner() {
        require(owner[msg.sender]);
        _;
    }

    function addOwner(address _newOwner) onlyOwner public {
        require(_newOwner != 0);
        owner[_newOwner] = true;
        AddedOwner(_newOwner);
    }

    function removeOwner(address _toRemove) onlyOwner public {
        require(_toRemove != 0);
        require(_toRemove != msg.sender);
        owner[_toRemove] = false;
        RemovedOwner(_toRemove);
    }
}

This way, owners[address] equals true when the given address is an owner.

I was wondering if this is a reasonable solution or if there are vulnerabilities that I hadn't thought of. I can't think of another solution that allows to add owners dynamically

Edit: I need multiple owners because I have functions that requires to be called by multiple contracts that are deployed after this one. So I can't even add the owners in the constructor and using one variable for each owner can't be done since I don't know how many I'll need

I know how to use OpenZeppelin libraries, but since the Ownable contract does not provide multiple owners functionality, I copied the code and modified it.

Update:

As @medvedev1088 suggested, I created a contract that extends Ownable and adds the mapping for authorized addresses

contract Authorizable is Ownable {

    mapping(address => bool) public authorized;

    modifier onlyAuthorized() {
        require(authorized[msg.sender] || owner == msg.sender);
        _;
    }

    function addAuthorized(address _toAdd) onlyOwner public {
        require(_toAdd != 0);
        authorized[_toAdd] = true;
    }

    function removeAuthorized(address _toRemove) onlyOwner public {
        require(_toRemove != 0);
        require(_toRemove != msg.sender);
        authorized[_toRemove] = false;
    }

}
  • 1- What is the reason you need multiple owners ? 2 - Recommended way to use OpenZepplin libraries – 1sn0s Feb 14 '18 at 19:19
  • Got compilation error. "owner" in place of owners – 1sn0s Feb 14 '18 at 19:29
  • Oh yeah, I forgot to change it back before posting here, now it should be working – Marco Feb 14 '18 at 19:32
  • A multisig might be what you're looking for – libertylocked Feb 15 '18 at 12:36
  • @Marco contracts/ownership/rbac/RBAC.sol from OpenZeppelin may be of use also. – medvedev1088 Feb 15 '18 at 15:18
6

An owner is like a superuser - a user that has all permissions. I think it's quite risky to give superuser permissions to more than 1 entity (that's probably why most systems allow a single superuser/rootuser). How about an alternative solution:

  • You keep Ownable untouched
  • You introduce your own contract called Authorizable, that extends Ownable and adds the onlyAuthorized modifier. This contract will keep a separate mapping for addresses that are authorized to perform actions. addAuthorized, removeAuthorized must only be marked with onlyOwner

With this solution you'll have a single superuser and a separate mapping for authorized users.

You can think about using the mapping value to store the permission mask e.g. bits can be used for read, write, admin permissions. Only add extra features if you actually need them though, as extra features expand the attack surface.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.