The whole reward distribution logic can become blocked because of out-of-gas error

[code423n4]

Lines of code

https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/Comptroller.sol#L1031-L1052

Vulnerability details

Impact

The way the rewards are distributed right now, in the MultiRewardDistributor.sol, can make the whole distribute process unusable because of out-of-gas errors.

Proof of Concept

The way the whole claiming rewards process, does multiple interactions with multiple contracts and updates multiple storage values in multiple loops, which could lead easily to out-of-gas errors, but let's see the whole claiming process.

• the claiming start from the Comptroller.sol contract where a user can claim rewards for all/multiple MTokens, or just one if the array provided contains one, we will take the only one example since that will cost the lowest amount of gas.
• claimReward function will be called by an user with only his address and one MToken, taking both rewards for borrowers and suppliers
• the function will do an external call to rewardDistributor which is the MultiRewardDistributor.sol which already cost more gas then calling the functions internally in the MultiRewardDistributor.sol
• first updateMarketSupplyIndex is called https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/Comptroller.sol#L1039 which will call updateMarketSupplyIndexInternal in MultiRewardDistributor.sol, function which loops over some storage variables, and also updates some of those storage variables as can be seen here https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/MultiRewardDistributor/MultiRewardDistributor.sol#L1009-L1024, this means that every SLOAD on a slot that wasn't accessed yet, which will be the case here, will be around 2100 gas, and every SLOAD done in this function that wasn't access yet would cost around 5000, since the values would be updated from non-zero to non-zero value, which happens 2 times in every loop https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/MultiRewardDistributor/MultiRewardDistributor.sol#L1022-L1024
• after that disburseSupplierRewards is then called on the MultiRewardDistributor.sol, another external call, which calls disburseSupplierRewardsInternal, function which again loops trough multiple storage values and updates some https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/MultiRewardDistributor/MultiRewardDistributor.sol#L1053-L1068, this time though the SLOAD for accessed slots would cost just 100 gas, only the new slots accessed would cost 2100, but the SSTORE in this function would cost 22100 for the first time an user call this function for that specific marketConfig, since it updates the value from zero to non-zero on a slot that wasn't accessed yet, which happens 2 times, for every loop, as an example for 3 loops it would cost around 132,600 just to update those 2 variables.
• after that it will send the reward tokens on every marketConfig which again can cost a lot of gas, depending on what logic there is on the specific token contract (if it checks for blacklist for exemple)
• then it will do the same thing for the borrowers part by calling updateMarketBorrowIndex and disburseBorrowerRewards which does exactly the same things as I specified above, but on new slots, which again costs a lot of gas. All of these calculations can very easily get to too much gas consumed by this claiming process, which will make the claims impossible, especially because the whole marketConfigs can't be removed, as it is stated in the NatSpec https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/MultiRewardDistributor/MultiRewardDistributor.sol#L35-L36 which means that every user would have to loop trough every config ever added with _addEmissionConfig doing the whole process that I talked above. Keep in mind that I talked only for claiming rewards, for only one MToken on only one user, for more than that it will get way more costly which will mostly fail for sure.

Tools Used

Manual review

Recommended Mitigation Steps

Consider splitting the functionality a bit, the claimReward function on Comptroller.sol does too many things at once, also try to work more with memory values by coping the storage values into memory, and then loop trough them, as it cost way less gas, and only update the storage values at the end

Assessed type

Other

[c4-pre-sort]

0xSorryNotSorry marked the issue as duplicate of #326

[c4-judge]

alcueca changed the severity to QA (Quality Assurance)