liquidationValue will be lost due to wrong Assumption of Fee Valuation in Oracle Contract

[c4-bot-3]

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/main/src/V3Vault.sol#L1054-L1055 https://github.com/code-423n4/2024-03-revert-lend/blob/main/src/V3Vault.sol#L1275 https://github.com/code-423n4/2024-03-revert-lend/blob/main/src/V3Oracle.sol#L121

Vulnerability details

Impact

The Protocol is wrongly assuming that the sum of fee0 and fee1 would always be equal fee value, which was used to share liquidation value at _sendPositionValue(...) function. In a situation fee0 + fee1 is not equal feeValue, the excess liquidation value would be lost completely as it is not accounted for in the contract.

Proof of Concept

  function _sendPositionValue(
        uint256 tokenId,
        uint256 liquidationValue,
        uint256 fullValue,
        uint256 feeValue,
        address recipient
    ) internal returns (uint256 amount0, uint256 amount1) {
        uint128 liquidity;
        uint128 fees0;
        uint128 fees1;

        // if full position is liquidated - no analysis needed
        if (liquidationValue == fullValue) {
            (,,,,,,, liquidity,,,,) = nonfungiblePositionManager.positions(tokenId);
            fees0 = type(uint128).max;
            fees1 = type(uint128).max;
        } else {
>>>            (,,, liquidity,,, fees0, fees1) = oracle.getPositionBreakdown(tokenId);

            // only take needed fees
            if (liquidationValue < feeValue) {
                liquidity = 0;
>>>                fees0 = uint128(liquidationValue * fees0 / feeValue);
>>>                fees1 = uint128(liquidationValue * fees1 / feeValue);
            } else {
 ...

The _sendPositionValue(...) function above in the V3Vault contract shows how liquidationValue is shared whenever it is less than feeValue, as noted from the 2nd and 3rd pointers above, from the implementation, liquidity is reduced to zero why the liquidationValue is shared in the ratio of fees0 to fees1 with the assumption that sum of both would be equal feeValue. The first pointer shows how fees0 & fees1 are gotten from Oracle contract and a trackdown of feeValue shows that it is also called from the Oracle contract as noted in the code provided below.

    function _checkLoanIsHealthy(uint256 tokenId, uint256 debt)
        internal
        view
        returns (bool isHealthy, uint256 fullValue, uint256 collateralValue, uint256 feeValue)
    {
>>>        (fullValue, feeValue,,) = oracle.getValue(tokenId, address(asset));
        uint256 collateralFactorX32 = _calculateTokenCollateralFactorX32(tokenId);
        collateralValue = fullValue.mulDiv(collateralFactorX32, Q32);
        isHealthy = collateralValue >= debt;
    }

Finally, The code Provided below is from the Oracle Contract, it shows the interaction between feeValue and fee1 and fee0. Due to the complexities of the interaction between price0X96 , price1X96 and priceTokenX96 in deriving feeValue, there is no guarantee that feeValue would equal the sum of fee1 & fee0. As prices are always changing

function getValue(uint256 tokenId, address token)
        external
        view
        override
        returns (uint256 value, uint256 feeValue, uint256 price0X96, uint256 price1X96)
    {
        (address token0, address token1, uint24 fee,, uint256 amount0, uint256 amount1, uint256 fees0, uint256 fees1) =
            getPositionBreakdown(tokenId);

        uint256 cachedChainlinkReferencePriceX96;

   >>     (price0X96, cachedChainlinkReferencePriceX96) =
            _getReferenceTokenPriceX96(token0, cachedChainlinkReferencePriceX96);
   >>     (price1X96, cachedChainlinkReferencePriceX96) =
            _getReferenceTokenPriceX96(token1, cachedChainlinkReferencePriceX96);

        uint256 priceTokenX96;
        if (token0 == token) {
            priceTokenX96 = price0X96;
        } else if (token1 == token) {
            priceTokenX96 = price1X96;
        } else {
            (priceTokenX96,) = _getReferenceTokenPriceX96(token, cachedChainlinkReferencePriceX96);
        }

        value = (price0X96 * (amount0 + fees0) / Q96 + price1X96 * (amount1 + fees1) / Q96) * Q96 / priceTokenX96;
>>>        feeValue = (price0X96 * fees0 / Q96 + price1X96 * fees1 / Q96) * Q96 / priceTokenX96;
        price0X96 = price0X96 * Q96 / priceTokenX96;
        price1X96 = price1X96 * Q96 / priceTokenX96;

        // checks derived pool price for price manipulation attacks
        // this prevents manipulations of pool to get distorted proportions of collateral tokens - for borrowing
        // when a pool is in this state, liquidations will be disabled - but arbitrageurs (or liquidator himself)
        // will move price back to reasonable range and enable liquidation
        uint256 derivedPoolPriceX96 = price0X96 * Q96 / price1X96;
        _checkPoolPrice(token0, token1, fee, derivedPoolPriceX96);
    }

Tools Used

Manual Review

Recommended Mitigation Steps

The protocol should not assume feeValue would always equal fee0 + fee1 instead should make arrangement to ensure the liquationValue in the _sendPositionValue(...) function of the V3Vault.sol contract is not lost due to this wrong assumption.

Assessed type

Math

[c4-pre-sort]

0xEVom marked the issue as insufficient quality report

[0xEVom]

Unclear that this assumption is indeed made.

[jhsagd76]

The units of feeValue and fee0, fee1 are not the same here; one represents value and the other represents tokens. The underlying calculation of feeValue is entirely the same as that of fee0 and fee1, with the only potential discrepancy arising from separately calculating their values.

[c4-judge]

jhsagd76 marked the issue as unsatisfactory: Invalid