Core Contracts

Summary

The execute function in the Governance contract is not marked as payable, even though it interacts with the TimeLock contract's executeBatch function, which is designed to handle transactions involving Ether (via the values parameter). This oversight prevents the execution of proposals that involve Ether transfers, breaking a core functionality of the governance system.

Vulnerability Details

Affected Code

1. Governance Contract:

   • The execute function is not marked as payable.

   • It calls _executeProposal, which interact with the TimeLock contract.

   function execute(uint256 proposalId) external override nonReentrant {

   ProposalCore storage proposal = _proposals[proposalId];

   if (proposal.executed) revert ProposalAlreadyExecuted(proposalId, block.timestamp);

   ProposalState currentState = state(proposalId);

   if (currentState == ProposalState.Succeeded) {

   _queueProposal(proposalId);

   } else if (currentState == ProposalState.Queued) {

   _executeProposal(proposalId);

   } else {

   revert InvalidProposalState(

   proposalId,

   currentState,

   currentState == ProposalState.Active ? ProposalState.Succeeded : ProposalState.Queued,

   "Invalid state for execution"

   );

   }

   }

2. TimeLock Contract:

   • The _executeProposal function accepts a values array, which can include non-zero values for Ether transfers.

   • This function is called by _queueProposal in the Governance contract.

   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);

   }

   } // audi if predecessor is canceled or expired

   // this will revert

   ​

   // Mark as executed before external calls

   op.executed = true;

   ​

   // Execute each call

   for (uint256 i = 0; i < targets.length; i++) {

   (bool success, bytes memory returndata) = targets[i].call{value: values[i]}(calldatas[i]);

   if (!success) {

   revert CallReverted(id, i);

   }

   }

   ​

   emit OperationExecuted(id, targets, values, calldatas, predecessor, salt);

   }

Root Cause

The execute function in the Governance contract is not marked as payable, even though it interacts with the TimeLock contract's executeBatch function, which is designed to handle Ether transfers. When a proposal involves sending Ether (i.e., values contains non-zero values), the execute function will revert because it cannot accept Ether.

Impact

Functionality Breakage:

• Proposals that involve Ether transfers cannot be executed, rendering the governance system ineffective for such proposals.

Tools Used

Manual Code Review

###Recommendations

Immediate Fix

Mark the execute function as payable to allow it to handle Ether transfers. This ensures compatibility with the TimeLock contract's functionality.