What is the alternative for Claimable in v2.0.0?

[barakman]

Not exactly a bug, for for those who have been using Claimable in earlier versions of OZ (for the purpose of transferOwnership followed by claimOwnership) - what are the options in v2.0.0?

Relying solely on Ownable.transferOwnership lacks a safety mechanism for accidentally transferring the ownership to an incorrect address.

At present, the only alternative that I see is copying Claimable.sol from v.12.0 to my repo, fixing the import "./Ownable.sol" statement, and inheriting my contracts from Claimable instead of Ownable.

This is far from being a clean solution.

Are there any alternatives?

I've been reading something about roles (issue 1274, issue 1146, issue 1291).

Is that possibly related?

Thanks

[barakman]

UPDATE:

The alternative above is not even feasible because:

1. The state variable owner has been renamed to _onwer
2. The access-level of this variable has changed from internal to private

The second reason is much more critical of course, because it means that I cannot even fix Claimable in order to make it work with the current version of Ownable, i.e., I need to grab both contracts from v1.12.0 into my repo. To make it worse, the compiler doesn't like to see two different contracts with the same name (Ownable v1.12.0 and Ownable v.2.0.0), so I would need to rename the one that I take from v.1.12.0.

I have looked into your roles folder, and it seems that any one of the contracts there can be used instead of Ownable.

However:

1. The gas cost on the only operation is higher, since you're using mapping (address => bool) instead of just address.
2. If I lose the initial owner, then there is no way for me to trace it (again, due to the use of a mapping).
3. There is still no support for claiming an ownership here.

Should I just implement a designated ClaimerRole or something like that?

[barakman]

UPDATE 2:

I believe that it would have been better if you had left the Ownable and Claimable contracts in v.2.0.0, as they were in v1.12.0 (i.e., address public owner instead of address private _owner).

If I understand correctly, then the only alternative at this point is to implement my own custom contract using the Roles library. Here is how I solved it without implementing such a contract:

Step 1 - added in file package.json:

  "scripts": {
    "postinstall": "node fix.js"
  },
  "devDependencies": {
    "download-file": "^0.1.5"
  }

Step 2 - added file fix.js next to file package.json:

let download = require("download-file");

for (let filename of ["Ownable.sol", "Claimable.sol"]) {
    let url = "https://raw.githubusercontent.com/OpenZeppelin/openzeppelin-solidity/v1.12.0/contracts/ownership/" + filename;
    let options = {directory: "./node_modules/openzeppelin-solidity/contracts/ownership/", filename: filename};
    console.log("Fixing " + options.directory + options.filename);
    download(url, options, function(error) {if (error) throw error;});
}

[nventuro]

Hey there @barakman! Thank you for bringing this up.

The reason why the owner state variable was made private is to prevent derived contracts from writing to it: this means that Ownable is the only contract that may modify it (via transferOwnership), making code easier to reason about and audit. We may add an internal method in the future for derived classes to use in a more controlled fashion, but that's not part of the 2.0 release.

What I'm wondering is why you need Claimable in the first place: we don't consider it to be very useful, and may even be an anti-pattern in some cases, which is why we got rid of it. Could you share some more information regarding your requirements? Thanks!

[barakman]

@nventuro: Hi.

I understand your motivation in changing the access-level of the owner variable from public to private. In fact, I have realized this while applying your new coding conventions throughout our code, and ended up changing one of our internal variables to private for the exact same reason (emphasizing that this variable is not to be altered by any deriving contract).

However, since the claimOwnership function is imperative in our off-chain activity, we must maintain a copy of the old contracts (Ownable and Claimable v1.12.0).

To elaborate a little more about this, some of our onlyOwner functions are very sensitive (security-wise), and we would like to impose multi-sig access on them.

For this purpose, we transfer the ownership of each relevant contract from ourselves (i.e., the deploying account) to a MultiSigWallet contract which we deploy beforehand.

And in order to ensure that we never accidentally transfer the ownership of a contract to the wrong address, we use the transferOwnership / claimOwnership mechanism that you have very nicely provided in v1.12.0.

Thanks.

[nventuro]

Hey @barakman,

I'm glad you're with us on the private/internal thing :)

The main reason behind Claimable no longer being a thing is that we're in the process of deprecating Ownable: we think having a single account with superpowers is too powerful, and doesn't fit some scenarios where some granularity regarding permissions is required (see #366).

As you noticed, our current access-control solution are the Roles contracts, which you could use to achieve what you intend with these have two functions: addRole and renounceRole.

1. add the multisig to the list of accounts with the role, from the original account
2. Check the multisig actually has the role
3. renounce the role from the original account

I'd argue however that all Claimable is doing for you in the workflow you described is saving you from having to redeploy all of your contracts once you notice you've mistakenly transferred ownership to an erroneous account (I'm assuming, a non-existent account, i.e. one that will not be able to call claim before you fix your mistake and re-transfer ownership). Is there something I'm missing here?

[barakman]

@nventuro: Hi.

Yes, your argument is accurate - if I transfer ownership of a contract to the wrong account (presumably a non-existing one, due to a typo for example), then I will need to redeploy the contract, and possibly a few other (related) contracts along with it.

This redeployment is equivalent to a Fork of our system, and we consider it pretty disastrous.

Thanks

[nventuro]

As a general rule, if such a deployment may cause issues in your system, I'd advise you to take a look at the whole architecture: ideally, your contracts should be worthless and not meaningful until you start using them (presumably, after checking deployment was successful).

That said, if (temporarily) transferring ownership to a non-existent account doesn't bother you, Roles will give you similar behavior: since roles are not transferred but added, you can simply call add again on the right account, before renouncing. Behavior is not exactly the same since that wrong account will always have the role (unless you specify a removal mechanism), but I take it this doesn't worry you?

[MicahZoltu]

While I am a fan of RBAC for large projects, for small contracts it is way overkill and adds a ton of unnecessary complexity (i.e., risk). I encourage keeping Ownable/Claimable around while also pursuing a RBAC system. The landing page for my personal blog doesn't need RBAC, while by 100 engineer company certainly does. IMO, Ownable/Claimable fill a different niche from an RBAC system and should not be deleted.

[nventuro]

@MicahZoltu indeed, we think Ownable does a good job for small projects and quick proofs of concept, and because of that we decided that removing it was not a great idea. We did however switch our contracts (e.g. ERC20Mintable) so that they use the Roles solution.

Regarding Claimable, what about it is it that you find needing? I've yet to find a compelling use case, but I'm not against adding these tiny bits of functionality if it ends up being useful.

[frangio]

@MicahZoltu How would you feel if the replacement for the old Claimable was a contract that was used "by composition" instead of inheritance? I.e. setting the owner to be a intermediary/forwarder contract that handles the transfer+claim process. This could be reused for roles also.

[MicahZoltu]

@nventuro I never use Ownable without Claimable, because Ownable alone makes it too easy to accidentally fat-finger-send-to-dev-null.

@frangio If the target audience for Ownable/Claimable is "simple contracts", it feels like requiring a transitive owner is the opposite direction from what we want. Now I have to author a second contract with passthrough functions to each of the onlyOwner functions on the contract (as well as some public functions most likely) and I need to have the infrastructure in place to ensure that the interfaces always line up, and when it comes time to deploy the deployment process is more complicated because I now need to deploy the main contract, then deploy the owner contract, then transfer ownership to the owner contract (a process during which I can still accidentally send-to-null).

The thing that makes Claimable nice (compared to something like RBAC) is that it is a relatively simple protection (dozens of lines of already-audited code) against catastrophic failure (sending contract to /dev/null). It requires almost zero additional operational/infrastructure work over Ownable, while a full RBAC system or proxy contract requires notably more effort to maintain/manage.

[nventuro]

Hmm, I could get behind those arguments. I take it though that by /dev/null you mean 'any random address' and not just the zero address, right? Arguably, as soon as you transfer ownership to another account it will be able to claim it, so you'd be relying on a) no-one having the private keys, or b) they not being fast enough.

[MicahZoltu]

Yeah, zero address is a common target as it is the default value for all local variables/parameters and easy to accidentally send to if you screw up your transaction creation. However, fat-fingering is another common source of error that can lead to the contract going to an address that no one controls.

I'm not worried about accidentally transferring to an address that someone does have the keys to. While that is possible, it isn't the vector I'm worried about.