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

Missing payable Modifier in Governance::execute() Prevents Ether Transactions

josh4324

Summary

The execute() function in the governance contract is responsible for executing proposals, including those that may involve sending Ether. However, it lacks the payable modifier, meaning the function cannot send Ether as part of a proposal execution.

Additionally, execution is delegated to the Timelock contract, which does not have a receive() function. This means that if a proposal attempts to transfer Ether, the transaction will fail because the contract cannot receive Ether so no ether will be in the contract.

Vulnerability Details

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"
);
}
}
​
// Timelock
​
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);
​
Operation storage op = _operations[id];
if (op.timestamp == 0) revert OperationNotFound(id);
if (op.executed) revert OperationAlreadyExecuted(id);
if (block.timestamp < op.timestamp) revert OperationNotReady(id);
if (block.timestamp > op.timestamp + GRACE_PERIOD) revert OperationExpired(id);
​
if (predecessor != bytes32(0)) {
if (!isOperationDone(predecessor)) {
revert PredecessorNotExecuted(predecessor);
}
}
​
op.executed = true;
​
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 execute() function is not payable, which means it cannot send Ether as part of a proposal execution. If a proposal involves transferring Ether to a contract or an address, the execution will fail.

Since execution is passed to _timelock.executeBatch(), the Timelock contract must hold Ether. However, the Timelock contract does not implement receive() or fallback() to accept Ether, leading to transaction failures.

Even if the proposal were executed directly on the Timelock contract by an admin with executor role, it would not update the proposal executed state in the governance contract.

This means the proposal state would indicate that it has not been executed in governance, causing inconsistencies.

Impact

Proposals that involve sending ETH will fail, rendering governance unable to execute critical transactions.

Tools Used

Manual Review

Recommendations

Modify the execute() function to include the payable modifier.

Updates

Lead Judging Commences

inallhonesty Lead Judge 7 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 7 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.

Give us feedback!