TimeLock controller misses key checks and admin functions makes is vulnerable to certain scenarios.
Summary
Time Lock controller by design is to provision a delay in execution of proposals so that users who dont want to risk
with the upcoming change can exit the position before the proposal goes live.
The constructor of the timelock controller contract accepts an array of proposers and executors along with governor roles.
Proposers are accounts which can create new proposals and executors are accounts which execute the proposal after the delay.
There is lack of critical validations for the below.
a) Delay time window -the delay time window should be large enough to give human being as opportunity to action before the
proposal goes live. Ideally. this time window should have a minimum time span of few days. There is no such check implemented.
b) Constructor: The constructor accepts array of executors and proposer roles are granted roles without check for 0x0 address.
if the array passed was for proposer and executors was 0x0, then the contract will become unusable as none one has private keys
to 0x0 address and hence no one can propose or can execute
c) Governor role is redundant. The governor role is not used. Also, the contract does not provision to administer the Proposer and executor roles incase the accounts that needs to propose or execute will need to be changed. This could be a gap in the cases where such keys are lost. In such incidents, this contract becomes unusable.
Vulnerability Detail
The contract lacks checks for whom it is assigning roles that will be key in proper functioning of the this contract.
It also lacks the ability to react incase there is a vulnerability due to human error and hence cannot recover. The error is loss of keys for executor or Proposer, in such case, the contract does not expose functionality to re assign to a new account.
Impact
The impact will be severe in the case where the keys are lost or some one evil gets hands on these keys and he proposes and executes by setting the time window to 0. In which case, the investors in this contract will have to bear the consequences and this contract has no counter measure to address this.
constructor(uint256 minDelay, address[] memory proposers, address[] memory executors, address governor) {
_setRoleAdmin(GOVERNOR_ROLE, GOVERNOR_ROLE);
_setRoleAdmin(PROPOSER_ROLE, GOVERNOR_ROLE);
_setRoleAdmin(EXECUTOR_ROLE, GOVERNOR_ROLE);
_setRoleAdmin(CANCELLER_ROLE, GOVERNOR_ROLE);
// Set GOVERNOR_ROLE
_grantRole(GOVERNOR_ROLE, governor);
// register proposers and cancellers
for (uint256 i = 0; i < proposers.length; ++i) {
_grantRole(PROPOSER_ROLE, proposers[i]);
_grantRole(CANCELLER_ROLE, proposers[i]);
}
// register executors
for (uint256 i = 0; i < executors.length; ++i) {
_grantRole(EXECUTOR_ROLE, executors[i]);
}
_minDelay = minDelay;
emit MinDelayChange(0, minDelay);
}
Tool used
Manual Review
Recommendation
a) Add checks in the constructor for Zero addresses for Proposers, Executors and Guardian
b) Make sure the delay has a time window of few days. The contract should not let that be set below that window.
c) Enable governor to manage the roles of Proposers and Executors incase there was a need to override the roles for the accounts and grant roles to new accounts
ravikiran.web3
medium
TimeLock controller misses key checks and admin functions makes is vulnerable to certain scenarios.
Summary
Time Lock controller by design is to provision a delay in execution of proposals so that users who dont want to risk with the upcoming change can exit the position before the proposal goes live.
The constructor of the timelock controller contract accepts an array of proposers and executors along with governor roles. Proposers are accounts which can create new proposals and executors are accounts which execute the proposal after the delay. There is lack of critical validations for the below.
a) Delay time window -the delay time window should be large enough to give human being as opportunity to action before the proposal goes live. Ideally. this time window should have a minimum time span of few days. There is no such check implemented.
b) Constructor: The constructor accepts array of executors and proposer roles are granted roles without check for 0x0 address. if the array passed was for proposer and executors was 0x0, then the contract will become unusable as none one has private keys to 0x0 address and hence no one can propose or can execute
c) Governor role is redundant. The governor role is not used. Also, the contract does not provision to administer the Proposer and executor roles incase the accounts that needs to propose or execute will need to be changed. This could be a gap in the cases where such keys are lost. In such incidents, this contract becomes unusable.
Vulnerability Detail
The contract lacks checks for whom it is assigning roles that will be key in proper functioning of the this contract. It also lacks the ability to react incase there is a vulnerability due to human error and hence cannot recover. The error is loss of keys for executor or Proposer, in such case, the contract does not expose functionality to re assign to a new account.
Impact
The impact will be severe in the case where the keys are lost or some one evil gets hands on these keys and he proposes and executes by setting the time window to 0. In which case, the investors in this contract will have to bear the consequences and this contract has no counter measure to address this.
Code Snippet
https://github.com/sherlock-audit/2023-04-unitasprotocol/blob/main/Unitas-Protocol/src/TimelockController.sol#L79-L101
Tool used
Manual Review
Recommendation
a) Add checks in the constructor for Zero addresses for Proposers, Executors and Guardian b) Make sure the delay has a time window of few days. The contract should not let that be set below that window. c) Enable governor to manage the roles of Proposers and Executors incase there was a need to override the roles for the accounts and grant roles to new accounts