Compound Timelock Emergency Rollback Audit

1

Summary

Timeline: From 2025-08-06 To 2025-08-13

Total Issues: 17 (11 resolved)

Medium Severity Issues: 3 (2 resolved)

Low Severity Issues: 3 (2 resolved)

Notes & Additional Information: 11 (7 resolved)

Scope

OpenZeppelin audited the ScopeLift/governance-rollback repository at commit 17954da.

In scope were the following files:

src
├── interfaces
│   ├── IRollbackManager.sol
│   ├── ITimelockMultiAdminShim.sol
│   ├── ITimelockTargetCompound.sol
│   └── ITimelockTargetControl.sol
├── RollbackManager.sol
├── RollbackManagerTimelockCompound.sol
├── RollbackManagerTimelockControl.sol
└── TimelockMultiAdminShim.sol

System Overview

The Governance Emergency Rollback system introduces a standardized way for DAOs to reverse already-executed governance actions when they produce harmful or unintended outcomes. The design supports two governance flavors:

  • Compound Governance: Rollbacks are proxied to a single-executor timelock via a TimelockMultiAdminShim wrapper instance that enables multiple executors.
  • OpenZeppelin Governance: Rollbacks are scheduled and executed through the multi-executor TimelockController.

At the system’s core is the RollbackManager contract, which manages the rollback lifecycle (proposing, queueing within a bounded queue window, and subsequent execution or cancellation) while respecting each target timelock’s enforced delay. Rollbacks are prepared at proposal time using a double-encoding pattern. The rollback bundle is first encoded as an input to RollbackManager::propose(), then that call is included as one transaction in the main governance proposal. Once a governance proposal has been executed, the rollback bundle is proposed via the RollbackManager contract as part of the proposal execution. If needed, a designated Guardian later reviews, queues, and executes the pre-staged rollback through the underlying timelock, ensuring that the system inherits the established governance safety guarantees.

Security Model and Trust Assumptions

The following assumptions about roles, timing, and executor behavior are critical to preserving the safety of emergency rollbacks:

  • Governance will control the RollbackManager contract via the timelock. As a result, any configuration changes, such as updating the guardian or queue window, would be made through standard governance flows.
  • The system inherits timing guarantees from the underlying timelock. Queued rollbacks must wait the configured delay before execution and (on Compound) must remain executable only within the timelock’s GRACE_PERIOD.
  • Since the actual execution is delegated to trusted timelocks (Compound Timelock or OZ TimelockController), correctness, liveness, and authorization checks (e.g., operation hashing/salts, executor roles, and state transitions) are assumed to be correctly enforced by those contracts.
  • Rollback proposals will be reviewed for content and correctness as part of the original proposal they are attached to.
  • The TimelockMultiAdminShim contract acts as a wrapper around the current Compound timelock contract that extends its current functionality. This changes the security model of the Compound timelock significantly by allowing the RollbackManagerCompoundTimelock to act as an executor for the current Compound Timelock. Any contract added on the TimelockMultiAdminShim must be thoroughly vetted, as a compromised executor will be catastrophic to the Compound protocol. An EOA, or a contract that can make arbitrary calls outside the Compound governance process, should never be added as an executor.

Privileged Roles

The RollbackManager contract defines two primary privileged roles, and the Compound path introduces a shim-mediated control surface. These roles are authorized to call the following functions:

  • Admin (RollbackManager) — Proposes and configures critical RollbackManager parameters.
  • Guardian (RollbackManager) — Emergency operator. Within a limited window, the Guardian can queue, execute, or cancel a proposed/queued rollback. The Guardian must respect the timelock’s wait time (and any grace period).
  • Timelock — In the Compound path, the timelock governs the TimelockMultiAdminShim configuration and performs the execution of relevant operations in the rollback lifecycle. In the OpenZeppelin path, the appropriate role (PROPOSER_ROLE, EXECUTOR_ROLE, and CANCELLER_ROLE) must be granted via the TimelockController contract to enable rollback scheduling and execution.
  • Admin (TimelockMultiAdminShim) - Governs the shim configuration, and wraps and forwards Compound Timelock operations with additional authorization checks. Only the admin may queue shim-targeting operations, while external-target operations are shared with executors.
  • Executor (TimelockMultiAdminShim) - Whitelisted operator that can control timelock actions for external targets alongside the admin. Cannot queue shim-configuration operations.

Medium Severity

Non-Payable execute Blocks Rollbacks That Transfer ETH

The RollbackManager contract’s execute function is not marked as payable. The abstract RollbackManager contract does not implement the internal _execute function, which is instead implemented in the RollbackManagerTimelockControl contract. However, the RollbackManagerTimelockControl contract’s _execute function forwards the msg.value to the target timelock. Since the execute function is not payable, the msg.value must always be 0. Thus, rollback proposals that intend to transfer native tokens such as ETH may be queued. However, such proposals will not be executable by the Guardians.

Consider adding the payable keyword to the RollbackManager contract’s execute function.

Update: Resolved in pull request #74 at commit 32cf57b.

Non-Payable fallback Contradicts Stated Purpose

The TimelockMultiAdminShim contract implements a fallback function with the explicit purpose of receiving native transfers from the Compound Governor that may occur as part of the _executeOperations internal function call. However, the fallback function has not been declared payable. Thus, transactions sending ETH directly to the TimelockAdminShim contract will always revert.

Consider adding the payable modifier to the fallback function to allow it to receive native token transfers.

Update: Resolved in pull request #75 at commit 5d3f7b7.

Locked ETH

The TimelockMultiAdminShim contract’s executeTransaction function is marked as payable. The intention is that ETH sent during the execution of proposals should be forwarded to the Compound Timelock. However, the executeTransaction implementation does not forward any ETH to the Compound Timelock. Therefore, any ETH forwarded with a call to executeTransaction will be trapped within the TimelockMultiAdminShim contract.

Consider forwarding the native tokens required by the proposed transaction with the call to the executeTransaction function of the Compound Timelock contract.

Update: The fix in pull request #85 at commit 026287d will not resolve the issue as intended. Passing msg.value to the Compound Timelock in the TIMELOCK::executeTransaction call, will always forward 0, since each call to TimelockMultiAdminShim::executeTransaction initiated by the Governor or RollbackManagerTimelockCompound is invoked with msg.value == 0 (the ETH is transferred before the executeTransaction call). There are two valid fixes available. One option is to treat the TimelockMultiAdminShim as the Timelock and the ETH holder and forward each _value of ETH per executeTransaction operation to the Compound Timelock. The other option is to keep the existing Governor + Compound Timelock model, where the TimelockMultiAdminShim transfers its ETH balance to the underlying Compound Timelock once, and calls executeTransaction with zero msg.value. Regardless of the chosen fix, RollbackManagerTimelockCompound::_execute should be updated to use Address.sendValue(payable(TARGET_TIMELOCK), msg.value) to forward ETH from the payable RollbackManager::execute call to the TimelockMultiAdminShim.

Low Severity

Identical Rollback Actions Impossible

The propose function of the RollbackManager contract allows the timelock to propose a set of transactions that can be used to roll back system changes should a bug be discovered after successful execution. The aforementioned propose function performs a check to ensure that the generated _rollbackId has not been used before. The _rollbackId is generated by hashing the encoded rollback proposal transactions. This means that any combination of proposed rollback transactions can only be used once. Should two different proposals have the same proposed rollback transactions, the Compound Timelock will not be able to execute the proposal, as it will revert due to the proposed rollback already existing.

While the possibility of a collision occurring due to different proposed rollback inputs hashing to the same _rollbackId is negligible, the possibility of some proposals proposing the same transactions as a remedy to an issue is greater. Due to the effect this could have on the Governance flow of the protocol, consider implementing a nonce within the generation of the _rollbackId of the RollbackManagerTimelockCompound contract.

Update: Acknowledged, not resolved. The ScopeLift team stated:

The expectation here is that rollbackId will always be unique and description is used to compute the rollbackId. This is the same pattern implemented on the Governor contract.

Executor Can Cancel removeExecutor Transaction in TimelockMultiAdminShim

In TimelockMultiAdminShim, the cancelTransaction function allows both the admin and any configured executor to cancel queued timelock operations, without distinguishing between external calls and operations targeting the shim itself. If a queued transaction invoking removeExecutor(executor) is scheduled, the targeted executor can front-run its execution by calling cancelTransaction, effectively preventing their removal.

A malicious or compromised executor can indefinitely block attempts to remove them and, therefore, can perform other critical configuration changes targeting the shim. This results in a denial-of-service attack against governance or administrative actions and breaks the intended separation of roles between admin and executor.

Consider restricting the cancelTransaction function so that only the admin can cancel operations targeting the TimelockMultiAdminShim contract.

Update: Resolved in pull request #77 at commit 33e54a2.

Missing Grace-Period Awareness Causes Stale Execution Reverts

The RollbackManagerTimelockCompound contract only enforces the lower bound timestamp for execution (i.e., block.timestamp >= executableAt) and never validates the upper bound (GRACE_PERIOD) imposed by the Compound Timelock (block.timestamp <= eta + GRACE_PERIOD). Consequently, even after an operation has become stale from the Compound Timelock’s perspective, the RollbackManager contract’s state view function can keep reporting Queued, and isRollbackExecutable can continue returning true once executableAt is reached, misleading operators.

When the guardian finally calls execute(), the call reverts inside the Compound Timelock contract because the grace period has elapsed. This creates operational confusion and a soft denial-of-service. The Guardian believes queued rollbacks are executable any time after the executableAt timestamp is reached, but execution fails after the grace window, wasting gas and delaying incident response. Recovery then requires canceling and re-proposing.

Consider enforcing the GRACE_PERIOD window in the Compound path. Add an upper-bound check on the block.timestamp before calling the timelock. Optionally, expose Compound path-specific view helpers like rollbackDeadline(rollbackId) or isExecutableWithinGrace(rollbackId) so operators/UIs can check staleness correctly. In addition, consider documenting the fact that the base state()/isRollbackExecutable() does not account for Compound’s grace period and that, in Compound deployments, callers should consult the path specific helper functions, before executing.

Update: Resolved in pull request #78 at commit 9b19f0d.

Notes & Additional Information

Missing Docstrings

The IRollbackManager and ITimelockMultiAdminShim interfaces do not contain any docstrings for the functions listed therein.

To improve clarity and consistency, consider thoroughly documenting all functions (and their parameters) within these two interfaces.

Update: Resolved in pull request #79 at commit d25edb0.

Unnecessary Cast

Within the RollbackManager contract, there is an unnecessary cast of an address value to an address type.

To improve the overall clarity and maintainability of the codebase, consider removing the unnecessary cast.

Update: Resolved in pull request #64 at commit f8a8bc2.

Possible Duplicate Event Emissions

When a setter function does not check if the value being set is different from the existing one, it becomes possible to set the same value repeatedly, creating a possibility for event spamming. Repeated emission of identical events can also confuse off-chain clients.

Throughout the codebase, multiple instances of potential duplicate event emission were identified:

  • The _setGuardian sets the guardian and emits an event without checking if the value has changed.
  • The _setRollbackQueueableDuration sets the rollbackQueueableDuration and emits an event without checking if the value has changed.
  • The _setAdmin sets the admin and emits an event without checking if the value has changed.
  • The _setAdmin sets the admin and emits an event without checking if the value has changed.

Consider adding a check that reverts the transaction if the value being set is the same as the existing one.

Update: Acknowledged, not resolved. The ScopeLift team stated:

Ack but will not fix

Floating Pragma

Pragma directives should be fixed to clearly identify the Solidity version with which the contracts will be compiled.

Throughout the codebase, multiple instances of floating pragma directives were identified:

  • RollbackManager.sol has the solidity ^0.8.30 floating pragma directive.
  • RollbackManagerTimelockCompound.sol has the solidity ^0.8.30 floating pragma directive.
  • RollbackManagerTimelockControl.sol has the solidity ^0.8.30 floating pragma directive.
  • IRollbackManager.sol has the solidity ^0.8.30 floating pragma directive.
  • ITimelockMultiAdminShim.sol has the solidity ^0.8.30 floating pragma directive.
  • ITimelockTargetCompound.sol has the solidity ^0.8.30 floating pragma directive.
  • ITimelockTargetControl.sol has the solidity ^0.8.30 floating pragma directive.

Consider using fixed pragma directives.

Update: Acknowledged, not resolved. The ScopeLift team stated:

The is intended to be used for other integrations and hence sticking to floating pragma instead of fixing it

Incomplete Docstrings

Within RollbackManager.sol, multiple instances of incomplete docstrings were identified:

  • The return values of the state function are not documented.
  • The return values of the isRollbackExecutable function are not documented.

For improved code clarity and consistency, consider adding the return values to the docstrings of the aforementioned functions.

Update: Resolved in pull request #76 at commit d531287.

Lack of Indexed Event Parameter

Within RollbackManager.sol, the RollbackQueueableDurationSet event does not have indexed parameters.

To improve the ability of off-chain services to search and filter for specific events, consider indexing event parameters.

Update: Acknowledged, not resolved. The ScopeLift team stated:

RollbackQueueableDurationSet event does not require search / filter for the params of this event

Comment References Non-Existent RollbackCreated Event

The docstring above the getRollbackId function states that the rollback ID can be reconstructed from the RollbackCreated event. However, no such event is declared in the contract (the correct event is RollbackProposed).

Consider correcting the docstring so that it refers to the RollbackProposed event instead of the RollbackCreated event.

Update: Resolved in pull request #67 at commit 6408ba5.

Missing Two-Step Admin Handover in RollbackManager and TimelockMultiAdminShim

Both the RollbackManager and TimelockMultiAdminShim contracts change admin in a single call without a pendingAdmin/acceptAdmin flow. This differs from Compound Timelock’s two-step pattern and allows for the immediate replacement of the admin (by the current admin in RollbackManager or by the timelock in TimelockMultiAdminShim) with no positive acknowledgment from the new admin and limited protection against misconfiguration. As a result, the operational and security risk increases since an approved but mistaken address (or an unresponsive contract) can be set as admin, potentially blocking admin-gated actions (e.g., TimelockMultiAdminShim self-targeted ops).

Consider implementing a two-step admin transfer process in both RollbackManager and TimelockMultiAdminShim contracts.

Update: Acknowledged, not resolved. The ScopeLift team stated:

Ack but will not fix as this change would happen via governance.

Comment Contradicts Implemented Logic

Inside the RollbackManager contract’s cancel function, the inline comment states, “Revert if the rollback has been queued”. However, the condition reverts when the rollback is not queued. While the code is correct, the comment states the opposite.

Consider updating the comment to describe the implemented logic. In this case, “Revert if the rollback has been queued” should most likely state, “Revert if the rollback has not been queued”.

Update: Resolved in pull request #68 at commit 727322b.

Missing Named Parameters in Mapping

Since Solidity 0.8.18, mappings can include named parameters to provide more clarity about their purpose. Named parameters allow mappings to be declared in the form mapping(KeyType KeyName? => ValueType ValueName?). This feature enhances code readability and maintainability.

In the isExecutor state variable of the TimelockMultiAdminShim contract, the mapping does not have any named parameters.

Consider adding named parameters to mappings in order to improve the readability and maintainability of the codebase.

Update: Resolved in pull request #69 at commit 436012f.

Missing Security Contact

Providing a specific security contact (such as an email address or ENS name) within a smart contract significantly simplifies the process for individuals to communicate if they identify a vulnerability in the code. This practice is quite beneficial as it permits the code owners to dictate the communication channel for vulnerability disclosure, eliminating the risk of miscommunication or failure to report due to a lack of knowledge on how to do so. In addition, if the contract incorporates third-party libraries and a bug surfaces in those, it becomes easier for their maintainers to contact the appropriate person about the problem and provide mitigation instructions.

Throughout the codebase, none of the in-scope contracts have a declared security contact. Consider adding a NatSpec comment containing a security contact above each contract definition. Using the @custom:security-contact convention is recommended as it has been adopted by the OpenZeppelin Wizard and the ethereum-lists.

Update: Resolved in pull request #70 at commit c8ebbad.

Conclusion

The Governance Emergency Rollback system introduces a structured, timelock-integrated process for reverting executed proposals in both Compound-style and OpenZeppelin Governor deployments, with clearly defined roles separating configuration and execution responsibilities. The design enforces existing delay and expiry rules, ensuring that governance actions remain secure while providing an emergency path for rollback operations.

During this review, particular emphasis was placed on Compound-related changes as these introduce significant modifications to the security model of the Compound Timelock. Three medium-severity issues and several low-severity findings were identified, primarily related to native token transfers via payable functions, GRACE_PERIOD consistency, conditional checks, and optimizations. All but one of these issues have been addressed: M-03 remains unresolved in the current implementation, though the audit team has provided a recommended fix. The development team is encouraged to apply this correction to ensure full alignment with the intended security model. Overall, the implementation now incorporates the necessary safeguards and tighter role governance, and the development team is appreciated for delivering a clearly documented, well-tested, and well-structured codebase.

2 Likes