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 Governor module connecting with AccessManager #4523

Merged
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
5fd9a98
add back GovernorTimelock
frangio Aug 8, 2023
1498539
WIP
frangio Aug 8, 2023
36dc884
rename GovernorTimelock -> GovernorTimelockAccess
frangio Aug 8, 2023
2d2b087
make base delay modifiable
frangio Aug 10, 2023
221261f
remove available since
frangio Aug 10, 2023
499a2a1
use relay to call the target contract
frangio Aug 10, 2023
d5ca8be
fix warning
frangio Aug 10, 2023
7ed7c1c
Update contracts/governance/extensions/GovernorTimelockAccess.sol
frangio Aug 11, 2023
bf9fdf9
fix initial set of base delay
frangio Aug 11, 2023
5ced769
rename maxDelay -> neededDelay
frangio Aug 11, 2023
3820637
address guardian cancellation risk
frangio Aug 11, 2023
31e20a1
use single manager per governor
frangio Aug 12, 2023
56ed5a4
add nonces
frangio Aug 12, 2023
6ca99ef
make _hashOperation private
frangio Aug 15, 2023
e34c093
add docs for nonce
frangio Aug 15, 2023
40adbb7
typo
frangio Aug 15, 2023
2c41d41
Update contracts/governance/extensions/GovernorTimelockAccess.sol
frangio Aug 15, 2023
1446af0
Update contracts/governance/extensions/GovernorTimelockAccess.sol
frangio Aug 15, 2023
bc2bfa5
Update contracts/access/manager/AccessManager.sol
frangio Aug 15, 2023
af274b1
add proposalNeedsQueuing
frangio Aug 15, 2023
bac912e
remove timepoint from executed and canceled events
frangio Aug 15, 2023
a1cbc83
add tests for proposalNeedsQueuing
frangio Aug 15, 2023
ebd6dcd
Apply suggestions from code review
frangio Aug 15, 2023
6902ad7
fix docs
frangio Aug 15, 2023
e96474c
remove unnecessary override
frangio Aug 15, 2023
452c65e
remove _authorityOverride
frangio Aug 15, 2023
f7b3b93
add missing docs
frangio Aug 15, 2023
3b47038
remove unused imports
frangio Aug 15, 2023
8d5e734
typo
frangio Aug 15, 2023
4f89411
lint
frangio Aug 15, 2023
40eea48
add basic tests
frangio Aug 16, 2023
0604f4d
add custom error
frangio Aug 16, 2023
16519fe
lint
frangio Aug 16, 2023
85148a9
make variable private
frangio Aug 16, 2023
a36618b
add changeset
frangio Aug 16, 2023
b37ed30
do not delete executionPlan
frangio Aug 16, 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
81 changes: 60 additions & 21 deletions contracts/access/manager/AccessManager.sol
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,14 @@ contract AccessManager is Context, Multicall, IAccessManager {
mapping(address target => AccessMode mode) private _contractMode;
mapping(uint64 classId => Class) private _classes;
mapping(uint64 groupId => Group) private _groups;
mapping(bytes32 operationId => uint48 schedule) private _schedules;

struct Schedule {
uint48 timepoint;
uint32 nonce;
}

mapping(bytes32 operationId => Schedule) private _schedules;

mapping(bytes4 selector => Time.Delay delay) private _adminDelays;

// This should be transcient storage when supported by the EVM.
Expand Down Expand Up @@ -568,56 +575,76 @@ contract AccessManager is Context, Multicall, IAccessManager {
* operation is not yet scheduled, has expired, was executed, or was canceled.
*/
function getSchedule(bytes32 id) public view virtual returns (uint48) {
uint48 timepoint = _schedules[id];
uint48 timepoint = _schedules[id].timepoint;
return _isExpired(timepoint) ? 0 : timepoint;
}

/**
* @dev Return the nonce for the latest scheduled operation with a given id. Returns 0 if the operation has never
* been scheduled.
*/
function getNonce(bytes32 id) public view virtual returns (uint32) {
return _schedules[id].nonce;
}

/**
* @dev Schedule a delayed operation for future execution, and return the operation identifier. It is possible to
* choose the timestamp at which the operation becomes executable as long as it satisfies the execution delays
* required for the caller. The special value zero will automatically set the earliest possible time.
*
* Returns the `operationId` that was scheduled. Since this value is a hash of the parameters, it can reoccur when
* the same parameters are used; if this is relevant, the returned `nonce` can be used to uniquely identify this
* scheduled operation from other occurrences of the same `operationId` in invocations of {relay} and {cancel}.
*
* Emits a {OperationScheduled} event.
*/
function schedule(address target, bytes calldata data, uint48 when) public virtual returns (bytes32) {
function schedule(address target, bytes calldata data, uint48 when) public virtual returns (bytes32 operationId, uint32 nonce) {
address caller = _msgSender();

// Fetch restriction to that apply to the caller on the targeted function
(bool allowed, uint32 setback) = _canCallExtended(caller, target, data);

uint48 minWhen = Time.timestamp() + setback;

if (when == 0) {
when = minWhen;
}

// If caller is not authorised, revert
if (!allowed && (setback == 0 || when.isSetAndPast(minWhen - 1))) {
if (!allowed && (setback == 0 || when < minWhen)) {
revert AccessManagerUnauthorizedCall(caller, target, bytes4(data[0:4]));
}

// If caller is authorised, schedule operation
bytes32 operationId = _hashOperation(caller, target, data);
operationId = _hashOperation(caller, target, data);

// Cannot reschedule unless the operation has expired
uint48 prevTimepoint = _schedules[operationId];
uint48 prevTimepoint = _schedules[operationId].timepoint;
if (prevTimepoint != 0 && !_isExpired(prevTimepoint)) {
revert AccessManagerAlreadyScheduled(operationId);
}

uint48 timepoint = when == 0 ? minWhen : when;
_schedules[operationId] = timepoint;
emit OperationScheduled(operationId, timepoint, caller, target, data);
nonce = _schedules[operationId].nonce + 1;
frangio marked this conversation as resolved.
Show resolved Hide resolved
_schedules[operationId].timepoint = when;
_schedules[operationId].nonce = nonce;
emit OperationScheduled(operationId, nonce, when, caller, target, data);

return operationId;
// Using named return values because otherwise we get stack too deep
}

/**
* @dev Execute a function that is delay restricted, provided it was properly scheduled beforehand, or the
* execution delay is 0.
*
* Returns the nonce that identifies the previously scheduled operation that is relayed, or 0 if the
* operation wasn't previously scheduled (if the caller doesn't have an execution delay).
*
* Emits an {OperationExecuted} event only if the call was scheduled and delayed.
*/
// Reentrancy is not an issue because permissions are checked on msg.sender. Additionally,
// _consumeScheduledOp guarantees a scheduled operation is only executed once.
// slither-disable-next-line reentrancy-no-eth
function relay(address target, bytes calldata data) public payable virtual {
function relay(address target, bytes calldata data) public payable virtual returns (uint32) {
address caller = _msgSender();

// Fetch restriction to that apply to the caller on the targeted function
Expand All @@ -630,9 +657,10 @@ contract AccessManager is Context, Multicall, IAccessManager {

// If caller is authorised, check operation was scheduled early enough
bytes32 operationId = _hashOperation(caller, target, data);
uint32 nonce;

if (setback != 0) {
_consumeScheduledOp(operationId);
nonce = _consumeScheduledOp(operationId);
}

// Mark the target and selector as authorised
Expand All @@ -644,6 +672,8 @@ contract AccessManager is Context, Multicall, IAccessManager {

// Reset relay identifier
_relayIdentifier = relayIdentifierBefore;

return nonce;
}

/**
Expand All @@ -663,9 +693,12 @@ contract AccessManager is Context, Multicall, IAccessManager {

/**
* @dev Internal variant of {consumeScheduledOp} that operates on bytes32 operationId.
*
* Returns the nonce of the scheduled operation that is consumed.
*/
function _consumeScheduledOp(bytes32 operationId) internal virtual {
uint48 timepoint = _schedules[operationId];
function _consumeScheduledOp(bytes32 operationId) internal virtual returns (uint32) {
uint48 timepoint = _schedules[operationId].timepoint;
uint32 nonce = _schedules[operationId].nonce;

if (timepoint == 0) {
revert AccessManagerNotScheduled(operationId);
Expand All @@ -676,24 +709,27 @@ contract AccessManager is Context, Multicall, IAccessManager {
}

delete _schedules[operationId];
emit OperationExecuted(operationId, timepoint);
emit OperationExecuted(operationId, nonce, timepoint);

return nonce;
}

/**
* @dev Cancel a scheduled (delayed) operation.
* @dev Cancel a scheduled (delayed) operation. Returns the nonce that identifies the previously scheduled
* operation that is cancelled.
*
* Requirements:
*
* - the caller must be the proposer, or a guardian of the targeted function
*
* Emits a {OperationCanceled} event.
*/
function cancel(address caller, address target, bytes calldata data) public virtual {
function cancel(address caller, address target, bytes calldata data) public virtual returns (uint32) {
address msgsender = _msgSender();
bytes4 selector = bytes4(data[0:4]);

bytes32 operationId = _hashOperation(caller, target, data);
if (_schedules[operationId] == 0) {
if (_schedules[operationId].timepoint == 0) {
revert AccessManagerNotScheduled(operationId);
} else if (caller != msgsender) {
// calls can only be canceled by the account that scheduled them, a global admin, or by a guardian of the required group.
Expand All @@ -705,9 +741,12 @@ contract AccessManager is Context, Multicall, IAccessManager {
}
}

uint48 timepoint = _schedules[operationId];
delete _schedules[operationId];
emit OperationCanceled(operationId, timepoint);
uint32 nonce = _schedules[operationId].nonce;
uint48 timepoint = _schedules[operationId].timepoint;
delete _schedules[operationId].timepoint;
emit OperationCanceled(operationId, nonce, timepoint);

return nonce;
}

/**
Expand Down
14 changes: 8 additions & 6 deletions contracts/access/manager/IAccessManager.sol
Original file line number Diff line number Diff line change
Expand Up @@ -9,17 +9,17 @@ interface IAccessManager {
/**
* @dev A delayed operation was scheduled.
*/
event OperationScheduled(bytes32 indexed operationId, uint48 schedule, address caller, address target, bytes data);
event OperationScheduled(bytes32 indexed operationId, uint32 indexed nonce, uint48 schedule, address caller, address target, bytes data);

/**
* @dev A scheduled operation was executed.
*/
event OperationExecuted(bytes32 indexed operationId, uint48 schedule);
event OperationExecuted(bytes32 indexed operationId, uint32 indexed nonce, uint48 schedule);

/**
* @dev A scheduled operation was canceled.
*/
event OperationCanceled(bytes32 indexed operationId, uint48 schedule);
event OperationCanceled(bytes32 indexed operationId, uint32 indexed nonce, uint48 schedule);

event GroupLabel(uint64 indexed groupId, string label);
event GroupGranted(uint64 indexed groupId, address indexed account, uint32 delay, uint48 since);
Expand Down Expand Up @@ -94,11 +94,13 @@ interface IAccessManager {

function getSchedule(bytes32 id) external returns (uint48);

function schedule(address target, bytes calldata data, uint48 when) external returns (bytes32);
function getNonce(bytes32 id) external returns (uint32);

function relay(address target, bytes calldata data) external payable;
function schedule(address target, bytes calldata data, uint48 when) external returns (bytes32, uint32);

function cancel(address caller, address target, bytes calldata data) external;
function relay(address target, bytes calldata data) external payable returns (uint32);

function cancel(address caller, address target, bytes calldata data) external returns (uint32);

function consumeScheduledOp(address caller, bytes calldata data) external;

Expand Down
Loading
Loading