If rewards are not distributed to some gauges in an epoch, it can lead to incorrect rewards distribution in the next epoch

[c4-bot-7]

Lines of code

https://github.com/code-423n4/2024-09-fenix-finance/blob/main/contracts/core/VotingEscrowUpgradeableV2.sol#L386

Vulnerability details

Impact

Some gauges may receive more rewards, while others may not receive any rewards at all.

Proof of Concept

In the VoterUpgradeableV2.notifyRewardAmount function, the index is accumulated for every reward distribution from the minter. When distributing rewards to gauges, the reward amount is calculated using the delta index and weightsPerEpoch.

        uint256 totalVotesWeight = weightsPerEpoch[currentTimestamp - _WEEK][state.pool];
        uint256 delta = index - state.index;
        if (delta > 0) {
L634:       uint256 amount = (totalVotesWeight * delta) / 1e18;
            if (state.isAlive) {
                gaugesState[gauge_].claimable += amount;
            } else {
                IERC20Upgradeable(token).safeTransfer(minter, amount);
            }
        }

If rewards are not distributed to a gauge in the current epoch, they can be distributed in the next epoch. If weightsPerEpoch for that gauge changes in the next epoch, the reward amount may be calculated incorrectly, leading to unfair distribution among the gauges.

Let's consider the following scenario:

1. There are two pools, poolA and poolB, with corresponding gauges gaugeA and gaugeB.
2. Users vote 50 for each pool individually in the first week: weightsPerEpoch = 50, totalWeightsPerEpoch = 100.
3. In the second week, users call the distribute function only for poolA, and the minter transfers 100 FNX. Of this, 50 FNX is transferred to gaugeA, while the remaining 50 FNX stays in the contract. At that time, users never call the distribute function for poolB. This situation can occur if the creator of poolB intentionally does not call the distribute function and other users lack the incentive to call it.
   • index = 100 * 1e18 / 100 = 1e18
   • rewards amount for gaugeA: 50 * 1e18 / 1e18 = 50
   • gaugesState[gaugeA].index = 1e18
   • gaugesState[gaugeB].index = 0
4. In this week, users vote 10 for poolA and 90 for poolB individually.
5. In the third week, users call the distribute function for both pools, and the minter transfers another 100 FNX.
   • index = index + 100 * 1e18 / 100 = 2e18
   • rewards amount for gaugeA: 10 * 1e18 / 1e18 = 10
   • rewards amount for gaugeB: 90 * 2e18 / 1e18 = 180

Even though the VoterUpgradeableV2 contract has 150 FNX from the minter, it attempts to transfer 190 FNX to the gauges. As a result, the rewards distribution to the gauges is reverted.

As a result, if rewards are not distributed to some gauges in an epoch, it can lead to incorrect rewards distribution in the next epoch.

Tools Used

Manual Review

Recommended Mitigation Steps

Rewards should be distributed to all gauges per epoch, or the reward index mechanism should be improved.

Assessed type

Other

[c4-judge]

alcueca marked the issue as primary issue

[b-hrytsak]

Thank you for your submission.

This issue is common for contracts like Voter ve(3,3), and it was also highlighted to us by the Hats audit providers: https://github.com/hats-finance/Fenix-Finance-0x83dbe5aa378f3ce160ed084daf85f621289fb92f/issues/26.

As you can see, we have the following mitigations:

1. The main method for distributing to the gaugeі is 'Voter.distributeAll()', which ensures that no gauge is skipped. In specific scenarios, other methods like 'Voter.distribute' are also available.

2. Although users may be interested in calling these methods, the protocol also itself will handle this process to ensure the protocol’s viability and prevent such cases from occurring. Additionally, a distribution window has been introduced during which these calls should be made.

3. Skipping a gauge for an entire epoch is highly unlikely

Thank you for your participation

[alcueca]

While the sponsor seems to be aware of this issue, and have some mitigations prepared, under the contest rules this is a valid finding because it is present in the code and wasn't disclosed by the sponsor.

[c4-judge]

alcueca marked the issue as satisfactory

[c4-judge]

alcueca changed the severity to 2 (Med Risk)

[alcueca]

Downgraded to Medium since skippign rewards in an epoch would be an unusual precondition.

[c4-judge]

alcueca marked the issue as selected for report