c4-bot-4 commented 5 months ago

Lines of code

https://github.com/code-423n4/2024-03-neobase/blob/d6e6127e6763b93c23ee95cdf7622fe950d9ed30/src/GaugeController.sol#L224-L229 https://github.com/code-423n4/2024-03-neobase/blob/d6e6127e6763b93c23ee95cdf7622fe950d9ed30/src/GaugeController.sol#L402

Vulnerability details

Impact

When a gauge that an user has voted for gets removed by the governance, their voting power allocated for that gauge will be lost forever

Proof of Concept

Due to the current way the GaugeController::vote_for_gauge_weights function is implemented, whenever a given gauge that users have voted for gets removed, all of the voting powers allocated by those users to that gauge will be permanently lost. The same issue has actually already been reported in this report from the last Code4rena contest of the codebase. And as it can be seen from the following snippet:

    function testLostVotingPower() public {
        // prepare
        uint256 v = 10 ether;
        vm.deal(gov, v);
        vm.startPrank(gov);
        ve.createLock{value: v}(v);

        // add gauges
        gc.add_gauge(gauge1, 0);
        gc.add_type("", 0);
        gc.add_gauge(gauge2, 1);

        // all-in on gauge1
        gc.vote_for_gauge_weights(gauge1, 10000);

        // governance removes gauge1
        gc.remove_gauge_weight(gauge1);
        gc.remove_gauge(gauge1);

        // cannot vote for gauge2
        vm.expectRevert("Used too much power");
        gc.vote_for_gauge_weights(gauge2, 10000);

        // cannot remove vote for gauge1
        vm.expectRevert("Gauge not added"); // @audit remove after mitigation
        gc.vote_for_gauge_weights(gauge1, 0);

        // cannot vote for gauge2 (to demonstrate again that voting power is not removed)
        vm.expectRevert("Used too much power");  // @audit remove after mitigation
        gc.vote_for_gauge_weights(gauge2, 10000);
    }

the recommended fix from that report has actually been implemented. However, there are two other changes there as well. The isValidGauge mapping has been replaced with a new one named gauge_types_, which practically serves the same purpose as the old one in the context of this snippet. More importantly though, a new require statement has been added, as it can be seen on the last code line of the snippet, that checks whether the gauge type is greater than 0 and reverts elsewise. Since the gauge type for a given gauge address can only be 0 in the case where the gauge does not actually exist (i.e. it has been removed or it was never created in the first place), this means that the implemented fix will no longer work and because of that the issue is once again present in the current implementation of the GaugeController.

The following coded PoC, which is a modification of the one in the above linked report verifies the existence of the issue:

    function testLostVotingPower() public {
        // prepare
        uint256 v = 10 ether;
        vm.deal(gov, v);
        vm.startPrank(gov);
        ve.createLock{value: v}(v);

        // add gauges
        gc.add_gauge(gauge1, 0);
        gc.change_gauge_weight(gauge1, 100);
        gc.add_type("", 100);
        gc.add_gauge(gauge2, 1);
        gc.change_gauge_weight(gauge2, 100);

        // all-in on gauge1
        gc.vote_for_gauge_weights(gauge1, 10000);

        // governance removes gauge1
        gc.remove_gauge_weight(gauge1);
        gc.remove_gauge(gauge1);

        // cannot vote for gauge2
        vm.expectRevert("Used too much power");
        gc.vote_for_gauge_weights(gauge2, 10000);

        // cannot remove vote for gauge1
        vm.expectRevert("Gauge not added"); // @audit remove after mitigation
        gc.vote_for_gauge_weights(gauge1, 0);

        // cannot vote for gauge2 (to demonstrate again that voting power is not removed)
        vm.expectRevert("Used too much power");  // @audit remove after mitigation
        gc.vote_for_gauge_weights(gauge2, 10000);
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Remove the additional require statement that checks whether the gauge type for the _gauge_addr is different from 0, in order to allow users to remove their votes from removed gauges:

    function vote_for_gauge_weights(address _gauge_addr, uint256 _user_weight) external {
        require(_user_weight >= 0 && _user_weight <= 10_000, "Invalid user weight");
        require(_user_weight == 0 || gauge_types_[_gauge_addr] != 0, "Can only vote 0 on non-gauges"); // We allow withdrawing voting power from invalid (removed) gauges
        VotingEscrow ve = votingEscrow;
        (
            ,
            /*int128 bias*/
            int128 slope_, /*uint256 ts*/

        ) = ve.getLastUserPoint(msg.sender);
        require(slope_ >= 0, "Invalid slope");
        uint256 slope = uint256(uint128(slope_));
        uint256 lock_end = ve.lockEnd(msg.sender);
        uint256 next_time = ((block.timestamp + WEEK) / WEEK) * WEEK;
        require(lock_end > next_time, "Lock expires too soon");

        int128 gauge_type = gauge_types_[_gauge_addr] - 1;
-       require(gauge_type >= 0, "Gauge not added");
        ...
    }