diff --git a/packages/contracts-bedrock/src/safe/TimelockGuard.sol b/packages/contracts-bedrock/src/safe/TimelockGuard.sol index f87ba843dbc..7654943921a 100644 --- a/packages/contracts-bedrock/src/safe/TimelockGuard.sol +++ b/packages/contracts-bedrock/src/safe/TimelockGuard.sol @@ -5,6 +5,7 @@ pragma solidity 0.8.15; import { GnosisSafe as Safe } from "safe-contracts/GnosisSafe.sol"; import { Enum } from "safe-contracts/common/Enum.sol"; import { GuardManager, Guard as IGuard } from "safe-contracts/base/GuardManager.sol"; +import { ExecTransactionParams } from "src/safe/Types.sol"; // Interfaces import { ISemver } from "interfaces/universal/ISemver.sol"; @@ -20,12 +21,21 @@ contract TimelockGuard is IGuard, ISemver { uint256 timelockDelay; } + /// @notice Scheduled transaction + struct ScheduledTransaction { + uint256 executionTime; + bool cancelled; + } + /// @notice Mapping from Safe address to its guard configuration mapping(address => GuardConfig) public safeConfigs; /// @notice Mapping from Safe address to its current cancellation threshold mapping(address => uint256) public safeCancellationThreshold; + /// @notice Mapping from Safe and tx id to scheduled transaction. + mapping(Safe => mapping(bytes32 => ScheduledTransaction)) public scheduledTransactions; + /// @notice Error for when guard is not enabled for the Safe error TimelockGuard_GuardNotEnabled(); @@ -38,12 +48,21 @@ contract TimelockGuard is IGuard, ISemver { /// @notice Error for invalid timelock delay error TimelockGuard_InvalidTimelockDelay(); + /// @notice Error for when a transaction is already scheduled + error TimelockGuard_TransactionAlreadyScheduled(); + /// @notice Emitted when a Safe configures the guard event GuardConfigured(address indexed safe, uint256 timelockDelay); /// @notice Emitted when a Safe clears the guard configuration event GuardCleared(address indexed safe); + /// @notice Emitted when a transaction is scheduled for a Safe. + /// @param safe The Safe whose transaction is scheduled. + /// @param txId The identifier of the scheduled transaction (nonce-independent). + /// @param when The timestamp when execution becomes valid. + event TransactionScheduled(Safe indexed safe, bytes32 indexed txId, uint256 when); + /// @notice Semantic version. /// @custom:semver 1.0.0 string public constant version = "1.0.0"; @@ -131,25 +150,65 @@ contract TimelockGuard is IGuard, ISemver { return guard == address(this); } - /// @notice Schedule a transaction for execution after the timelock delay - /// @dev Called by anyone using signatures from Safe owners - NOT IMPLEMENTED YET - function scheduleTransaction( - address, - address, - uint256, - bytes memory, - Enum.Operation, - uint256, - uint256, - uint256, - address, - address payable, - bytes memory - ) - external - pure - { - // TODO: Implement + /// @notice Schedule a transaction for execution after the timelock delay. + /// @dev Minimal implementation: checks enabled+configured, uniqueness, cancellation, stores execution time and + /// emits. + /// @dev The txId is computed independent of Safe nonce using all exec params (with keccak(data)). + function scheduleTransaction(Safe _safe, uint256 _nonce, ExecTransactionParams memory _params) external { + // Check that this guard is enabled on the calling Safe + if (!_isGuardEnabled(address(_safe))) { + revert TimelockGuard_GuardNotEnabled(); + } + + // Get the encoded transaction data as defined in the Safe + // The format of the string returned is: "0x1901{domainSeparator}{safeTxHash}" + bytes memory txHashData = _safe.encodeTransactionData( + _params.to, + _params.value, + _params.data, + _params.operation, + _params.safeTxGas, + _params.baseGas, + _params.gasPrice, + _params.gasToken, + _params.refundReceiver, + _nonce + ); + + // Get the transaction hash and data as defined in the Safe + // This value is identical to keccak256(txHashData), but we prefer to use the Safe's own + // internal logic as it is more future-proof in case future versions of the Safe change + // the transaction hash derivation. + bytes32 txHash = _safe.getTransactionHash( + _params.to, + _params.value, + _params.data, + _params.operation, + _params.safeTxGas, + _params.baseGas, + _params.gasPrice, + _params.gasToken, + _params.refundReceiver, + _nonce + ); + + // Verify signatures using the Safe's signature checking logic + // This function call reverts if the signatures are invalid. + _safe.checkSignatures(txHash, txHashData, _params.signatures); + + // Check if the transaction exists + // A transaction can only be scheduled once, regardless of whether it has been cancelled or not. + if (scheduledTransactions[_safe][txHash].executionTime != 0) { + revert TimelockGuard_TransactionAlreadyScheduled(); + } + + // Calculate the execution time + uint256 executionTime = block.timestamp + safeConfigs[address(_safe)].timelockDelay; + + // Schedule the transaction + scheduledTransactions[_safe][txHash] = ScheduledTransaction({ executionTime: executionTime, cancelled: false }); + + emit TransactionScheduled(_safe, txHash, executionTime); } /// @notice Returns the list of all scheduled but not cancelled transactions for a given safe diff --git a/packages/contracts-bedrock/src/safe/Types.sol b/packages/contracts-bedrock/src/safe/Types.sol new file mode 100644 index 00000000000..d628d5d293e --- /dev/null +++ b/packages/contracts-bedrock/src/safe/Types.sol @@ -0,0 +1,19 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.0; + +import { Enum } from "safe-contracts/common/Enum.sol"; + +/// @notice Parameters for the Safe's execTransaction function +struct ExecTransactionParams { + address to; + uint256 value; + bytes data; + Enum.Operation operation; + uint256 safeTxGas; + uint256 baseGas; + uint256 gasPrice; + address gasToken; + address payable refundReceiver; + // TODO: Life might be easier if this was left out of the struct + bytes signatures; +} diff --git a/packages/contracts-bedrock/test/safe-tools/SafeTestTools.sol b/packages/contracts-bedrock/test/safe-tools/SafeTestTools.sol index fa86111dd79..332039b5cf6 100644 --- a/packages/contracts-bedrock/test/safe-tools/SafeTestTools.sol +++ b/packages/contracts-bedrock/test/safe-tools/SafeTestTools.sol @@ -191,6 +191,8 @@ library SafeTestLib { refundReceiver: refundReceiver, _nonce: _nonce }); + console.log('txDataHash:'); + console.logBytes32(txDataHash); } (v, r, s) = Vm(VM_ADDR).sign(pk, txDataHash); diff --git a/packages/contracts-bedrock/test/safe/TimelockGuard.t.sol b/packages/contracts-bedrock/test/safe/TimelockGuard.t.sol index 8be77e3b7e3..d289b000359 100644 --- a/packages/contracts-bedrock/test/safe/TimelockGuard.t.sol +++ b/packages/contracts-bedrock/test/safe/TimelockGuard.t.sol @@ -2,11 +2,15 @@ pragma solidity 0.8.15; import { Test } from "forge-std/Test.sol"; +import { GnosisSafe as Safe } from "safe-contracts/GnosisSafe.sol"; import { Enum } from "safe-contracts/common/Enum.sol"; import { GuardManager } from "safe-contracts/base/GuardManager.sol"; import { StorageAccessible } from "safe-contracts/common/StorageAccessible.sol"; +import { ExecTransactionParams } from "src/safe/Types.sol"; import "test/safe-tools/SafeTestTools.sol"; +import { console2 as console } from "forge-std/console2.sol"; + import { TimelockGuard } from "src/safe/TimelockGuard.sol"; /// @title TimelockGuard_TestInit @@ -17,6 +21,7 @@ contract TimelockGuard_TestInit is Test, SafeTestTools { // Events event GuardConfigured(address indexed safe, uint256 timelockDelay); event GuardCleared(address indexed safe); + event TransactionScheduled(Safe indexed safe, bytes32 indexed txId, uint256 when); uint256 constant INIT_TIME = 10; uint256 constant TIMELOCK_DELAY = 7 days; @@ -26,9 +31,7 @@ contract TimelockGuard_TestInit is Test, SafeTestTools { TimelockGuard timelockGuard; SafeInstance safeInstance; - SafeInstance safeInstance2; - address[] owners; - uint256[] ownerPKs; + SafeInstance unguardedSafe; function setUp() public virtual { vm.warp(INIT_TIME); @@ -37,12 +40,14 @@ contract TimelockGuard_TestInit is Test, SafeTestTools { timelockGuard = new TimelockGuard(); // Create Safe owners - (address[] memory _owners, uint256[] memory _keys) = SafeTestLib.makeAddrsAndKeys("owners", NUM_OWNERS); - owners = _owners; - ownerPKs = _keys; + (, uint256[] memory keys) = SafeTestLib.makeAddrsAndKeys("owners", NUM_OWNERS); // Set up Safe with owners - safeInstance = _setupSafe(ownerPKs, THRESHOLD); + safeInstance = _setupSafe(keys, THRESHOLD); + + // Safe without guard enabled + // Reduce the threshold just to prevent a CREATE2 collision when deploying this safe. + unguardedSafe = _setupSafe(keys, THRESHOLD - 1); // Enable the guard on the Safe SafeTestLib.execTransaction( @@ -54,6 +59,54 @@ contract TimelockGuard_TestInit is Test, SafeTestTools { ); } + /// @notice Helper to create a dummy transaction with signatures and a tx hash + // TODO: separate into two functions: one for the params+hash, one for the signatures + function _getDummyTx() internal view returns (ExecTransactionParams memory, bytes32) { + // Get the nonce of the safe to sign + uint256 nonce = safeInstance.safe.nonce(); + + // Declare the dummy transaction params with an empty signature + ExecTransactionParams memory dummyTxParams = ExecTransactionParams({ + to: address(0xabba), + value: 0, + data: hex"acdc", + operation: Enum.Operation.Call, + safeTxGas: 0, + baseGas: 0, + gasPrice: 0, + gasToken: address(0), + refundReceiver: payable(address(0)), + signatures: new bytes(0) + }); + + // Get the tx hash + bytes32 txHash; + { + txHash = safeInstance.safe.getTransactionHash({ + to: dummyTxParams.to, + value: dummyTxParams.value, + data: dummyTxParams.data, + operation: dummyTxParams.operation, + safeTxGas: dummyTxParams.safeTxGas, + baseGas: dummyTxParams.baseGas, + gasPrice: dummyTxParams.gasPrice, + gasToken: dummyTxParams.gasToken, + refundReceiver: dummyTxParams.refundReceiver, + _nonce: nonce + }); + } + + // Sign the tx hash with the owners' private keys + for (uint256 i; i < THRESHOLD; ++i) { + (uint8 v, bytes32 r, bytes32 s) = vm.sign(safeInstance.ownerPKs[i], txHash); + + // The signature format is a compact form of: {bytes32 r}{bytes32 s}{uint8 v} + dummyTxParams.signatures = bytes.concat(dummyTxParams.signatures, abi.encodePacked(r, s, v)); + } + + return (dummyTxParams, txHash); + } + /// @notice Helper to configure the TimelockGuard for a Safe function _configureGuard(SafeInstance memory _safe, uint256 _delay) internal { SafeTestLib.execTransaction( @@ -103,10 +156,6 @@ contract TimelockGuard_ConfigureTimelockGuard_Test is TimelockGuard_TestInit { } function test_configureTimelockGuard_revertsIfGuardNotEnabled_reverts() external { - // Create a safe without enabling the guard - // Reduce the threshold just to prevent a CREATE2 collision when deploying this safe. - SafeInstance memory unguardedSafe = _setupSafe(ownerPKs, THRESHOLD - 1); - vm.expectRevert(TimelockGuard.TimelockGuard_GuardNotEnabled.selector); vm.prank(address(unguardedSafe.safe)); timelockGuard.configureTimelockGuard(TIMELOCK_DELAY); @@ -192,14 +241,11 @@ contract TimelockGuard_ClearTimelockGuard_Test is TimelockGuard_TestInit { /// @notice Tests for cancellationThreshold function contract TimelockGuard_CancellationThreshold_Test is TimelockGuard_TestInit { function test_cancellationThreshold_returnsZeroIfGuardNotEnabled_succeeds() external { - // Safe without guard enabled should return 0 - SafeInstance memory unguardedSafe = _setupSafe(ownerPKs, THRESHOLD - 1); - uint256 threshold = timelockGuard.cancellationThreshold(address(unguardedSafe.safe)); assertEq(threshold, 0); } - function test_cancellationThreshold_returnsZeroIfGuardNotConfigured_succeeds() external { + function test_cancellationThreshold_returnsZeroIfGuardNotConfigured_succeeds() external view { // Safe with guard enabled but not configured should return 0 uint256 threshold = timelockGuard.cancellationThreshold(address(safeInstance.safe)); assertEq(threshold, 0); @@ -217,3 +263,36 @@ contract TimelockGuard_CancellationThreshold_Test is TimelockGuard_TestInit { // Note: Testing increment/decrement behavior will require scheduleTransaction, // cancelTransaction and execution functions to be implemented first } + +/// @title TimelockGuard_ScheduleTransaction_Test +/// @notice Tests for scheduleTransaction function +contract TimelockGuard_ScheduleTransaction_Test is TimelockGuard_TestInit { + function setUp() public override { + super.setUp(); + _configureGuard(safeInstance, TIMELOCK_DELAY); + } + + function test_scheduleTransaction_succeeds() public { + (ExecTransactionParams memory dummyTxParams, bytes32 txHash) = _getDummyTx(); + + vm.expectEmit(true, true, true, true); + emit TransactionScheduled(safeInstance.safe, txHash, INIT_TIME + TIMELOCK_DELAY); + timelockGuard.scheduleTransaction(safeInstance.safe, safeInstance.safe.nonce(), dummyTxParams); + } + + function test_scheduleTransaction_reschedulingIdenticalTransaction_reverts() external { + (ExecTransactionParams memory dummyTxParams,) = _getDummyTx(); + timelockGuard.scheduleTransaction(safeInstance.safe, safeInstance.safe.nonce(), dummyTxParams); + + vm.expectRevert(TimelockGuard.TimelockGuard_TransactionAlreadyScheduled.selector); + timelockGuard.scheduleTransaction(safeInstance.safe, safeInstance.safe.nonce(), dummyTxParams); + } + + function test_scheduleTransaction_identicalPreviouslyCancelled_reverts() external { } + + function test_scheduleTransaction_guardNotEnabled_reverts() external { } + + function test_scheduleTransaction_guardNotConfigured_reverts() external { } + + function test_scheduleTransaction_canScheduleIdenticalWithSalt_succeeds() external { } +}