Core Contracts
Regnum Aurum Acquisition Corp
HardhatReal World AssetsNFT
77,280 USDC
View results
Previous Next
Submission Details
Severity: low
Valid

Timelock contract will not be able to execute any calls if the calldata expects ETH value

avoloder

Summary

TimelockController.sol will fail to execute any proposal where the execution call originates from the Governance.sol contract if the calldata requires an ETH value

Vulnerability Details

TimelockController.sol manages time-delayed execution of governance proposals with role-based access control. In the Governance.sol users with enough voting power can create a new proposal where they provide following parameters:

/**
* @dev Creates a new proposal
* @param targets Array of target addresses for proposal actions
* @param values Array of ETH values for proposal actions
* @param calldatas Array of calldata for proposal actions
* @param description Description of the proposal
* @param proposalType Type of the proposal
* @return proposalId The ID of the created proposal
*/
function propose(
address[] memory targets,
uint256[] memory values,
bytes[] memory calldatas,
string memory description,
ProposalType proposalType
) external override returns (uint256)

Users with enough voting power have the ability to vote either for or against the proposal through function castVote(). At some point in time a user can call execute() function to execute the proposal. There are different checks before the proposal can be executed (like quorum check, status, etc.) and the proposal is first queued in the TimelockController with the following function:

function _queueProposal(uint256 proposalId) internal {
ProposalCore storage proposal = _proposals[proposalId];
bytes32 salt = proposal.descriptionHash;
bytes32 id = _timelock.hashOperationBatch(
proposal.targets,
proposal.values,
proposal.calldatas,
bytes32(0),
salt
);
​
// Check if already queued
if (_timelock.isOperationPending(id)) {
revert ProposalAlreadyExecuted(proposalId, block.timestamp);
}
​
// Schedule in timelock
_timelock.scheduleBatch(
proposal.targets,
proposal.values,
proposal.calldatas,
bytes32(0),
salt,
_timelock.getMinDelay()
);
​
emit ProposalQueued(proposalId, block.timestamp, id);
}

After the delay has passed, user can call execute() function again, this time not to queue the proposal execution (since it has already been queued) but to actually execute it through the TimelockController. The function _executeProposal() in Governance.sol is defined as following:

function _executeProposal(uint256 proposalId) internal {
ProposalCore storage proposal = _proposals[proposalId];
bytes32 salt = proposal.descriptionHash;
bytes32 id = _timelock.hashOperationBatch(
proposal.targets,
proposal.values,
proposal.calldatas,
bytes32(0),
salt
);
​
// 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
);
​
proposal.executed = true;
emit ProposalExecuted(proposalId, msg.sender, block.timestamp);
}

We can see that all the parameters from the proposal (targets, values, calldatas) are passed to the TimelockController without sending any ETH. The problem occurs in the executeBatch() function of TimelockController.sol, which iterates over the targets array, executes each call, and sends the specified ETH value. If any of the proposal actions requires ETH to be sent, these calls will fail if the balance of the TimelockController is smaller then what is requested. Since the TimelockController cannot be funded (i.e., no receive() or fallback() function defined), the only option to be able to execute these target calls is to send ETH when calling executeBatch() function which is payable. As it can be seen from the above defined functions, Governance.sol does not check if the values array contains non-null values and does not send any ETH to TimelockController when executing the proposal, causing these calls to revert.

/**
* @notice Executes a scheduled operation
* @dev Only callable by addresses with EXECUTOR_ROLE
* @param targets Target addresses for calls
* @param values ETH values for calls
* @param calldatas Calldata for calls
* @param predecessor ID of operation that must be executed before
* @param salt Random value for operation ID
*/
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);
}
}
​
// 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);
}

The only workaround is that some other user with EXECUTOR_ROLE executes queued proposals and provides needed ETH. Otherwise, all proposals that are directly executed from the Governance.sol that require ETH will fail.

POC

Make setValue() function in TimelockTestTarget.sol payable, then add this test to the TimelockController.test.js:

it("should fail if the values array contains non-null values", async() => {
​
// override values array
let overrideValues = [ethers.parseEther("1")];
await timelock.connect(proposer).scheduleBatch(
targets,
overrideValues,
calldatas,
ethers.ZeroHash,
ethers.ZeroHash,
MIN_DELAY
);
​
await time.increase(MIN_DELAY);
​
await expect(
timelock.connect(executor).executeBatch(
targets,
overrideValues,
calldatas,
ethers.ZeroHash,
ethers.ZeroHash
)
).to.be.revertedWithCustomError(timelock, "CallReverted");
});

Impact

The impact is high since the likelihood of this occurring is significant (it will happen sooner or later). The actual impact is also high, as the only way to address these proposals is for another executor to execute the call and provide ETH. However, the feasibility of this workaround is highly questionable.

Tools Used

Manual Review

Recommendations

The solution to this problem requires several implementation and reconstruction points, because it is nowhere defined who should provide ETH if the proposal action requires it. Is it the creator of the proposal? Should users who voted for the proposal split the costs?

This needs to be tought through but here are some points, assuming that the user who created a proposal needs to pay the costs for all the actions

  1. Make propose() function payable

  2. When creating a proposal in Governance.sol check if the values array contains non-null values. If yes, make sure that msg.value() is equal to the sum of all values

  3. If the proposal get's rejected, make sure to refund the user who provided necessary ETH for the proposal

  4. When executing the proposal thorugh TimelockController make sure to use low-level call and forward needed ETH to the TimelockController::executeBatch() function so that the function can execute these calls

  5. Make sure to carefully handle reverts and errors to be able to refund the ETH to the original caller and not have them stuck in the contract.

Updates

Lead Judging Commences

inallhonesty Lead Judge 3 months ago
Submission Judgement Published
Validated
Assigned finding tags:

Governance.execute lacks payable modifier and ETH forwarding mechanism, preventing proposals with ETH transfers from being executed through TimelockController

inallhonesty Lead Judge 3 months ago
Submission Judgement Published
Validated
Assigned finding tags:

Governance.execute lacks payable modifier and ETH forwarding mechanism, preventing proposals with ETH transfers from being executed through TimelockController

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.