Submitted by rscodes, also found by 0xpiken, hash, and novamanbg (1, 2)
In ChefIncentivesController.sol, the _newRewards function calculates the new rewards accumulated since the last update of that specific pool Line 988-994:
For example, when a user changes his debt in CDPVault, handleActionAfter is called by the vault, which only changes the pool.lastRewardTime of that individual pool representing that vault through _updatePool.
This is problematic as it means different pools can have different pool.lastRewardTime, which means the way that rewards are calculated in _newRewards will cause one pool to receive more rewards than it is supposed to, at the loss of another pool.
A summary would be that availableRewards() returns depositedRewards - accountedRewards; and pool.lastRewardTime being different means that one pool has been adding to accountedRewards and its struct variables ahead of another pool. That one pool should then have a lesser share in the value returned by availableRewards(); however, the current code does not take that into account, allowing that pool to dig into rewards meant for other pools who has a more outdated pool.lastRewardTime.
Consider this symbol diagram example:
Suppose there are 2 pools (represented by the first and second [] block respectively) that are eligible for rewards. And day y is the value endRewardTime() returns. And day z is a value larger than day y. Part A represents the rewards that are to be claimed by pool 1 during the time period [0,x), Part B represents [x,y) for pool 1 as well, while Part C represents the full rewards that are to be claimed by pool 2 during [0,y). Below I will explain the sequence which allows pool 1 to steal 1/4 of part C from pool 2.
The below code is the coded version of the explanation above:
Console Output:
Notable comments:
After those changes, console output is now showing that rewards are distributed accurately (60 ether each):
Foundry, VSCode
Math
amarcu (LoopFi) confirmed and commented:
Koolex (judge) decreased severity to Medium
