Core Contracts

Summary

When executing successful proposals, the RAAC protocol implements a time-lock mechanism. A proposal can only be scheduled if the predecessor has been executed. However, the value of the predecessor is hardcoded - all proposals can be scheduled and executed even if the previous batch has not been executed. The proposal queueing hence works incorrectly.

Vulnerability Details

executeBatch()

Assume this scenario:

1. execute()function in Goveernance.sol has been called

2. state()function is called to determine the state of the proposal. This function returns the state as ProposalState.Queued

3. Back to execute()function, due to the above returned state, _executeProposal()will be called

   1. In the code snippet below from _executeProposal(), function checks if it is ready for execution. Assume _timelock.isOperationReady(id) = true and revert will not occur

      // Check if ready for execution

      if (!_timelock.isOperationReady(id)) {

      revert ProposalNotQueued(proposalId, id);

      }

      ​

      // Execute through timelock

      _timelock.executeBatch(

      proposal.targets,

      proposal.values,

      proposal.calldatas,

      bytes32(0),

      salt

      );

4. _timelock.executeBatch()will be called, whereby the bytes32(0) is passed as the predecessor.

   1. As seen in line 20 in the snippet below, the function checks for the predecessor whether it has been executed or not

   2. since the value of predecessor is hardcoded to bytes32(0), the actual predecessor is never checked for.

      function executeBatch(

      address[] calldata targets,

      uint256[] calldata values,

      bytes[] calldata calldatas,

      bytes32 predecessor,

      bytes32 salt

      ) external override payable nonReentrant onlyRole(EXECUTOR_ROLE) {

      bytes32 id = hashOperationBatch(targets, values, calldatas, predecessor, salt);

      // Check operation status

      Operation storage op = _operations[id];

      if (op.timestamp == 0) revert OperationNotFound(id);

      if (op.executed) revert OperationAlreadyExecuted(id);

      // Check timing conditions

      if (block.timestamp < op.timestamp) revert OperationNotReady(id);

      if (block.timestamp > op.timestamp + GRACE_PERIOD) revert OperationExpired(id);

      // Check predecessor if specified

      if (predecessor != bytes32(0)) {

      if (!isOperationDone(predecessor)) {

      revert PredecessorNotExecuted(predecessor);

      }

      }

5. The proposal can be executed even if the previous successful proposal has not been executed yet.

scheduleBatch()

The hardcoded predecessor value of bytes32(0) also occurs in _queueProposal()when scheduling a batch. In TimelockController.scheduleBatch()which is internally called in _queueProposal(), it also checks if the predecessor has been executed or not.

Impact

The time-lock and executing mechanism is dysfunctional as it does not require the predecessor to be executed in order to execute or schedule the current proposal.

Tools Used

Manual

Recommendations

Ensure the predecessor value is not hard-coded to bytes32(0).