30910 - [SC - High] Processing of voting results is not implemented...
Submitted on May 7th 2024 at 23:52:40 UTC by @cryptoticky for Boost | Alchemix
Report ID: #30910
Report type: Smart Contract
Report severity: High
Target: https://github.com/alchemix-finance/alchemix-v2-dao/blob/main/src/Voter.sol
Impacts:
Contract fails to deliver promised returns, but doesn't lose value
Manipulation of governance voting result deviating from voted outcome and resulting in a direct change from intended effect of original results
Description
Brief/Intro
Processing of voting results is not implemented in the next epoch.
Vulnerability Details
When Voting.distribute
function is calling, the Voting.notifyRewardAmount
is called at the end. It is also inconsistent in calls of the _updateFor
function.
_updateFor
function is called before other variables are updated in Voter.vote
and Voter.reset
functions. But in Voter._distribute
function, the voter sends alcx
token with old claimable
value. So in the code, Voter._updateFor
function is called before sending the alcx
token but this produces the same result that this call is made at the end of the function.
Impact Details
As a result, the voting result goes against what was expected, and the processing of each voting result has an epoch-sized delay.
However, I registered this report as medium because there is no actual loss of funds.
Recommendation
Move
IMinter(minter).updatePeriod();
at the start of distribute function
/// @inheritdoc IVoter
function distribute() external {
IMinter(minter).updatePeriod();
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]]);
}
}
}
Update the
_distribute
function like this
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"
);
_updateFor(_gauge);
uint256 _claimable = claimable[_gauge];
// Reset claimable amount
claimable[_gauge] = 0;
if (_claimable > 0) {
IBaseGauge(_gauge).notifyRewardAmount(_claimable);
}
IBribe(bribes[_gauge]).resetVoting();
emit DistributeReward(msg.sender, _gauge, _claimable);
}
Don't call
_updateFor
function in other functions.claimable
variable is only used in_distribute
function anddistribute
function is called only one time in an epoch period. So don't need to update that variable in vote and reset function.
This is just a recommendation. You can find a better solution.
Proof of Concept
// SPDX-License-Identifier: GPL-3
pragma solidity ^0.8.15;
import "./BaseTest.sol";
contract VoterPoC is BaseTest {
uint256 constant DURATION = 2 weeks;
uint256 constant SECONDS_PER_BLOCK = 12;
uint256 public epochTime;
uint256 public epochBlock;
address public sushiBribeAddress;
address public balancerBribeAddress;
function setUp() public {
setupContracts(block.timestamp);
epochTime = minter.activePeriod();
epochBlock = block.number;
}
function goNextEpoch() private {
epochTime = epochTime + DURATION;
epochBlock = epochBlock + DURATION / 12;
hevm.warp(epochTime);
hevm.roll(epochBlock);
sushiBribeAddress = voter.bribes(address(sushiGauge));
balancerBribeAddress = voter.bribes(address(balancerGauge));
}
function testBugDepositReward() public {
address[] memory poolsOfAdmin = new address[](1);
poolsOfAdmin[0] = sushiPoolAddress;
uint256[] memory weights = new uint256[](1);
weights[0] = 10_000;
// go epoch 1
uint256 period = minter.activePeriod();
hevm.warp(period + nextEpoch);
// Voter.totalWeight is 0
// Voter.notifyRewardAmount is not called
// Voter.index is 0
voter.distribute();
uint256 targetTokenId = createVeAlcx(admin, TOKEN_1, MAXTIME, false);
hevm.prank(admin);
voter.vote(targetTokenId, poolsOfAdmin, weights, 0);
uint256 claimableForSushi = voter.claimable(address(sushiGauge));
// at this time, the claimable variable is 0
assertEq(claimableForSushi, 0, "claimableForSushi > 0");
// go epoch 2
period = minter.activePeriod();
hevm.warp(period + nextEpoch);
// Calling Voter.distribute function in the new epoch, the reward token should be sent to gauges.
// But no reward is sent to gauges
// 1. call _distribute function
// 2. call _updateFor function => claimable is 0 because delta index is still 0
// 3. call Minter.updatePeriod function
// 4. call Voter.notifyRewardAmount function because Voter.totalWeight is greater than 0.
voter.distribute();
claimableForSushi = voter.claimable(address(sushiGauge));
// but the updateFor function is not called yet
// at this time, the claimable variable is still 0.
assertEq(claimableForSushi, 0, "claimableForSushi > 0");
// Now the reward token is in Voter contract.
assertGt(alcx.balanceOf(address(voter)), 0, "alcx balance of Voter contract is 0");
// This means that processing of voting results is not done in the next epoch.
// call reset function to avoid the problem pointed out in the report #30886
hevm.prank(admin);
// totalWeight -> 0 because of there is no vote.
voter.reset(targetTokenId);
// go epoch 3
period = minter.activePeriod();
hevm.warp(period + nextEpoch);
voter.distribute();
// Now the alcx token is sent to pool
// Small amounts of alcx may remain due to rounding calculations.
// In fact, 1 wei of alcx is left in this test.
assertLt(alcx.balanceOf(address(voter)), 2, "alcx balance of Voter contract is not 0");
// In the end, it can be seen that the processing of voting results in epoch 1 takes place only after epoch 3.
}
}
Last updated
Was this helpful?