Summary
The Governance::_queueProposal function checks for isOperationPending and reverts with ProposalAlreadyExecuted, while Governance::_executeProposal checks for !isOperationReady and reverts with ProposalNotQueued. These conditions and error messages are mismatched with their intended functionality.
Vulnerability Details
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
);
if (_timelock.isOperationPending(id)) {
@> revert ProposalAlreadyExecuted(proposalId, block.timestamp);
}
_timelock.scheduleBatch(
proposal.targets,
proposal.values,
proposal.calldatas,
bytes32(0),
salt,
_timelock.getMinDelay()
);
emit ProposalQueued(proposalId, block.timestamp, id);
}
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
);
if (!_timelock.isOperationReady(id)) {
@> revert ProposalNotQueued(proposalId, id);
}
_timelock.executeBatch(
proposal.targets,
proposal.values,
proposal.calldatas,
bytes32(0),
salt
);
proposal.executed = true;
emit ProposalExecuted(proposalId, msg.sender, block.timestamp);
}
Impact
In _queueProposal, checking isOperationPending and reverting with ProposalAlreadyExecuted is incorrect because a pending operation means it's queued, not executed and the error message suggests execution when it's actually checking for queueing.
In _executeProposal, checking !isOperationReady and reverting with ProposalNotQueued is incomplete because it doesn't check if the proposal was already executed and a proposal could be queued but not yet ready (delay not elapsed).
The mismatch between conditions and error messages can lead monitoring systems and front-end applications to process incorrect information.
Tools Used
Manual review
Recommendations
Modify the revert conditions and messages to properly reflect the state checks.
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);
+ revert ProposalNotQueued(proposalId, id);
}
// Schedule in timelock
_timelock.scheduleBatch(
proposal.targets,
proposal.values,
proposal.calldatas,
bytes32(0),
salt,
_timelock.getMinDelay()
);
emit ProposalQueued(proposalId, block.timestamp, id);
}
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);
+ revert ProposalAlreadyExecuted(proposalId, block.timestamp);
}
// Execute through timelock
_timelock.executeBatch(
proposal.targets,
proposal.values,
proposal.calldatas,
bytes32(0),
salt
);
proposal.executed = true;
emit ProposalExecuted(proposalId, msg.sender, block.timestamp);
}
