TimelockController

When execute or executeWhitelistedBatch attempts to parse the function selector via bytes4(data[:4]), any call where data.length < 4 (e.g. a pure Ether transfer that passes an empty data payload) will revert because slicing data[:4] triggers an out-of-bounds error.

This deviates from the standard TimelockController behavior, which allows zero-length data for native asset transfers.

Notice the following check in the _execute function:

function _execute(address target, uint256 value, bytes calldata data) internal virtual override {
      if (data.length > 0 target.code.length = 0) revert InvalidTarget(target);
      super._execute(target, value, data);
}

The current test testExecuteWithValue passes because: 1. target is a smart contract. 2. abi.encodeWithSignature("") actually returns non-empty calldata: 0xc5d24601

However, if the target was an EOA it would revert with InvalidTarget(EOA_ADDR) error, because data.length check would pass in the _execute function but target.code.length = 0 would not.

And, if we tried to, set data.length to 0, it would revert in the EthenaTimelockController.execute function, when trimming the call data as the call data would be empty as we initially stated.

Consider checking that data.length is > than 4 before setting: bytes4 selector = bytes4(data[:4]);.

Ethena Team: The issue was resolved in PR#4. We have added _extractSelector to mitigate this and followed up with the test testCanWhitelistAndExecuteEmptyBytes4DataToSmartContract to verify the functionality.

In the execute, executeBatch, and executeWhitelistedBatch functions the msg.value is validated to be exactly equal to the total of the value parameters provided. This validation is performed in an effort to ensure that no ETH is retained in the contract as mentioned in the README.

However, in the event that Ether is sent to a function and any portion is refunded by the arbitrary function call then this amount of ETH will be left in the EthenaTimelockController function and will not be rescuable due to the msg.value validations.

Refunds from arbitrary function calls are made possible by the receive function in the OpenZeppelin base TimelockController contract. Furthermore, if any calls with nonzero value are made with the EthenaTimelockController contract as the target, this Ether will be left within the contract.

Consider performing a balance check at the end of the execute, executeBatch, and executeWhitelistedBatch functions and refunding any ether in the contract to the caller, under the assumption that the caller can accept ether, or a holding address.

Ethena Team: The issue was resolved in PR#4. We have removed this check to allow funds to end up in the TimelockController contract and followed up with test testEthCanBeRescuedFromTimelock and testErc20TokensCanBeRescuedFromTimelock to validate that funds can be recovered if they do end up in the controller. Under normal circumstances we do not expect funds to end up in the controller unless there is a difference between msg.value and the value parameters or a refunds from an arbitrary function call is made or someone sends Ether directly into the TimelockController contract.

In the schedule function there is no validation to ensure that a whitelisted function is not scheduled through the normal timelock flow. As a result, when the scheduled whitelisted function becomes executable, it cannot be executed because the overridden execute function does not invoke super.execute and clear the action from the timelock.

If any actions were based on the whitelisted function call that was scheduled as a predecessor, these subsequent actions are stuck. Even in the case where the scheduled whitelisted action is cancelled, as it never reaches a OperationState.Done state.

Consider the following series of actions as an example:

• Action A is created with a whitelisted function selector in it’s payload
• Action B is created for a non-whitelisted function with Action A as its predecessor
• Action A cannot be completed from the scheduled queue since the execute function does not allow

the base TimelockController logic to be executed.

• Action B cannot be executed since Action A is listed as its predecessor, even if Action A is

cancelled.

• All actions will have to be cancelled and re-queued, forcing an additional wait time of the minDelay.

Furthermore, this scenario can also arise if the function selector of a pending action is whitelisted after that action was already scheduled.

Consider overriding the schedule function to prevent whitelisted actions from being queued. Be aware that if this solution is adopted, whitelisting a function in the queue will also yield this behavior.

Alternatively, consider separating the whitelisted execution logic and non-whitelisted execution logic into separate functions. Notice that this is similar to the approach for batch executions, which do not exhibit this deficiency.

Ethena Team: The issue was resolved in PR#4. We have opted to separate the whitelisted execution logic and non-whitelisted execution logic into execute and executedWhitelisted.

The base TimelockController contract from OpenZeppelin implements a receive function and therefore ether can be sent directly to the EthenaTimelockController contract.

Any ether sent to the EthenaTimelockController contract this way will be unrescuable as a result of the msg.value checks in the execute, executeBatch, and executeWhitelistedBatch functions.

Consider either adding a rescue function, implementing refunds in the execution functions as mentioned in L-01, or implementing a receive function that reverts to disable direct Ether transfers.

Ethena Team: The issue was resolved in PR#4.

In the removeFromWhitelist function the FunctionRemovedFromWhitelist event is emitted regardless of whether the specific target address and function selector were in the whitelist to begin with.

This could hypothetically be misleading to consumers of the FunctionRemovedFromWhitelist in the event that the target and selector combination value was not previously set as true in the _functionWhitelist mapping.

Similarly the addToWhitelist function performs no validation that the function was not already added to the whitelist.

Consider validating that the _functionWhitelist entry for the specified target and selector is true in the removeFromWhitelist function and false in the addToWhitelist function.

Ethena Team: The issue was resolved in PR#4.

There is no validation on the minDelay value in the constructor of the EthenaTimelockController contract. As a result the minDelay may technically be assigned to an insufficient delay, even though the deployment script currently sets the minDelay as 1 day.

Consider implementing validation for the minDelay value, otherwise be aware of this lack of validation during deployment.

Ethena Team: Acknowledged.

Arbitrary target contracts may be whitelisted with the addToWhitelist function. The target address in question may be a proxy contract which can have the implementation of it’s whitelisted selector upgraded.

For this reason, care should be taken to examine the centralized risk of whitelisted target contracts.

Be aware of this risk and consider the centralized upgrade risks for any upgradeable contracts which are whitelisted.

Ethena Team: Acknowledged. We do not intend to use addToWhitelist on functions outside of contracts that are currently controlled by the Ethena multisig. Assuming that is true and that the upgrade function on the target contract is not whitelisted, any upgrades happening to target whitelist contracts would have to go through the minDelay.

In the README it is mentioned that there are strict msg.value checks for the execute and executeWhitelistedBatch functions, however there is also the same strict msg.value check in the executeBatch function which is not mentioned.

Amend the README file to include that the executeBatch function also implements the msg.value invariant check.

Ethena Team: Acknowledged.

The WhitelistedFunctionExecuted event does not include relevant information such as the value and payload of the execution. These data fields are in the base TimelockController CallExecuted event and if anything would provide parity with this event.

Consider including value and payload fields in the WhitelistedFunctionExecuted event.

Ethena Team: Resolved.

The EXECUTOR_ROLE is planned to be left as an open role, however this introduces a potential censoring vector whereby a malicious actor may be able to control the execution flow of certain external calls.

For a particular external call where the execution of the target function uses a try/catch or allows the failure of a nested external call, a malicious actor may be able to perform the execution while providing insufficient gas to the execute or executeBatch functions such that the nested external call runs out of gas and fails but allows the top level transaction to succeed with the remaining 1/64 gas.

None of the existing permissioned functions which may be used by the EthenaTimelockController were observed to use a try/catch or allow the failure of an external call, therefore this is not an immediate concern. However be aware of this risk when adapting future use-cases for the timelock contract.

Ethena Team: Acknowledged.

The base TimelockController contract does not implement the DefaultAdminRules contract nor any base default admin role protections.

Similarly, other Ethena related contracts use a SingleAdminAccessControl contract which provides a simpler implementation of default admin role protections.

Consider implementing the protections from the SingleAdminAccessControl contract for the default admin role as seen in other Ethena contracts.

Ethena Team: Acknowledged. Worst case here is that someone could propose the Admin to renounce or revoke their role which would mean that other roles cannot be managed. In this scenario it is still possible for the timelock contract the propose the underlying contract to change its admin.

In the AccessControl contract the renounceRole function is exposed to be called by any address without going through the timelock period. This does not pose any notable risk and this finding serves only to document that this action is not kept behind a timelock.

Be aware of this behavior, and if desired consider overriding the renounceRole function and requiring that it goes through the queued delay with the onlyTimelock modifier.

Ethena Team: The issue was resolved in commit 573bbdb. We’ve removed the ability for addresses to renounce roles directly. They must be revoked through the timelock

https://github.com/ethena-labs/timelock-contract/pull/4/commits/573bbdb8aa5e213df79f219693

9fecb3b3c82cf7.

In the constructor of the EthenaTimelockController contract the proposers list is validated to have all nonzero entries, however no such validation is performed for the whitelistedExecutors list which should also not contain the zero address.

Consider implementing a zero address validation for the whitelistedExecutors list.

Ethena Team: The issue was resolved in PR#4.

In the deployment script for the EthenaTimelockController contract there is validation performed on the first entry of the proposers and executors lists.

Firstly, the entire proposers and executors lists could be validated to ensure that all proposers and executors received the expected roles, in the event that these lists were extended before deployment.

Secondly, the whitelistedExecutors list is not validated to ensure that the intended whitelisted executors received the expected WHITELISTED_EXECUTOR_ROLE role.

Finally, the proposer addresses will each also receive the CANCELLER_ROLE which can also be validated for.

Be aware of these potential deployment script improvements and consider implementing them if you wish.

Ethena Team: Resolved.

In the EthenaTimelockController contract the value 4 is repeatedly used to denote the length of a function selector and create a slice of the selector. As a best practice magic numbers can be referenced as named constants to improve code readability.

Consider creating a constant SELECTOR_LENGTH value to use when slicing selectors.

Ethena Team: Resolved.

Within the _execute override in EthenaTimelockController, there is a conditional that reverts if data.length > 0 and target.code.length = 0:

if (data.length > 0 && target.code.length = 0) {revert InvalidTarget(target);}

This prevents the contract from calling addresses where code.length is zero. While this is intended to block whitelisting for addresses with empty code, it also blocks calls to Ethereum precompiles (e.g., 0x1 through 0x9 on mainnet), which often have no bytecode in the traditional sense.

Consequently, any attempt to schedule or execute interactions with precompiles will revert. Keep in mind that since the Pectra update EOAs can also have code in their accounts since they via delegatation. This will bypass the check in _execute and allow execution of an EOA’s code.

• If interacting with precompiles is a requirement, remove or relax the target.code.length = 0 check.
• Alternatively, explicitly allow known precompile addresses in the code or via a separate allow-list so

that legitimate precompiles can be called without inadvertently allowing empty addresses.

Ethena Team: The issue was resolved in PR#4. Precompiled are not intended to be called directly by the TimelockController contract. Fixed through the splitting of execute and executeWhitelisted which allows us to remove this override.

Since Solidity 0.8.22, the compiler performs advanced range analysis to determine when overflow checks can safely be omitted.

In a typical loop increment scenario such as for (uint256 i = 0; i < length; i++), the compiler can prove that i will not overflow a uint256 and therefore it automatically removes the checks.

Manually placing increments in an unchecked block is now redundant, making the code less clear without providing a meaningful gas optimization.

Replace unchecked { ++i; } with a simple ++i, allowing the compiler’s range analysis to handle overflow optimizations automatically. This cleaner style increases code clarity while maintaining code efficiency in Solidity 0.8.22 and above.

Ethena Team: The issue was resolved in PR#4.

When a whitelisted operation is being executed by calling execute(), the predecessor parameter is ignored, even though the NatSpec says * @param predecessor The operation that must be executed before this one.

Because of this, whitelisted executors may expect that their operations will be executed only if the predecessor is already done, but in reality their operations will be executed regardless of the predecessor.

Either clarify that the predecessor is not used for whitelisted operations in the NatSpec or modify the code to actually use it.

Ethena Team: The issue was resolved in PR#4. Fixed through splitting out of executeWhitelisted function.

The protocol documentation states that “only whitelisted executors can execute whitelisted functions” as a invariant. It also claims that batches cannot be scheduled if they contain whitelisted functions.

However, the current implementation allows scheduling such batches. This breaks the documented feature and effectively bypasses the invariant, as any address with EXECUTOR_ROLE can then execute whitelisted functions via the executeBatch function.

If this behaviour is intended, clarify it in the documentation. If not, add a check to prevent scheduling batches that include whitelisted functions.

Ethena Team: Acknowledged. One caveat here is that a non-whitelisted function can be scheduled and then subsequently added to the whitelist before the scheduled function is executed.

When TimelockController is created, all of the proposers receive the PROPOSER_ROLE and CANCELLER_ROLE, which allow them to both propose and cancel transactions.

This gives them a lot of power, since they can cancel any transaction they want to before the timelock delay has passed.

Normally, if any of the proposers becomes malicious, their role should be revoked by calling AccessControl.revokeRole() which can be only executed by the admin of the given role.

In the case of the timelock, the admin is the address with the DEFAULT_ADMIN_ROLE - this address is the timelock itself.

Because of this, the revoke transaction itself is a subject to the timelock delay, which makes it possible for the malicious proposer to just cancel it, keeping their CANCELLER_ROLE.

Once a proposer is compromised, the result is DOS of the timelock and permanent freezing of the funds it holds (since any transaction can be cancelled).

Be careful with who you give the CANCELLER_ROLE, ideally only to one account.

Ethena Team: Acknowledged. Proposer and canceller roles are intended to be assigned only to the multisig that is the current controller of the ethena protocol contracts.

The timelock inherits from ReentrancyGuardTransient which makes use of the tload and tstore opcodes introduced in EIP-1153. If the contract were to be deployed to a chain with no support for these opcodes, the functionality would be broken.

Keep in mind you need to change the reentrancy guard if you are going to deploy to such chains.

Ethena Team: Resolved. We’ve opted to replace with ReentrancyGuard instead. The gas saving is not a concern.

The NatSpec for the addToBlacklist function in the StakedUSDe contract states that it Allows the owner (DEFAULT_ADMIN_ROLE) and blacklist managers to blacklist addresses.

However, the addToBlacklist and removeFromBlacklist functions use the onlyRole(BLACKLIST_MANAGER_ROLE) modifier which only allows the blacklist manager role to execute these functions.

As a result if the EthenaTimelockController is only granted the DEFAULT_ADMIN_ROLE over the contracts it will be unable to invoke the addToBlacklist or removeFromBlacklist functions.

Be aware of this discrepancy and be sure to assign the BLACKLIST_MANAGER_ROLE to the EthenaTimelockController in addition to the DEFAULT_ADMIN_ROLE for the StakedUSDe contract.

Ethena Team: Acknowledged. It should be noted that the DEFAULT_ADMIN_ROLE can manage addresses with the BLACKLIST_MANAGER_ROLE. The blacklisting role in StakedUSDe is intended to be given to addresses other than the admin.