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.

[nventuro]

What if we embedded the old Claimable into Ownable? That'd way it'd be less optional, and we could more easily suggest the recommended usage.

Granted, we can't simply change how Ownable's transferOwnership works, so we'd have to add a new method for it (transferOwnershipWithConfirmation? 😕).

[MicahZoltu]

I would be fine with Claimable only and just never using transferOwnership. Having them separate does allow one to disable transferOwnership's original functionality which is kind of nice, but not as important IMO.

My vote would be Ownable + Claimable, but would not fight against a single Ownable contract with a confirmableTransferOwnership function.

[barakman]

@nventuro :

Is this one due to be released any time soon?

Following an (extremely thorough) audit, my workaround for fetching Ownable 1.12.0 and Claimable 1.12.0 (thus overriding Ownable 2.0.0) didn't quite "go down well" in the perspective of the auditer, who stated that "This is a dangerous practice as it modifies openzeppelin-solidity in a potentially unexpected way".

The auditer further recommends to add it explicitly to our system. We would highly prefer to avoid it, and to continue using your widely-used and thoroughly-verified Ownable / Claimable pair.

Is there any chance that you reinstate them in the coming release?

Thanks again :)

[frangio]

@barakman We can't guarantee to release this soon. Given that, I'd recommend to implement Claimable from scratch yourself, and have it audited, as it should be a fairly small contract.

[barakman]

@frangio:
Of course, but we'd lose the "widely-used and thoroughly-verified by the community" feature.
Thanks!

[nventuro]

didn't quite "go down well" in the perspective of the auditer, who stated that "This is a dangerous practice as it modifies openzeppelin-solidity in a potentially unexpected way"

A statement I wholeheartedly agree with :)

@barakman We'll be exploring options soon (specially considering this shouldn't be such a big change), but I'm not sure yet regarding a release date, sorry.

[barakman]

@nventuro:

I've already come to realize that even if you do reinstate Claimable back into the next version of OZ, it will be under solc v0.5.x.

And since we've decided to stick with solc v0.4.x for the time being, it will probably yield some other "collisions" in our project.

So I have resolved the audit-warning by downloading Ownable and Claimable into a folder named openzeppelin-solidity-v1.12.0 (instead of openzeppelin-solidity, which is the one created by npm).

This way, the following is achieved:

1. OZ package installed by npm (openzeppelin-solidity v2.0.0) is not modified.
2. Anyone reading the contracts is fully aware of the fact that Ownable and Claimable belong to openzeppelin-solidity-v1.12.0 (because this string appears explicitly in the the import statements).
3. Our project leverages from the fact that these modules are well-known open source, and have been verified thoroughly by the community (which would not be the case should we choose to implement them ourselves).

Of course, if I am wrong in my initial assumption, and you do reinstate Claimable with a pragma solidity 0.4.25 statement at the beginning of the file, then I would be happy to upgrade our OZ version from 2.0.0 to the newer version.

Thanks for your help :)

[k06a]

Next version compatible implementation:

pragma solidity ^0.5.0;

import "github.com/openzeppelin/openzeppelin-contracts/contracts/ownership/Ownable.sol";


contract Claimable is Ownable {

    address public pendingOwner;
    
    modifier onlyPendingOwner() {
        require(_msgSender() == pendingOwner);
        _;
    }
    
    function transferOwnership(address newOwner) public onlyOwner {
        pendingOwner = newOwner;
    }
    
    function claimOwnership() public onlyPendingOwner {
        _transferOwnership(pendingOwner);
        delete pendingOwner;
    }
}

@nventuro do you think this worth Pull Request?

[k06a]

^^ @nventuro? @frangio? @MicahZoltu?

[MicahZoltu]

@k06a That looks like the original implementation, so fine with me.

[MicahZoltu]

Oh, actually there should be a cancelTransfer function that looks something like:

function cancelOwnershipTransfer() public onlyOwner {
    delete pendingOwner;
}

You could transfer ownership to yourself (owner), but that feels dirty when your goal is to cancel an ownership transfer.

Also, there may be value in firing events for transferOwnership and claimOwnership. If Ownable already fires an event in _transferOwnership then that is probably enough and only an event for NewPendingOwner (or similar name) is necessary.

[k06a]

@MicahZoltu it seems more like transferOwnership(0) than transferring to self

[MicahZoltu]

Ah, good point. You could do transferOwnership(0) to "clear" the pending transfer. I feel like it would be more correct to have a cancelTransfer function, especially if events are added.

Funnily enough, I believe this exact feedback is what I got from OpenZeppelin in a contract review that had Claimable functionality. 😁 We have gone full circle it seems.

[k06a]

Updated implementation:

pragma solidity ^0.5.0;

import "github.com/openzeppelin/openzeppelin-contracts/contracts/ownership/Ownable.sol";


contract Claimable is Ownable {

    address public pendingOwner;

    event NewPendingOwner(address index owner);

    modifier onlyPendingOwner() {
        require(_msgSender() == pendingOwner);
        _;
    }
    
    function transferOwnership(address newOwner) public onlyOwner {
        require(pendingOwner == address(0));
        pendingOwner = newOwner;
        emit NewPendingOwner(newOwner);
    }

    function cancelTransferOwnership() public onlyOwner {
        require(pendingOwner != address(0));
        delete pendingOwner;
        emit NewPendingOwner(address(0));
    }
    
    function claimOwnership() public onlyPendingOwner {
        _transferOwnership(msg.sender);
        delete pendingOwner;
    }
}

[MicahZoltu]

Is there value in having a separate event for CancelPendingOwner? I'm generally not a fan of magic numbers (in this case, 0 being a magic number for cancellation).

[barakman]

This comment makes sense because function transferOwnership does not require that newOwner is non-zero.

Once this function requires this condition (which it should IMO), emitting event NewPendingOwner in function cancelTransferOwnership actually makes sense, because this event is now emitted whenever the value of pendingOwner changes (indicating accurately the new value of pendingOwner).

In either case, there is nothing "magical" about 0 here (i.e., it is not used as a magic number).

---

Alternatively, instead of adding require(newOwner != address(0)) in function transferOwnership, you could simply get rid of function cancelTransferOwnership altogether...
But I guess that's a design-decision (i.e., allowing the user to cancel by passing zero).

[MicahZoltu]

Currently, the value of 0 is magic by nature of the fact that when there is no pending transfer, pendingOwner == 0. I can appreciate that getting rid of that particular magic value is hard and probably not worth it, but I think we can at least make it so the magic value doesn't propagate out to the API/events as much as it does now.

Adding require(newOwner != 0) to transferOwnership(...) and having a separate event for NewPendingOwner and TransferCancelled is probably the closest we can get to making it so API users don't have to know about the magic 0 value (where 0 means unset). More than that and we would have to store a separate pendingOwnerIsSet boolean which I don't think is worth it.

We could add a method for hasPendingOwner if we wanted to take it one step further to removing the need for users/integrators to understand that 0 is magic if we wanted, and this wouldn't have any impact on internal complexity or gas costs like having a separate bool flag would.

[barakman]

A zero address is the default value of all uninitialized address variables.

Hence pendingOwner == address(0) is equivalent to "No Pending Owner".

Hence address(0) is not a magic number.

[MicahZoltu]

Ehhh, I would accept the argument that 0 is a well known magic value, but I wouldn't call it not-magic.

[barakman]

A zero address is the default value of all uninitialized address variables.

That one is dictated by the Solidity language standard.

So no, address(0) is not a magic number.