Lines of code

https://github.com/code-423n4/2023-05-maia/blob/main/src/rewards/rewards/FlywheelGaugeRewards.sol#L88-L90

Vulnerability details

Proof of Concept

FlywheelGaugeRewards.queueRewardsForCycle() will make a check to test if reward tokens were received from the minter (IBaseV2Minter), by checking the balance before and after the transfer.

// queue the rewards stream and sanity check the tokens were received
uint256 balanceBefore = rewardToken.balanceOf(address(this));
totalQueuedForCycle = minter.getRewards();
require(rewardToken.balanceOf(address(this)) - balanceBefore >= totalQueuedForCycle);

https://github.com/code-423n4/2023-05-maia/blob/main/src/rewards/rewards/FlywheelGaugeRewards.sol#L88-L90

However, it's possible for IBaseV2Minter to don't return any tokens. In this case, balanceBefore will be equal to rewardToken.balanceOf(address(this)) and the require check will pass.

The correct behavior would be to revert if no tokens were received, to prevent updating gaugeQueuedRewards.storedCycle, since calling without receiving any rewards should not be marked as a cycle. Note that the cycle gets update with the current timestamp in FlywheelGaugeRewards._updateRewards().

function queueRewardsForCycle() external returns (uint256 totalQueuedForCycle) {
    ...
    uint32 currentCycle = (block.timestamp.toUint32() / gaugeCycleLength) * gaugeCycleLength;
    ...
    _queueRewards(gauges, currentCycle, lastCycle, totalQueuedForCycle);

function _queueRewards(address[] memory gauges, uint32 currentCycle, uint32 lastCycle, uint256 totalQueuedForCycle)
    internal
{
        ...
        gaugeQueuedRewards[gauge] = QueuedRewards({
            priorCycleRewards: queuedRewards.priorCycleRewards + completedRewards,
            cycleRewards: uint112(nextRewards),
            storedCycle: currentCycle
        });
}

https://github.com/code-423n4/2023-05-maia/blob/main/src/rewards/rewards/FlywheelGaugeRewards.sol#L79

https://github.com/code-423n4/2023-05-maia/blob/main/src/rewards/rewards/FlywheelGaugeRewards.sol#L189-L193

The following POC shows that, even if IBaseV2Minter doesn't return any tokens, an attacker could keep calling queueRewardsForCycle() to forcefully update the gaugeCycle and corrupt this value. For demonstration, let's assume the attacker calls queueRewardsForCycle 100 times each time 1000 seconds apart, resulting in a final gaugeCycle of 100 * 1000 (100000) + 1000 (initial value on the tests setup) = 101000.

Steps to reproduce the POC:

Mock zero tokens returned from IBaseV2Minter:

diff --git a/MockRewardsStream.sol.orig b/MockRewardsStream.sol
--- a/MockRewardsStream.sol.orig
+++ b/MockRewardsStream.sol
@@ -21,6 +21,7 @@ contract MockRewardsStream {

     function getRewards() external returns (uint256 amount) {
         amount = rewardAmount;
+        amount = 0; // simulate amount zero returned
         rewardToken.transfer(msg.sender, amount);
     }
 }

Copy the following test inside FlywheelGaugeRewardsTest.t.sol.

function test_callQueueRewards_noTokensReceived_corruptGaugeCycle() public {
    gaugeToken.addGauge(gauge1);
    gaugeToken.addGauge(gauge2);
    gaugeToken.incrementGauge(gauge1, 1e18);
    gaugeToken.incrementGauge(gauge2, 3e18);

    for (uint256 i = 0; i < 100; ++i) {
        hevm.warp(block.timestamp + 1000);
        rewards.queueRewardsForCycle();
    }

    (uint112 prior, uint112 stored, uint32 cycle) = rewards.gaugeQueuedRewards(ERC20(gauge1));

    uint256 initialTimeWarp = 1000;

    require(prior == 0);
    require(stored == 0);
    require(cycle == 100000 + initialTimeWarp); // 101000
    require(rewards.gaugeCycle() == 100000 + initialTimeWarp); // 101000
}

Impact

There are two scenarios where an attacker might exploit this bug:

Scenario 1:

FlywheelGaugeRewards gets deployed
assume during this initialization period IBaseV2Minter is returning zero tokens.
attacker keeps calling FlywheelGaugeRewards.queueRewardsForCycle() to keep computing cycles without rewards in the state variable gaugeQueuedRewards.

Scenario 2:

FlywheelGaugeRewards gets deployed
IBaseV2Minter is returning rewards tokens
normal rewards cycles are computed
IBaseV2Minter stops returning rewards tokens
attacker keeps calling FlywheelGaugeRewards.queueRewardsForCycle() to compute invalid rewards cycles
IBaseV2Minter is returning rewards tokens and the system is back to normal
Steps 2 to 6 keep repeating over time

With this, what the attacker achieves is to compute corrupt data in gaugeQueuedRewards, this could cause the need for redeployments or the contracts will have to live with corrupted data/bad values injected into gaugeQueuedRewards.

Also, this could result in emitting dirty events from FlywheelGaugeRewards.queueRewardsForCycle() which will clutter and create noise when retriving data from events which could cause harm to frontends/projects building on top.

Severity Rationale

The likelihood this exploit might be medium to small, since the attacker will have to pay for the gas on each tx, (although contracts deployed on Arbitrum will have a small gas cost).

However, the severity should not be disconsidered since FlywheelGaugeRewards.queueRewardsForCycle() is permissionless and can be called by anyone and therefore any entity looking to cause damage to the protocol could use this exploit. The attacker could even write a bot that checks when IBaseV2Minter returns zero tokens and call FlywheelGaugeRewards.queueRewardsForCycle() continuously until IBase2Minter starts to return reward tokens again.

Tools Used

Manual review, foundry/forge.

Recommended Mitigation Steps

The = in >= in (L90)[https://github.com/code-423n4/2023-05-maia/blob/main/src/rewards/rewards/FlywheelGaugeRewards.sol#L90] was likely added to catch an exact match of returned tokens. For example, if IBaseV2Minter returns 100e18 tokens, then balanceBefore - rewardToken.balanceOf(address(this)) will be equal to 100e18.

The simplest solution would be to check for zero tokens returned from IBaseV2Minter, e.g:

diff --git a/FlywheelGaugeRewards.sol.orig b/FlywheelGaugeRewards.sol
--- a/FlywheelGaugeRewards.sol.orig
+++ b/FlywheelGaugeRewards.sol
@@ -87,7 +87,7 @@ contract FlywheelGaugeRewards is Ownable, IFlywheelGaugeRewards {
         // queue the rewards stream and sanity check the tokens were received
         uint256 balanceBefore = rewardToken.balanceOf(address(this));
         totalQueuedForCycle = minter.getRewards();
-        require(rewardToken.balanceOf(address(this)) - balanceBefore >= totalQueuedForCycle);
+        require(rewardToken.balanceOf(address(this)) - balanceBefore >= totalQueuedForCycle && totalQueuedForCycle != 0);

         // include uncompleted cycle
         totalQueuedForCycle += nextCycleQueuedRewards;

Note, the same fix needs to be applied to the paginated version of queueRewardsForCycle, (queueRewardsForCyclePaginated)[https://github.com/code-423n4/2023-05-maia/blob/main/src/rewards/rewards/FlywheelGaugeRewards.sol#L132], e.g.

diff --git a/FlywheelGaugeRewards.sol.orig b/FlywheelGaugeRewards.sol
index 48c81b3..419a035 100644
--- a/FlywheelGaugeRewards.sol.orig
+++ b/FlywheelGaugeRewards.sol
@@ -129,7 +129,7 @@ contract FlywheelGaugeRewards is Ownable, IFlywheelGaugeRewards {
             // queue the rewards stream and sanity check the tokens were received
             uint256 balanceBefore = rewardToken.balanceOf(address(this));
             uint256 newRewards = minter.getRewards();
-            require(rewardToken.balanceOf(address(this)) - balanceBefore >= newRewards);
+            require(rewardToken.balanceOf(address(this)) - balanceBefore >= newRewards && newRewards != 0);
             require(newRewards <= type(uint112).max); // safe cast
             nextCycleQueuedRewards += uint112(newRewards); // in case a previous incomplete cycle had rewards, add on
         }

Assessed type

Invalid Validation

code-423n4 / 2023-05-maia-findings

`FlywheelGaugeRewards.queueRewardsForCycle()` will not revert even if no tokens are received leaving the contract susceptible to data corruption #886