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 AccessControlDefaultAdminRules #4009

Merged
Merged
Show file tree
Hide file tree
Changes from 75 commits
Commits
Show all changes
78 commits
Select commit Hold shift + click to select a range
3989054
Create `AccessControlAdminRules` and its corresponding interface
ernestognw Jan 27, 2023
da52678
Add tests
ernestognw Jan 28, 2023
f7738a6
Add changeset
ernestognw Jan 28, 2023
e2eb45d
Merge branch 'master' into lib-618-accesscontrol-admin-rules
ernestognw Jan 28, 2023
68adaf5
Correctly set unlocked crite
ernestognw Jan 28, 2023
0aff3ff
Completed docs
ernestognw Jan 28, 2023
4090a3e
Fix linter
ernestognw Jan 28, 2023
21aacf9
Refactor tests so coverage is not reduced
ernestognw Jan 28, 2023
2863047
Address PR review comments
ernestognw Jan 30, 2023
a9a3495
Update contracts/access/AccessControlAdminRules.sol
ernestognw Jan 30, 2023
59f3b62
Add draft-ERC5313
ernestognw Jan 30, 2023
2bfb028
Remove property-based test
ernestognw Jan 30, 2023
7ae4aa7
Fix typo
ernestognw Jan 30, 2023
2d630c3
Add IERC5313 docs
ernestognw Jan 30, 2023
192c5bc
Change owner for admin
ernestognw Jan 30, 2023
021ee37
Call overrideable functions
ernestognw Jan 30, 2023
a54dd2b
Update IERC5313 to final
ernestognw Jan 31, 2023
85cff36
Reword private function
ernestognw Jan 31, 2023
8baa13b
Remove property test again
ernestognw Jan 31, 2023
fb895d2
Rename again
ernestognw Jan 31, 2023
3fbfbac
Merge branch 'master' into lib-618-accesscontrol-admin-rules
ernestognw Feb 4, 2023
b70ae34
Merge branch 'master' into lib-618-accesscontrol-admin-rules
ernestognw Feb 8, 2023
d120c1d
Update .changeset/silent-dancers-type.md
ernestognw Feb 16, 2023
4322ed7
Update contracts/access/AccessControl.sol
ernestognw Feb 16, 2023
d52b371
Update docs/modules/ROOT/pages/access-control.adoc
ernestognw Feb 16, 2023
54184af
Rename to defaultAdmin
ernestognw Feb 16, 2023
d1064f4
Update contracts/access/AccessControlAdminRules.sol
ernestognw Feb 16, 2023
d352694
Update contracts/access/AccessControlAdminRules.sol
ernestognw Feb 16, 2023
53e8087
Rename delayedUntil
ernestognw Feb 16, 2023
56b5df6
Lint
ernestognw Feb 16, 2023
30306c3
Merge branch 'master' into lib-618-accesscontrol-admin-rules
ernestognw Feb 16, 2023
701d341
Small fixes
ernestognw Feb 17, 2023
8ebb27b
More typos and minor corrections
ernestognw Feb 17, 2023
f7c6596
Allow defaultAdmin to start transfer again
ernestognw Feb 17, 2023
5092d70
Add extra test
ernestognw Feb 17, 2023
4642a12
Merge branch 'master' into lib-618-accesscontrol-admin-rules
ernestognw Feb 17, 2023
630df0d
Rename to AccessControlDefaultAdminRules
ernestognw Feb 17, 2023
07d12a2
Removed IAccessControlDefaultAdminRules
ernestognw Feb 17, 2023
41f4ba3
Add note in Extending Contracts section
ernestognw Feb 18, 2023
6da27ad
Update contracts/access/AccessControlDefaultAdminRules.sol
ernestognw Feb 21, 2023
4b39c30
Fix comments example
ernestognw Feb 21, 2023
da8227f
Add virtual to delay()
ernestognw Feb 21, 2023
6139905
Replace _delay with getter
ernestognw Feb 21, 2023
f420162
Add IAccessControlDefaultAdmin back
ernestognw Feb 21, 2023
8747bf4
Created interal functions for beginning and accepting transfers
ernestognw Feb 21, 2023
444908c
Merge branch 'master' into lib-618-accesscontrol-admin-rules
ernestognw Feb 21, 2023
5743d7a
Update contracts/access/AccessControlDefaultAdminRules.sol
ernestognw Feb 21, 2023
0b9ea0b
Update contracts/access/AccessControlDefaultAdminRules.sol
ernestognw Feb 21, 2023
780e901
Fixed test error message
ernestognw Feb 22, 2023
24ce7fa
Change delay to defaultAdminDelay
ernestognw Feb 22, 2023
a8bbf18
Merge branch 'master' into lib-618-accesscontrol-admin-rules
ernestognw Feb 22, 2023
2c4e15e
Change long variable name
ernestognw Feb 22, 2023
d171299
Run prettier
ernestognw Feb 22, 2023
281209c
Remove IAccessControlDefaultAdminRules from docs
ernestognw Feb 22, 2023
2c49a5d
Remove camel case from error messages
ernestognw Feb 22, 2023
cb98ad9
Update test/access/AccessControl.behavior.js
ernestognw Feb 22, 2023
88168be
Update test/access/AccessControl.behavior.js
ernestognw Feb 22, 2023
39d176a
Use beforeEach in begin transfer tests
ernestognw Feb 22, 2023
9e52530
Update test/access/AccessControl.behavior.js
ernestognw Feb 22, 2023
4721d63
Update test/access/AccessControl.behavior.js
ernestognw Feb 22, 2023
0e532c7
Update contracts/access/AccessControlDefaultAdminRules.sol
ernestognw Feb 22, 2023
70c5cae
Update contracts/access/AccessControlDefaultAdminRules.sol
ernestognw Feb 22, 2023
667e3da
Update contracts/access/AccessControlDefaultAdminRules.sol
ernestognw Feb 22, 2023
5a31431
Remove unnecessary async
ernestognw Feb 22, 2023
14994be
Change `is met` for `has passed`
ernestognw Feb 22, 2023
fd64f5d
Change note in docs
ernestognw Feb 22, 2023
9e04336
Update contracts/access/AccessControlDefaultAdminRules.sol
ernestognw Feb 22, 2023
ac093f4
Simplify renouncing tests
ernestognw Feb 22, 2023
3948246
Extend cancel tests
ernestognw Feb 22, 2023
34ed74f
Rename delay test variables
ernestognw Feb 22, 2023
880ac0b
Remove confusing comment
ernestognw Feb 22, 2023
3337f61
Replace met with passed
ernestognw Feb 22, 2023
a43abe7
Simplified acceptance tests
ernestognw Feb 22, 2023
b4ee08a
Add IAccessControlEnumerable interface removed accidentally from docs
ernestognw Feb 22, 2023
b2b9e81
Merge branch 'master' into lib-618-accesscontrol-admin-rules
ernestognw Feb 22, 2023
d1289a2
Merge branch 'master' into lib-618-accesscontrol-admin-rules
frangio Feb 24, 2023
7d37c81
fix flaky tests
frangio Feb 24, 2023
b164929
lint
frangio Feb 24, 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/silent-dancers-type.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'openzeppelin-solidity': minor
---

`AccessControlDefaultAdminRules`: Add an extension of `AccessControl` with additional security rules for the `DEFAULT_ADMIN_ROLE`.
3 changes: 2 additions & 1 deletion contracts/access/AccessControl.sol
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,8 @@ import "../utils/introspection/ERC165.sol";
*
* WARNING: The `DEFAULT_ADMIN_ROLE` is also its own admin: it has permission to
* grant and revoke this role. Extra precautions should be taken to secure
* accounts that have been granted it.
* accounts that have been granted it. We recommend using {AccessControlDefaultAdminRules}
* to enforce additional security measures for this role.
*/
abstract contract AccessControl is Context, IAccessControl, ERC165 {
struct RoleData {
Expand Down
240 changes: 240 additions & 0 deletions contracts/access/AccessControlDefaultAdminRules.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,240 @@
// SPDX-License-Identifier: MIT
// OpenZeppelin Contracts (last updated v4.8.0) (access/AccessControlDefaultAdminRules.sol)

pragma solidity ^0.8.0;

import "./AccessControl.sol";
import "./IAccessControlDefaultAdminRules.sol";
import "../utils/math/SafeCast.sol";
import "../interfaces/IERC5313.sol";

/**
* @dev Extension of {AccessControl} that allows specifying special rules to manage
* the `DEFAULT_ADMIN_ROLE` holder, which is a sensitive role with special permissions
* over other roles that may potentially have privileged rights in the system.
*
* If a specific role doesn't have an admin role assigned, the holder of the
* `DEFAULT_ADMIN_ROLE` will have the ability to grant it and revoke it.
*
* This contract implements the following risk mitigations on top of {AccessControl}:
*
* * Only one account holds the `DEFAULT_ADMIN_ROLE` since deployment until it's potentially renounced.
* * Enforce a 2-step process to transfer the `DEFAULT_ADMIN_ROLE` to another account.
* * Enforce a configurable delay between the two steps, with the ability to cancel in between.
* - Even after the timer has passed to avoid locking it forever.
* * It is not possible to use another role to manage the `DEFAULT_ADMIN_ROLE`.
*
* Example usage:
*
* ```solidity
* contract MyToken is AccessControlDefaultAdminRules {
* constructor() AccessControlDefaultAdminRules(
* 3 days,
* msg.sender // Explicit initial `DEFAULT_ADMIN_ROLE` holder
* ) {}
*}
* ```
*
* NOTE: The `delay` can only be set in the constructor and is fixed thereafter.
*
* _Available since v4.9._
*/
abstract contract AccessControlDefaultAdminRules is IAccessControlDefaultAdminRules, IERC5313, AccessControl {
uint48 private immutable _defaultAdminDelay;

address private _currentDefaultAdmin;
address private _pendingDefaultAdmin;

uint48 private _defaultAdminTransferDelayedUntil;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure we discussed that, but is there a reason to favor DelayedUntil over Deadline ?

IMO deadline is simpler and works just as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it expresses the opposite idea.

While a delayedUntil gives a sense of something that can be done after, whereas deadline seems to communicate that an action can be done before.

Any reason to keep it as Deadline?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that deadline seems like the opposite idea. But I would also like it if we had a shorter simpler option than "delayed until".


/**
* @dev Sets the initial values for {defaultAdminDelay} in seconds and {defaultAdmin}.
*
* The `defaultAdminDelay` value is immutable. It can only be set at the constructor.
*/
constructor(uint48 defaultAdminDelay_, address initialDefaultAdmin) {
_defaultAdminDelay = defaultAdminDelay_;
_grantRole(DEFAULT_ADMIN_ROLE, initialDefaultAdmin);
}

/**
* @dev See {IERC5313-owner}.
*/
function owner() public view virtual returns (address) {
return defaultAdmin();
}

/**
* @inheritdoc IAccessControlDefaultAdminRules
*/
function defaultAdminDelay() public view virtual returns (uint48) {
return _defaultAdminDelay;
}

/**
* @inheritdoc IAccessControlDefaultAdminRules
*/
function defaultAdmin() public view virtual returns (address) {
return _currentDefaultAdmin;
}

/**
* @inheritdoc IAccessControlDefaultAdminRules
*/
function pendingDefaultAdmin() public view virtual returns (address) {
return _pendingDefaultAdmin;
}

/**
* @inheritdoc IAccessControlDefaultAdminRules
*/
function defaultAdminTransferDelayedUntil() public view virtual returns (uint48) {
return _defaultAdminTransferDelayedUntil;
}

/**
* @dev See {IERC165-supportsInterface}.
*/
function supportsInterface(bytes4 interfaceId) public view virtual override returns (bool) {
return interfaceId == type(IAccessControlDefaultAdminRules).interfaceId || super.supportsInterface(interfaceId);
}

/**
* @inheritdoc IAccessControlDefaultAdminRules
*/
function beginDefaultAdminTransfer(address newAdmin) public virtual onlyRole(DEFAULT_ADMIN_ROLE) {
ernestognw marked this conversation as resolved.
Show resolved Hide resolved
_beginDefaultAdminTransfer(newAdmin);
}

/**
* @inheritdoc IAccessControlDefaultAdminRules
*/
function acceptDefaultAdminTransfer() public virtual {
ernestognw marked this conversation as resolved.
Show resolved Hide resolved
require(_msgSender() == pendingDefaultAdmin(), "AccessControl: pending admin must accept");
_acceptDefaultAdminTransfer();
}

/**
* @inheritdoc IAccessControlDefaultAdminRules
*/
function cancelDefaultAdminTransfer() public virtual onlyRole(DEFAULT_ADMIN_ROLE) {
_resetDefaultAdminTransfer();
}

/**
* @dev Revokes `role` from the calling account.
*
* For `DEFAULT_ADMIN_ROLE`, only allows renouncing in two steps, so it's required
* that the {defaultAdminTransferDelayedUntil} has passed and the pending default admin is the zero address.
* After its execution, it will not be possible to call `onlyRole(DEFAULT_ADMIN_ROLE)`
* functions.
*
* For other roles, see {AccessControl-renounceRole}.
*
* NOTE: Renouncing `DEFAULT_ADMIN_ROLE` will leave the contract without a defaultAdmin,
* thereby disabling any functionality that is only available to the default admin, and the
* possibility of reassigning a non-administrated role.
*/
function renounceRole(bytes32 role, address account) public virtual override(AccessControl, IAccessControl) {
if (role == DEFAULT_ADMIN_ROLE) {
require(
pendingDefaultAdmin() == address(0) && _hasDefaultAdminTransferDelayPassed(),
"AccessControl: only can renounce in two delayed steps"
);
}
super.renounceRole(role, account);
}

/**
* @dev See {AccessControl-grantRole}. Reverts for `DEFAULT_ADMIN_ROLE`.
*/
function grantRole(bytes32 role, address account) public virtual override(AccessControl, IAccessControl) {
require(role != DEFAULT_ADMIN_ROLE, "AccessControl: can't directly grant default admin role");
super.grantRole(role, account);
}

/**
* @dev See {AccessControl-revokeRole}. Reverts for `DEFAULT_ADMIN_ROLE`.
*/
function revokeRole(bytes32 role, address account) public virtual override(AccessControl, IAccessControl) {
require(role != DEFAULT_ADMIN_ROLE, "AccessControl: can't directly revoke default admin role");
super.revokeRole(role, account);
}

/**
* @dev See {AccessControl-_setRoleAdmin}. Reverts for `DEFAULT_ADMIN_ROLE`.
*/
function _setRoleAdmin(bytes32 role, bytes32 adminRole) internal virtual override {
require(role != DEFAULT_ADMIN_ROLE, "AccessControl: can't violate default admin rules");
super._setRoleAdmin(role, adminRole);
}

/**
* @dev Grants `role` to `account`.
*
* For `DEFAULT_ADMIN_ROLE`, it only allows granting if there isn't already a role's holder
* or if the role has been previously renounced.
*
* For other roles, see {AccessControl-renounceRole}.
*
* NOTE: Exposing this function through another mechanism may make the
* `DEFAULT_ADMIN_ROLE` assignable again. Make sure to guarantee this is
* the expected behavior in your implementation.
*/
function _grantRole(bytes32 role, address account) internal virtual override {
if (role == DEFAULT_ADMIN_ROLE) {
require(defaultAdmin() == address(0), "AccessControl: default admin already granted");
_currentDefaultAdmin = account;
}
super._grantRole(role, account);
}

/**
* @dev See {acceptDefaultAdminTransfer}.
*
* Internal function without access restriction.
*/
function _acceptDefaultAdminTransfer() internal virtual {
require(_hasDefaultAdminTransferDelayPassed(), "AccessControl: transfer delay not passed");
_revokeRole(DEFAULT_ADMIN_ROLE, defaultAdmin());
_grantRole(DEFAULT_ADMIN_ROLE, pendingDefaultAdmin());
_resetDefaultAdminTransfer();
}

/**
* @dev See {beginDefaultAdminTransfer}.
*
* Internal function without access restriction.
*/
function _beginDefaultAdminTransfer(address newAdmin) internal virtual {
_defaultAdminTransferDelayedUntil = SafeCast.toUint48(block.timestamp) + defaultAdminDelay();
_pendingDefaultAdmin = newAdmin;
emit DefaultAdminRoleChangeStarted(pendingDefaultAdmin(), defaultAdminTransferDelayedUntil());
}

/**
* @dev See {AccessControl-_revokeRole}.
*/
function _revokeRole(bytes32 role, address account) internal virtual override {
if (role == DEFAULT_ADMIN_ROLE) {
delete _currentDefaultAdmin;
}
super._revokeRole(role, account);
}

/**
* @dev Resets the pending default admin and delayed until.
*/
function _resetDefaultAdminTransfer() private {
delete _pendingDefaultAdmin;
delete _defaultAdminTransferDelayedUntil;
}

/**
* @dev Checks if a {defaultAdminTransferDelayedUntil} has been set and passed.
*/
function _hasDefaultAdminTransferDelayPassed() private view returns (bool) {
uint48 delayedUntil = defaultAdminTransferDelayedUntil();
return delayedUntil > 0 && delayedUntil < block.timestamp;
}
}
73 changes: 73 additions & 0 deletions contracts/access/IAccessControlDefaultAdminRules.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
// SPDX-License-Identifier: MIT
// OpenZeppelin Contracts v4.9.0 (access/IAccessControlDefaultAdminRules.sol)

pragma solidity ^0.8.0;

import "./IAccessControl.sol";

/**
* @dev External interface of AccessControlDefaultAdminRules declared to support ERC165 detection.
*
* _Available since v4.9._
*/
interface IAccessControlDefaultAdminRules is IAccessControl {
/**
* @dev Emitted when a `DEFAULT_ADMIN_ROLE` transfer is started, setting `newDefaultAdmin`
* as the next default admin, which will have rights to claim the `DEFAULT_ADMIN_ROLE`
* after `defaultAdminTransferDelayedUntil` has passed.
*/
event DefaultAdminRoleChangeStarted(address indexed newDefaultAdmin, uint48 defaultAdminTransferDelayedUntil);

/**
* @dev Returns the delay between each `DEFAULT_ADMIN_ROLE` transfer.
*/
function defaultAdminDelay() external view returns (uint48);

/**
* @dev Returns the address of the current `DEFAULT_ADMIN_ROLE` holder.
*/
function defaultAdmin() external view returns (address);

/**
* @dev Returns the address of the pending `DEFAULT_ADMIN_ROLE` holder.
*/
function pendingDefaultAdmin() external view returns (address);

/**
* @dev Returns the timestamp after which the pending default admin can claim the `DEFAULT_ADMIN_ROLE`.
*/
function defaultAdminTransferDelayedUntil() external view returns (uint48);

/**
* @dev Starts a `DEFAULT_ADMIN_ROLE` transfer by setting a pending default admin
* and a timer to pass.
*
* Requirements:
*
* - Only can be called by the current `DEFAULT_ADMIN_ROLE` holder.
*
* Emits a {DefaultAdminRoleChangeStarted}.
*/
function beginDefaultAdminTransfer(address newAdmin) external;

/**
* @dev Completes a `DEFAULT_ADMIN_ROLE` transfer.
*
* Requirements:
*
* - Caller should be the pending default admin.
* - `DEFAULT_ADMIN_ROLE` should be granted to the caller.
* - `DEFAULT_ADMIN_ROLE` should be revoked from the previous holder.
*/
function acceptDefaultAdminTransfer() external;

/**
* @dev Cancels a `DEFAULT_ADMIN_ROLE` transfer.
*
* Requirements:
*
* - Can be called even after the timer has passed.
* - Can only be called by the current `DEFAULT_ADMIN_ROLE` holder.
*/
function cancelDefaultAdminTransfer() external;
}
4 changes: 2 additions & 2 deletions contracts/access/Ownable.sol
Original file line number Diff line number Diff line change
Expand Up @@ -53,10 +53,10 @@ abstract contract Ownable is Context {

/**
* @dev Leaves the contract without owner. It will not be possible to call
* `onlyOwner` functions anymore. Can only be called by the current owner.
* `onlyOwner` functions. Can only be called by the current owner.
*
* NOTE: Renouncing ownership will leave the contract without an owner,
* thereby removing any functionality that is only available to the owner.
* thereby disabling any functionality that is only available to the owner.
*/
function renounceOwnership() public virtual onlyOwner {
_transferOwnership(address(0));
Expand Down
2 changes: 2 additions & 0 deletions contracts/access/README.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -23,3 +23,5 @@ This directory provides ways to restrict who can access the functions of a contr
{{IAccessControlEnumerable}}

{{AccessControlEnumerable}}

{{AccessControlDefaultAdminRules}}
5 changes: 4 additions & 1 deletion contracts/interfaces/README.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ are useful to interact with third party contracts that implement them.
- {IERC3156FlashLender}
- {IERC3156FlashBorrower}
- {IERC4626}
- {IERC5313}

== Detailed ABI

Expand All @@ -53,4 +54,6 @@ are useful to interact with third party contracts that implement them.

{{IERC3156FlashBorrower}}

{{IERC4626}}
{{IERC4626}}

{{IERC5313}}
2 changes: 2 additions & 0 deletions docs/modules/ROOT/pages/access-control.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,8 @@ Every role has an associated admin role, which grants permission to call the `gr

This mechanism can be used to create complex permissioning structures resembling organizational charts, but it also provides an easy way to manage simpler applications. `AccessControl` includes a special role, called `DEFAULT_ADMIN_ROLE`, which acts as the **default admin role for all roles**. An account with this role will be able to manage any other role, unless `_setRoleAdmin` is used to select a new admin role.

Since it is the admin for all roles by default, and in fact it is also its own admin, this role carries significant risk. To mitigate this risk we provide xref:api:access.adoc#AccessControlDefaultAdminRules[`AccessControlDefaultAdminRules`], a recommended extension of `AccessControl` that adds a number of enforced security measures for this role: the admin is restricted to a single account, with a 2-step transfer procedure with a delay in between steps.

Let's take a look at the ERC20 token example, this time taking advantage of the default admin role:

[source,solidity]
Expand Down
2 changes: 2 additions & 0 deletions docs/modules/ROOT/pages/extending-contracts.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,8 @@ contract ModifiedAccessControl is AccessControl {

The `super.revokeRole` statement at the end will invoke ``AccessControl``'s original version of `revokeRole`, the same code that would've run if there were no overrides in place.

NOTE: The same rule is implemented and extended in xref:api:access.adoc#AccessControlDefaultAdminRules[`AccessControlDefaultAdminRules`], an extension that also adds enforced security measures for the `DEFAULT_ADMIN_ROLE`.

[[using-hooks]]
== Using Hooks

Expand Down
Loading