AccessManagedProxy: is a good idea?

Smart Contracts
1

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.

2

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!

3

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);
    }
}
4

Hey @gnarvaja and @ernestognw,

Thanks for sharing this discussion! I've been thinking about this idea after meeting @gnarvaja at DevConnect and seeing its presentation. I was in love with the concept but found that the implementation suggested in the repo to handle skips was not ideal - it splits the access control logic between the proxy and the AccessManager, which makes it harder to audit and maintain. Plus, having to manually create and maintain skip lists is tedious.

I've been exploring the same AccessManagedProxy pattern and hit the exact issue you mentioned: needing to configure permissions for ALL methods, including public ones.

I've implemented a `CustomAccessManager` that extends AccessManager to solve this. It tracks which selectors have been explicitly configured, and makes unconfigured functions return `PUBLIC_ROLE` by default.

So now:

- Unconfigured functions = public (no configuration needed)

- Configured functions = use their explicitly set role

You only need to configure what should be restricted, not every public function. This addresses your disadvantage #2 and #3 while keeping all the benefits.

The implementation is pretty simple - just overrides `getTargetFunctionRole()` to return `PUBLIC_ROLE` for unconfigured selectors, and tracks configured ones via `setTargetFunctionRole()`. Setting a function to `PUBLIC_ROLE` effectively "unsets" it too, which is nice for flexibility.

This way, all access control logic stays in the AccessManager (nothing split between proxy and manager), and you get sensible defaults that match the "public by default" behavior. It's the same effect as using `restricted` with `PUBLIC_ROLE`, just more convenient.

Still testing it a bit more, but would be happy to share the implementation if you're interested!

1 Like
5

The main reason for using skip methods is reducing the gas overhead. Check the slide #12 in https://docs.google.com/presentation/d/1uBKJ5OUCE5WWTAN4zvAeov9R4_g_rkmUEGfUQoun4Hw/edit?usp=sharing .

Most of the skip methods are usually the views and pure functions and for that we have an option in the specificy deploy function (see https://github.com/ensuro/access-managed-proxy/blob/main/js/deployProxy.js#L11).

Regarding the other methods, what I did in our repo is to have a file with the AMP Config, that tracks the recommended skip methods: https://github.com/ensuro/ensuro/blob/main/js/ampConfig.js

Another advantage of the skip methods as they are now is you can make some methods immutably public. In some protocols it might make sense to provide such guarantees (even when you can effectively change it by upgrading the contract).

In any case, I agree this increases the complexity of the AMP, but, while a bit more complex (you have to check the AccessManager and the PASS_THRU_METHODS()), the access control configuration is still visible without any code analysis.

I haven’t tested it, but if it calls the custom AccessManager for each call, the overhead will be significant.

If you want to have all the configuration in one place (the access manager) you can do it with the standard AccessManager just making explicit that skipped methods are delegated to the PUBLIC_ROLE, even when the AccessManager won’t be called. And run some check that verifies all the PASS_THRU_METHODS() return PUBLIC_ROLE when calling getTargetFunctionRole in the AccessManager.

6

“I haven’t tested it, but if it calls the custom AccessManager for each call, the overhead will be significant.”

Yep, I agree, the CustomAccessManager I implemented may not be the most gas efficient solution. Dont have the numbers yet.

But the thing is that yes, with this CustomAccessManager u are enforced to use the “restrict” logic even for public and view pure functions.

Quick question on your side: in a proxy pattern, you deploy new implementations while the proxy contract remains unchanged. But if the new implementation adds new functions that you want to skip (or ignore), how do you handle that, given that you can’t add a new immutable variable to control the skip logic? Do you need another proxy, or what’s the correct approach?

7

Sorry for the late reply… I never saw the message with your response.

In my last iteration of the AccessManagedProxy contract, I switched from a proxy using immutable variables to using storage to store the two configurations of the AccessManagedProxy:

  • AccessManager
  • Skippable methods

While the AccessManagedProxy doesn’t include methods to modify these values, since they are saved in storage, they can be modified by the implementation contracts.

Check https://github.com/ensuro/access-managed-proxy/blob/main/contracts/AccessManagedProxy.sol

Also, here’s a sample implementation that allows changing the skipped methods and the access manager (I used the method name “setAuthority” to match the interface of IAccessManaged of the OZ library): https://github.com/ensuro/access-managed-proxy/blob/main/contracts/mock/DummyImplementation.sol#L57

The gas impact of using storage instead of immutable variables is minimal, and this gives much more flexibility. This will allow changing the skipped methods if a new implementation has new pure/view methods. Anyway, doing that would be just a gas-optimization measure, since you can achieve the same effect by changing permissions in the AccessManager, assigning those new methods to the PUBLIC_ROLE.