Removing a BribeFlywheel from a Gauge does not remove the reward asset from the rewards depo, making it impossible to add a new Flywheel with the same reward token

[code423n4]

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/main/src/gauges/BaseV2Gauge.sol#L144-L152

Vulnerability details

Impact

Removing a bribe Flywheel (FlywheelCore) from a Gauge (via BaseV2Gauge::removeBribeFlywheel) does not remove the reward asset (call MultiRewardsDepot::removeAsset) from the rewards depo (BaseV2Gauge::multiRewardsDepot), making it impossible to add a new Flywheel (by calling BaseV2Gauge::addBribeFlywheel) with the same reward token (because MultiRewardsDepot::addAsset reverts as the assets already exists).

Impact is limiting protocol functionality in unwanted ways, possibly impacting gains in the long run. Example due to incentives lost by not having a specific token bribe reward.

Proof of Concept

Observation: a BribeFlywheel is a FlywheelCore with a FlywheelBribeRewards set as the FlywheelRewards, typically created using the BribesFactory::createBribeFlywheel

Scenario and execution flow

• project decides to add an initial BribeFlywheel to the recently deployed UniswapV3Gauge contract.
• this is done by calling UniswapV3GaugeFactory::BaseV2GaugeFactory::addBribeToGauge
  • executions further goes to BaseV2Gauge::addGaugetoFlywheel where the bribe flywheel reward token is added to the multi reward depo
• project decides, for whatever reason (a bug in the contract, an exploit, a decommission, a more profitable wheel that would use the same rewards token), that they want to replace the old flywheel with a new one
• removing is done via calling UniswapV3GaugeFactory::BaseV2GaugeFactory::removeBribeFromGauge
  • executions further goes to BaseV2Gauge::removeBribeFlywheel where the flywheel is removed but the reward token asset is not remove from the multi reward depo, there is no call to MultiRewardsDepot::removeAsset:

    function removeBribeFlywheel(FlywheelCore bribeFlywheel) external onlyOwner {
        /// @dev Can only remove active flywheels
        if (!isActive[bribeFlywheel]) revert FlywheelNotActive();

        /// @dev This is permanent; can't be re-added
        delete isActive[bribeFlywheel];

        emit RemoveBribeFlywheel(bribeFlywheel);
    }

• after removal, when trying to add a new flywheel with the same rewards token, execution fails with ErrorAddingAsset since the addAsset call reverts since the rewards token was not removed with the previous call to BaseV2Gauge::removeBribeFlywheel.

Tools Used

Manual analysis

Recommended Mitigation Steps

when BaseV2Gauge::removeBribeFlywheel is called for a particular flywheel, also remove it's corresponding reward depo token.

Example implementation

diff --git a/src/gauges/BaseV2Gauge.sol b/src/gauges/BaseV2Gauge.sol
index c2793a7..8ea6c1e 100644
--- a/src/gauges/BaseV2Gauge.sol
+++ b/src/gauges/BaseV2Gauge.sol
@@ -148,6 +148,9 @@ abstract contract BaseV2Gauge is Ownable, IBaseV2Gauge {
         /// @dev This is permanent; can't be re-added
         delete isActive[bribeFlywheel];

+        address flyWheelRewards = address(bribeFlywheel.flywheelRewards());        
+        multiRewardsDepot.removeAsset(flyWheelRewards);
+
         emit RemoveBribeFlywheel(bribeFlywheel);
     }

Assessed type

Other

[c4-judge]

trust1995 marked the issue as primary issue

[c4-judge]

trust1995 marked the issue as satisfactory

[c4-judge]

trust1995 changed the severity to 3 (High Risk)

[c4-sponsor]

0xLightt marked the issue as sponsor acknowledged

[c4-sponsor]

0xLightt marked the issue as disagree with severity

[c4-sponsor]

0xLightt marked the issue as sponsor confirmed

[0xLightt]

Hey, this happens due to the not being able to remove strategies from FlyWheelCore and the immutability in bribes. In accruing bribes for gauges, there is only one general FlyWheel per token, so removing it from the RewardsDepot would actually brick all rewards of the FlyWheel's token.

The goal with removing the flywheel from the gauge is to stop forcing the user to call accrue and update the rewardIndex for that flywheel to save gas or remove an unwanted token. After removing this forced accrual, users can increase their voting balance, accrue and then decrease the voting balance without accruing again. So the balances to accrue rewards can't be trusted and would lead to issues if we tried to reuse the same FlyWheel for the same strategy. One solution would be to add the option to remove the strategy from the flywheel but could lead to not accrued rewards being bricked.

If there is a need to migrate bribe system, there needs to be a migration of the gauge system as well. This is intended so that users can opt in into the migration, in turn, protecting them.

I believe the best solution would be to leave it up to users to choose the bribes they want to accrue. By default all user could have all bribes set as optOut for all strategies and FlywheelBooster would always return 0 when querying boostedBalanceOf and wouldn't take the user's balance into account in boostedTotalSupply. If the user decides to optIn into a bribe for strategy (we would mimic a minting scenario), would accrue with 0 balance, have his current balance added to the the strategy's boostedTotalSupply and boostedBalanceOf would return the allocated gaugeWeight instead of 0. The opposite when a user tries to optOut after being optIn, but there should be the option to give up rewards, actually bricking in them, but would be useful in case there is an issue with the token and for example reverts when transferring from the rewardsDepot. The gauge would force the user to accrue rewards for all optIn bribes when changing it's balance. This way we can completely remove governance around bribes, but would still keep the immutability of the bribes system intact.

[c4-judge]

trust1995 marked the issue as selected for report

[0xLightt]

Addressed https://github.com/Maia-DAO/eco-c4-contest/tree/address-214