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.

Sign up for GitHub

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

[SC-472] Move everything to AccessControl roles #20

Merged
merged 2 commits into from
Jun 14, 2024
Merged

[SC-472] Move everything to AccessControl roles #20

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
999d935
refactor guardian to be part of access control, also queue becomes a …
hexonaut Jun 13, 2024
2e7518d
formatting
hexonaut Jun 13, 2024
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
55 changes: 13 additions & 42 deletions src/Executor.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,15 +15,16 @@ contract Executor is IExecutor, AccessControl {

using Address for address;

bytes32 public constant SUBMISSION_ROLE = keccak256('SUBMISSION_ROLE');
bytes32 public constant GUARDIAN_ROLE = keccak256('GUARDIAN_ROLE');

// Minimum allowed grace period, which reduces the risk of having an actions set expire due to network congestion
uint256 constant MINIMUM_GRACE_PERIOD = 10 minutes;

// Time between queuing and execution
uint256 private _delay;
// Time after the execution time during which the actions set can be executed
uint256 private _gracePeriod;
// Address with the ability of canceling actions sets
address private _guardian;

// Number of actions sets
uint256 private _actionsSetCounter;
Expand All @@ -32,43 +33,28 @@ contract Executor is IExecutor, AccessControl {
// Map of queued actions (actionHash => isQueued)
mapping(bytes32 => bool) private _queuedActions;

/**
* @dev Only guardian can call functions marked by this modifier.
**/
modifier onlyGuardian() {
if (msg.sender != _guardian) revert NotGuardian();
_;
}

/**
* @dev Only this contract can call functions marked by this modifier.
**/
modifier onlyThis() {
if (msg.sender != address(this)) revert OnlyCallableByThis();
_;
}

/**
* @dev Constructor
*
* @param delay The delay before which an actions set can be executed
* @param gracePeriod The time period after a delay during which an actions set can be executed
* @param guardian The address of the guardian, which can cancel queued proposals (can be zero)
*/
constructor(
uint256 delay,
uint256 gracePeriod,
address guardian
uint256 gracePeriod
) {
if (
gracePeriod < MINIMUM_GRACE_PERIOD
) revert InvalidInitParams();

_updateDelay(delay);
_updateGracePeriod(gracePeriod);
_updateGuardian(guardian);

_setRoleAdmin(SUBMISSION_ROLE, DEFAULT_ADMIN_ROLE);
_setRoleAdmin(GUARDIAN_ROLE, DEFAULT_ADMIN_ROLE);

_grantRole(DEFAULT_ADMIN_ROLE, msg.sender);
_grantRole(DEFAULT_ADMIN_ROLE, address(this)); // Necessary for self-referential calls to change configuration
}

/// @inheritdoc IExecutor
Expand All @@ -78,7 +64,7 @@ contract Executor is IExecutor, AccessControl {
string[] memory signatures,
bytes[] memory calldatas,
bool[] memory withDelegatecalls
) external override onlyRole(DEFAULT_ADMIN_ROLE) {
) external override onlyRole(SUBMISSION_ROLE) {
if (targets.length == 0) revert EmptyTargets();
uint256 targetsLength = targets.length;
if (
Expand Down Expand Up @@ -160,7 +146,7 @@ contract Executor is IExecutor, AccessControl {
}

/// @inheritdoc IExecutor
function cancel(uint256 actionsSetId) external override onlyGuardian {
function cancel(uint256 actionsSetId) external override onlyRole(GUARDIAN_ROLE) {
if (getCurrentState(actionsSetId) != ActionsSetState.Queued) revert OnlyQueuedActions();

ActionsSet storage actionsSet = _actionsSets[actionsSetId];
Expand All @@ -185,17 +171,12 @@ contract Executor is IExecutor, AccessControl {
}

/// @inheritdoc IExecutor
function updateGuardian(address guardian) external override onlyThis {
_updateGuardian(guardian);
}

/// @inheritdoc IExecutor
function updateDelay(uint256 delay) external override onlyThis {
function updateDelay(uint256 delay) external override onlyRole(DEFAULT_ADMIN_ROLE) {
_updateDelay(delay);
}

/// @inheritdoc IExecutor
function updateGracePeriod(uint256 gracePeriod) external override onlyThis {
function updateGracePeriod(uint256 gracePeriod) external override onlyRole(DEFAULT_ADMIN_ROLE) {
if (gracePeriod < MINIMUM_GRACE_PERIOD) revert GracePeriodTooShort();
_updateGracePeriod(gracePeriod);
}
Expand All @@ -205,7 +186,7 @@ contract Executor is IExecutor, AccessControl {
external
payable
override
onlyThis
onlyRole(DEFAULT_ADMIN_ROLE)
returns (bytes memory)
{
return target.functionDelegateCall(data);
Expand All @@ -224,11 +205,6 @@ contract Executor is IExecutor, AccessControl {
return _gracePeriod;
}

/// @inheritdoc IExecutor
function getGuardian() external view override returns (address) {
return _guardian;
}

/// @inheritdoc IExecutor
function getActionsSetCount() external view override returns (uint256) {
return _actionsSetCounter;
Expand Down Expand Up @@ -264,11 +240,6 @@ contract Executor is IExecutor, AccessControl {
return _queuedActions[actionHash];
}

function _updateGuardian(address guardian) internal {
emit GuardianUpdate(_guardian, guardian);
_guardian = guardian;
}

function _updateDelay(uint256 delay) internal {
emit DelayUpdate(_delay, delay);
_delay = delay;
Expand Down
36 changes: 10 additions & 26 deletions src/interfaces/IExecutor.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,21 +11,14 @@ import { IAccessControl } from 'openzeppelin-contracts/contracts/access/IAccessC
interface IExecutor is IAccessControl {

error InvalidInitParams();
error NotGuardian();
error OnlyCallableByThis();
error MinimumDelayTooLong();
error MaximumDelayTooShort();
error GracePeriodTooShort();
error DelayShorterThanMin();
error DelayLongerThanMax();
error OnlyQueuedActions();
error TimelockNotFinished();
error InvalidActionsSetId();
error EmptyTargets();
error InconsistentParamsLength();
error DuplicateAction();
error InsufficientBalance();
error FailedActionExecution();

/**
* @notice This enum contains all possible actions set states
Expand Down Expand Up @@ -97,13 +90,6 @@ interface IExecutor is IAccessControl {
**/
event ActionsSetCanceled(uint256 indexed id);

/**
* @dev Emitted when a new guardian is set
* @param oldGuardian The address of the old guardian
* @param newGuardian The address of the new guardian
**/
event GuardianUpdate(address oldGuardian, address newGuardian);

/**
* @dev Emitted when the delay (between queueing and execution) is updated
* @param oldDelay The value of the old delay
Expand All @@ -118,6 +104,16 @@ interface IExecutor is IAccessControl {
**/
event GracePeriodUpdate(uint256 oldGracePeriod, uint256 newGracePeriod);

/**
* @notice The role that allows submission of a queued action.
**/
function SUBMISSION_ROLE() external view returns (bytes32);

/**
* @notice The role that allows a guardian to cancel a pending action.
**/
function GUARDIAN_ROLE() external view returns (bytes32);

/**
* @notice Queue an ActionsSet
* @dev If a signature is empty, calldata is used for the execution, calldata is appended to signature otherwise
Expand Down Expand Up @@ -147,12 +143,6 @@ interface IExecutor is IAccessControl {
**/
function cancel(uint256 actionsSetId) external;

/**
* @notice Update guardian
* @param guardian The address of the new guardian
**/
function updateGuardian(address guardian) external;

/**
* @notice Update the delay, time between queueing and execution of ActionsSet
* @dev It does not affect to actions set that are already queued
Expand Down Expand Up @@ -195,12 +185,6 @@ interface IExecutor is IAccessControl {
**/
function getGracePeriod() external view returns (uint256);

/**
* @notice Returns the address of the guardian
* @return The address of the guardian
**/
function getGuardian() external view returns (address);

/**
* @notice Returns the total number of actions sets of the executor
* @return The number of actions sets
Expand Down
7 changes: 4 additions & 3 deletions test/ArbitrumOneCrosschainTest.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,14 +45,15 @@ contract ArbitrumOneCrosschainTest is CrosschainTestBase {
bridge.destination.selectFork();
bridgeExecutor = new Executor(
defaultL2BridgeExecutorArgs.delay,
defaultL2BridgeExecutorArgs.gracePeriod,
defaultL2BridgeExecutorArgs.guardian
defaultL2BridgeExecutorArgs.gracePeriod
);
bridgeReceiver = address(new ArbitrumReceiver(
defaultL2BridgeExecutorArgs.ethereumGovernanceExecutor,
address(bridgeExecutor)
));
bridgeExecutor.grantRole(bridgeExecutor.DEFAULT_ADMIN_ROLE(), bridgeReceiver);
bridgeExecutor.grantRole(bridgeExecutor.SUBMISSION_ROLE(), bridgeReceiver);
bridgeExecutor.grantRole(bridgeExecutor.GUARDIAN_ROLE(), defaultL2BridgeExecutorArgs.guardian);
bridgeExecutor.revokeRole(bridgeExecutor.DEFAULT_ADMIN_ROLE(), address(this));

bridge.source.selectFork();
vm.deal(L1_EXECUTOR, 0.01 ether);
Expand Down
7 changes: 4 additions & 3 deletions test/BaseChainCrosschainTest.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,14 +43,15 @@ contract BaseChainCrosschainTest is CrosschainTestBase {
bridge.destination.selectFork();
bridgeExecutor = new Executor(
defaultL2BridgeExecutorArgs.delay,
defaultL2BridgeExecutorArgs.gracePeriod,
defaultL2BridgeExecutorArgs.guardian
defaultL2BridgeExecutorArgs.gracePeriod
);
bridgeReceiver = address(new OptimismReceiver(
defaultL2BridgeExecutorArgs.ethereumGovernanceExecutor,
address(bridgeExecutor)
));
bridgeExecutor.grantRole(bridgeExecutor.DEFAULT_ADMIN_ROLE(), bridgeReceiver);
bridgeExecutor.grantRole(bridgeExecutor.SUBMISSION_ROLE(), bridgeReceiver);
bridgeExecutor.grantRole(bridgeExecutor.GUARDIAN_ROLE(), defaultL2BridgeExecutorArgs.guardian);
bridgeExecutor.revokeRole(bridgeExecutor.DEFAULT_ADMIN_ROLE(), address(this));

bridge.source.selectFork();
}
Expand Down
31 changes: 20 additions & 11 deletions test/CrosschainTestBase.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -306,6 +306,13 @@ abstract contract CrosschainTestBase is Test {
function test_selfReconfiguration() public {
bridge.destination.selectFork();

L2BridgeExecutorArguments memory newL2BridgeExecutorParams = L2BridgeExecutorArguments({
ethereumGovernanceExecutor: defaultL2BridgeExecutorArgs.ethereumGovernanceExecutor,
delay: 1200,
gracePeriod: 1800,
guardian: makeAddr('newGuardian')
});

assertEq(
bridgeExecutor.getDelay(),
defaultL2BridgeExecutorArgs.delay
Expand All @@ -315,20 +322,18 @@ abstract contract CrosschainTestBase is Test {
defaultL2BridgeExecutorArgs.gracePeriod
);
assertEq(
bridgeExecutor.getGuardian(),
defaultL2BridgeExecutorArgs.guardian
bridgeExecutor.hasRole(bridgeExecutor.GUARDIAN_ROLE(), defaultL2BridgeExecutorArgs.guardian),
true
);
assertEq(
bridgeExecutor.hasRole(bridgeExecutor.GUARDIAN_ROLE(), newL2BridgeExecutorParams.guardian),
false
);

L2BridgeExecutorArguments memory newL2BridgeExecutorParams = L2BridgeExecutorArguments({
ethereumGovernanceExecutor: defaultL2BridgeExecutorArgs.ethereumGovernanceExecutor,
delay: 1200,
gracePeriod: 1800,
guardian: makeAddr('newGuardian')
});

IPayload reconfigurationPayload = IPayload(new ReconfigurationPayload(
newL2BridgeExecutorParams.delay,
newL2BridgeExecutorParams.gracePeriod,
defaultL2BridgeExecutorArgs.guardian,
newL2BridgeExecutorParams.guardian
));

Expand Down Expand Up @@ -360,8 +365,12 @@ abstract contract CrosschainTestBase is Test {
newL2BridgeExecutorParams.gracePeriod
);
assertEq(
bridgeExecutor.getGuardian(),
newL2BridgeExecutorParams.guardian
bridgeExecutor.hasRole(bridgeExecutor.GUARDIAN_ROLE(), defaultL2BridgeExecutorArgs.guardian),
false
);
assertEq(
bridgeExecutor.hasRole(bridgeExecutor.GUARDIAN_ROLE(), newL2BridgeExecutorParams.guardian),
true
);
}

Expand Down
Loading
Loading