Dug - Timelock functionally is implemented superficially

[sherlock-admin]

Dug

medium

Timelock functionally is implemented superficially

Summary

The timelock only provides superficial assurances that certain function calls cannot be immediately executed.

Vulnerability Detail

Currently, the timelock functionality is implemented via a role system using an OpenZeppelin Access Control TIMELOCK_ROLE. This role is given to the TimelockController contract and a modifier is added to certain functions in the Unitas contract.

The issue is that the TIMELOCK_ROLE can easily be given to other accounts/contracts, bypassing all timelock functionality. This is because the TIMELOCK_ROLE is not a role that is specific to the TimelockController contract, but rather a role that can be given to any actor. All timelock functionality is internal to the TimeLockController contract.

Impact

The impact is that the timelock functionality is not actually enforced in a meaningful way.

Code Snippet

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

https://github.com/sherlock-audit/2023-04-unitasprotocol/blob/main/Unitas-Protocol/src/Unitas.sol#L93-L97

Tool used

Manual Review

Recommendation

At a bare minimum the TIMELOCK_ROLE should be administrated by the TimelockController contract. Ideally, the timelock functionality should be implemented in a way that is not dependent on a role system.

[dugdaniels]

Escalate for 10 USDC

This sort of issue has been considered valid in the past. Please see this example: https://github.com/sherlock-audit/2023-03-Y2K-judging/issues/337

At a bare minimum, changes to the TIMELOCK_ROLE should be executed by the timelock.

[sherlock-admin]

Escalate for 10 USDC

This sort of issue has been considered valid in the past. Please see this example: https://github.com/sherlock-audit/2023-03-Y2K-judging/issues/337

At a bare minimum, changes to the TIMELOCK_ROLE should be executed by the timelock.

You've created a valid escalation for 10 USDC!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[ctf-sec]

In Y2k contest, admin is consider restricted

in this contest,

centralization risk is not in scope, can be a valid low / informational finding

[jacksanford1]

Admins are considered TRUSTED according to the README of this contest. While I agree that it's concerning that the timelock role can be given to anyone (essentially bypassing the timelock), the admin role is considered trusted in this protocol so the issue cannot be valid.

[hrishibhat]

Result: Low Unique

[sherlock-admin]

Escalations have been resolved successfully!

Escalation status:

• dugdaniels: rejected