M-11 Unmitigated

[c4-bot-7]

Lines of code

https://github.com/revert-finance/lend/blob/audit/src/V3Vault.sol#L566

Vulnerability details

C4 issue

M-11: Lack of safety buffer in _checkLoanIsHealthy could subject users who take out the max loan into a forced liquidation

Comment

The original function _checkLoanIsHealthy does not include a buffer:

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;
    }

If a user borrows max value (debt = collateralValue), then their loan is still considered healthy and borrow is successful. This makes their loans very sensitive to market changes since just a small decrease in collateralValue could makes their loans considered not healthy and be liquidatable.

Impact

User loans is not protected with buffer zone in case they transform their loans.

Proof of concept

The code mitigates the issue by PR #17 The mitigation added a new variable withBuffer to function _checkLoanIsHealthy:

function _checkLoanIsHealthy(uint256 tokenId, uint256 debt, bool withBuffer)
        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 = (withBuffer ? collateralValue * BORROW_SAFETY_BUFFER_X32 / Q32 : collateralValue) >= debt;
    }

This flag withBuffer value depends on the function it's called. For borrow and decreaseLiquidityAndCollect, withBuffer is true; for liquidate it's false. This makes sense because if you calls borrow or decreaseLiquidityAndCollect, this makes their debt increase or their collateral value decrease and the buffer is required to protect them; while in liquidate buffer is not used meaning it's only liquidatable if full collateralValue < debt.

However, function transform currently has withBuffer = false:

function transform(uint256 tokenId, address transformer, bytes calldata data)
        external
        override
        returns (uint256 newTokenId)
    {
        ...

        (bool success,) = transformer.call(data);
        if (!success) {
            revert TransformFailed();
        }

        ...
        _requireLoanIsHealthy(tokenId, debt, false);

    }

The makes user loans not protected by buffer zone after transformation. The repo already noted that the fix Added safety buffer for borrow and decreaseLiquidity (not for transformers). However, I want to point out that this creates inconsistency because if user call transformation with transformer V3Utils and mode = WITHDRAW_AND_COLLECT_AND_SWAP, it's basically the same with calling V3Vault.decreaseLiquidityAndCollect (both will decrease liquidity and collect, the only difference is that calling V3Utils with mode = WITHDRAW_AND_COLLECT_AND_SWAP will do an optional swap afterward).

However, only decreaseLiquidityAndCollect protects user with buffer zone and transform will not. This can make collateralValue only slightly larger then debt and we're back to the original issue.

Recommended Mitigation Steps

Use _requireLoanIsHealthy with withBuffer = true in transform function.

Assessed type

Invalid Validation

[kalinbas]

This is as designed. Methods that use transform are advanced integrations which should not have this Safety Buffer (like leveragetransformer)

[jhsagd76]

The issue points out inconsistencies in certain similar functionalities, but I believe, as the sponsor stated, that this is by design. Some interfaces focus on usability, while others prioritize capt efficiency, this is the essence of advanced integrations design.

[c4-judge]

jhsagd76 marked the issue as nullified

[jhsagd76]

If our Transformer is designed to push the position to maximum leverage without any buffer, then MEV bots are motivated to lose a bit of tokens within the range allowed by TWAP to launch a griefing sandwich attack. The goal is to force the position to be liquidated so that the liquidation reward covers the cost of the sandwich attack. If I have missed any crucial checks in the liquidation, please let me know.

[c4-judge]

jhsagd76 marked the issue as satisfactory

[c4-judge]

jhsagd76 marked the issue as confirmed for report

[kalinbas]

The price of the oracle is used for collateral valuation and not the price of the pool (in the normal mode of the oracle). So this means by moving the pool price you can not force a liquiditation.

[jhsagd76]

The price of the oracle is used for collateral valuation and not the price of the pool (in the normal mode of the oracle). So this means by moving the pool price you can not force a liquiditation.

yep yep, you are right, I missed that. Is that why I feel like I'm missing something like a check in the liquiditation. thx

[c4-judge]

jhsagd76 marked the issue as not confirmed for report

[c4-judge]

jhsagd76 marked the issue as nullified