Make some private functions internal to allow the development of "withSignature" functions (like permit)

[Amxx]

🧐 Motivation
It seems to be a global trend to allow some "priviledged" operation to be performed by a third party provided they can show a valid signature by the legitimate account. For example, the ERC20Permit contract allows anyone to forward a signed message (ERC712) that will set token approval. The comp governance token offers a similar feature to delegate vote using a signature.

This mechanism is very powerfull, and we can expect more of these "with signature" function to be implemented.

As ERC20Permit shows, this is easy to do when an internal function is available (_approve), and can be called when the signature is verified. This is something that is widely available in the token smart contract.

However, some of these functions are either non-existing, or private, which prevent the implementation of such "with signature" functions. In particular I can think of:

• AccessControl.grantRole: The msgSender() must be admin, and the underlying function _grantRole is private
• AccessControl.revokeRole: The msgSender() must be admin, and the underlying function _revokeRole is private
• AccessControl.renounceRole: The msgSender() must be the account, and the underlying function _revokeRole is private
• Ownable.transferOwnership: The msgSender() must be owner, and there is no equivalent _transferOwnership internal function

📝 Details
In order to improve the possibility to write such wrappers, I believe:

• _grantRole, _revokeRole and _revokeRole should be internal
• There should be an internal Ownable._transferOwnership that changes the owner and emit an event, without doing any msgSender() check.

[frangio]

I'm not fond of this change, I still believe we need to protect the internal admin logic of AccessControl.

For generic "withSignature" functions it would be interesting to evaluate whether a Context variant can implement this pattern, although it seems impossible to do without using storage.

For AccessControl this is not really necessary. The contract itself can be made an admin of a role, and it can have a public function that validates a signature and calls itself with a role management operation.

[Amxx]

I agree that having the contract itself have the admin role can work ... even though it would be a mess if there are many different admin roles. I don't think its a good solution though.

Also tweaking that in the Context doesn't sound right. It would be possible but I fear it would require sload in context which would be bad in the long run.

[frangio]

Using storage in a Context variant may not be so bad, since this would be one of the cheaper uses of storage, where the slot is reset to its original value in the end.

Regardless though, I think the major point of disagrement here is whether AccessControl should remain closed-down and try to protect its invariants, or it should allow for more extensibility. I personally don't find this particular example a good motivation for this sort of extensibility because it is already doable with the existing admin mechanism.

[Amxx]

Two things:

• If we put AccessControl on the side, would you agree that the change to Ownable is useful? We could do a PR with just this part.
• In AccessControl, _grantRole is already somehow internal ... you just have to call _setupRole. I know this is not intended, and you'd like _setupRole to only be used in constructors or initializers, but it remains accessible, and people will eventually use it because its a easy way to solve that. So since _grantRole is somehow already accessible, why not just bite the bullet and do the same for _revokeRole ?

[frangio]

I'm just as undecided with respect to Ownable, the only difference is there is no native workaround for Ownable.

I get the point about _setupRole, and it's true that it is already violating the supposed invariant.

---

I do want to explore the Context alternative a little more, though, because it's interesting and this is a pattern that has actually come up before.

What if we had a Context that in its _msgSender implementation looked for an EIP712 signature at the end of msg.data? Wouldn't this be a more general solution to the problem? In a sense this is very similar to ERC2771Context but it would remove the need for a trusted forwarder (and the corresponding indirection) if the signature was verified directly in the contract.

[zemse]

I'm using an ERC1167 clones pattern where I'm trying to inherit Ownable on the deployed clone. Since a constructor is not available for ERC1167 clones, I have to set the initial owner by having an initialize function on the clone contract called through factory contract exactly after deploying the clone. Such clones have zero address as owner when deployed and hence without an internal _transferOwnership API in Ownable, I don't see any way to set up the initial owner.

[Amxx]

Hello @zemse

When building "underlying implementaton" for proxy / clones, I recommend you use @openzeppelin/contracts-upgradeable. Its a variant of contracts that will provide an "initializer" function which replaces the constructor.

It does however have the same restriction than the constructor:

• can only be called once (protected by the initializer modifier)
• will set ownership to the message sender.

I continue to believe that having an internal _transferOwnership would be great, but for now using the upgradeable version of contract is the way to go (and might be useful if you inherit other contracts such as ERC20)

[Amxx]

Note: #2832 somehow fits in the scope of this issue.

[julianmrodri]

Hi! question are these changes going to be included in a release soon? Im particularly interested in the _transferOwnership internal function to be able to set the owner in a constructor to an arbitrary address. In the current version couldn't find a way to do it except in two separate transactions. Thanks!

[frangio]

These changes will be included in a 4.4 release candidate that will be published this week. However, note that it will remain a release candidate for several weeks before 4.4 is released. We are likely announcing the timeline and details next week.

[julianmrodri]

Thanks @frangio works for us!

[frangio]

v4.4.0-rc.0 was just released.