Zero utilization of any market blocks upgrade

Summary

The case of zero utilization isn't treated in the MigratePrimeCash's _calculateInterestRateCurves() logic, so it looks like the upgrade will fail if any market has zero utilization.

Vulnerability Detail

When for any currencyId for any market in the dictionary it is market.totalfCash == 0, the MigratePrimeCash's _patchFix() -> _setfCashInterestRateCurves(currencyId, settings.fCashCurves) -> _calculateInterestRateCurves(...) call reverts and migration fails.

In the same time zero utilization is a normal state of a market and treating it can be straightforward.

Impact

Migration fails whenever there is a market with zero totalfCash, which might be orchestrated by an attacker to implement a griefing attack.

Since any zero cash market fails the whole _patchFix() call, which involves substantial amount of operations and will be carried out on the mainnet, the impact/cost profile of such attack can look attractive enough.

Code Snippet

Zero utilization returned by InterestRateCurve.getfCashUtilization will cause _calculateInterestRateCurves() revert:

https://github.com/sherlock-audit/2023-03-notional/blob/main/contracts-v2/contracts/external/patchfix/MigratePrimeCash.sol#L304-L318

            // Market utilization cannot change because cash / fCash is already set in the market
@>          uint256 utilization = InterestRateCurve.getfCashUtilization(
                0, market.totalfCash, market.totalPrimeCash.mul(assetRate).div(assetRateDecimals)
            );

            require(utilization < uint256(Constants.RATE_PRECISION), "Over Utilization");
            // Cannot overflow the new market's max rate
            require(market.lastImpliedRate < irParams.maxRate, "Over Max Rate");

@>          if (utilization <= irParams.kinkUtilization1) {
                // interestRate = (utilization * kinkRate1) / kinkUtilization1
                // kinkRate1 = (interestRate * kinkUtilization1) / utilization
                uint256 newKinkRate1 = market.lastImpliedRate
                    .mul(irParams.kinkUtilization1)
@>                  .div(utilization);

getfCashUtilization() is zero when totalfCash == 0:

https://github.com/sherlock-audit/2023-03-notional/blob/main/contracts-v2/contracts/internal/markets/InterestRateCurve.sol#L230-L242

    /// @notice Returns the utilization for an fCash market:
    /// (totalfCash +/- fCashToAccount) / (totalfCash + totalCash)
    function getfCashUtilization(
        int256 fCashToAccount,
        int256 totalfCash,
        int256 totalCashUnderlying
    ) internal pure returns (uint256 utilization) {
        require(totalfCash >= 0);
        require(totalCashUnderlying >= 0);
        utilization = totalfCash.subNoNeg(fCashToAccount)
            .divInRatePrecision(totalCashUnderlying.add(totalfCash))
            .toUint();
    }

As an example, zero utilization can be the live case and it is handled within InterestRateCurve (and other parts of rate logic):

https://github.com/sherlock-audit/2023-03-notional/blob/main/contracts-v2/contracts/internal/markets/InterestRateCurve.sol#L354-L402

    function calculatefCashTrade(
        MarketParameters memory market,
        CashGroupParameters memory cashGroup,
        int256 fCashToAccount,
        uint256 timeToMaturity,
        uint256 marketIndex
    ) internal view returns (int256, int256) {
        ...

        {
            // Do not allow utilization to go above 100 on trading, calculate the utilization after
            // the trade has taken effect, meaning that fCash changes and cash changes are applied to
            // the market totals.
            market.totalfCash = market.totalfCash.subNoNeg(fCashToAccount);
            totalCashUnderlying = totalCashUnderlying.add(netUnderlyingToMarket);

@>          uint256 utilization = getfCashUtilization(0, market.totalfCash, totalCashUnderlying);
            if (utilization > uint256(Constants.RATE_PRECISION)) return (0, 0);

@>          uint256 newPreFeeImpliedRate = getInterestRate(irParams, utilization);

            // It's technically possible that the implied rate is actually exactly zero we will still
            // fail in this case. If this does happen we may assume that markets are not initialized.
@>          if (newPreFeeImpliedRate == 0) return (0, 0);

Tool used

Manual Review

Recommendation

Consider treating it as a special case, for example:

https://github.com/sherlock-audit/2023-03-notional/blob/main/contracts-v2/contracts/external/patchfix/MigratePrimeCash.sol#L313-L318

            if (utilization <= irParams.kinkUtilization1) {
                // interestRate = (utilization * kinkRate1) / kinkUtilization1
                // kinkRate1 = (interestRate * kinkUtilization1) / utilization
-               uint256 newKinkRate1 = market.lastImpliedRate
+               uint256 newKinkRate1 = utilization == 0 ? irParams.kinkRate1 : market.lastImpliedRate
                    .mul(irParams.kinkUtilization1)
                    .div(utilization);

sherlock-audit / 2023-03-notional-judging

iglyx - Zero utilization of any market blocks upgrade #217