Timelock operations can be executed due to incorrect usage of expirationPeriod

[howlbot-integration]

Lines of code

https://github.com/code-423n4/2024-10-kleidi/blob/ab89bcb443249e1524496b694ddb19e298dca799/src/Timelock.sol#L399-L404
https://github.com/code-423n4/2024-10-kleidi/blob/ab89bcb443249e1524496b694ddb19e298dca799/test/unit/Timelock.t.sol#L651-L678
https://github.com/code-423n4/2024-10-kleidi/blob/ab89bcb443249e1524496b694ddb19e298dca799/src/Timelock.sol#L972-L977
https://github.com/code-423n4/2024-10-kleidi/blob/ab89bcb443249e1524496b694ddb19e298dca799/test/unit/Timelock.t.sol#L691-L748

Vulnerability details

isOperationReady function uses the current expirationPeriod value, which can be updated after an operation is scheduled, instead of using the expirationPeriod value that was active when the operation was scheduled.

Here's what causes it: Combination of two factors

1. isOperationReady using the current expirationPeriod value
2. updateExpirationPeriod allowing the expirationPeriod to be increased after operations are scheduled

Can be confirmed by

1. Scheduling an operation with a delay of MINIMUM_DELAY and an expirationPeriod of EXPIRATION_PERIOD.
2. Wait until the operation becomes ready (after MINIMUM_DELAY).
3. Call updateExpirationPeriod to increase the expirationPeriod to EXPIRATION_PERIOD + 1.
4. Wait until after the original expiration time (timestamp + EXPIRATION_PERIOD).
5. Call execute on the operation.
6. The operation will be executed successfully, even though it should have expired.

Proof of Concept

In Timelock.sol, the isOperationReady function uses the current expirationPeriod instead of the one that was set when the operation was scheduled isOperationReady#L399-L404

function isOperationReady(bytes32 id) public view returns (bool) {
    /// cache timestamp, save up to 2 extra SLOADs
    uint256 timestamp = timestamps[id];
    return timestamp > _DONE_TIMESTAMP && timestamp <= block.timestamp
        && timestamp + expirationPeriod > block.timestamp; // <@Issue: Uses current expirationPeriod instead of the one set when operation was scheduled
}

In Timelock.t.sol, does not cover the case where expirationPeriod is increased after scheduling Timelock.t.sol#L651-L677

function testExecuteCallSucceedsWhenReady() public {
    // Prepare and schedule a call
    bytes32 id = testScheduleProposalSafeSucceeds();

    // Simulate time passing
    vm.warp(block.timestamp + MINIMUM_DELAY);

    // Execute the call as anyone, should succeed
    _execute({
        caller: address(this),
        timelock: address(timelock),
        target: address(timelock),
        value: 0,
        payload: abi.encodeWithSelector(
            timelock.updateDelay.selector, MINIMUM_DELAY
        ),
        salt: bytes32(0)
    });

    vm.expectRevert("Timelock: operation already executed");
    timelock.isOperationExpired(id);

    assertEq(
        timelock.minDelay(), MINIMUM_DELAY, "minDelay should be updated"
    );
    assertTrue(timelock.isOperationDone(id), "operation should be done");
} // @Issue: Test does not cover the case where expirationPeriod is increased after scheduling

And the updateExpirationPeriod function allows the expirationPeriod to be increased after operations have been scheduled updateExpirationPeriod#L972-L977

function updateExpirationPeriod(uint256 newPeriod) external onlyTimelock {
    require(newPeriod >= MIN_DELAY, "Timelock: delay out of bounds");

    emit ExpirationPeriodChange(expirationPeriod, newPeriod);
    expirationPeriod = newPeriod; // <@Issue: Allows increasing expirationPeriod after operations are scheduled
}

The isOperationReady() uses the current expirationPeriod value instead of the one that was active when the operation was scheduled. The updateExpirationPeriod() function allows increasing this value after the fact, which enables executing expired operations. The test suite does not cover this scenario.

The following test in Timelock.t.sol#L691-L748 demonstrates the vulnerability

function testExecuteCallUpdateExpirationPeriodSucceedsWhenReady() public {
    uint256 newExpirationPeriod = 50 days;
    // Prepare and schedule a call
    _schedule({
        caller: address(safe),
        timelock: address(timelock),
        target: address(timelock),
        value: 0,
        data: abi.encodeWithSelector(
            timelock.updateExpirationPeriod.selector, newExpirationPeriod
        ),
        salt: bytes32(0),
        delay: MINIMUM_DELAY
    });

    // Simulate time passing
    vm.warp(block.timestamp + MINIMUM_DELAY);

    // Execute the call as anyone, should succeed
    _execute({
        caller: address(this),
        timelock: address(timelock),
        target: address(timelock),
        value: 0,
        payload: abi.encodeWithSelector(
            timelock.updateExpirationPeriod.selector, newExpirationPeriod
        ),
        salt: bytes32(0)
    });

    bytes32 id = timelock.hashOperation(
        address(timelock),
        0,
        abi.encodeWithSelector(
            timelock.updateExpirationPeriod.selector, newExpirationPeriod
        ),
        bytes32(0)
    );

    assertEq(
        timelock.expirationPeriod(),
        newExpirationPeriod,
        "expirationPeriod should be updated"
    );

    assertEq(timelock.timestamps(id), 1, "operation should be pending");
    assertTrue(timelock.isOperationDone(id), "operation should be done");
    assertTrue(timelock.isOperation(id), "operation should exist");

    assertEq(
        timelock.expirationPeriod(),
        newExpirationPeriod,
        "expirationPeriod should be updated"
    );
    assertEq(
        timelock.minDelay(), MINIMUM_DELAY, "minDelay should be updated"
    );
}

This test schedules an operation to update the expirationPeriod, waits until it's ready, executes it, and then verifies that the operation is done and the expirationPeriod is updated. However, it doesn't check if the operation can still be executed after the original expiration time has passed.

Recommended Mitigation Steps

isOperationReady() should check against the expiration period that was active when the operation was scheduled, not the current value by storing the expiration period with each scheduled operation.

Assessed type

Governance

[c4-judge]

GalloDaSballo changed the severity to QA (Quality Assurance)

[c4-judge]

GalloDaSballo marked the issue as grade-b