`OrigamiLendingClerk::borrowMax()` will revert everytime `_availableToBorrow()` returns a higer value than the remaining amount until hitting the circuit-breaker cap

[hats-bug-reporter[bot]]

Github username: @JacoboLansac Twitter username: jacolansac Submission hash (on-chain): 0xbbfc4bc5d3a1251d5b8a7eee3671dc280d7c9f3786e5f3b83443f253db216898 Severity: low

Description: The _borrow() function implements a circuit breaker to limit the amount of USDC that can be borrowed within a 24h period:

    function _borrow(
        address borrower, 
        address recipient, 
        BorrowerConfig storage borrowerConfig, 
        uint256 borrowAmount
    ) private {
        if (borrowAmount == 0) revert CommonEventsAndErrors.ExpectedNonZero();
        if (globalBorrowPaused) revert BorrowPaused();
        if (borrowerConfig.borrowPaused) revert BorrowPaused();

        // Check this borrow amount against the circuit breaker
@>      circuitBreakerProxy.preCheck(
            address(asset),  // USDC
            msg.sender, // onBehalfOf (not used, as all users go into the same circuit-breaker cap)
            borrowAmount
        );

        emit Borrow(borrower, recipient, borrowAmount);

        // ...

The function OrigamiLendingClerk::borrowMax() reads the maximum borrowable amount from _availableToBorrow(), and attempts to borrow it using _borrow():

    /**
     * @notice A an approved borrower calls to request the most funding it can.
     * `debtToken` will be minted 1:1 for the amount of asset borrowed
     * @param recipient The receiving address of the `asset` tokens
     */
    function borrowMax(address recipient) external override returns (uint256 borrowedAmount) {
        BorrowerConfig storage _borrowerConfig = _getBorrowerConfig(msg.sender);

@>      borrowedAmount = _availableToBorrow(msg.sender, _borrowerConfig);
        if (borrowedAmount != 0) {
@>          _borrow(msg.sender, recipient, _borrowerConfig, borrowedAmount);
        }
    }

However, the function OrigamiLendingClerk::_availableToBorrow() does not check the remaining amount before hitting the circuit breaker cap. Even in the function documentation, there is no statement that suggests that the circuit breaker cap is checked.

    /**
     * @notice Calculate the amount remaining that can be borrowed for a particular borrower.
     * The min of the global available capacity and the remaining capacity given that borrower's
     * existing debt and configured ceiling.
     * @dev Represented in the underlying asset's decimals (eg 6dp for USDC)
     */
    function _availableToBorrow(
        address borrower,
        BorrowerConfig storage borrowerConfig
    ) private view returns (uint256) {

        // review circuit breaker not checked kni this function

        // Calculate the specific borrower's max available
        uint256 _borrowerAmount;   // asset's dp
        {
            // Equivalent logic here to `_borrowerUtilisationRatio()`
            uint256 _borrowerDebtBalance = debtToken.balanceOf(borrower); // 18dp
            uint256 _borrowerDebtCeiling = borrowerConfig.debtCeiling;      // 18 dp
            unchecked {
                _borrowerAmount = _borrowerDebtCeiling > _borrowerDebtBalance
                    // This scaleDown intentionally rounds down (so it's not in the borrower's benefit)
                    ? (_borrowerDebtCeiling - _borrowerDebtBalance).scaleDown(_assetScalar, OrigamiMath.Rounding.ROUND_DOWN)
                    : 0;
            }
        }

        // review circuit breaker not checked here either
        uint256 _globalAmount = totalAvailableToWithdraw(); // asset's dp

        // Return the min of the globally available and
        // this borrower's specific amount
        return _globalAmount < _borrowerAmount
            ? _globalAmount
            : _borrowerAmount;
    }

Therefore, every time the _availableToBorrow() returns a higher value than the remaining amount until hitting the circuit breaker cap, the borrowedAmount will be too high, and the circuit breaker inside _borrow() will revert.

Impact: medium

It makes the OrigamiLendingClerk::borrowMax() unusable under the above-described conditions. Moreover, OrigamiLendingClerk::availableToBorrow() will return non-reliable values for those same conditions. Any future integration with the clerk that attempts to read these values will also be affected.

I am aware that this bug is very similar to another one already reported from another contract #39. The main difference being that this one causes a revert in a state-changing function.

Recommendation

Make sure the output from _availableToBorrow() is never aboube the circuit breaker 24h-cap:

    function _availableToBorrow(
        address borrower,
        BorrowerConfig storage borrowerConfig
    ) private view returns (uint256) {

        // review circuit breaker not checked kni this function

        // Calculate the specific borrower's max available
        uint256 _borrowerAmount;   // asset's dp
        {
            // Equivalent logic here to `_borrowerUtilisationRatio()`
            uint256 _borrowerDebtBalance = debtToken.balanceOf(borrower); // 18dp
            uint256 _borrowerDebtCeiling = borrowerConfig.debtCeiling;      // 18 dp
            unchecked {
                _borrowerAmount = _borrowerDebtCeiling > _borrowerDebtBalance
                    // This scaleDown intentionally rounds down (so it's not in the borrower's benefit)
                    ? (_borrowerDebtCeiling - _borrowerDebtBalance).scaleDown(_assetScalar, OrigamiMath.Rounding.ROUND_DOWN)
                    : 0;
            }
        }

+       // update the _borrowerAmount accounting for the circuit breaker cap
+       uint256 amountUntilCbCap = circuitBreakerProxy.cap() - circuitBreakerProxy.currentUtilisation();
+       _borrowerAmount = _borrowerAmount < amountUntilCbCap ? _borrowerAmount : amountUntilCbCap;

        // review circuit breaker not checked here either
        uint256 _globalAmount = totalAvailableToWithdraw(); // asset's dp

        // Return the min of the globally available and
        // this borrower's specific amount
        return _globalAmount < _borrowerAmount
            ? _globalAmount
            : _borrowerAmount;
    }

PoC

Happy to implement a PoC if the issue is not clear.

[frontier159]

borrowMax() isn't utilised by any borrower (so no impact), but agree this should also be capped by remaining availability.

It's worth noting that while the comment in _borrowerUtilisationRatio() says it's equivalent logic to _availableToBorrow() we will not want to limit by the remaining available in the circuit breaker. The UR should be based on the policy set debtCeiling

[JacoboLansac]

yeah, I saw that on the _borrowerUtilisationRatio() but it does not make sense to apply the circuit breaker there, I agree.