FlywheelCore.setFlywheelRewards can remove current reward funds from the current users' reach as it doesn't check that newFlywheelRewards' FlywheelCore is this contract.
If it's not, by mistake or with a malicious intent, the users will lose the access to reward funds as this FlywheelCore will not be approved for any fund access to the new flywheelRewards, while all the reward funds be moved there.
Setting severity to medium as on one hand that's system breaking issue (no rewards can be claimed after that, users are rugged reward-wise), on the other hand setFlywheelRewards function is requiresAuth. Also, a room for operational mistake isn't too small here as new flywheelRewards contract can be correctly configured and not malicious in all other regards.
Proof of Concept
FlywheelCore.setFlywheelRewards doesn't check that newFlywheelRewards' FlywheelCore is this FlywheelCore instance:
This way if new flywheelRewards contract have any different FlywheelCore then current users' access to reward funds will be irrevocably lost as both claiming functionality and next run of setFlywheelRewards will revert, not being able to transfer any funds from flywheelRewards with rewardToken.safeTransferFrom(address(flywheelRewards), ...):
As FlywheelCore holds user funds accounting via rewardsAccrued mapping, all these accounts became non-operational, as all the unclaimed rewards will be lost for the users.
Recommended Mitigation Steps
Consider adding the require for address(newFlywheelRewards.flywheel) == address(flywheelRewards.flywheel) in setFlywheelRewards so that users always retain funds access.
Lines of code
https://github.com/fei-protocol/flywheel-v2/blob/77bfadf388db25cf5917d39cd9c0ad920f404aad/src/FlywheelCore.sol#L164-L171
Vulnerability details
Impact
FlywheelCore.setFlywheelRewards can remove current reward funds from the current users' reach as it doesn't check that newFlywheelRewards' FlywheelCore is this contract.
If it's not, by mistake or with a malicious intent, the users will lose the access to reward funds as this FlywheelCore will not be approved for any fund access to the new flywheelRewards, while all the reward funds be moved there.
Setting severity to medium as on one hand that's system breaking issue (no rewards can be claimed after that, users are rugged reward-wise), on the other hand setFlywheelRewards function is requiresAuth. Also, a room for operational mistake isn't too small here as new flywheelRewards contract can be correctly configured and not malicious in all other regards.
Proof of Concept
FlywheelCore.setFlywheelRewards doesn't check that newFlywheelRewards' FlywheelCore is this FlywheelCore instance:
https://github.com/fei-protocol/flywheel-v2/blob/77bfadf388db25cf5917d39cd9c0ad920f404aad/src/FlywheelCore.sol#L164-L171
FlywheelCore is immutable within flywheelRewards and its access to the flywheelRewards' funds is set on construction:
https://github.com/fei-protocol/flywheel-v2/blob/77bfadf388db25cf5917d39cd9c0ad920f404aad/src/rewards/BaseFlywheelRewards.sol#L30
This way if new flywheelRewards contract have any different FlywheelCore then current users' access to reward funds will be irrevocably lost as both claiming functionality and next run of setFlywheelRewards will revert, not being able to transfer any funds from flywheelRewards with
rewardToken.safeTransferFrom(address(flywheelRewards), ...)
:https://github.com/fei-protocol/flywheel-v2/blob/77bfadf388db25cf5917d39cd9c0ad920f404aad/src/FlywheelCore.sol#L125
https://github.com/fei-protocol/flywheel-v2/blob/77bfadf388db25cf5917d39cd9c0ad920f404aad/src/FlywheelCore.sol#L168
As FlywheelCore holds user funds accounting via rewardsAccrued mapping, all these accounts became non-operational, as all the unclaimed rewards will be lost for the users.
Recommended Mitigation Steps
Consider adding the require for
address(newFlywheelRewards.flywheel) == address(flywheelRewards.flywheel)
in setFlywheelRewards so that users always retain funds access.