nobody2018 - Nobody can cast for any proposal

[sherlock-admin]

nobody2018

high

Nobody can cast for any proposal

Summary

[castVote](https://github.com/sherlock-audit/2024-01-olympus-on-chain-governance/blob/main/bophades/src/external/governance/GovernorBravoDelegate.sol#L369)/[[castVoteWithReason](https://github.com/sherlock-audit/2024-01-olympus-on-chain-governance/blob/main/bophades/src/external/governance/GovernorBravoDelegate.sol#L385)](https://github.com/sherlock-audit/2024-01-olympus-on-chain-governance/blob/main/bophades/src/external/governance/GovernorBravoDelegate.sol#L385)/[[castVoteBySig](https://github.com/sherlock-audit/2024-01-olympus-on-chain-governance/blob/main/bophades/src/external/governance/GovernorBravoDelegate.sol#L403)](https://github.com/sherlock-audit/2024-01-olympus-on-chain-governance/blob/main/bophades/src/external/governance/GovernorBravoDelegate.sol#L403) are used to vote for the specified proposal. These functions internally call [castVoteInternal](https://github.com/sherlock-audit/2024-01-olympus-on-chain-governance/blob/main/bophades/src/external/governance/GovernorBravoDelegate.sol#L433-L437) to perform voting logic. However, castVoteInternal can never be executed successfully.

Vulnerability Detail

File: bophades\src\external\governance\GovernorBravoDelegate.sol
433:     function castVoteInternal(
434:         address voter,
435:         uint256 proposalId,
436:         uint8 support
437:     ) internal returns (uint256) {
......
444:         // Get the user's votes at the start of the proposal and at the time of voting. Take the minimum.
445:         uint256 originalVotes = gohm.getPriorVotes(voter, proposal.startBlock);
446:->       uint256 currentVotes = gohm.getPriorVotes(voter, block.number);
447:         uint256 votes = currentVotes > originalVotes ? originalVotes : currentVotes;
......
462:     }

The second parameter of gohm.getPriorVotes(voter, block.number) can only a number smaller than block.number. Please see the [code](https://etherscan.io/token/0x0ab87046fBb341D058F17CBC4c1133F25a20a52f#code#L703) deployed by gOHM on the mainnet:

function getPriorVotes(address account, uint256 blockNumber) external view returns (uint256) {
->      require(blockNumber < block.number, "gOHM::getPriorVotes: not yet determined");
......
    }

Therefore, L446 will always revert. Voting will not be possible.

Copy the coded POC below to one project from Foundry and run forge test -vvv to prove this issue.

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.20;

import "forge-std/Test.sol";

interface CheatCodes {
    function prank(address) external;
    function createSelectFork(string calldata,uint256) external returns(uint256);
}

interface IGOHM {
    function getPriorVotes(address account, uint256 blockNumber) external view returns (uint256);
}

contract ContractTest is DSTest{
    address gOHM = 0x0ab87046fBb341D058F17CBC4c1133F25a20a52f;
    CheatCodes cheats = CheatCodes(0x7109709ECfa91a80626fF3989D68f67F5b1DD12D);

    function setUp() public {
        cheats.createSelectFork("https://rpc.ankr.com/eth", 19068280);
    }

    function testRevert() public {
        address user = address(0x12399543949349);
        cheats.prank(user);
        IGOHM(gOHM).getPriorVotes(address(0x1111111111), block.number);
    }

    function testOk() public {
        address user = address(0x12399543949349);
        cheats.prank(user);
        IGOHM(gOHM).getPriorVotes(address(0x1111111111), block.number - 1);
    }
}
/**output
[PASS] testOk() (gas: 13019)
[FAIL. Reason: revert: gOHM::getPriorVotes: not yet determined] testRevert() (gas: 10536)
Traces:
  [10536] ContractTest::testRevert()
    ├─ [0] VM::prank(0x0000000000000000000000000012399543949349)
    │   └─ ← ()
    ├─ [540] 0x0ab87046fBb341D058F17CBC4c1133F25a20a52f::getPriorVotes(0x0000000000000000000000000000001111111111, 19068280 [1.906e7]) [staticcall]  
    │   └─ ← revert: gOHM::getPriorVotes: not yet determined
    └─ ← revert: gOHM::getPriorVotes: not yet determined

Test result: FAILED. 1 passed; 1 failed; 0 skipped; finished in 1.80s
**/

Impact

Nobody can cast for any proposal. Not being able to vote means the entire governance contract will be useless. Core functionality is broken.

Code Snippet

https://github.com/sherlock-audit/2024-01-olympus-on-chain-governance/blob/main/bophades/src/external/governance/GovernorBravoDelegate.sol#L446

Tool used

Manual Review

Recommendation

File: bophades\src\external\governance\GovernorBravoDelegate.sol
445:         uint256 originalVotes = gohm.getPriorVotes(voter, proposal.startBlock);
446:-        uint256 currentVotes = gohm.getPriorVotes(voter, block.number);
446:+        uint256 currentVotes = gohm.getPriorVotes(voter, block.number - 1);
447:         uint256 votes = currentVotes > originalVotes ? originalVotes : currentVotes;

 

[sherlock-admin2]

1 comment(s) were left on this issue during the judging contest.

haxatron commented:

Medium. It would be caught immediately on deployment and implementation is upgradeable. There can be no loss of funds which is requisite of a high.

[IllIllI000]

Agree with haxatron that this is Medium, not High, based on Sherlock's rules

[nevillehuang]

Can agree, since this is purely a DoS, no malicious actions can be performed since no voting can be done anyways.

@Czar102 I am interested in hearing your opinion, but I will set medium for now, because governance protocols fund loss impact is not obvious but I initially rated it high because it quite literally breaks the whole protocol. I believe sherlock needs to cater to different types of protocols and not only associate rules to defi/financial losses (example protocols include: governance, on chain social media protocols etc..)

[0xLienid]

Fix: https://github.com/OlympusDAO/bophades/pull/293

[IllIllI000]

The PR follows the suggested recommendation and correctly modifies the only place that solely block.number is used, changing it to block.number - 1. The only place not using this value is the call above it which uses proposal.startBlock. The state() when startBlock is equal to block.number is ProposalState.Pending, so this case will never cause problems, since there are checks of the state. The PR also modifies the mock gOHM contract to mirror the behavior that caused the bug.

[s1ce]

Escalate

This is a high. Voting is a core part of a governance protocol, and this bricks all voting functionality.

[sherlock-admin2]

Escalate

This is a high. Voting is a core part of a governance protocol, and this bricks all voting functionality.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[0xf1b0]

Besides the fact that this issue breaks the core logic of the contract, it won't be immediately detected upon deployment, as previously mentioned as the reason for downgrading the severity. The voting process only becomes possible after a proposal has been made and time has elapsed. At this point, the issue will be raised, necessitating the deployment of an update. While the new version is being prepared, the proposal may expire, and a new one will have to be created. If the proposal includes some critical changes, this time delay can pose a serious problem.

[IllIllI000]

Ignore this part since, while true, it appears to be confusing some:
The sponsor mentioned this test file as where to look for how things will be deployed. The first action is to propose and start a vote for assigning the whitelist guardian, and that will flag the issue before anything else.

Furthermore, the timelock needs to pull in order to become and admin with access to the treasury. Until that happens, the existing admin has the power to do anything, so there's no case where something critical can't be done. The 'pull' requirement for transferring the admin to the timelock is a requirement of the code, not of the test. The Sherlock rules also state that opportunity costs (e.g. delays in voting for example, due to a loss of core functionality) do not count as a loss of funds.

[r0ck3tzx]

The test file within setUp() function configures the environment for testing, not for the actual deployment. The deployment process can and probably will look different, so no assumptions should be made based on the test file. The mention just shows how the whitelistGuardian will be configured, and not at what time/stage it will be done.

The LSW creates hypotheticals about how the deployment process might look, and because of that, the issue would be caught early. Anyone who has ever deployed a protocol knows that the process is complex and often involves use of private mempools. Making assumptions about the deployment without having actual deployment scripts is dangerous and might lead to serious issues.

[0xf1b0]

Even though some proposals may be initiated at the time of deployment, it will take between 3 to 7 days before the issue becomes apparent, as voting will not be available until then.

[nevillehuang]

I agree with watsons here, but would have to respect the decision of @Czar102 and his enforcement of sherlock rules. Governance protocols already have nuances of funds loss being not obvious, and the whole protocol revolves around voting as the core mechanism, if you cannot vote, you essentially lose the purpose of the whole governance.

[0xf1b0]

I've seen a lot of discussion regarding the rule of funds at risk. It seems that they never take into account the lost profits. A scenario where the core functionality of the system is broken could result in a loss of confidence in the protocol, causing users to be hesitant about investing their money due to the fear of such an issue recurring.

[Czar102]

From my understanding, due to the fact that the timelock needs to pull, the new governance contract needs to call it. And since it's completely broken, it will never pull the admin rights.

Hence, this is not a high severity impact of locking funds and rights in a governance, but a medium severity issue since the contract fails to work. Is it accurate? @IllIllI000

[IllIllI000]

Yes, that's correct

[Czar102]

Planning to reject the escalation and leave the issue as is.

[0xf1b0]

By the way, it will not be possible to update the contract, because a new implementation can only be set through the voting process, which does not work.

That's at least 2 out of 3:

• it won't be immediately detected upon deployment
• it's not upgradeable

[IllIllI000]

it's being deployed fresh for this project, so it'll just be redeployed. The 2/3 stuff I think you're referring to is for new contests

[Czar102]

Result:
Medium
Has duplicates

[sherlock-admin]

Escalations have been resolved successfully!

Escalation status:

• s1ce: rejected