No limit on the number of emission configs per MToken in `MultiRewardDistributor`

[code423n4]

Lines of code

https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/MultiRewardDistributor/MultiRewardDistributor.sol#L564-L571 https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/MultiRewardDistributor/MultiRewardDistributor.sol#L605-L612

Vulnerability details

Impact

In the MultiRewardDistributor contract, the _addEmissionConfig function allow the admin (timelock) to add multiple emission config to a given MToken, each emission config corresponds to a certain emission token (ERC20).

All the emission configs are stored in a list marketConfigs[mtoken], and there is no limit on the number of emission config that can be added to this list, so the admin (timelock) is allowed to add multiple emission config.

This can cause an issue in some protocol functions that loops over all those emission configs in their logic, as if the number of emission configs stored in the list marketConfigs[mtoken] get relatively big, those function can run out of gas which will make them impossible to call (cause a DOS of their functionality).

Some of the affected functions could be : updateMarketSupplyIndexAndDisburseSupplierRewards, updateMarketBorrowIndexAndDisburseBorrowerRewards.

Proof of Concept

This issue can occur if the number of emission configs for a given MToken become relatively big, the _addEmissionConfig function allow the admin (timelock) to add multiple configs (if the emission token remain unique) for each MToken.

The risk of this issue causing a run out of gas denial of service is very low for the functions that only loops once over all the emission configs like the functions : updateMarketSupplyIndex, disburseSupplierRewards, updateMarketBorrowIndex, disburseBorrowerRewards. But the risk of DOS gets higher when a function run the loop twice (over all the emission configs) as it is the case in the functions : updateMarketSupplyIndexAndDisburseSupplierRewards, updateMarketBorrowIndexAndDisburseBorrowerRewards.

If we take for example the function updateMarketSupplyIndexAndDisburseSupplierRewards :

function updateMarketSupplyIndexAndDisburseSupplierRewards(
    MToken _mToken,
    address _supplier,
    bool _sendTokens
) external onlyComptrollerOrAdmin {
    updateMarketSupplyIndexInternal(_mToken);
    disburseSupplierRewardsInternal(_mToken, _supplier, _sendTokens);
}

It call both functions updateMarketSupplyIndex and disburseSupplierRewards and thus perform two loops over the emission config list.

The updateMarketSupplyIndexAndDisburseSupplierRewards is used in the updateAndDistributeSupplierRewardsForToken function inside the Comptroller contract, the later function is implemented in many critical functions : a single time in both mintAllowed, redeemAllowed and twice in seizeAllowed, transferAllowed.

So in the case the number of emission configs becomes relatively big (not necessarily too big), the function updateMarketSupplyIndexAndDisburseSupplierRewards will be blocked which could block those functions especially the two functions seizeAllowed, transferAllowed because they basically loop 4 time over the configs list (which will cost a lot of gas).

The same issue could happen with the updateAndDistributeBorrowerRewardsForToken function which is implemented in borrowAllowed, repayBorrowAllowed.

If those functions are blocked, it will negatively impact the protocol as no mint, borrow, repay, redeem or transfer operation will be allowed which are among the main functionality of the protocol.

This issue is even more critical as there is no way to remove certain emission configs that were added to marketConfigs[mtoken] list.

Tools Used

Manual review

Recommended Mitigation Steps

I recommend to add a maximum limit on the number of emission configs associated to each MToken.

Assessed type

DoS

[0xSorryNotSorry]

A coded POC would be ideal to see the referenced edge.

[c4-pre-sort]

0xSorryNotSorry marked the issue as primary issue

[c4-sponsor]

ElliotFriedman marked the issue as disagree with severity

[ElliotFriedman]

agreed this is an issue, however, governance is trusted to not add too many reward configs which would brick the system due to out of gas

[ElliotFriedman]

disagree with severity

[c4-sponsor]

ElliotFriedman marked the issue as sponsor acknowledged

[c4-judge]

alcueca changed the severity to QA (Quality Assurance)

[c4-judge]

alcueca marked the issue as grade-a