Why is "transferOwnership" of "Ownable"-contract delegating to an internal method?

Reading SourceCode of Openzeppelin I just stumbled upon the method "transferOwnership" in Contract "Ownable.sol":

/**
 * @dev Allows the current owner to transfer control of the contract to a newOwner.
 * @param newOwner The address to transfer ownership to.
 */
function transferOwnership(address newOwner) public onlyOwner {
    _transferOwnership(newOwner);
}

It delegates all logic to a internal method _transferOwnership:

/**
 * @dev Transfers control of the contract to a newOwner.
 * @param newOwner The address to transfer ownership to.
 */
function _transferOwnership(address newOwner) internal {
    require(newOwner != address(0));
    emit OwnershipTransferred(_owner, newOwner);
    _owner = newOwner;
}

Since internal functions can be executed from derived classes, and since _transferOwnership doesn't have the modifier onlyOwner, I see a slightly danger here that derived classes could call this method per accident instead of the public one without using onlyOwner themselves. It could open gates that should be closed in my oppinion. Any thoughts? Or where is my mistake?

2 Likes

No mistake on your part here! It’s true that it “opens gates”. It was our decision to allow this. There’s other similar cases throughout OpenZeppelin. Note for example how ERC20._transfer also places no constraints on msg.sender.

I disagree that these things could be used accidentally, though I have been reconsidering if this was a good decision, simply because disallowing this would give stronger guarantees about the semantics. Unfortunately removing it now is a breaking change so it will have to wait until v3.

3 Likes

There has been some recent conversation about this. See openzeppelin-solidity#1766.

2 Likes

Hello @itinance, thank you for this question! This comes up often enough that I think it may warrant a guide of its own.

As a general rule, in OpenZeppelin we attempt to provide tools for you to be able to write correct code, but we don’t make (too many) assumptions on what it is you may want to write. Additionally, we do this ways that prevent you from making avoidable errors. For example, we won’t let you modify storage variables directly, since those usually go hand in hand with emitting events, updating some other variables, etc. (see in @frangio’s example above how updating _balances in ERC20 requires a Transfer event to be emitted).

However, for users to be able to write anything other than the most basic contracts, some added flexibility is required. That’s why we usually expose internal functions (which always have a leading underscore, like _transferOwnership) that have none of the usual access control, but still perform required safety checks,emit events, etc.: they are there for you to build on top of OpenZeppelin.

For example, you may want to provide a social recovery mechanism, where e.g. if 5 predetermined accounts (friends of yours) send a specific message, ownership of the contract is transferred to some other account (because you may have lost access to your own owner account). The only way to implement such a feature is via _transferOwnership. Take a look also at this guide, which shows how different ERC20 supply mechanisms can be implemented by using _mint.

Hope this helps clarify things!

2 Likes

Thank you very much for clarification @nventuro and @frangio!

2 Likes

Moving to #openzeppelin category as this is an implementation choice, with discussion welcome.

Thanks @frangio

1 Like