lil.eth - Gauges cannot be added back to gaugeController

[sherlock-admin2]

lil.eth

medium

Gauges cannot be added back to gaugeController

Summary

Convergence protocol allows to add gauges contracts that gets distributed CVG inflations tokens regarding how many votes are allocated on them.
Gauges can be killed using gaugeController.vy#kill_gauge() but types mapping is not cleared when removing a gauge, which prevents the same gauge from being added back later and killed_gauges mapping cannot be toggled to False when adding a gauge which could prevent user from voting to a re-added gauge.

Vulnerability Detail

The kill_gauge() function marks a gauge as killed by setting killed_gauges[addr] = True :

@external
def kill_gauge(addr: address):
    assert msg.sender == self.admin, "NOT_ADMIN"
    self._change_gauge_weight(addr, 0)
    self.killed_gauges[addr] = True # @audit cannot be toggled ?
    # CvgRewards.sol :  Remove a gauge from the array, replace it by the last gauge of the array
    self.control_tower.cvgRewards().removeGauge(addr)

but there is no functionality to toggle this back to False, even if the gauge is re-added.

Moreover the gauge_types_[addr] is also maintained to the set-up value of the gauge on gauge addition which also render impossible to re-add due to the check on add_gauge() function :

assert self.gauge_types_[addr] == 0, "GAUGE_ALREADY_ADDED"

Impact

The add_gauge() function is broken in the edge case when the updater tries to add the gauge back into the registry after removing it. It affects all the operations of the protocol that rely on the gauge registry.

POC

Add this on 01_integration-test.spec.ts :

//E npx hardhat test --grep "Finding 1"*/
    it("Fail : Finding 1 Kill a gauge,re-Add and vote cannot work", async () => {
        //E Preparation
        await (await gaugeControllerContract.connect(treasuryDao).add_gauge(staking1, 1, 10000)).wait();
        await (await gaugeControllerContract.connect(treasuryDao).add_gauge(staking2, 1, 10000)).wait();
        //E gauge types different from 0
        let type1 = await gaugeControllerContract.gauge_types(staking1);
        let type2 = await gaugeControllerContract.gauge_types(staking2);
        console.log("Gauge Type : %d,%d",Number(type1),Number(type2));
        
        expect(await gaugeControllerContract.killed_gauges(staking2)).to.be.false;
        expect(await gaugeControllerContract.get_gauge_weight(staking2)).to.be.eq(10000n);
        //E Kill Gauge
        await gaugeControllerContract.connect(treasuryDao).kill_gauge(staking2);
        expect(await gaugeControllerContract.killed_gauges(staking2)).to.be.true;
        expect(await gaugeControllerContract.get_gauge_weight(staking2)).to.be.eq(0);
        //E Checks
        await expect(cvgRewards.gauges(2)).to.be.rejected;
        expect(await cvgRewards.gaugesId(staking2)).to.be.eq(0);

        // Re-Add it 
        await expect(createAndActivateGauge(staking2)).to.be.revertedWith("GAUGE_ALREADY_ADDED");
        //E gauge_types_[addr] still equal to 2 even after kill
        expect(await gaugeControllerContract.gauge_types(staking2)).to.be.eq(1); 
    });

Code Snippet

https://github.com/sherlock-audit/2023-11-convergence/blob/main/sherlock-cvg/contracts/Locking/GaugeController.vy#L396

Tool used

Manual Review
Old Tokemak report : sherlock-audit/2023-06-tokemak-judging#674

Recommendation

Toggle back to false the killed gauge mapping when adding a gauge and set gauge_types_[addr] to 0 when killing it :

@external
def add_gauge(addr: address, gauge_type: int128, weight: uint256):
+   self.killed_gauges[addr] = False
    assert msg.sender == self.admin
    assert (gauge_type >= 0) and (gauge_type < self.n_gauge_types)
    assert self.gauge_types_[addr] == 0 , "GAUGE_ALREADY_ADDED" # dev: cannot add the same gauge twice
    ...


@external
def kill_gauge(addr: address):
    assert msg.sender == self.admin, "NOT_ADMIN"
    self._change_gauge_weight(addr, 0)
+   self.gauge_types[addr] = 0
     ...

Duplicate of #187