Skip to content

Commit

Permalink
Merge pull request #20 from marsfoundation/SC-472-guardian-merge-into…
Browse files Browse the repository at this point in the history 
…-access-control

[SC-472] Move everything to AccessControl roles
  • Loading branch information
@hexonaut
hexonaut authored Jun 14, 2024
2 parents 758da27 + 2e7518d commit 56bfd0a
Show file tree
Hide file tree
Showing 9 changed files with 86 additions and 134 deletions.
55 changes: 13 additions & 42 deletions src/Executor.sol
View file
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
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
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
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
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

0 comments on commit 56bfd0a

Please sign in to comment.