Skip to content

Commit

Permalink
♻️ Timelock fixes (#1231)
Browse files Browse the repository at this point in the history 
Co-authored-by: rholterhus <[email protected]>
  • Loading branch information
@Vectorized @rholterhus
Vectorized and rholterhus authored Dec 17, 2024
1 parent ab966a6 commit df3ed40
Show file tree
Hide file tree
Showing 2 changed files with 91 additions and 45 deletions.
100 changes: 71 additions & 29 deletions src/accounts/Timelock.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
pragma solidity ^0.8.4;

import {ERC7821} from "./ERC7821.sol";
import {LibERC7579} from "./LibERC7579.sol";
import {EnumerableRoles} from "../auth/EnumerableRoles.sol";

/// @notice Simple timelock.
Expand All @@ -12,6 +13,24 @@ import {EnumerableRoles} from "../auth/EnumerableRoles.sol";
/// - This implementation only supports ERC7821 style execution.
/// - This implementation uses EnumerableRoles for better auditability.
/// - This implementation uses custom errors with arguments for easier debugging.
/// - `executionData` can be encoded in three different ways:
/// 1. `abi.encode(calls)`.
/// 2. `abi.encode(calls, abi.encode(predecessor))`.
/// 3. `abi.encode(calls, abi.encode(predecessor, salt))`.
/// - Where `calls` is of type `(address,uint256,bytes)[]`, and
/// `predecessor` is the id of the proposal that is required to be already executed.
/// - If `predecessor` is `bytes32(0)`, it will be ignored (treated as if not required).
/// - The optional `salt` allows for multiple proposals representing the same payload.
/// - The proposal id is given by:
/// `keccak256(abi.encode(mode, keccak256(executionData)))`.
///
/// Supported modes:
/// - `bytes32(0x01000000000000000000...)`: does not support optional `opData`.
/// - `bytes32(0x01000000000078210001...)`: supports optional `opData`.
/// Where `opData` is `abi.encode(predecessor)` or `abi.encode(predecessor, salt)`,
/// and `...` is the remaining 22 bytes which can be anything.
/// For ease of mind, just use:
/// `0x0100000000007821000100000000000000000000000000000000000000000000`.
contract Timelock is ERC7821, EnumerableRoles {
/*´:°•.°+.*•´.*:˚.°*.˚•´.°:°•.°•.*•´.*:˚.°*.˚•´.°:°•.°+.*•´.*:*/
/* CONSTANTS */
Expand Down Expand Up @@ -82,7 +101,12 @@ contract Timelock is ERC7821, EnumerableRoles {
/// @dev The storage slot for the timelock's minimum delay.
/// We restrict the `minDelay` to be less than `2 ** 254 - 1`, and store the negation of it.
/// This allows us to check if it has been initialized via a non-zero check.
/// Slot of operation `id` is given by: `xor(shl(72, id), _TIMELOCK_SLOT)`.
/// Slot of operation `id` is given by:
/// ```
/// mstore(0x09, _TIMELOCK_SLOT)
/// mstore(0x00, id)
/// let operationIdSlot := keccak256(0x00, 0x29)
/// ```
/// Bits layout:
/// - [0] `done`.
/// - [1..255] `readyTimestamp`.
Expand All @@ -93,24 +117,24 @@ contract Timelock is ERC7821, EnumerableRoles {
/*.•°:°.´+˚.*°.˚:*.´•*.+°.•°:´*.´•*.•°.•°:°.´:•˚°.*°.˚:*.´+°.•*/

/// @dev The proposal `id` has been created.
event Proposed(bytes32 indexed id, bytes executionData, uint256 readyTimestamp);
event Proposed(bytes32 indexed id, bytes32 mode, bytes executionData, uint256 readyTimestamp);

/// @dev The proposal `id` has been executed.
event Executed(bytes32 indexed id, bytes executionData);
event Executed(bytes32 indexed id, bytes32 mode, bytes executionData);

/// @dev The proposal `id` has been cancelled.
event Cancelled(bytes32 indexed id);

/// @dev The minimum delay has been set to `newMinDelay`.
event MinDelaySet(uint256 newMinDelay);

/// @dev `keccak256(bytes("Proposed(bytes32,bytes,uint256)"))`.
/// @dev `keccak256(bytes("Proposed(bytes32,bytes32,bytes,uint256)"))`.
uint256 private constant _PROPOSED_EVENT_SIGNATURE =
0xbebd4e757b91b0f4e9671796f940352b8ce72b4878d18387feb19859ae5c9e25;
0x9b40ebcd599cbeb62eedb5e0c1db0879688a09d169ab92dbed4957d49a44b671;

/// @dev `keccak256(bytes("Executed(bytes32,bytes)"))`.
/// @dev `keccak256(bytes("Executed(bytes32,bytes32,bytes)"))`.
uint256 private constant _EXECUTED_EVENT_SIGNATURE =
0xda66fcfe5711520a570ced34d4cdebbe652fe74713bf2bc9db4ba54357e5a96f;
0xb1fdd61c3a5405a73ea1f8fb29bfd62c6152241cb59843d3def17bfadb7cb0bf;

/// @dev `keccak256(bytes("Cancelled(bytes32)"))`.
uint256 private constant _CANCELLED_EVENT_SIGNATURE =
Expand Down Expand Up @@ -157,14 +181,15 @@ contract Timelock is ERC7821, EnumerableRoles {
/* PUBLIC UPDATE FUNCTIONS */
/*.•°:°.´+˚.*°.˚:*.´•*.+°.•°:´*.´•*.•°.•°:°.´:•˚°.*°.˚:*.´+°.•*/

/// @dev Proposes an `executionData` with `delay`.
/// @dev Proposes an execute payload (`mode`, `executionData`) with `delay`.
/// Emits a {Proposed} event.
function propose(bytes calldata executionData, uint256 delay)
function propose(bytes32 mode, bytes calldata executionData, uint256 delay)
public
virtual
onlyRole(PROPOSER_ROLE)
returns (bytes32 id)
{
LibERC7579.decodeBatchAndOpData(executionData); // Check if properly encoded.
uint256 t = minDelay();
/// @solidity memory-safe-assembly
assembly {
Expand All @@ -179,25 +204,30 @@ contract Timelock is ERC7821, EnumerableRoles {
revert(0x1c, 0x44)
}
let m := mload(0x40)
calldatacopy(add(m, 0x60), executionData.offset, executionData.length)
id := keccak256(add(m, 0x60), executionData.length)
let s := xor(shl(72, id), _TIMELOCK_SLOT) // Operation slot.
calldatacopy(add(m, 0x80), executionData.offset, executionData.length)
mstore(0x00, mode)
mstore(0x20, keccak256(add(m, 0x80), executionData.length))
id := keccak256(0x00, 0x40)
mstore(0x09, _TIMELOCK_SLOT)
mstore(0x00, id)
let s := keccak256(0x00, 0x29) // Operation slot.
if sload(s) {
mstore(0x00, 0xd639b0bf) // `TimelockInvalidOperation(bytes32,uint256)`.
mstore(0x20, id)
mstore(0x40, 1) // `1 << OperationState.Unset`
revert(0x1c, 0x44)
}
mstore(m, 0x40)
// Emits the {Proposed} event.
mstore(m, mode)
mstore(add(m, 0x20), 0x60)
let r := add(delay, timestamp()) // `readyTimestamp`.
sstore(s, shl(1, r)) // Update the operation in the storage.
// Emits the {Proposed} event.
mstore(add(m, 0x20), r)
mstore(add(m, 0x40), executionData.length)
mstore(add(m, 0x40), r)
mstore(add(m, 0x60), executionData.length)
// Some indexers require the bytes to be zero-right padded.
mstore(add(add(m, 0x60), executionData.length), 0) // Zeroize the slot after the end.
mstore(add(add(m, 0x80), executionData.length), 0) // Zeroize the slot after the end.
// forgefmt: disable-next-item
log2(m, add(0x60, and(not(0x1f), add(0x1f, executionData.length))),
log2(m, add(0x80, and(not(0x1f), add(0x1f, executionData.length))),
_PROPOSED_EVENT_SIGNATURE, id)
}
}
Expand All @@ -207,7 +237,9 @@ contract Timelock is ERC7821, EnumerableRoles {
function cancel(bytes32 id) public virtual onlyRole(CANCELLER_ROLE) {
/// @solidity memory-safe-assembly
assembly {
let s := xor(shl(72, id), _TIMELOCK_SLOT) // Operation slot.
mstore(0x09, _TIMELOCK_SLOT)
mstore(0x00, id)
let s := keccak256(0x00, 0x29) // Operation slot.
let p := sload(s)
if or(and(1, p), iszero(p)) {
mstore(0x00, 0xd639b0bf) // `TimelockInvalidOperation(bytes32,uint256)`.
Expand Down Expand Up @@ -257,15 +289,19 @@ contract Timelock is ERC7821, EnumerableRoles {
function readyTimestamp(bytes32 id) public view virtual returns (uint256 result) {
/// @solidity memory-safe-assembly
assembly {
result := shr(1, sload(xor(shl(72, id), _TIMELOCK_SLOT)))
mstore(0x09, _TIMELOCK_SLOT)
mstore(0x00, id)
result := shr(1, sload(keccak256(0x00, 0x29)))
}
}

/// @dev Returns the current operation state of `id`.
function operationState(bytes32 id) public view virtual returns (OperationState result) {
/// @solidity memory-safe-assembly
assembly {
result := sload(xor(shl(72, id), _TIMELOCK_SLOT))
mstore(0x09, _TIMELOCK_SLOT)
mstore(0x00, id)
result := sload(keccak256(0x00, 0x29))
// forgefmt: disable-next-item
result := mul(iszero(iszero(result)),
add(and(result, 1), sub(2, lt(timestamp(), shr(1, result)))))
Expand Down Expand Up @@ -301,7 +337,7 @@ contract Timelock is ERC7821, EnumerableRoles {
/// To ensure that the operation is ready to be executed.
/// Updates the operation state and emits a {Executed} event after the calls.
function _execute(
bytes32,
bytes32 mode,
bytes calldata executionData,
Call[] calldata calls,
bytes calldata opData
Expand All @@ -313,8 +349,12 @@ contract Timelock is ERC7821, EnumerableRoles {
assembly {
// Copies the `executionData` for the event and to compute the `id`.
calldatacopy(mload(0x40), executionData.offset, executionData.length)
id := keccak256(mload(0x40), executionData.length)
s := xor(shl(72, id), _TIMELOCK_SLOT)
mstore(0x00, mode)
mstore(0x20, keccak256(mload(0x40), executionData.length))
id := keccak256(0x00, 0x40)
mstore(0x09, _TIMELOCK_SLOT)
mstore(0x00, id)
s := keccak256(0x00, 0x29)
let p := sload(s)
if or(or(and(1, p), iszero(p)), lt(timestamp(), shr(1, p))) {
mstore(0x00, 0xd639b0bf) // `TimelockInvalidOperation(bytes32,uint256)`.
Expand All @@ -325,7 +365,8 @@ contract Timelock is ERC7821, EnumerableRoles {
// Check if optional predecessor has been executed.
if iszero(lt(opData.length, 0x20)) {
let b := calldataload(opData.offset) // Predecessor's id.
if iszero(or(iszero(b), and(1, sload(xor(shl(72, xor(b, id)), s))))) {
mstore(0x00, b) // `_TIMELOCK_SLOT` is already at `0x09`.
if iszero(or(iszero(b), and(1, sload(keccak256(0x00, 0x29))))) {
mstore(0x00, 0x90a9a618) // `TimelockUnexecutedPredecessor(bytes32)`.
mstore(0x20, b)
revert(0x1c, 0x24)
Expand All @@ -345,14 +386,15 @@ contract Timelock is ERC7821, EnumerableRoles {
}
let m := mload(0x40)
// Copies the `executionData` for the event.
calldatacopy(add(m, 0x40), executionData.offset, executionData.length)
calldatacopy(add(m, 0x60), executionData.offset, executionData.length)
// Emits the {Executed} event.
mstore(m, 0x20)
mstore(add(m, 0x20), executionData.length)
mstore(m, mode)
mstore(add(m, 0x20), 0x40)
mstore(add(m, 0x40), executionData.length)
// Some indexers require the bytes to be zero-right padded.
mstore(add(add(m, 0x60), executionData.length), 0) // Zeroize the slot after the end.
// forgefmt: disable-next-item
log2(m, add(0x40, and(not(0x1f), add(0x1f, executionData.length))),
log2(m, add(0x60, and(not(0x1f), add(0x1f, executionData.length))),
_EXECUTED_EVENT_SIGNATURE, id)
sstore(s, or(1, p)) // Set the operation as executed in the storage.
}
Expand Down
36 changes: 20 additions & 16 deletions test/Timelock.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,8 @@ contract TimelockTest is SoladyTest {
bytes data;
}

event Proposed(bytes32 indexed id, bytes executionData, uint256 readyTimestamp);
event Executed(bytes32 indexed id, bytes executionData);
event Proposed(bytes32 indexed id, bytes32 mode, bytes executionData, uint256 readyTimestamp);
event Executed(bytes32 indexed id, bytes32 mode, bytes executionData);
event Cancelled(bytes32 indexed id);
event MinDelaySet(uint256 newMinDelay);

Expand Down Expand Up @@ -71,14 +71,14 @@ contract TimelockTest is SoladyTest {
t.calls0[0].data =
abi.encodeWithSignature("setRole(address,uint256,bool)", _BOB, executorRole, true);
t.executionData0 = abi.encode(t.calls0);
t.id0 = keccak256(t.executionData0);
t.id0 = _id(t.executionData0);

t.calls1 = new Call[](1);
t.calls1[0].target = address(timelock);
t.calls1[0].data =
abi.encodeWithSignature("setRole(address,uint256,bool)", _CHARLIE, executorRole, true);
t.executionData1 = abi.encode(t.calls1, abi.encodePacked(t.id0));
t.id1 = keccak256(t.executionData1);
t.id1 = _id(t.executionData1);

// Must revert if try to execute on an empty id.
vm.expectRevert(
Expand All @@ -90,8 +90,8 @@ contract TimelockTest is SoladyTest {
);
timelock.execute(_SUPPORTED_MODE, t.executionData0);

assertEq(timelock.propose(t.executionData0, _DEFAULT_MIN_DELAY), t.id0);
assertEq(timelock.propose(t.executionData1, _DEFAULT_MIN_DELAY), t.id1);
assertEq(timelock.propose(_SUPPORTED_MODE, t.executionData0, _DEFAULT_MIN_DELAY), t.id0);
assertEq(timelock.propose(_SUPPORTED_MODE, t.executionData1, _DEFAULT_MIN_DELAY), t.id1);

t.readyTimestamp = block.timestamp + _DEFAULT_MIN_DELAY;
vm.warp(t.readyTimestamp);
Expand Down Expand Up @@ -123,17 +123,17 @@ contract TimelockTest is SoladyTest {
"setRole(address,uint256,bool)", timelock.OPEN_ROLE_HOLDER(), executorRole, true
);
t.executionData0 = abi.encode(t.calls0);
t.id0 = keccak256(t.executionData0);
t.id0 = _id(t.executionData0);

t.calls1 = new Call[](1);
t.calls1[0].target = address(timelock);
t.calls1[0].data =
abi.encodeWithSignature("setRole(address,uint256,bool)", _CHARLIE, executorRole, true);
t.executionData1 = abi.encode(t.calls1, abi.encodePacked(t.id0));
t.id1 = keccak256(t.executionData1);
t.id1 = _id(t.executionData1);

assertEq(timelock.propose(t.executionData0, _DEFAULT_MIN_DELAY), t.id0);
assertEq(timelock.propose(t.executionData1, _DEFAULT_MIN_DELAY), t.id1);
assertEq(timelock.propose(_SUPPORTED_MODE, t.executionData0, _DEFAULT_MIN_DELAY), t.id0);
assertEq(timelock.propose(_SUPPORTED_MODE, t.executionData1, _DEFAULT_MIN_DELAY), t.id1);

t.readyTimestamp = block.timestamp + _DEFAULT_MIN_DELAY;
vm.warp(t.readyTimestamp);
Expand Down Expand Up @@ -188,7 +188,7 @@ contract TimelockTest is SoladyTest {
t.calls[0].data = abi.encodeWithSignature("setMinDelay(uint256)", newMinDelay);

t.executionData = abi.encode(t.calls);
t.id = keccak256(t.executionData);
t.id = _id(t.executionData);

if (_randomChance(16)) {
t.delay = _random();
Expand All @@ -199,11 +199,11 @@ contract TimelockTest is SoladyTest {
"TimelockInsufficientDelay(uint256,uint256)", t.delay, t.minDelay
)
);
timelock.propose(t.executionData, t.delay);
timelock.propose(_SUPPORTED_MODE, t.executionData, t.delay);
return;
} else if (t.delay > _MAX_DELAY) {
vm.expectRevert(Timelock.TimelockDelayOverflow.selector);
timelock.propose(t.executionData, t.delay);
timelock.propose(_SUPPORTED_MODE, t.executionData, t.delay);
return;
}
}
Expand All @@ -224,8 +224,8 @@ contract TimelockTest is SoladyTest {

t.readyTimestamp = block.timestamp + _DEFAULT_MIN_DELAY;
vm.expectEmit(true, true, true, true);
emit Proposed(t.id, t.executionData, t.readyTimestamp);
assertEq(timelock.propose(t.executionData, _DEFAULT_MIN_DELAY), t.id);
emit Proposed(t.id, _SUPPORTED_MODE, t.executionData, t.readyTimestamp);
assertEq(timelock.propose(_SUPPORTED_MODE, t.executionData, _DEFAULT_MIN_DELAY), t.id);

assertEq(uint8(timelock.operationState(t.id)), uint8(Timelock.OperationState.Waiting));
assertEq(timelock.readyTimestamp(t.id), t.readyTimestamp);
Expand Down Expand Up @@ -257,7 +257,7 @@ contract TimelockTest is SoladyTest {
vm.expectEmit(true, true, true, true);
emit MinDelaySet(newMinDelay);
vm.expectEmit(true, true, true, true);
emit Executed(t.id, t.executionData);
emit Executed(t.id, _SUPPORTED_MODE, t.executionData);
timelock.execute(_SUPPORTED_MODE, t.executionData);
assertEq(timelock.minDelay(), newMinDelay);
assertEq(uint8(timelock.operationState(t.id)), uint8(Timelock.OperationState.Done));
Expand Down Expand Up @@ -414,4 +414,8 @@ contract TimelockTest is SoladyTest {
uint256 readyTimestamp = delay + blockTimestamp;
assert(readyTimestamp <= 2 ** 255 - 1);
}

function _id(bytes memory executionData) internal pure returns (bytes32) {
return keccak256(abi.encode(_SUPPORTED_MODE, keccak256(executionData)));
}
}

0 comments on commit df3ed40

Please sign in to comment.