`MaxLoopLimitHelper._ensureMaxLoop()` is validating the wrong input in `Comptroller.setActionsPaused()`

[code423n4]

Lines of code

https://github.com/code-423n4/2023-05-venus/blob/main/contracts/Comptroller.sol#L885-L902 https://github.com/code-423n4/2023-05-venus/blob/main/contracts/MaxLoopsLimitHelper.sol#L25-L32 https://github.com/code-423n4/2023-05-venus/blob/main/contracts/MaxLoopsLimitHelper.sol#L39-L43

Vulnerability details

Proof of Concept

The protocol is using MaxLoopLimitHelper._ensureMaxLoop() throughout various functions to prevent DOS. However, Comptroller.setActionsPaused() contains a nested loop with a time complexity of marketsList.length * actionsList.length, while _ensureMaxLoop() is only validating marketsList.length.

uint256 marketsCount = marketsList.length;
uint256 actionsCount = actionsList.length;

_ensureMaxLoops(marketsCount);

for (uint256 marketIdx; marketIdx < marketsCount; ++marketIdx) {
    for (uint256 actionIdx; actionIdx < actionsCount; ++actionIdx) {
        _setActionPaused(address(marketsList[marketIdx]), actionsList[actionIdx], paused);
    }
}

https://github.com/code-423n4/2023-05-venus/blob/main/contracts/Comptroller.sol#L885-L902

If x is the number of tokens and y is the number of actions, _ensureMaxLoop() should validate x * y and not only x.

1. For demonstration, assuming .MaxLoopLimitHelper_ensureMaxLoop() contains a limit of 200
2. 100 tokens are being passed to .Comptroller.setActionsPaused()
3. 5 actions are being paused (there are 9 possible actions)
4. MaxLoopLimitHelper._ensureMaxLoop() will incorrectly pass assuming 100 (iterations) < 200 (limit), however it should revert since 500 (iterations) > 200 (limit)

https://github.com/code-423n4/2023-05-venus/blob/main/contracts/MaxLoopsLimitHelper.sol#L25-L32

https://github.com/code-423n4/2023-05-venus/blob/main/contracts/MaxLoopsLimitHelper.sol#L39-L43

https://github.com/code-423n4/2023-05-venus/blob/main/contracts/ComptrollerStorage.sol#L44-L54

Impact

Letting Comptroller.setActionsPaused() consume all gas and revert will be more costly than failing early. Also, failing with the custom error MaxLoopsLimitExceeded(maxLoopsLimit, len) is the expected behavior. Is is possible that failing directly instead of through MaxLoopLimitHelper._ensureMaxLoop() will damage monitoring/frontend tooling, since MaxLoopLimitHelper._ensureMaxLoop() will incorrectly pass the input data and it won't trigger the custom error.

The usage of MaxLoopLimitHelper._ensureMaxLoop() indicates failing early against DOS is important for the project. Loop validation will have undefined behavior in Comptroller.setActionsPaused(). Pausing actions will likely be done in times of market instability, therefore the functions used for pausing should have a stable and pre-defined behavior.

Tools Used

Manual review.

Recommended Mitigation Steps

Replace marketsCount with marketsCount * actionsCount in Comptroller.setActionsPaused() when calling MaxLoopLimitHelper._ensureMaxLoops(), e.g.

diff --git a/Comptroller.sol.orig b/Comptroller.sol
index 1559520..0c29eb6 100644
--- a/Comptroller.sol.orig
+++ b/Comptroller.sol
@@ -892,7 +892,7 @@ contract Comptroller is
         uint256 marketsCount = marketsList.length;
         uint256 actionsCount = actionsList.length;

-        _ensureMaxLoops(marketsCount);
+        _ensureMaxLoops(marketsCount * actionsCount);

         for (uint256 marketIdx; marketIdx < marketsCount; ++marketIdx) {
             for (uint256 actionIdx; actionIdx < actionsCount; ++actionIdx) {

Assessed type

Invalid Validation

[c4-sponsor]

chechu marked the issue as disagree with severity

[chechu]

No funds at risk here

[0xean]

@chechu - the severity of M doesn't require that funds are at risk. Here is the quick reference for M severity per c4 docs

2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.

[c4-sponsor]

chechu marked the issue as sponsor confirmed

[c4-judge]

0xean changed the severity to QA (Quality Assurance)