Consider adding _authorizeUpgrade to UpgradeableBeacon instead of using Ownable

[fruiz08]

🧐 Motivation
We want a more flexible way to be able to make upgrades and at the same time, maintain consistency on the approach used on UUPS pattern

📝 Details

Now, we cannot inherit from UpgradeableBeacon.sol and override the upgradeTo function removing the OnlyOwner modifier beacause _setImplementation is a private function.

function upgradeTo(address newImplementation) public virtual onlyOwner {
        _setImplementation(newImplementation);
    }

    function _setImplementation(address newImplementation) private {
        if (newImplementation.code.length == 0) {
            revert BeaconInvalidImplementation(newImplementation);
        }
        _implementation = newImplementation;
        emit Upgraded(newImplementation);
    }

Suggestion: Use the _authorizeUpgrade like UUPSUpgradeable.sol to override it with the protected method you want

function upgradeTo(address newImplementation) public virtual {
       _authorizeUpgrade(newImplementation);
       _setImplementation(newImplementation);
    }

function _authorizeUpgrade(address newImplementation) internal virtual;

[ernestognw]

Hi @fruiz08,

Ownable also has an internal _checkOwner function that's also virtual. Overriding it has the same effect as overriding _authorizeUpgrade. Alternatively, you can set an AccessManager as the owner of the UpgradeableBeacon, so that it can handle roles and delays more dynamically for authorizing upgrades.

Still, I agree the UpgradeableBeacon should have an _authorizeUpgrade function instead.

[hrik2001]

Hey @ernestognw, do you think I can open a PR for this? seems like it wouldn't take much time

[ernestognw]

Hi @hrik2001. Sounds good to me.

Still, I'd like to hear @Amxx input on this one.

If he agrees we're happy to review a PR

[Amxx]

What woukld be the content of the PR?

We cannot remove the Ownable inheritance from UpgradeableBeacon and replace it with an internal function, because that would be breaking the current UpgradeableBeacon. Any change that we do in 5.x has to be backward compatible.

UpgradeableBeacon is one of the contracts we have that can be deployed "as is", without adding anything. If I remember correctly, the idea was that a Beacon is a very simple/small piece of code, and that its easy to rewrite it with a different permission system. The UpgradeableBeacon was a simple version that would fit most needs, and that would be a good example for people with more specific requirements to write their own.

[Amxx]

function upgradeTo(address newImplementation) public virtual {
    _authorizeUpgrade(newImplementation);
    _setImplementation(newImplementation);
}
    
function _authorizeUpgrade(address newImplementation) internal virtual { 
    _checkOwner(); 
}

would be backward compatible, and it would allow a dev to override _authorizeUpgrade (without calling super) to change the permission system ... but we have to keep in mind that this override would be possibly confusing:

• the ownership would still be present/transferable, but would not give any permission. It feels wrong and could lead to missunderstanding that would affect the management of the beacon
• the upgrades plugin would be confused and would not work with these instances.

It would be a clear case of "the code is safe", but its difficult to observe and can lead to mad management.

[Amxx]

Any reason why someone with specific requirements would not write their own beacon ?

[jmendiola222]

Any reason why someone with specific requirements would not write their own beacon?

There is benefit of using an OZ implementation as it is, as you kind of give security for granted. Even small adaptations would require securities reviews, inflates audits and so on. There are also gains from maintainability perceptive, as if you move versions on OZ, you can trust retro-compatibility and mayors and consistency.