unify execution for multisigs

PufferFinance · May 31, 2024 · a5f243a · a5f243a
1 parent 8faf441
commit a5f243a
Show file tree

Hide file tree

Showing 3 changed files with 22 additions and 38 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 security@puffer.fi
  */
 contract Timelock {
+    using Address for address;
+
     /**
      * @notice Error to be thrown when a bad address is encountered
      */
@@ -33,11 +36,6 @@ contract Timelock {
      */
     error Locked(bytes32 txHash, uint256 lockedUntil);
 
-    /**
-     * @notice Error to be thrown when an the target call fails
-     */
-    error ExecutionFailedAtTarget();
-
     /**
      * @notice Error to be thrown when an the params are invalid
      */
@@ -226,45 +224,33 @@ 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)
     {
         bytes32 txHash = keccak256(abi.encode(target, callData, operationId));
         uint256 lockedUntil = queue[txHash];
         queue[txHash] = 0;
 
-        // 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) {
+        if (msg.sender == OPERATIONS_MULTISIG) {
+            // Operations Multisig must follow queue and delay rules
+            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();
         }
 
-        // slither-disable-next-line incorrect-equality
-        if (lockedUntil == 0) {
-            revert InvalidTransaction(txHash);
-        }
-
-        if (block.timestamp < lockedUntil) {
-            revert Locked(txHash, lockedUntil);
-        }
-
-        (success, returnData) = _executeTransaction(target, callData);
-
-        if (!success) {
-            revert ExecutionFailedAtTarget();
-        }
+        // Execute the transaction
+        returnData = _executeTransaction(target, callData);
 
         emit TransactionExecuted(txHash, target, callData, operationId);
-
-        return (success, returnData);
     }
 
     /**
@@ -305,9 +291,9 @@ contract Timelock {
         delay = newDelay;
     }
 
-    function _executeTransaction(address target, bytes calldata callData) internal returns (bool, bytes memory) {
+    function _executeTransaction(address target, bytes calldata callData) internal returns (bytes memory) {
         // slither-disable-next-line arbitrary-send-eth
-        return target.call(callData);
+        return target.functionCall(callData);
     }
 
     function _validateAddresses(

diff --git a/test/unit/Timelock.t.sol b/test/unit/Timelock.t.sol
@@ -199,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);
 
@@ -310,7 +308,7 @@ contract TimelockTest is Test {
 
         uint256 gasToUse = 214_640;
 
-        vm.expectRevert(abi.encodeWithSelector(Timelock.ExecutionFailedAtTarget.selector));
+        vm.expectRevert();
         timelock.executeTransaction{ gas: gasToUse }(address(this), callData, operationId);
     }