Public state-modifying `cancel()` function may lead to future vulnerabilties

[c4-bot-10]

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/governance/GuildVetoGovernor.sol#L234-L267

Vulnerability details

Impact

The GuildVetoGovernor contract has a public state-modifying cancel() function inherited from the Governor contract. This function sets the proposal.canceled boolean, leading to Governor.state() returning Proposal.Canceled.

In GuildVetoGovernor.state()'s NatSpec documentation it is stated that the state can be one of:

    - [...]
    - ProposalState.Canceled  (2) If a veto was created but the timelock action has been cancelled through another mean before the veto vote succeeded. The internal `_cancel()` function is not reachable by another mean (no public `cancel()` function), so this is the only case where a proposal will have Canceled status.

And in the inline documentation:

// Proposal cannot be Canceled because there is no public cancel() function.

// at this stage, status from super can be one of: Active, Succeeded, Defeated

These assumptions aren't true as super.state() can indeed return ProposalState.Canceled if the proposer has called the public cancel() function. While this does not affect the current functionality of GuildVetoGovernor, it could potentially lead to vulnerabilities in future code updates, as the developers might not account for the Proposal.Canceled state.

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/governance/GuildVetoGovernor.sol#L234-L267 https://github.com/OpenZeppelin/openzeppelin-contracts/blob/fd81a96f01cc42ef1c9a5399364968d0e07e9e90/contracts/governance/Governor.sol#L348

Proof of Concept

Consider a scenario where a future update to the GuildVetoGovernor contract introduces additional logic that depends on the state of a proposal returned by super.state(). If the developers are not aware that Proposal.Canceled is a possible state, they might not account for it in their new logic, leading to unexpected behavior or vulnerabilities.

Tools Used

Manual review

Recommended Mitigation Steps

Override the cancel() function in the GuildVetoGovernor contract to prevent its usage, as it is not intended to be used in this context. This can be done by adding a cancel() function in GuildVetoGovernor that simply reverts with an "Unimplemented" message.

Assessed type

Other

[c4-pre-sort]

0xSorryNotSorry marked the issue as sufficient quality report

[c4-pre-sort]

0xSorryNotSorry marked the issue as duplicate of #1125

[c4-judge]

Trumpero marked the issue as not a duplicate

[c4-judge]

Trumpero marked the issue as primary issue

[Trumpero]

This issue and its duplicates aren't dups of #1125, and I believe they are invalid. The GuildVetoGovernor.cancel() can be called but doesn't have any impact, as the contract doesn't handle the canceled state in code. Additionally, GuildVetoGovernor.createVeto() is unable to be exploited by front-running and canceling since it doesn't have any delay time for proposing.

[c4-judge]

Trumpero marked the issue as unsatisfactory: Invalid

[0xEVom]

Hi @Trumpero, would you ming marking this one QA rather than invalid, since it explicitly mentions that calling cancel() does not have any impact and simply warns against the explicit assumption in the code that there is no such function?

I would also appreciate it if you could reevaluate my QA findings, since I have a large number of submissions that have been downgraded and I wonder if I don't at least qualify for a B. Thanks in advance.

[Trumpero]

@0xEVom In my evaluation of QA reports, I assign 5 points for a low, 1 point for NC/R, and 0 points for info. You don't have enough low issues in your downgraded issues to surpass the threshold of grade-b in this contest (25 points), so all downgraded issues are marked as grade-c. Even if I accept this issue an info, your QA points won't change.

The 25-point threshold is adjusted from 60% of the maximum points in this contest (40 points of best QA report).