AccessManager awkwardness

Support Contracts
1

Hello there,

I'm using AccessManager with upgradeable contracts. Some feedback/questions on this:

  1. Inheriting from AccessManagerUpgradeable, if you want to write an admin method with other role than the default admin, you have to resort to using hasRole() in a way similar to AccessControl
  • onlyAuthorized is limited to a fixed set of methods defined in AccessManager, and it's not designed to be extended.
  • restricted is not available, since it is in AccessManaged
  1. What is the recommended way of doing ACL in UUPSUpgradeable _authorizeUpgrade? It's an internal method, so restricted is not recommended. Selector for upgradeToAndCall can't be restricted, since it's onlyProxy.

Again, we can use hasRole in a similar way AccessControl has onlyRole, but it's awkward since restricted roles are wired outside of the correspondent AccessManaged contracts code... instead of having 1 consistent paradigm for ACL we have to use 2.

hasRole does not take into account execution delays, which was the intention of using AccessManager.

_checkCanCall(address caller, bytes calldata data) does take into account delays, but I would have to fake a selector, since internal methods don't really have one, I'm not sure if that could have some interactions. EDIT: can't fake a selector, because data is calldata and I can only pass memory inside _checkCanCall

  1. This issue

Thanks for your time!

1 Like
2

Mind you, adding restricted to authorizeUpgrade will work because it will default to ask for protocol admin method as I explain in the GH issue.

The problem is I need a separate role since I want to assign an execution delay to upgrades independent of other admin methods.

3

Fun thing is that we were discussing this very same thing today before you posted it.

The AccessManager has essentiall 2 ways of authorizing:

  1. A set of admin rules for interactions with the manager itself.
  2. A set of roles and permissions for external contracts.

This means that a pattern like Transparent Proxy may work. Though, if you want to have the ProxyAdmin restricted by the manager itself that's another conversation.

In regards to UUPSUpgradeable, I think it's possible to restrict the _authorizeUpgrade just by checking if the caller is the manager itself:

function _authorizeUpgrade(address) internal {
  require(address(this) === msg.sender);
}

Then you would add add an upgrader role:

setTargetFunctionRole(
  address(this), // AccessManager address
  ["0x00..00"] // upgradeToAndCall selector
  uint64(420) // Any role 
)

In this way, you can relay the upgrade using the execute function, so that the manager calls itself after being instructed by a role holder. Operations through execute need to be scheduled through schedule first if the operation requires a delay .

Again, we can use hasRole in a similar way AccessControl has onlyRole , but it's awkward since restricted roles are wired outside of the correspondent AccessManaged contracts code... instead of having 1 consistent paradigm for ACL we have to use 2.

The consistent paradigm here is that the manager is the source of truth in both cases. There's no point in keeping the restricted roles within the AccessManaged since it fragments the permissions and not doing so is one of the goals of the AccessManager.

In case you want to keep the roles in each corresponding contract, you can still use the manager. See Using with Access Control.

2 Likes
4

Thanks! The example you provide serves to upgrade the AccessManager itself.

What about other UUPSUpgradeable contracts? Example taken from the Wizard

// SPDX-License-Identifier: MIT
// Compatible with OpenZeppelin Contracts ^5.0.0
pragma solidity ^0.8.20;

import "@openzeppelin/contracts-upgradeable/token/ERC20/ERC20Upgradeable.sol";
import "@openzeppelin/contracts-upgradeable/token/ERC20/extensions/ERC20PermitUpgradeable.sol";
import "@openzeppelin/contracts-upgradeable/access/manager/AccessManagedUpgradeable.sol";
import "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";
import "@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol";

contract MyToken is Initializable, ERC20Upgradeable, ERC20PermitUpgradeable, AccessManagedUpgradeable, UUPSUpgradeable {
    /// @custom:oz-upgrades-unsafe-allow constructor
    constructor() {
        _disableInitializers();
    }

    function initialize(address initialAuthority) initializer public {
        __ERC20_init("MyToken", "MTK");
        __ERC20Permit_init("MyToken");
        __AccessManaged_init(initialAuthority);
        __UUPSUpgradeable_init();
    }

    function _authorizeUpgrade(address newImplementation)
        internal
        restricted
        override
    {}
}

What is the recommended way to configure the roleUPGRADER_ROLE` with a delay to upgrade MyToken?

       //What do I put in ?????
        accessManager.setTargetFunctionRole(address(mytoken), ?????, ProtocolAdmin.UPGRADER_ROLE, execDelay);
        accessManager.grantRole(multisig, ProtocolAdmin.UPGRADER_ROLE);
5

The Wizard example is well configured, though.

You should set bytes4(keccak256('upgradeToAndCall(address,bytes)')). Given you already have an execDelay for the upgradeToAndCall function, then the only way to execute it is by scheduling an upgrade operaton using schedule and then execute from the manager.

1 Like
6

Nevermind @ernestognw , I found the example in the demo you mentioned, thanks!

1 Like