Unmitigated Timelock Admin#

Medium Risk

While conventionally the contract admin will remain the governor contract itself, there are two functions (_setPendingAdmin, and _acceptAdmin) exposed in Governor.sol which have the ability to update the admin to any address. This admin has direct access to timelocking functionality and can schedule arbitrary transactions.

/**
  * @notice Begins transfer of admin rights. The newPendingAdmin must call `_acceptAdmin` to finalize the transfer.
  * @param newPendingAdmin New pending admin.
  */
function _setPendingAdmin(address newPendingAdmin) external {
    // Check caller = admin
    require(msg.sender == admin, "Governor::_setPendingAdmin: admin only");

    // Save current value, if any, for inclusion in log
    address oldPendingAdmin = pendingAdmin;

    // Store pendingAdmin with value newPendingAdmin
    pendingAdmin = newPendingAdmin;

    // Emit NewPendingAdmin(oldPendingAdmin, newPendingAdmin)
    emit NewPendingAdmin(oldPendingAdmin, newPendingAdmin);
}

/**
  * @notice Accepts transfer of admin rights. msg.sender must be pendingAdmin
  */
function _acceptAdmin() external {
    // Check caller is pendingAdmin and pendingAdmin ≠ address(0)
    require(
        msg.sender == pendingAdmin && msg.sender != address(0),
        "Governor::_acceptAdmin: pending admin only"
    );

    // Save current values for inclusion in log
    address oldAdmin = admin;
    address oldPendingAdmin = pendingAdmin;

    // Store admin with value pendingAdmin
    admin = pendingAdmin;

    // Clear the pending value
    pendingAdmin = address(0);

    emit NewAdmin(oldAdmin, admin);
    emit NewPendingAdmin(oldPendingAdmin, pendingAdmin);
}

If a malicious actor is able to pass a proposal to grant themselves admin either through honest governance, phishing, or otherwise, they have arbitrary control over the governance contract and can drain any tokens / ether held in the contract.

What is more, there is no throttling on how often a compromised admin might queue transactions via the timelock. This could come in handy if the attacker is ever in a race with honest governance participants to perform some action, potentially making exploits more damaging.

For instance, say the governance treasury has some form of immutable income stream. If a malicious admin has several "withdraw ETH" transactions queued which have already cleared the timelock window, they might have an advantage over honest governance participants to be first to withdraw when new ether lands in the contract. Honest governance participants are throttled to one active proposal at a time per user, and thus have to coordinate among several users in order to queue as many transactions as the malicious admin.

Recommendation#

Consider adding some checks and balances on the contract admin in case it is compromised.

For instance, you might implement a "Pause Guardian" address which has the ability to pause admin execution.

bool pausedByGuardian = false;
address pauseGuardian = 0x...;

function _pauseAdminTimelock() {
  require(msg.sender === pauseGuardian, "Only pause guardian can pause");
  pausedByGuardian = true;
}

function _unpauseAdminTimelock() {
  require(msg.sender === pauseGuardian, "Only pause guardian can unpause");
  pausedByGuardian = false;
}

function _executeTransaction(
    address target,
    uint256 value,
    bytes memory data,
    uint256 eta
) public payable returns (bytes memory) {
    require(
        msg.sender == admin,
        "Governor::executeTransaction: Call must come from admin"
    );
    require(!pausedByGuardian, "Timelock paused by guardian");
...

Additionally, consider throttling the number of active transactions a timelock admin can queue to one at at time (this may not be necessary if there is a pause guardian).