From 122e116ee05eaf0babed90293903aecf954c07cf Mon Sep 17 00:00:00 2001 From: Walid Date: Fri, 31 May 2024 10:29:31 -0700 Subject: [PATCH] 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 --- script/Roles.sol | 2 +- src/Timelock.sol | 88 +++++++++++++++++++++++++++------------- test/unit/Timelock.t.sol | 85 ++++++++++++++++++++++++++++++++++++-- 3 files changed, 142 insertions(+), 33 deletions(-) diff --git a/script/Roles.sol b/script/Roles.sol index 38d63c3..05f1994 100644 --- 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 index 7eb80ac..ff04021 100644 --- a/src/Timelock.sol +++ b/src/Timelock.sol @@ -2,6 +2,7 @@ pragma solidity >=0.8.18; import { AccessManager } from "openzeppelin/access/manager/AccessManager.sol"; +import { Address } from "@openzeppelin/contracts/utils/Address.sol"; /** * @title Timelock @@ -9,6 +10,8 @@ import { AccessManager } from "openzeppelin/access/manager/AccessManager.sol"; * @custom:security-contact security@puffer.fi */ 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 index c3b362d..fa3b846 100644 --- 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) + } + } + } }