AccessManagedProxy: is a good idea?

[gnarvaja]

Hey, I wanted to get your feedback on a design decision...

In our company, we mostly use upgradeable contracts, UUPS variant using the ERC1967Proxy. This means, there's no logic in the proxy, all the calls are forwarded to the implementation.

We also use something similar to OZ 5.x AccessManager, a centralized contract that manages the permissions for all the contracts in the protocol.

For our new version, we will use the OZ 5.x AccessManager, and I was thinking of moving the access validation (call to .canCall) to the proxy. In the proxy, before calling the implementation contract, I will check with the AccessManager if the called is authorized to call that method.

It's a change of paradigm from the public by default current approach of contracts, to permissioned by default.

This way, without adding any logic, ALL the methods in my contracts will be permissioned by default. I won't need to add onlyRole modifiers to the methods and I will move all the access control rules from code to deployment.

So our contract code, instead of looking like this:

function setCriticalParam(uint256 newValue) external onlyRole(ADMIN) {
    criticalParam = newValue;
}

Will be like this

function setCriticalParam(uint256 newValue) external {
    criticalParam = newValue;
}

And then on the operation recommendations, I will check the setCriticalParam access is only granted to ADMIN role.

I see some advantages of this approach:

1. Full flexibility in the access control to my contracts.
2. Less complexity in my contracts' code. We will probably add recommendations like "This method should be public" or "This method should be enabled only to top-level admins". But they will be just comments.
3. Closing access to public methods (full or granular pause of the contract) can be implemented without hardwiring a particular logic in the contract.
4. We can implement cool rollup processes like "closed beta".
5. Fewer things to audit for static code audits, more or less the same amount of things to audit for "deployment audits".

Disadvantages:

1. Higher gas costs for the public methods that will require an external call to the AccessManager contract just to check they are public.
2. For each contract will need to setup the permissions on the access manager for ALL the methods.
3. This also includes the views (in principle, see comment below).

To avoid calling the AccessManager contract for views (assuming the views are public by default), we can implement something to detect if we are in a static call and, in that case, skip the access control validation. It can be calling a method that modifies a trasient variable, for example, an if it fails we assume we are in a static call.

[ernestognw]

Hi @gnarvaja, sorry for the late response.

I understand the idea is to restrict access to a contract by default using the proxy. In principle, the idea sort of sense but I see you're already aware of the disadvantages. Also, I can't think of a reliable way to know whether a function was executed in an of a way to know if the execution is part of a staticcall.

Although I agree with the advantages you see for this approach, I don't think they're all exclusive to the approach of intervening the Proxy. Most can be achieved with the AccessManager itself:

1. The manager also offers full flexibility, I'm curious if you think there's something missing
2. Your contracts can abstract away the access control complexity by inheriting from AccessManaged, you can decorate all your functions with the restricted modifier and you're good to go.
3. The AccessManager allows admins to close a target. A target can't be called at all if it's closed, which allows for a granular readjustment of permissions
4. This is also doable by establishing a closed beta role in the AccessManager and assign a subset of your users to such role.
5. This is still true, but I'd argue the AccessManaged contract is still a simple abstraction, you don't need to audit it as we already did for 5.0

Hope this helps!

[gnarvaja]

The main difference between making an AccessManagedProxy and just inheriting from AccessManaged and adding the restricted modifier to ALL the methods, is exactly that one:

You have to explicitly add the restricted modifier to all the methods. If you forget to add it, that method won't be access-restricted or pausable.

Since the code that calls the access manager is part of the bytecode of all the contracts, to have 100% coverage and to effectively check that you haven't missed any of the methods, you have to develop test cases for each of the methods.

On a deployed contract, by reading the source code of the contract and the proxy, to assess the effective access controls implemented, you need to parse the source code of the contract and identify which methods have the restricted modifier. And then check the permissions configured in the AccessManager.

Doing an automated process that checks a deployed contract against a specification of access controls is trivial in the AccessManagedProxy case, and very complicated and fragile in the inherit AccessManaged case.

Another benefit is regarding the compiled size of the contracts, that will be smaller in the AccessManagedProxy case.

Using the AccessManagedProxy you just need to check the permissions configured in the AccessManager.

BTW, I'm sharing here a possible implementation for the AccessManagedProxy. As you can see is very simple to understand and audit.

contract AccessManagedProxy is ERC1967Proxy {
    IAccessManager public immutable ACCESS_MANAGER;

    constructor(address implementation, bytes memory _data, IAccessManager manager) payable ERC1967Proxy(implementation, _data) {
        ACCESS_MANAGER = manager;
    }

    /**
     * @dev Checks with the ACCESS_MANAGER if msg.sender is authorized to call the current call's function,
     * and if so, delegates the current call to `implementation`.
     *
     * This function does not return to its internal call site, it will return directly to the external caller.
     */
    function _delegate(address implementation) internal virtual override {
        (bool immediate, ) = ACCESS_MANAGER.canCall(msg.sender, address(this), bytes4(msg.data[0:4]));
        if (!immediate) revert IAccessManaged.AccessManagedUnauthorized(msg.sender);
        super._delegate(implementation);
    }
}