Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add AccessManager contracts #4121

Merged
merged 79 commits into from
Mar 24, 2023

Conversation

frangio
Copy link
Contributor

@frangio frangio commented Mar 16, 2023

Implements a pattern of access control where permissions for a multi-contract system are managed in a central location (an AccessManager instance) and with function granularity.

@changeset-bot
Copy link

changeset-bot bot commented Mar 16, 2023

🦋 Changeset detected

Latest commit: 33f5ace

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
openzeppelin-solidity Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

contract AccessManaged {
event AuthorityUpdated(IAuthority indexed oldAuthority, IAuthority indexed newAuthority);

IAuthority private _authority;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know that is restrictive, but making this immutable would save a non insignificant amount of gas.
Should we have two versions ?

In the case of proxy/clones created by a factory, would it make sens that all implementation use the same authority? I think yes.

Copy link
Contributor Author

@frangio frangio Mar 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we have two versions ?

You know I hate multiple versions... 😄

The main reason why I left this as a mutable variable is because I think it can be used to freeze the permissions in a contract by moving it off of an AccessManager onto an immutable Authority. We could also implement freezing in the AccessManager itself, by having frozen contract groups in which permissions can't be altered, and whose contracts can't be moved out of that group. I felt that having mutability of the authority at the managed contract was the simpler option, but it's true that it makes it more expensive.

I'm open to this discussion though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it can be used to freeze the permissions in a contract by moving it off of an AccessManager onto an immutable Authority

This makes sense, but I also think it violates the goal of consolidation. In this way, now a managed contract may have the right to update itself without the Manager's approval.

While that still makes sense to me in some cases, I am not sure if a significant share of is AccessManaged contracts will want that capability when the main value proposition is having everything on the manager.

My opinion here would be to make it immutable since any freezing capability can be handled by the Manager by making it is AccessManaged as well, and using the CLOSED group. Maybe a bit complex, but I think users may naturally try it.

Copy link
Contributor Author

@frangio frangio Mar 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

now a managed contract may have the right to update itself without the Manager's approval.

I don't think this is true. Only the current manager should be able to "eject" a managed contract from itself.

and using the CLOSED group

Note that adding a contract in the Closed or Open group doesn't freeze the permissions, because the manager retains the ability to move it outside of the group.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is true.

Why not? Any is AccessManaged contract will have access to that storage slot even if the variable is private.
Now I see that a counterargument is that accessing that storage slot has to be deliberated.

We can keep this private, but I have some bad feelings about letting the user "eject" the contract itself even if done deliberately.

Note that adding a contract in the Closed or Open group doesn't freeze the permissions

I see what you meant, but a previous idea I remember we discussed is suggesting overrides like:

function setContractModeOpern(address target) ... override {
  require(_cantReopen[target]);
  super.setContractModeCustom(target);
}

Not sure exactly how this can play out, but I think there might be a setup we may like to explore (eg. not reopening by default unless explicitly stated).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO it's fine if the target contract deliberately inserts a custom-gated _setAuthority function call. I wouldn't expect it normally, but I can imagine some sort of emergency mechanism.

contracts/access/manager/AccessManager.sol Outdated Show resolved Hide resolved
contracts/access/manager/AccessManager.sol Outdated Show resolved Hide resolved
contracts/access/manager/AccessManager.sol Outdated Show resolved Hide resolved
contracts/access/manager/AccessManager.sol Outdated Show resolved Hide resolved
contracts/access/manager/AccessManager.sol Outdated Show resolved Hide resolved
@frangio
Copy link
Contributor Author

frangio commented Mar 16, 2023

Should this contract support batching? If so, should it be ad hoc or simply through Multicall?

Note that the default admin should almost always be a multisig or DAO which tend to have batching built in anyway.

@Amxx
Copy link
Collaborator

Amxx commented Mar 16, 2023

Should this contract support batching? If so, should it be ad hoc or simply through Multicall?

Note that the default admin should almost always be a multisig or DAO which tend to have batching built in anyway.

It would go with multicall. Simpler, cleaner

@frangio
Copy link
Contributor Author

frangio commented Mar 17, 2023

@ggonzalez94 suggested simply making the selector argument an array of selectors in setFunctionAllowedTeam, since that is likely to be the main thing people do in batch. I think that's simple and it makes sense.

I'm not a fan of adding Multicall because usability and auditability are not great. For example, a Multicall-batched transaction in Gnosis Safe becomes unauditable without extra steps to manually decode calldata.

This was referenced Sep 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants