`remove_gauge` will access wrong gauge types when removing gauge's weight

[c4-bot-2]

Lines of code

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

Vulnerability details

Impact

remove_gauge will remove the weight of non-existent gauge types due to incorrect execution order.

Proof of Concept

When remove_gauge is called, it will first remove the gauge_types_ of the provided _gauge address by setting it to 0. Then, it will remove the gauge weight by calling _remove_gauge_weight.

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

    function remove_gauge(address _gauge) external onlyGovernance {
        require(gauge_types_[_gauge] != 0, "Invalid gauge address");
>>>     gauge_types_[_gauge] = 0;
        _remove_gauge_weight(_gauge);
        emit GaugeRemoved(_gauge);
    }

However, calling _remove_gauge_weight will always remove the weight of non-existent gauge types because it accesses gauge_types_ to get the gauge type, which has already been removed before this function is called.

https://github.com/code-423n4/2024-03-neobase/blob/main/src/GaugeController.sol#L350-L374

    function _remove_gauge_weight(address _gauge) internal {
>>>     int128 gauge_type = gauge_types_[_gauge] - 1;
        uint256 next_time = ((block.timestamp + WEEK) / WEEK) * WEEK;

        uint256 old_weight_bias = _get_weight(_gauge);
        uint256 old_weight_slope = points_weight[_gauge][next_time].slope;
        uint256 old_sum_bias = _get_sum(gauge_type);

        points_weight[_gauge][next_time].bias = 0;
        points_weight[_gauge][next_time].slope = 0;

        uint256 new_sum = old_sum_bias - old_weight_bias;
        points_sum[gauge_type][next_time].bias = new_sum;
        points_sum[gauge_type][next_time].slope -= old_weight_slope;
        // We have to cancel all slope changes (gauge specific and global) that were caused by this gauge
        // This is not very efficient, but does the job for a governance function that is called very rarely
        for (uint256 i; i < 263; ++i) {
            uint256 time_to_check = next_time + i * WEEK;
            uint256 gauge_weight_change = changes_weight[_gauge][time_to_check];
            if (gauge_weight_change > 0) {
                changes_weight[_gauge][time_to_check] = 0;
                changes_sum[gauge_type][time_to_check] -= gauge_weight_change;
            }
        }
    }

This will cause the remove_gauge function to not properly remove gauge's weight and update gauge type's point sum.

Tools Used

Manual review.

Recommended Mitigation Steps

Set gauge_types_ to 0 after _remove_gauge_weight is called.

    function remove_gauge(address _gauge) external onlyGovernance {
        require(gauge_types_[_gauge] != 0, "Invalid gauge address");
-       gauge_types_[_gauge] = 0;
        _remove_gauge_weight(_gauge);
+       gauge_types_[_gauge] = 0;
        emit GaugeRemoved(_gauge);
    }

Assessed type

Error

[c4-judge]

MarioPoneder marked the issue as primary issue

[MarioPoneder]

Related to #22 (same mitigation measures)
Not duplicating for now.

[c4-sponsor]

zjesko (sponsor) confirmed

[c4-sponsor]

zjesko (sponsor) acknowledged

[c4-sponsor]

zjesko (sponsor) confirmed

[zjesko]

mitigation PR:

https://github.com/mkt-market/canto-neofinance-coordinator/pull/17

[c4-judge]

MarioPoneder marked the issue as duplicate of #22

[c4-judge]

MarioPoneder changed the severity to 2 (Med Risk)

[MarioPoneder]

https://docs.code4rena.com/awarding/judging-criteria/supreme-court-decisions-fall-2023#verdict-similar-exploits-under-a-single-issue

The findings are duplicates if they share the same root cause. More specifically, if fixing the Root Cause (in a reasonable manner) would cause the finding to no longer be exploitable, then the findings are duplicates.

[c4-judge]

MarioPoneder marked the issue as satisfactory