Description
Governance allows users to cancel proposals through the cancel function. However, if the proposal's underlying operation has already been queued, it will not be canceled. As a result, any address with the EXECUTOR_ROLE can still execute the operation.
Context
Impact
High. Any canceled proposal with an already queued operation can still be executed, compromising the integrity of the governance mechanism.
Likelihood
Medium. The lack of cancellation for underlying operations will occur for every canceled proposal with an already queued operation. However, only addresses with the EXECUTOR_ROLE will be able to execute them.
Proof of Concept
pragma solidity ^0.8.19;
import {Test, console} from "../../lib/forge-std/src/Test.sol";
import {Governance, IGovernance} from "../../contracts/core/governance/proposals/Governance.sol";
import {TimelockController} from "../../contracts/core/governance/proposals/TimelockController.sol";
import {RAACToken} from "../../contracts/core/tokens/RAACToken.sol";
import {veRAACToken, IveRAACToken} from "../../contracts/core/tokens/veRAACToken.sol";
contract GovernanceTest is Test {
Governance governance;
TimelockController timelock;
RAACToken raac;
veRAACToken veRaac;
address OWNER = makeAddr("owner");
address PROPOSER = makeAddr("proposer");
address EXECUTOR = makeAddr("executor");
address USER = makeAddr("user");
function setUp() public {
vm.startPrank(OWNER);
address[] memory proposers = new address[](1);
proposers[0] = PROPOSER;
address[] memory executors = new address[](1);
executors[0] = EXECUTOR;
timelock = new TimelockController(2 days, proposers, executors, OWNER);
raac = new RAACToken(OWNER, 0, 0);
veRaac = new veRAACToken(address(raac));
governance = new Governance(address(veRaac), address(timelock));
timelock.grantRole(timelock.PROPOSER_ROLE(), address(governance));
timelock.grantRole(timelock.EXECUTOR_ROLE(), address(governance));
vm.stopPrank();
}
function test_proposalOperationNotCanceled_cancel() public {
vm.mockCall(
address(veRaac),
abi.encodeWithSelector(bytes4(keccak256("getVotingPower(address)"))),
abi.encode(100_000e18)
);
address[] memory targets = new address[](1);
uint256[] memory values = new uint256[](1);
bytes[] memory calldatas = new bytes[](1);
string memory description = "Some Proposal";
IGovernance.ProposalType proposalType = IGovernance.ProposalType.ParameterChange;
vm.startPrank(PROPOSER);
uint256 proposalId = governance.propose(targets, values, calldatas, description, proposalType);
skip(1 days);
governance.castVote(proposalId, true);
vm.warp(governance.getProposal(proposalId).endTime);
governance.execute(proposalId);
governance.cancel(proposalId);
assertEq(uint256(governance.state(proposalId)), uint256(IGovernance.ProposalState.Canceled));
bytes32 operationId = timelock.hashOperationBatch(
targets, values, calldatas, bytes32(0), governance.getProposal(proposalId).descriptionHash
);
assertTrue(timelock.isOperationPending(operationId));
}
}
Instructions
First, integrate Foundry by running the following commands in your terminal, in the project's root directory:
mkdir out lib
git submodule add https://github.com/foundry-rs/forge-std lib/forge-std
touch foundry.toml
Next, configure Foundry by adding the following settings to foundry.toml:
[profile.default]
src = "contracts"
out = "out"
lib = "lib"
After that, create a foundry/ directory inside the test/ directory. Inside foundry/, create the following files:
Finally, paste the provided (PoC) into GovernanceTest.t.sol and run:
forge test --mt test_proposalOperationNotCanceled_cancel -vvv
Recommendation
Consider canceling the proposal's underlying operation in the cancel function if it has been already queued.
function cancel(uint256 proposalId) external override {
ProposalCore storage proposal = _proposals[proposalId];
if (proposal.startTime == 0) revert ProposalDoesNotExist(proposalId);
ProposalState currentState = state(proposalId);
if (currentState == ProposalState.Executed) {
revert InvalidProposalState(proposalId, currentState, ProposalState.Active, "Cannot cancel executed proposal");
}
+ if (currentState == ProposalState.Queued) {
+ bytes32 opId = _timelock.hashOperationBatch(
+ proposal.targets, proposal.values, proposal.calldatas, bytes32(0), proposal.descriptionHash
+ );
+ _timelock.cancel(opId);
+ }
// Only proposer or if proposer's voting power dropped below threshold
if (msg.sender != proposal.proposer &&
_veToken.getVotingPower(proposal.proposer) >= proposalThreshold) {
revert InsufficientProposerVotes(proposal.proposer,
_veToken.getVotingPower(proposal.proposer), proposalThreshold, "Proposer lost required voting power");
}
proposal.canceled = true;
emit ProposalCanceled(proposalId, msg.sender, "Proposal canceled by proposer");
}
