0xadrii - Multiplying the collateral amount by the `STANDARD_EXPANSION_FACTOR` when checking the required collateral is incorrect and allows borrowers to get undercollateralized loans

[sherlock-admin4]

0xadrii

high

Multiplying the collateral amount by the STANDARD_EXPANSION_FACTOR when checking the required collateral is incorrect and allows borrowers to get undercollateralized loans

Summary

It is possible to obtain undercollateralized loans due to the fact that LenderCommitmentGroup_Smart checks the required collateral wrongly.

Vulnerability Detail

The LenderCommitmentGroup_Smart contract acts as a lending pool in TellerV2. Users can request loans and the LenderCommitmentGroup_Smart will act as the lender of those loans. However, these loans are constrained and must be overcollateralized.

This report describes a vulnerability related with scaling a price incorrectly and allowing borrowers to take undercollateralized loans. In order to understand it properly, we must first understand how LenderCommitmentGroup_Smart obtains the required collateral amount so that it can verify if the user is actually supplying the proper collateral amount.

In order to compute the required collateral, the LenderCommitmentGroup_Smart will make use of a uniswap v3 pool from the collateral and principal tokens. The required amount of collateral will then be given by the getCollateralRequiredForPrincipalAmount function:

// LenderCommitmentGroup_Smart.sol

function getCollateralRequiredForPrincipalAmount(uint256 _principalAmount)
        public
        view
        returns (uint256)
    {
        uint256 baseAmount = _calculateCollateralTokensAmountEquivalentToPrincipalTokens(
                _principalAmount
            );

        //this is an amount of collateral
        return baseAmount.percent(collateralRatio);
    }

This function internally calls the _calculateCollateralTokensAmountEquivalentToPrincipalTokens function, which will fetch and compute the price given by the uniswap pool:

// LenderCommitmentGroup_Smart.sol

function _calculateCollateralTokensAmountEquivalentToPrincipalTokens(
        uint256 principalTokenAmountValue
    ) internal view returns (uint256 collateralTokensAmountToMatchValue) {
        //same concept as zeroforone
        (address token0, ) = _getPoolTokens();

        bool principalTokenIsToken0 = (address(principalToken) == token0);

        uint256 pairPriceWithTwap = _getUniswapV3TokenPairPrice(twapInterval);
        uint256 pairPriceImmediate = _getUniswapV3TokenPairPrice(0);

        return
            _getCollateralTokensAmountEquivalentToPrincipalTokens(
                principalTokenAmountValue,
                pairPriceWithTwap,
                pairPriceImmediate,
                principalTokenIsToken0
            );
    }

It is important to understand the format in which the prices pairPriceWithTwap and pairPriceImmediate are returned. As we can see, prior to executing the _getCollateralTokensAmountEquivalentToPrincipalTokens function, the pair price is fetched by querying _getUniswapV3TokenPairPrice. This function will return the pair’s price in Uniswap’s X96 format (you can check more about this concept [here](https://blog.uniswap.org/uniswap-v3-math-primer)):

// LenderCommitmentGroup_Smart.sol

function _getUniswapV3TokenPairPrice(uint32 _twapInterval)
        internal
        view
        returns (uint256)
    {
        // represents the square root of the price of token1 in terms of token0

        uint160 sqrtPriceX96 = getSqrtTwapX96(_twapInterval);

        //this output is the price ratio expanded by 1e18
        return _getPriceFromSqrtX96(sqrtPriceX96);
    }

    //this result is expanded by UNISWAP_EXPANSION_FACTOR
    function _getPriceFromSqrtX96(uint160 _sqrtPriceX96)
        internal
        pure
        returns (uint256 price_)
    {

        uint256 priceX96 = (uint256(_sqrtPriceX96) * uint256(_sqrtPriceX96)) / 
            (2**96);

        // sqrtPrice is in X96 format so we scale it down to get the price
        // Also note that this price is a relative price between the two tokens in the pool
        // It's not a USD price
        price_ = priceX96;
    }

After fetching the prices, the last function to finally compute the required collateral is _getCollateralTokensAmountEquivalentToPrincipalTokens :

// LenderCommitmentGroup_Smart.sol

 function _getCollateralTokensAmountEquivalentToPrincipalTokens(
        uint256 principalTokenAmountValue,
        uint256 pairPriceWithTwap,
        uint256 pairPriceImmediate,
        bool principalTokenIsToken0
    ) internal pure returns (uint256 collateralTokensAmountToMatchValue) {
        if (principalTokenIsToken0) {
            //token 1 to token 0 ?
            uint256 worstCasePairPrice = Math.min( 
                pairPriceWithTwap,
                pairPriceImmediate
            );

            collateralTokensAmountToMatchValue = token1ToToken0(
                principalTokenAmountValue,
                worstCasePairPrice //if this is lower, collateral tokens amt will be higher
            );
        } else {
            //token 0 to token 1 ?
            uint256 worstCasePairPrice = Math.max(
                pairPriceWithTwap,
                pairPriceImmediate
            );

            collateralTokensAmountToMatchValue = token0ToToken1(
                principalTokenAmountValue,
                worstCasePairPrice //if this is lower, collateral tokens amt will be higher
            );
        }
    }

    function token0ToToken1(uint256 amountToken0, uint256 priceToken1PerToken0)
        internal
        pure
        returns (uint256)
    {
        return
            MathUpgradeable.mulDiv(
                amountToken0,
                UNISWAP_EXPANSION_FACTOR,
                priceToken1PerToken0,
                MathUpgradeable.Rounding.Up
            );
    }

    //note: the price is still expanded by UNISWAP_EXPANSION_FACTOR
    function token1ToToken0(uint256 amountToken1, uint256 priceToken1PerToken0)
        internal
        pure
        returns (uint256)
    {
        return
            MathUpgradeable.mulDiv( 
                amountToken1,
                priceToken1PerToken0,
                UNISWAP_EXPANSION_FACTOR,
                MathUpgradeable.Rounding.Up
            );
    }

This function will convert the principalTokenAmountValue to the required collateral amount given the prices pairPriceWithTwap and pairPriceImmediate obtained from the pool. The token1ToToken0 and token0ToToken1 will perform the corresponding mulDiv operation so that the principal is converted properly.

As we can see, token1ToToken0 and token0ToToken1 perform the operation multiplying and dividing by UNISWAP_EXPANSION_FACTOR (which is 2**96). This is correct, and is done because as mentioned previously, the prices returned by the pool are in X96 notation.

The important concept to be aware of here is that after performing these computations, the amount of collateral computed will directly be given in the collateral units, because the token0ToToken1 and token1ToToken0 functions already scale and return the value in collateral units.

This leads us to the actual bug. As mentioned, when the getCollateralRequiredForPrincipalAmount function is called the amount returned will already be in the collateral units. However, when this function is called in acceptFundsForAcceptBid, the requiredCollateral returned by getCollateralRequiredForPrincipalAmount will be incorrectly multiplied by STANDARD_EXPANSION_FACTOR (1e18). This is because the code wrongly assumes that the amount required by getCollateralRequiredForPrincipalAmount is expanded by 10**18:

// LenderCommitmentGroup_Smart.sol

function acceptFundsForAcceptBid(
        address _borrower,
        uint256 _bidId,
        uint256 _principalAmount,
        uint256 _collateralAmount,
        address _collateralTokenAddress,
        uint256 _collateralTokenId, 
        uint32 _loanDuration,
        uint16 _interestRate
    ) external onlySmartCommitmentForwarder whenNotPaused {
        ...

        //this is expanded by 10**18
        uint256 requiredCollateral = getCollateralRequiredForPrincipalAmount(
            _principalAmount
        );

        require(
            (_collateralAmount * STANDARD_EXPANSION_FACTOR) >= 
                requiredCollateral,
            "Insufficient Borrower Collateral"
        );

        ...
    }

This makes it possible for users to pass a _collateralAmount smaller than requiredCollateral, given that the check to verify that the collateral passed by the user is actually enough will always scale the user’s supplied _collateralAmount value, making it possible for users to obtain undercollateralized loans.

Proof of Concept

The following proof of concept mimics the price that would be returned by the getCollateralRequiredForPrincipalAmount. In order to run it, create a foundry project that forks mainnet and paste the following code snippet:

function testVuln() public {
        uint32[] memory secondsAgos = new uint32[](2);
        secondsAgos[0] = uint32(3600); // from (before)
        secondsAgos[1] = 0; // to (now)

        (int56[] memory tickCumulatives, ) = IUniswapV3Pool(
            0xC2e9F25Be6257c210d7Adf0D4Cd6E3E881ba25f8
        ).observe(secondsAgos); // DAI / ETH pool with 3600 seconds observation

        uint160 sqrtPriceX96 = TickMath.getSqrtRatioAtTick(
            int24((tickCumulatives[1] - tickCumulatives[0]) / int32(3600))
        );

        uint256 priceX96 = FullMath.mulDiv(sqrtPriceX96, sqrtPriceX96, 2 ** 96);
        console.log("Computed price in X96 notation: ", priceX96);
        uint256 principalAmount = 1e18; // ETH is principal

        console.log("Amount returned by getCollateralRequiredForPrincipalAmount: ", (principalAmount * 2 ** 96) / priceX96);
    }

The computation performed in the final console log ((principalAmount * 2 ** 96) / priceX96)) corresponds to the computation that would be performed by the token0ToToken1 function. After running the code, we can see how the logged amount is in the correct collateral units (DAI units, 18 decimals).

Note: FullMath.mulDiv() is used to perform the multiplication executed in _getPriceFromSqrtX96 because the implementation of _getPriceFromSqrtX96 is flawed and overflows (related to another bug).

Impact

High. Scaling the user-supplied _collateralAmount by the STANDARD_EXPANSION_FACTOR when checking the required collateral will always make the _collateralAmount supplied be greater than the required collateral computed by the uniswap prices. This will allow users to obtain undercollateralized loans and drain the pool.

Code Snippet

https://github.com/sherlock-audit/2024-04-teller-finance/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/LenderCommitmentForwarder/extensions/LenderCommitmentGroup/LenderCommitmentGroup_Smart.sol#L368

Tool used

Manual Review

Recommendation

Remove the price scaling when checking the required collateral:

// LenderCommitmentGroup_Smart.sol

function acceptFundsForAcceptBid(
        address _borrower,
        uint256 _bidId,
        uint256 _principalAmount,
        uint256 _collateralAmount,
        address _collateralTokenAddress,
        uint256 _collateralTokenId, 
        uint32 _loanDuration,
        uint16 _interestRate
    ) external onlySmartCommitmentForwarder whenNotPaused {
        ...

        //this is expanded by 10**18
        uint256 requiredCollateral = getCollateralRequiredForPrincipalAmount(
            _principalAmount
        );

        require(
-            (_collateralAmount * STANDARD_EXPANSION_FACTOR) >= 
+            _collateralAmount >= 
                requiredCollateral,
            "Insufficient Borrower Collateral"
        );

        ...
    }

Duplicate of #58