Spearbit Audit Changes (#25)

* admin is already default admin role by default * merge dupe grace period check * spacing * remove unused param * better wording on comment
sparkdotfi · Aug 22, 2024 · 5c16676 · 5c16676
1 parent 38da912
commit 5c16676
Show file tree

Hide file tree

Showing 3 changed files with 4 additions and 14 deletions.
diff --git a/src/Executor.sol b/src/Executor.sol
@@ -40,16 +40,9 @@ contract Executor is IExecutor, AccessControl {
         uint256 delay_,
         uint256 gracePeriod_
     ) {
-        if (
-            gracePeriod_ < MINIMUM_GRACE_PERIOD
-        ) revert InvalidInitParams();
-
         _updateDelay(delay_);
         _updateGracePeriod(gracePeriod_);
 
-        _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
     }
@@ -120,7 +113,6 @@ contract Executor is IExecutor, AccessControl {
                 actionsSet.values[i],
                 actionsSet.signatures[i],
                 actionsSet.calldatas[i],
-                actionsSet.executionTime,
                 actionsSet.withDelegatecalls[i]
             );
         }
@@ -151,7 +143,6 @@ contract Executor is IExecutor, AccessControl {
     function updateGracePeriod(uint256 newGracePeriod)
         external override onlyRole(DEFAULT_ADMIN_ROLE)
     {
-        if (newGracePeriod < MINIMUM_GRACE_PERIOD) revert GracePeriodTooShort();
         _updateGracePeriod(newGracePeriod);
     }
 
@@ -191,7 +182,7 @@ contract Executor is IExecutor, AccessControl {
     function getCurrentState(uint256 actionsSetId) public view override returns (ActionsSetState) {
         if (actionsSetCount <= actionsSetId) revert InvalidActionsSetId();
 
-        ActionsSet storage actionsSet =_actionsSets[actionsSetId];
+        ActionsSet storage actionsSet = _actionsSets[actionsSetId];
 
         if      (actionsSet.canceled) return ActionsSetState.Canceled;
         else if (actionsSet.executed) return ActionsSetState.Executed;
@@ -208,7 +199,6 @@ contract Executor is IExecutor, AccessControl {
         uint256 value,
         string memory signature,
         bytes memory data,
-        uint256 executionTime,
         bool withDelegatecall
     ) internal returns (bytes memory) {
         if (address(this).balance < value) revert InsufficientBalance();
@@ -232,6 +222,7 @@ contract Executor is IExecutor, AccessControl {
     }
 
     function _updateGracePeriod(uint256 newGracePeriod) internal {
+        if (newGracePeriod < MINIMUM_GRACE_PERIOD) revert GracePeriodTooShort();
         emit GracePeriodUpdate(gracePeriod, newGracePeriod);
         gracePeriod = newGracePeriod;
     }

diff --git a/src/interfaces/IExecutor.sol b/src/interfaces/IExecutor.sol
@@ -14,7 +14,6 @@ interface IExecutor is IAccessControl {
     /*** Errors                                                                                                     ***/
     /******************************************************************************************************************/
 
-    error InvalidInitParams();
     error GracePeriodTooShort();
     error OnlyQueuedActions();
     error TimelockNotFinished();
@@ -161,7 +160,7 @@ interface IExecutor is IAccessControl {
      * @dev    If a signature is empty, calldata is used for the execution, calldata is appended to signature otherwise.
      * @param  targets           Array of targets to be called by the actions set.
      * @param  values            Array of values to pass in each call by the actions set.
-     * @param  signatures        Array of function signatures to encode in each call by the actions (can be empty).
+     * @param  signatures        Array of function signatures to encode in each call by the actions which can be empty strings.
      * @param  calldatas         Array of calldata to pass in each call by the actions set.
      * @param  withDelegatecalls Array of whether to delegatecall for each call of the actions set.
      **/

diff --git a/test/Executor.t.sol b/test/Executor.t.sol
@@ -137,7 +137,7 @@ contract ExecutorTestBase is Test {
 contract ExecutorConstructorTests is ExecutorTestBase {
 
     function test_constructor_invalidInitParams_boundary() public {
-        vm.expectRevert(abi.encodeWithSignature("InvalidInitParams()"));
+        vm.expectRevert(abi.encodeWithSignature("GracePeriodTooShort()"));
         executor = new Executor({
             delay_:       DELAY,
             gracePeriod_: 10 minutes - 1