Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GuildVetoGovernor's timelock doesn't need DAO proposal to be changed #740

Closed
c4-bot-4 opened this issue Dec 27, 2023 · 6 comments
Closed
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working edited-by-warden insufficient quality report This report is not of sufficient quality unsatisfactory does not satisfy C4 submission criteria; not eligible for awards

Comments

@c4-bot-4
Copy link
Contributor

c4-bot-4 commented Dec 27, 2023

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/governance/GuildVetoGovernor.sol#L99-L102

Vulnerability details

Impact

GuildVetoGovernor and GuildGovernor can point to different timelock contracts. This happens due to a possible state diversion in the code where changing the timelock reference contract in one place doesn't affect the other. This could result in voters vetoing changes in the wrong timelock without realizing it.

Proof of Concept

Currently, in order to update GuildVetoGovernor's timelock contract, someone who is GOVERNOR needs to just call the updateTimelock function. This action is instantaneous and doesn't require creating a proposal.

function updateTimelock(address newTimelock) external onlyCoreRole(CoreRoles.GOVERNOR) {
  _updateTimelock(newTimelock);
}

function _updateTimelock(address newTimelock) private {
  emit TimelockChange(timelock, newTimelock);
  timelock = newTimelock;
}

To change GuildGovernor's timelock though, one needs to create a DAO proposal and pass the quorum of votes needed in order to get it through. Admins can't change it directly.

/**
 * @dev Public endpoint to update the underlying timelock instance. Restricted to the timelock itself, so updates
 * must be proposed, scheduled, and executed through governance proposals.
 *
 * CAUTION: It is not recommended to change the timelock while there are other queued governance proposals.
 */
function updateTimelock(TimelockController newTimelock) external virtual onlyGovernance {
  _updateTimelock(newTimelock);
}

function _updateTimelock(TimelockController newTimelock) private {
  emit TimelockChange(address(_timelock), address(newTimelock));
  _timelock = newTimelock;
}

The behavior is forced by the onlyGovernance modifier (OZ's Governor.sol):

modifier onlyGovernance() {
  require(_msgSender() == _executor(), "Governor: onlyGovernance");
  if (_executor() != address(this)) {
      bytes32 msgDataHash = keccak256(_msgData());
      // loop until popping the expected operation - throw if deque is empty (operation not authorized)
      while (_governanceCall.popFront() != msgDataHash) {}
  }
  _;
}

function _executor() internal view virtual returns (address) {
  return address(this);
}

POC (use GuildVetoGovernor.t.sol):

function testTimelockGetsChangedWhileVoting() public {
  // It will be granted GOVERNOR upon deployment
  // look at GIP_0.sol:321
  vm.prank(governorAddress);
  core.grantRole(CoreRoles.GOVERNOR, address(timelock));

  // set up proposer
  address voter1 = makeAddr('voter1');

  // schedule an action in the timelock
  // this is a proposal to change the governor
  address[] memory targets = new address[](1);
  targets[0] = address(core);
  uint256[] memory values = new uint256[](1);
  bytes[] memory payloads = new bytes[](1);
  payloads[0] = abi.encodeWithSelector(core.grantRole.selector, CoreRoles.GOVERNOR, address(voter1));
  bytes32 predecessor = bytes32(0);
  bytes32 salt = keccak256(bytes('add new governor'));
  timelock.scheduleBatch(targets, values, payloads, predecessor, salt, _TIMELOCK_MIN_DELAY);
  bytes32 timelockId = timelock.hashOperationBatch(targets, values, payloads, 0, salt);

  // voter1 is not governor yet
  assert(!core.hasRole(CoreRoles.GOVERNOR, address(voter1)));

  // create a veto proposal
  vm.roll(block.number + 1);
  vm.warp(block.timestamp + 10);
  token.mockSetVotes(address(voter1), _VETO_QUORUM);
  vm.prank(voter1);
  uint256 proposalId = governor.createVeto(timelockId);
  assertEq(uint256(governor.state(proposalId)), uint256(IGovernor.ProposalState.Pending));

  // cannot vote in the same block as the propose()
  // on next block, the vote is active
  // we vote and reach quorum
  vm.roll(block.number + 1);
  vm.warp(block.timestamp + 10);
  assertEq(uint256(governor.state(proposalId)), uint256(IGovernor.ProposalState.Active));

  vm.prank(voter1);
  governor.castVote(proposalId, uint8(GuildVetoGovernor.VoteType.Against));
  assertEq(uint256(governor.state(proposalId)), uint256(IGovernor.ProposalState.Succeeded));

  // an admin changes the timelock
  vm.startPrank(governorAddress);
  core.grantRole(CoreRoles.TIMELOCK_CANCELLER, address(this));
  GuildTimelockController newTimelock = new GuildTimelockController(address(core), 12345);
  governor.updateTimelock(address(newTimelock));
  vm.stopPrank();

  // cannot execute veto, it is too late
  vm.expectRevert('Governor: unknown proposal id');
  governor.executeVeto(timelockId);

  // the timelock action can be executed
  assertEq(timelock.isOperation(timelockId), true);
  assertEq(timelock.isOperationPending(timelockId), true);
  assertEq(timelock.isOperationReady(timelockId), false);
  assertEq(timelock.isOperationDone(timelockId), false);

  // wait for the timelock to be ready
  vm.roll(block.number + 1);
  vm.warp(block.timestamp + _TIMELOCK_MIN_DELAY);

  // execute
  vm.prank(voter1);
  timelock.executeBatch(targets, values, payloads, predecessor, salt);

  // voter1 is now governor
  assert(core.hasRole(CoreRoles.GOVERNOR, address(voter1)));
}

This causes the following issues:

  1. Admins change the timelock in GuildVetoGovernor but GuildGovernor isn't pointing to the new one. This may cause users unaware of this to veto a proposal from the new timelock without realizing it's not associated with the DAO's current timelock.
  2. Users may lose some trust in the protocol. If the GuildVetoGovernor changes while the DAO's one hasn't, then they lose the ability to veto in the old timelock. The old one could still execute already queued changes that they didn't like.
  3. A 51% attack that the guardian didn't notice but vetoers did (Unlikely).

Tools Used

Manual Analysis

Recommended Mitigation Steps

  1. Remove updateTimelock from GuildVetoGovernor. Think of a way to bulk-update both when such a proposal is created. Maybe through some code override in TimelockController, or maybe a separate function in GuildGovernor that is specifically meant for creating such proposals. Then, strip the old Timelock from its roles so it can't execute actions anymore.
  2. The more centralized way of solving this would be to override the function in GuildGovernor as well so both can be updated by an admin at the same time, in order to avoid the desynchronization issue.

Assessed type

Governance

@c4-bot-4 c4-bot-4 added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Dec 27, 2023
c4-bot-3 added a commit that referenced this issue Dec 27, 2023
@0xSorryNotSorry
Copy link

As per the docs, Governor is trusted with the actions.

@c4-pre-sort
Copy link

0xSorryNotSorry marked the issue as insufficient quality report

@c4-pre-sort c4-pre-sort added the insufficient quality report This report is not of sufficient quality label Jan 4, 2024
@Trumpero
Copy link

Governor is trusted, and this issue is an instance of Governor's centralization risk ([M-01] Centralization risk for privileged functions)
-> OOS

@c4-judge c4-judge added the unsatisfactory does not satisfy C4 submission criteria; not eligible for awards label Jan 24, 2024
@c4-judge
Copy link
Contributor

Trumpero marked the issue as unsatisfactory:
Invalid

@0xNentoR
Copy link

0xNentoR commented Feb 1, 2024

Hi @Trumpero,

I believe the context of this submission was not fully understood during the presort phase. There are two possible scenarios covered. I do agree that admin actions are trusted and that them being able to change the timelock of GuildVetoGovernor is a trusted action.

However, the issue here is bigger than that. While it's possible for them to do it, it's not possible for them to do the same with GuildGovernor. It's because OpenZeppelin's Governor contract, which it inherits from, allows for this action to be performed only through a DAO proposal. This leads to the problem where if a proposal goes through that changes the timelock controller of GuildGovernor, it will get desynchronized from GuildVetoGovernor's and leave it unable to cast vetos for actions in the new timelock until it gets changed there as well.

I've covered both sides of the coin in the submission. I believe both GuildGovernor and GuildVetoGovernor should be synced with the same timelock controller at all times. The solution can be centralized (them being able to change both) or decentralized (both getting changed when such an action is queued).

@Trumpero
Copy link

Trumpero commented Feb 4, 2024

Forwarding this statement from contest docs

Trusted actors: The addresses with GOVERNOR and GUARDIAN role are trusted. DAO timelock can execute arbitrary calls and impersonate all protocol contracts, as it holds the GOVERNOR role. The team multisig has the GUARDIAN role, and it is expected to be able to freeze new usage of the protocol, but not prevent users from withdrawing their funds.

The Governor role in the protocol is GuildTimelockController, which serves as the timelock for GuildGovernor. Therefore, updating the timelock of GuildVetoGovernor is similar with GuildGovernor and already decentralized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working edited-by-warden insufficient quality report This report is not of sufficient quality unsatisfactory does not satisfy C4 submission criteria; not eligible for awards
Projects
None yet
Development

No branches or pull requests

7 participants