Openzeppelin-contracts: Create a simple MultiOwnable ownership contract

[maraoz]

🎉 Description

The recommended way to implement multi-user ownership of a contract is to simply use Ownable, passing a multisig contract (e.g https://github.com/gnosis/MultiSigWallet) as the owner. However, I think it would be educational to provide a MultiOwnable contract that simply receives the constructor parameters for a MultiSigWallet, instantiates that, and then passes it as the owner of the contract.

Example: (untested)

pragma solidity ^0.4.18;

import 'gnosis/MultiSigWallet.sol';
import "./Ownable.sol";

contract MultiOwnable is Ownable {
  function MultiOwnable(address[] _owners, uint _required) public Ownable() {
    MultiSigWallet ms = new MultiSigWallet(_owners, _required);
    transferOwnership(ms);
  }
}

Thoughts?

• [ ] 🐛 This is a bug report.
• [x] 📈 This is a feature request.

[buybackoff]

We are doing exactly that with the Gnosis contract here: https://github.com/dbrainio/Soltsice/blob/master/contracts/MultiOwnable.sol

Three are two access levels: onlyWallet for consensus and onlyOwner for any owner. The contract also implements pausable: any owner could call pause, but only majority could unpause. Imagine one owner is pawned, then you do not want to wait for majority and just pause ASAP to have time to disable the hacked owner.

There is also BotManageable contract for backend bots managing contacts. We assume that they could always be hacked and design contacts in such a way that owners could quickly pause a contract until we disable a bot and enable another one.

I am writing docs now, release is coming next week.

[k06a]

@buybackoff @maraoz look at one more universal Multiownable implementation: https://github.com/bitclave/Multiownable

Do you think it is a good idea to add it to the zepplein-solidity library?

[buybackoff]

@k06a I've seen this but it looks like reinventing a wheel. Gnosis's MultiSig has convenient GUI, was audited/reviewed probably many times and trusted by many. It's de-facto the standard and even this repository recommends to use it instead of creating a new one.

[tavakyan]

@buybackoff would you say in this context for integrating with zeppelin, considering a multi-ownable implementation like in Solistice, is the 'pausable' feature necessary? It's not immediately apparent to me why this feature is in multiownable.

[buybackoff]

@tavakyan it is an example that some sensitive action requires majority approval, but an opposite one could work just with a single owner. We need this so I added the functionality inside the contract (in this case overriding OZ's Pausible is not simpler). But the core idea is two modifiers onlyWallet and onlyOwner. I have reduced the wallet interface to a single method, so not only wallet from Gnosis could work.

I believe such asymmetric Pausible is quite common requirement for cases when something could go wrong.

[tavakyan]

I tried to structure it out by integrating your implementation @buybackoff . Its one possible way and the multiowner contract is fairly simple.

See: https://github.com/tavakyan/zeppelin-solidity/tree/MultiOwnable-769/contracts/ownership

I separated the MultiPausable since they have a implementation of Pausable.

It might be easier to view through here: https://github.com/tavakyan/zeppelin-solidity/pull/1/files I didn't write the tests and stuff, but its a general approach.

[buybackoff]

@tavakyan this looks good. If I were to send a PR I would split in a similar way. What concerned me initially that when inheriting Ownable we store wallet as the owner field but then override isOwner modifier to mean something different. Probably we could come up with other naming. E.g. if a wallet with majority is actually an owner then we could leave the modifier onlyOwner to mean the same thing, but we could add onlyPriviledged for a single owner. This will also help with default security, e.g. in your MultiPausible you override unpause to require higher security, but it's better I think to explicitly require lower security and leave the defualt the most strict and intuitive.

[tavakyan]

@buybackoff seems like a sensible suggestion. It is indeed a source of confusion and I think that would clarify things for clients. I don't mind finishing the tests and making the changes for a PR if other people think this is a useful design or can just leave it up to the core team / community to decide.

[frangio]

Our current position on this is that complex authorization mechanisms like multisig should be separate from the contract they manage. So contracts should only have a single owner which is a itself a multisig wallet.