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
Merged
Show file tree
Hide file tree
Changes from 61 commits
Commits
Show all changes
79 commits
Select commit Hold shift + click to select a range
1327b8f
add AccessManager.sol
frangio Mar 1, 2023
b54b922
docs
frangio Mar 1, 2023
5a0fd33
enforce team existence
frangio Mar 1, 2023
ee5de96
remove on-chain enumerability
frangio Mar 15, 2023
6284768
add Authority interface
frangio Mar 15, 2023
bce3641
remove redundancy
frangio Mar 15, 2023
223e6af
simplify and complete features
frangio Mar 16, 2023
eafb571
split in separate files
frangio Mar 16, 2023
24af159
write AccessManager docs
frangio Mar 16, 2023
833f219
add docs for AccessManagerAdapter
frangio Mar 16, 2023
0f27739
add to docs site
frangio Mar 16, 2023
2ac5aa1
docs for IAuthority
frangio Mar 16, 2023
f26c4d3
lint
frangio Mar 16, 2023
63f9393
add initial AccessManaged docs
frangio Mar 16, 2023
6fecd25
typos
frangio Mar 16, 2023
589b5b1
Update contracts/access/manager/AccessManager.sol
frangio Mar 16, 2023
619cc73
fix docs
frangio Mar 16, 2023
1b681a5
rename team "all" -> "public"
frangio Mar 16, 2023
48b31c0
ad hoc selector batching
frangio Mar 16, 2023
b078157
whitespace
frangio Mar 17, 2023
274c04d
add docs on team number choice
frangio Mar 17, 2023
082b05c
add missing function argument
frangio Mar 17, 2023
52f27c8
fix team role decoding
frangio Mar 17, 2023
1285c49
add ungrouped interface
frangio Mar 17, 2023
5c7472a
improve docs
frangio Mar 17, 2023
3cc1e73
remove todo
frangio Mar 17, 2023
ae3482d
add initial tests for teams
frangio Mar 19, 2023
535ea13
rename team -> badge
frangio Mar 19, 2023
68bb5fa
improve testing
frangio Mar 19, 2023
fa58ec5
Update contracts/access/manager/AccessManager.sol
frangio Mar 19, 2023
6d76058
use mapping labels from solidity 0.8.18
frangio Mar 20, 2023
3447438
add AccessManaged tests
frangio Mar 20, 2023
1d10639
fix group encoding
frangio Mar 20, 2023
d875a7b
add test for allowing
frangio Mar 20, 2023
d517d27
lint
frangio Mar 20, 2023
6d33cda
add changeset
frangio Mar 20, 2023
9ed1aef
lint
frangio Mar 20, 2023
6adc0d8
fix mock
frangio Mar 21, 2023
0624794
remove contract groups
frangio Mar 21, 2023
17ec3b6
add transferContractAuthority
frangio Mar 21, 2023
f9210c5
lint
frangio Mar 21, 2023
a54cd10
rename badge -> group
frangio Mar 21, 2023
8af9ca4
change test name
frangio Mar 21, 2023
2a05bcc
add setContractModeCustom and tests
frangio Mar 21, 2023
5f22f2d
remove use of mapping labels
frangio Mar 21, 2023
6a9fbed
Merge branch 'master' into accessmanager
frangio Mar 22, 2023
0580198
tweak comments
frangio Mar 22, 2023
e01e362
add tests for adapter
frangio Mar 22, 2023
cb026bc
lint
frangio Mar 22, 2023
76d35da
add revert reasons and group tests
frangio Mar 22, 2023
4069781
add tests for allowing and disallowing roles
frangio Mar 22, 2023
80ffd2a
lint
frangio Mar 22, 2023
3094d0c
add docs for AccessManaged
frangio Mar 22, 2023
b3a8b1b
Update contracts/access/manager/AccessManager.sol
frangio Mar 22, 2023
8a68322
Update contracts/access/manager/AccessManager.sol
frangio Mar 22, 2023
eeab8cf
Update contracts/access/manager/AccessManager.sol
frangio Mar 22, 2023
ca61e38
add tests for onlyDefaultAdmin
frangio Mar 22, 2023
8824c31
lint
frangio Mar 22, 2023
67e33b6
add internal _setContractMode
frangio Mar 22, 2023
5f270c7
Update .changeset/quiet-trainers-kick.md
frangio Mar 22, 2023
6f7ac96
Update contracts/access/manager/AccessManager.sol
frangio Mar 22, 2023
7ec5311
Update contracts/access/manager/AccessManager.sol
frangio Mar 22, 2023
82402d2
typo
frangio Mar 22, 2023
27807e6
Apply suggestions from code review
frangio Mar 22, 2023
80bc88b
roll back to 0.8.13
frangio Mar 22, 2023
cf4df22
add test for setFunctionAllowedGroup events
frangio Mar 22, 2023
bab4d34
remove mode restriction on setFunctionAllowedGroup
frangio Mar 22, 2023
642e279
simplify use of setFunctionAllowedGroup
frangio Mar 22, 2023
51aff23
lint
frangio Mar 22, 2023
6e3da66
remove unused import
frangio Mar 22, 2023
6b56c8f
remove onlyDefaultAdmin modifier
frangio Mar 22, 2023
b7e3b3f
lint
frangio Mar 22, 2023
8200986
use return value names for _decodeGroupRole
frangio Mar 22, 2023
18e53e2
rename RestrictedMode -> AccessMode
frangio Mar 22, 2023
f94a881
use Context._msgSender
frangio Mar 22, 2023
cd8babd
reorder arguments to grant/revoke/renounceGroup
frangio Mar 22, 2023
597edc0
add IAccessControlDefaultAdminRules
frangio Mar 22, 2023
ee560d0
Apply suggestions from code review
frangio Mar 22, 2023
33f5ace
Fix expected event parameter name in tests
ernestognw Mar 22, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/quiet-trainers-kick.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'openzeppelin-solidity': minor
---

`AccessManager`: Added a new contract for managing access control of complex systems in a consolidated location.
5 changes: 5 additions & 0 deletions contracts/access/AccessControl.sol
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,11 @@ abstract contract AccessControl is Context, IAccessControl, ERC165 {
_;
}

modifier onlyDefaultAdmin() {
_checkRole(DEFAULT_ADMIN_ROLE);
_;
}
frangio marked this conversation as resolved.
Show resolved Hide resolved

/**
* @dev See {IERC165-supportsInterface}.
*/
Expand Down
10 changes: 10 additions & 0 deletions contracts/access/README.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -25,3 +25,13 @@ This directory provides ways to restrict who can access the functions of a contr
{{AccessControlEnumerable}}

{{AccessControlDefaultAdminRules}}

== AccessManager
ernestognw marked this conversation as resolved.
Show resolved Hide resolved

{{IAuthority}}

{{AccessManager}}

{{AccessManaged}}

{{AccessManagerAdapter}}
75 changes: 75 additions & 0 deletions contracts/access/manager/AccessManaged.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
// SPDX-License-Identifier: MIT

pragma solidity ^0.8.0;

import "./IAuthority.sol";

/**
* @dev This contract module makes available a {restricted} modifier. Functions decorated with this modifier will be
* permissioned according to an "authority": a contract like {AccessManager} that follows the {IAuthority} interface,
* implementing a policy that allows certain callers access to certain functions.
*
* IMPORTANT: The `restricted` modifier should never be used on `internal` functions, judiciously used in `public`
* functions, and ideally only used in `external` functions. See {restricted}.
*/
contract AccessManaged {
event AuthorityUpdated(IAuthority indexed oldAuthority, IAuthority indexed newAuthority);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is very rarely triggered, so the gas cost is not really an issue... but I'm still wondering if emitting the old value really brings anything.

Copy link
Contributor Author

@frangio frangio Mar 22, 2023

Choose a reason for hiding this comment

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

I wanted to match the event emitted by Solmate:

    event AuthorityUpdated(address indexed user, Authority indexed newAuthority);

It looks different but the semantics are the same with the exception of the constructor event.

Copy link
Collaborator

@Amxx Amxx Mar 22, 2023

Choose a reason for hiding this comment

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

Remember that there is an internal _setAuthority that can called by someone that is not the oldAuthority.

The semantics are not the same!

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd go for exaclty matching Solmate's semantics

Copy link
Contributor Author

@frangio frangio Mar 22, 2023

Choose a reason for hiding this comment

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

The semantics are not the same!

Solmate's semantics subsume AccessManaged semantics, because in AccessManaged the oldAuthority is the user that has triggered the transfer (msg.sender).

I don't see what we would need to change to match the semantics exactly. Is it just renaming oldAuthority to user and making sure we use msg.sender?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it just renaming oldAuthority to user and making sure we use msg.sender?

yes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok to use sender instead of user? We have precedent for sender in AccessControl.RoleGranted and so on.

Copy link
Member

Choose a reason for hiding this comment

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

Ok to use sender instead of user?

If sender == msg.sender for every case. Yes.


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.


/**
* @dev Restricts access to a function as defined by the connected Authority for this contract and the
* caller and selector of the function that entered the contract.
*
* [IMPORTANT]
* ====
* In general, this modifier should only be used on `external` functions. It is okay to use it on `public` functions
* that are used as external entry points and are not called internally. Unless you know what you're oding, it
* should never be used on `internal` functions. Failure to follow these rules can have critical security
* implications! This is because the permissions are determined by the function that entered the contract, i.e. the
* function at the bottom of the call stack, and not the function where the modifier is visible in the source code.
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 have any concrete recommendations but didn't want to skip saying that I'm concerned about this if a developer is creating a bridge.

We should provide at least a way around this issue if the restricted modifier is used for functions calling arbitrary contracts (i.e. forwarding calls), which may be a common use case in L2 precompiles, or system contracts.

I'll update if I think of something

* ====
*/
modifier restricted() {
_checkCanCall(msg.sender, msg.sig);
_;
}

/**
* @dev Initializes the contract connected to an initial authority.
*/
constructor(IAuthority initialAuthority) {
_setAuthority(initialAuthority);
}

/**
* @dev Returns the current authority.
*/
function authority() public view virtual returns (IAuthority) {
return _authority;
}

/**
* @dev Transfers control to a new authority. The caller must be the current authority.
*/
function setAuthority(IAuthority newAuthority) public virtual {
require(msg.sender == address(_authority), "AccessManaged: not current authority");
frangio marked this conversation as resolved.
Show resolved Hide resolved
_setAuthority(newAuthority);
}

/**
* @dev Transfers control to a new authority. Internal function with no access restriction.
*/
function _setAuthority(IAuthority newAuthority) internal virtual {
IAuthority oldAuthority = _authority;
_authority = newAuthority;
emit AuthorityUpdated(oldAuthority, newAuthority);
}

/**
* @dev Reverts if the caller is not allowed to call the function identified by a selector.
*/
function _checkCanCall(address caller, bytes4 selector) internal view virtual {
require(_authority.canCall(caller, address(this), selector), "AccessManaged: authority rejected");
}
}
Loading