UpdateExpirattionPeriod() cannot be execute when the newExpirationPeriod is less than currentExpirationPeriod.

mediumCode4rena

Lines of code

https://github.com/code-423n4/2024-10-kleidi/blob/ab89bcb443249e1524496b694ddb19e298dca799/src/Timelock.sol#L608 https://github.com/code-423n4/2024-10-kleidi/blob/ab89bcb443249e1524496b694ddb19e298dca799/src/Timelock.sol#L1009-L1015 https://github.com/code-423n4/2024-10-kleidi/blob/ab89bcb443249e1524496b694ddb19e298dca799/src/Timelock.sol#L399-L404

Vulnerability details

Description

Safe cannot reduce expirationPeriod to a newExpirationPeriod when

currentTimeStamp < timestamp[id] +  expirationPeriod and
currentTimeStamp >= timestamp[id] +  newExpirationPeriod

where id is the hash of updateExpirationPeriod() and timestamp[id] is the timestamp when the id can be executed.

Safe shuold be able to update the expirationPeriod to any values >= MIN_DELAY by scheduling the updateExpirationPeriod() and later execute from timelock when the operation is ready (before the expiry).

solidity📋
    require(newPeriod >= MIN_DELAY, "Timelock: delay out of bounds");

But the protocol has overlooked the situation and added an reduntant check inside _afterCall() which is executed at the end of _execute().

solidity📋
    function _afterCall(bytes32 id) private {
        /// unreachable state because removing the proposal id from the
        /// _liveProposals set prevents this function from being called on the
        /// same id twice
        require(isOperationReady(id), "Timelock: operation is not ready"); //@audit
        timestamps[id] = _DONE_TIMESTAMP;
    }

Here the isOperationReady(id) will be executed with the newExpirationPeriod. code

solidity📋
    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;
    }

there it is checking whether the currentTimestamp is less than the timestamp + updated EpirationPeriod instead of the actual expirationPeriod.

Proof Of Concept

forge test --match-test testDUpdateExpirationPeriodRevert -vvv

solidity📋
function testDUpdateExpirationPeriodRevert() public {
        // Prepare the scheduling parameters
        // Call schedule() first time
        
        uint256 newExpirationPeriod =  EXPIRATION_PERIOD - 2 days; //newExpirationPeriod =  3 days since EXPIRATION_PERIOD = 5 days intially

        _schedule({       //safe has scheduled updateExpirationPeriod() call
            caller: address(safe),
            timelock: address(timelock),
            target: address(timelock),
            value: 0,
            data: abi.encodeWithSelector(
                timelock.updateExpirationPeriod.selector,newExpirationPeriod
            ),
            salt: bytes32(0),
            delay: MINIMUM_DELAY
        });
      
        //delay time has passed
        vm.warp(block.timestamp + MIN_DELAY + EXPIRATION_PERIOD - 1 days); //current timestamp is 1 day before the expiry period.

        vm.expectRevert("Timelock: operation is not ready"); //it will  revert with this msg

        timelock.execute(address(timelock),0, abi.encodeWithSelector(
                timelock.updateExpirationPeriod.selector,newExpirationPeriod
            ),bytes32(0));

    }

Updating to the new expirationPeriod will revert in this case. This can affect the protocols core design features.

Recommended Mitigation Steps

solidity📋
 function _afterCall(bytes32 id) private {
       //no need to check
        timestamps[id] = _DONE_TIMESTAMP;
    }

Assessed type

Other