Timelock cannot run repeated operations

Summary

Timelock breaks for repeat operations.

Vulnerability Detail

The contract TimelockController.sol takes care of scheduling operations. The contract uses a value of 0 to denote a non-existent operation, and a value of 1 to denote a completed operation. The functions isOperation and isOperationPending checks against these values.

https://github.com/sherlock-audit/2023-04-unitasprotocol/blob/main/Unitas-Protocol/src/TimelockController.sol#L125-L127

This function checks if the scheduled timestamp is non zero, checking if it exists.

https://github.com/sherlock-audit/2023-04-unitasprotocol/blob/main/Unitas-Protocol/src/TimelockController.sol#L132-L134

This function checks if the scheduled timestamp is more than 1, meaning it has to exist and be pending.

In the function schedule, the contract checks the timestamp value of an operation in the internal function _schedule.

https://github.com/sherlock-audit/2023-04-unitasprotocol/blob/main/Unitas-Protocol/src/TimelockController.sol#L254-L258

Here the contract checks against the isOperation modifier. Now this modifier returns returns true only if the action does not exist at all. This means that if the timelock controller passes an action, it will be marked with a timestamp of 1, instead of 0, and thus the timelock will not be able to pass the same action again, since isOperation check will always fail since 1>0. Thus the timelock controller cannot schedule actions it has scheduled and completed in the past.

This can be a major issue on all the onlyTimelock functions. For example, the function sendPortfolio is guarded by this modifier, and thus the timelock can can send an amount to an address only once. The next call, either the address or the amount has to be different to generate a new hash since old actions cannot be repeated due to the incorrect function. Same goes for the setOracle function, where if an oracle is set and unset, it cannot be reset to the same address since the hash of that action already exists, and is marked with a timestamp of 1.

Impact

Broken timelock contract. Cannot schedule repeat operations.

Code Snippet

https://github.com/sherlock-audit/2023-04-unitasprotocol/blob/main/Unitas-Protocol/src/TimelockController.sol#L254-L258

Tool used

Manual Review

Recommendation

Use the modifier isOperationPending. The timelock is meant to stop repeat operations being executed within a timelock period, not stop operations which have already been executed.

sherlock-audit / 2023-04-unitasprotocol-judging

carrotsmuggler - Timelock cannot run repeated operations #136