Core Contracts

Summary

The Governance.sol contract has two critical issues related to proposal execution:

1. State Update Dependency: The execute function in the Governance contract relies on the _executeProposal function to mark a proposal as executed. However, if the proposal is executed directly through the TimelockController contract, the state in the Governance contract will not be updated, leading to inconsistencies.

2. Ether Transfer Failure: The Governance contract has no way to receive Ether, causing proposals that require Ether transfers to fail. This is because the executeBatch function in the TimelockController contract requires Ether to be sent along with the transaction, but the Governance contract cannot facilitate this.

Vulnerability Details

Explanation

1. State Update Dependency

When a proposal is executed, the Governance contract calls the _executeProposal function, which in turn calls the executeBatch function in the TimelockController contract. The _executeProposal function marks the proposal as executed (proposal.executed = true) only if the execution is initiated through the Governance contract. If the proposal is executed directly through the TimelockController, the state in the Governance contract will not be updated, leading to inconsistencies.

2. Ether Transfer Failure

The executeBatch function in the TimelockController contract requires Ether to be sent along with the transaction if the proposal involves Ether transfers. However, the Governance contract has no way to receive or forward Ether, causing such proposals to fail.

Root Cause in the Contract Function

1. State Update Dependency:

   • The _executeProposal function in the Governance contract marks the proposal as executed:

     function _executeProposal(uint256 proposalId) internal {

     // ... (other logic)

     _timelock.executeBatch(

     proposal.targets,

     proposal.values,

     proposal.calldatas,

     bytes32(0),

     salt

     );

     proposal.executed = true; //@audit-issue If it requires ethers, it will not go through in executeBatch

     emit ProposalExecuted(proposalId, msg.sender, block.timestamp);

     }

   • If the proposal is executed directly through the TimelockController, the proposal.executed flag will not be set.

2. Ether Transfer Failure:

   • The executeBatch function in the TimelockController contract requires Ether to be sent along with the transaction:

     function executeBatch(

     address[] calldata targets,

     uint256[] calldata values,

     bytes[] calldata calldatas,

     bytes32 predecessor,

     bytes32 salt

     ) external override payable nonReentrant onlyRole(EXECUTOR_ROLE) { //@audit-issue No way to send Ether since no way to deposit ether. Needs to be called from Governance so it can be marked as executed

     // ... (other logic)

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

     }

     }

     // ... (other logic)

     }

   • The Governance contract cannot send Ether, causing proposals that require Ether transfers to fail. The Governance::execute function is not payable.

Proof of Concept

Scenario Example

1. State Update Dependency:

   • A proposal is executed directly through the TimelockController contract.

   • The proposal.executed flag in the Governance contract is not updated, leading to inconsistencies.

2. Ether Transfer Failure:

   • A proposal involves transferring Ether to a target address.

   • The executeBatch function in the TimelockController contract requires Ether to be sent along with the transaction.

   • The Governance contract cannot send Ether, causing the proposal to fail.

Impact

• State Inconsistencies: Proposals executed directly through the TimelockController will not be marked as executed in the Governance contract, leading to inconsistencies.

• Failed Proposals: Proposals that require Ether transfers will fail, as the Governance contract cannot send Ether.

Tools Used

• Manual Code Review: The vulnerabilities were identified through a manual review of the Governance and TimelockController contracts.

Recommendations

1. Enable Ether Transfers:

   • Modify the Governance contract to receive and forward Ether for proposals that require Ether transfers. This can be done by making the execute function payable and forwarding the Ether to the TimelockController.