Daily limit invariant can break when setting limits without daily limit updates

[c4-bot-4]

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/main/src/V3Vault.sol?plain=1#L807-L833
https://github.com/code-423n4/2024-03-revert-lend/blob/main/src/V3Vault.sol?plain=1#L1246-L1268

Vulnerability details

Impact

A vault owner has the capability to update the daily limits for both lending and debt increases by invoking the Vault.setLimits() function. Upon calling this function, any changes made to either dailyLendIncreaseLimitMin or dailyDebtIncreaseLimitMin will not apply retroactively for the upcoming cycle. However, if there's no change in the daily limit values, the limits will still be enforced. This behavior can potentially lead to situations where the daily limit increases by up to twice the expected amount, thereby compromising the integrity of the daily limit rule.

Proof of Concept

One of the major usages of setLimits() is to set the lending/debt limits to zero. This acts as a halt on debt/lending operations when an emergency occurs. This feature works as expected. However, a problem occurs if the protocol calls Vault.setLimits() and doesn't update either daily limit value.

I will make the following assumption:

It is reasonable to assume that not all values will always be updated at once. For example, the protocol may just want to update the minimum size of a loan.

Now let's review both daily limit functions below:

function _resetDailyLendIncreaseLimit(uint256 newLendExchangeRateX96, bool force) internal {
    // daily lend limit reset handling
    uint256 time = block.timestamp / 1 days;
    if (force || time > dailyLendIncreaseLimitLastReset) {
        uint256 lendIncreaseLimit = _convertToAssets(totalSupply(), newLendExchangeRateX96, Math.Rounding.Up)
            * (Q32 + MAX_DAILY_LEND_INCREASE_X32) / Q32;
        dailyLendIncreaseLimitLeft =
            dailyLendIncreaseLimitMin > lendIncreaseLimit ? dailyLendIncreaseLimitMin : lendIncreaseLimit;
        dailyLendIncreaseLimitLastReset = time;
    }
}

function _resetDailyDebtIncreaseLimit(uint256 newLendExchangeRateX96, bool force) internal {
    // daily debt limit reset handling
    uint256 time = block.timestamp / 1 days;
    if (force || time > dailyDebtIncreaseLimitLastReset) {
        uint256 debtIncreaseLimit = _convertToAssets(totalSupply(), newLendExchangeRateX96, Math.Rounding.Up)
            * (Q32 + MAX_DAILY_DEBT_INCREASE_X32) / Q32;
        dailyDebtIncreaseLimitLeft =
            dailyDebtIncreaseLimitMin > debtIncreaseLimit ? dailyDebtIncreaseLimitMin : debtIncreaseLimit;
        dailyDebtIncreaseLimitLastReset = time;
    }
}

Both functions are nearly identical so let's focus on discussing just _resetDailyDebtIncreaseLimit(). When Vault.setLimits() is called, the admin may or may not update the value of dailyDebtIncreaseLimitMin. Regardless if it's updated or not, Vault.setLimit() forces an update for both daily limits by setting the force argument to true.

function setLimits(
    uint256 _minLoanSize,
    uint256 _globalLendLimit,
    uint256 _globalDebtLimit,
    uint256 _dailyLendIncreaseLimitMin,
    uint256 _dailyDebtIncreaseLimitMin
) external {
    ...
    dailyLendIncreaseLimitMin = _dailyLendIncreaseLimitMin;
    dailyDebtIncreaseLimitMin = _dailyDebtIncreaseLimitMin;

    (, uint256 newLendExchangeRateX96) = _updateGlobalInterest();

    // force reset daily limits with new values
    // AUDIT: daily limit increases are forced, regardless if the value is updated.
    _resetDailyLendIncreaseLimit(newLendExchangeRateX96, true);
    _resetDailyDebtIncreaseLimit(newLendExchangeRateX96, true);
    
    ...
}

By forcing an update when the _dailyDebtIncreaseLimitMin hasn't changed, the daily limit will automatically be reset and users will be able to at most increase the daily limit up to 2x.

Tools Used

Manual inspection.

Recommended Mitigation Steps

The protocol should check if the _dailyDebtIncreaseLimitMin has changed since the last update. If there is a change, then force an update. Otherwise, do nothing.

Assessed type

Other

[c4-pre-sort]

0xEVom marked the issue as duplicate of #367

[c4-pre-sort]

0xEVom marked the issue as sufficient quality report

[c4-pre-sort]

0xEVom marked the issue as insufficient quality report

[c4-judge]

jhsagd76 changed the severity to QA (Quality Assurance)

[jhsagd76]

2L-B

[c4-judge]

jhsagd76 marked the issue as grade-a