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.
functionnotifyRewardAmount(uint256amount)external{require(msg.sender== minter,"only minter can send rewards");require(totalWeight >0,"no votes");_safeTransferFrom(base,msg.sender,address(this), amount);// transfer rewards inuint256 _ratio =(amount *1e18)/ totalWeight;// 1e18 adjustment is removed during claimif(_ratio >0){ index += _ratio;}emitNotifyReward(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.
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.
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.
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
function updateFor(address[] memory _gauges) external {
for (uint256 i = 0; i < _gauges.length; i++) {
_updateFor(_gauges[i]);
}
}
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;
}
}
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);
}
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;
}
}
