Timelock Fixes (#80)

* Timelock Fixes * remove constant * adds selector pausing logic * checks for txHash on cancelation * unify execution for multisigs * slither fix * opitmization * slither fix
PufferFinance · May 31, 2024 · 122e116 · 122e116
1 parent d73e83f
commit 122e116
Show file tree

Hide file tree

Showing 3 changed files with 142 additions and 33 deletions.
diff --git a/script/Roles.sol b/script/Roles.sol
@@ -9,7 +9,7 @@ uint64 constant ROLE_ID_UPGRADER = 1;
 // Role assigned to Operations Multisig
 uint64 constant ROLE_ID_OPERATIONS_MULTISIG = 22;
 uint64 constant ROLE_ID_OPERATIONS_PAYMASTER = 23;
-uint64 constant ROLE_ID_OPERATIONS_COORDINATOR= 24;
+uint64 constant ROLE_ID_OPERATIONS_COORDINATOR = 24;
 
 // Role assigned to validator ticket price setter
 uint64 constant ROLE_ID_VT_PRICER = 25;

diff --git a/src/Timelock.sol b/src/Timelock.sol
@@ -2,13 +2,16 @@
 pragma solidity >=0.8.18;
 
 import { AccessManager } from "openzeppelin/access/manager/AccessManager.sol";
+import { Address } from "@openzeppelin/contracts/utils/Address.sol";
 
 /**
  * @title Timelock
  * @author Puffer Finance
  * @custom:security-contact [email protected]
  */
 contract Timelock {
+    using Address for address;
+
     /**
      * @notice Error to be thrown when a bad address is encountered
      */
@@ -33,6 +36,11 @@ contract Timelock {
      */
     error Locked(bytes32 txHash, uint256 lockedUntil);
 
+    /**
+     * @notice Error to be thrown when an the params are invalid
+     */
+    error InvalidParams();
+
     /**
      * @notice Emitted when the delay changes from `oldDelay` to `newDelay`
      */
@@ -155,12 +163,40 @@ contract Timelock {
         bytes[] memory callDatas = new bytes[](targets.length);
 
         for (uint256 i = 0; i < targets.length; ++i) {
+            // slither-disable-next-line calls-loop
             callDatas[i] = abi.encodeCall(AccessManager.setTargetClosed, (targets[i], true));
         }
 
         ACCESS_MANAGER.multicall(callDatas);
     }
 
+    /**
+     * @notice Pauses the system by closing access to specified targets selectors
+     * @param targets An array of addresses to which access will be paused
+     * @param selectors An array of selectors to which access will be paused
+     */
+    function pauseSelectors(address[] calldata targets, bytes4[][] calldata selectors) public {
+        if (targets.length != selectors.length) {
+            revert InvalidParams();
+        }
+
+        // Community multisig can call this by via executeTransaction
+        if (msg.sender != pauserMultisig && msg.sender != address(this)) {
+            revert Unauthorized();
+        }
+
+        bytes[] memory callDatas = new bytes[](targets.length);
+
+        for (uint256 i = 0; i < targets.length; ++i) {
+            // slither-disable-next-line calls-loop
+            callDatas[i] = abi.encodeCall(
+                AccessManager.setTargetFunctionRole, (targets[i], selectors[i], ACCESS_MANAGER.ADMIN_ROLE())
+            );
+        }
+
+        ACCESS_MANAGER.multicall(callDatas);
+    }
+
     /**
      * @notice Cancels a queued transaction
      * @param target The address to which the transaction was to be sent
@@ -174,6 +210,12 @@ contract Timelock {
         }
 
         bytes32 txHash = keccak256(abi.encode(target, callData, operationId));
+
+        // slither-disable-next-line incorrect-equality
+        if (queue[txHash] == 0) {
+            revert InvalidTransaction(txHash);
+        }
+
         queue[txHash] = 0;
 
         emit TransactionCanceled(txHash, target, callData, operationId);
@@ -185,41 +227,36 @@ contract Timelock {
      * @param target The address to which the transaction will be sent
      * @param callData The data to be sent along with the transaction
      * @param operationId The id of the operation used to identify the transaction
-     * @return success A boolean indicating whether the transaction was successful
      * @return returnData The data returned by the transaction
      */
     function executeTransaction(address target, bytes calldata callData, uint256 operationId)
         external
-        returns (bool success, bytes memory returnData)
+        returns (bytes memory returnData)
     {
-        // Community Multisig can do things without any delay
-        if (msg.sender == COMMUNITY_MULTISIG) {
-            return _executeTransaction(target, callData);
-        }
-
-        // Operations multisig needs to queue it and then execute after a delay
-        if (msg.sender != OPERATIONS_MULTISIG) {
-            revert Unauthorized();
-        }
-
         bytes32 txHash = keccak256(abi.encode(target, callData, operationId));
-        uint256 lockedUntil = queue[txHash];
 
-        // slither-disable-next-line incorrect-equality
-        if (lockedUntil == 0) {
-            revert InvalidTransaction(txHash);
-        }
-
-        if (block.timestamp < lockedUntil) {
-            revert Locked(txHash, lockedUntil);
+        if (msg.sender == OPERATIONS_MULTISIG) {
+            uint256 lockedUntil = queue[txHash];
+            // Operations Multisig must follow queue and delay rules
+            // slither-disable-next-line incorrect-equality
+            if (lockedUntil == 0) {
+                revert InvalidTransaction(txHash);
+            }
+            if (block.timestamp < lockedUntil) {
+                revert Locked(txHash, lockedUntil);
+            }
+        } else if (msg.sender != COMMUNITY_MULTISIG) {
+            // All other senders are unauthorized
+            revert Unauthorized();
         }
 
         queue[txHash] = 0;
-        (success, returnData) = _executeTransaction(target, callData);
 
-        emit TransactionExecuted(txHash, target, callData, operationId);
+        // Execute the transaction
+        // slither-disable-next-line arbitrary-send-eth
+        returnData = target.functionCall(callData);
 
-        return (success, returnData);
+        emit TransactionExecuted(txHash, target, callData, operationId);
     }
 
     /**
@@ -260,11 +297,6 @@ contract Timelock {
         delay = newDelay;
     }
 
-    function _executeTransaction(address target, bytes calldata callData) internal returns (bool, bytes memory) {
-        // slither-disable-next-line arbitrary-send-eth
-        return target.call(callData);
-    }
-
     function _validateAddresses(
         address communityMultisig,
         address operationsMultisig,

diff --git a/test/unit/Timelock.t.sol b/test/unit/Timelock.t.sol
@@ -132,6 +132,29 @@ contract TimelockTest is Test {
 
         vm.expectRevert(abi.encodeWithSelector(Timelock.InvalidTransaction.selector, txHash));
         timelock.executeTransaction(address(timelock), callData, operationId);
+
+        vm.expectRevert(abi.encodeWithSelector(Timelock.InvalidTransaction.selector, txHash));
+        timelock.cancelTransaction(address(timelock), callData, operationId);
+    }
+
+    function test_community_transaction() public {
+        vm.startPrank(timelock.OPERATIONS_MULTISIG());
+
+        bytes memory callData = abi.encodeCall(Timelock.setDelay, (15 days));
+
+        uint256 operationId = 1234;
+
+        bytes32 txHash = timelock.queueTransaction(address(timelock), callData, operationId);
+
+        uint256 lockedUntil = block.timestamp + timelock.delay();
+
+        assertTrue(timelock.queue(txHash) != 0, "queued");
+
+        vm.startPrank(timelock.COMMUNITY_MULTISIG());
+        timelock.executeTransaction(address(timelock), callData, operationId);
+
+        assertEq(timelock.queue(txHash), 0, "executed");
+        assertEq(timelock.delay(), 15 days, "updated the delay");
     }
 
     function test_cancel_reverts_if_caller_unauthorized(address caller) public {
@@ -176,10 +199,8 @@ contract TimelockTest is Test {
         uint256 operationId = 1234;
 
         // revert if the timelock is too small
-        (bool success, bytes memory returnData) =
-            timelock.executeTransaction(address(timelock), tooSmallDelayCallData, operationId);
-        assertEq(returnData, abi.encodeWithSelector(Timelock.InvalidDelay.selector, 1 days), "return data should fail");
-        assertFalse(success, "should fail");
+        vm.expectRevert(abi.encodeWithSelector(Timelock.InvalidDelay.selector, 1 days));
+        bytes memory returnData = timelock.executeTransaction(address(timelock), tooSmallDelayCallData, operationId);
 
         timelock.executeTransaction(address(timelock), callData, 1234);
 
@@ -229,6 +250,33 @@ contract TimelockTest is Test {
         assertTrue(!canCall, "should not be able to call");
     }
 
+    function test_pause_depositor_slectors(address caller) public {
+        vm.startPrank(timelock.pauserMultisig());
+        vm.assume(caller != address(timelock));
+
+        address[] memory targets = new address[](1);
+        targets[0] = address(pufferDepositor);
+
+        bytes4[][] memory selectors = new bytes4[][](1);
+        selectors[0] = new bytes4[](1);
+
+        selectors[0][0] = PufferDepositor.swapAndDeposit.selector;
+
+        timelock.pauseSelectors(targets, selectors);
+
+        (bool canCall, uint32 delay) =
+            accessManager.canCall(caller, address(pufferDepositor), PufferDepositor.swapAndDeposit.selector);
+        assertTrue(!canCall, "should not be able to call");
+
+        (canCall, delay) =
+            accessManager.canCall(caller, address(pufferDepositor), PufferDepositor.swapAndDepositWithPermit.selector);
+        assertTrue(canCall, "should able to call");
+
+        (canCall, delay) =
+            accessManager.canCall(caller, address(pufferDepositor), PufferDepositor.depositWstETH.selector);
+        assertTrue(canCall, "should be able to call");
+    }
+
     function test_change_pauser() public {
         vm.startPrank(timelock.COMMUNITY_MULTISIG());
 
@@ -244,4 +292,33 @@ contract TimelockTest is Test {
 
         assertEq(newPauser, timelock.pauserMultisig(), "pauser did not change");
     }
+
+    function test_execute_fails_due_to_gas() public {
+        vm.startPrank(timelock.OPERATIONS_MULTISIG());
+
+        bytes memory callData = abi.encodeCall(this.gasConsumingFunc, ());
+
+        uint256 operationId = 1234;
+
+        bytes32 txHash = timelock.queueTransaction(address(this), callData, operationId);
+
+        uint256 lockedUntil = block.timestamp + timelock.delay();
+
+        vm.warp(lockedUntil + 20);
+
+        uint256 gasToUse = 214_640;
+
+        vm.expectRevert();
+        timelock.executeTransaction{ gas: gasToUse }(address(this), callData, operationId);
+    }
+
+    function gasConsumingFunc() external {
+        uint256 gasToConsume = 209595;
+        uint256 gasStart = gasleft();
+        for (uint256 i = 0; gasStart - gasleft() < gasToConsume; i++) {
+            assembly {
+                let x := mload(0x1337)
+            }
+        }
+    }
 }