c4-bot-10 commented 3 months ago

Lines of code

https://github.com/revert-finance/lend/blob/dcfa79924c0e0ba009b21697e5d42d938ad9e5e3/src/V3Vault.sol#L1241

Vulnerability details

Original issue details

Finding [M-20] - Tokens can't be removed as a collateral without breaking liquidations and other core functions describes a case where disabling collateral token while there are outstanding loans collateralized by such token leads to liquidation being DOSed.

Here is a short summary of the problem:

A token gets disabled by configuring its collateralFactorX32 to 0
collateralFactorX32 is used inside _checkLoanIsHealthy(), which calculates the collateral value
_checkLoanIsHealthy() returns 0 when collateralFactorX32 is 0
there is a calculation inside liquidate() that uses the result of _checkLoanIsHealthy() in its divisor and since it is 0 it reverts the whole function.

The problem has been mitigated by introducing a 0 value check in this PR-25

New issue details

There is another serious problem that arises when a collateral token is disabled. It is that all outstanding debt positions collateralized by the disabled token will become immediately unhealthy and liquidatable even if they are actually healthy and have enough collateral value to cover the debt 100 times over.

Here is a more detailed breakdown of the problem

V3Vault.liquidate() requires a position to become unhealthy before it can be liquidated:

https://github.com/revert-finance/lend/blob/dcfa79924c0e0ba009b21697e5d42d938ad9e5e3/src/V3Vault.sol#L742

 function liquidate(LiquidateParams calldata params) external override returns (uint256 amount0, uint256 amount1) {
   ....

  (state.isHealthy, ....) = _checkLoanIsHealthy(params.tokenId, state.debt, false);

  if (state.isHealthy) {
      revert NotLiquidatable();
  }
   ....
}

The health status of a function is calculated inside V3Vault._checkLoanIsHealthy():

https://github.com/revert-finance/lend/blob/dcfa79924c0e0ba009b21697e5d42d938ad9e5e3/src/V3Vault.sol#L1371-L1372

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

The collateralFactorX32 variable from the above function is detrimental for calculating the final value of the collateral. It basically determines the LTV (Loan-To-Value) for a position based on the pair tokens it is composed of. If collateralFactorX32 is 0 it means that the position tokens have either not been enabled as collateral or that they have been disabled. In both cases _checkLoanIsHealthy will return isHealthy=false regardless of the actual collateral value in the position.

In order to understand the problem it is very important to look at the _calculateTokenCollateralFactorX32() function that calculates collateralFactorX32 inside _checkLoanIsHealthy()

https://github.com/revert-finance/lend/blob/dcfa79924c0e0ba009b21697e5d42d938ad9e5e3/src/V3Vault.sol#L1241

 function _calculateTokenCollateralFactorX32(uint256 tokenId) internal view returns (uint32) {
        (,, address token0, address token1,,,,,,,,) = nonfungiblePositionManager.positions(tokenId);
        uint32 factor0X32 = tokenConfigs[token0].collateralFactorX32;
        uint32 factor1X32 = tokenConfigs[token1].collateralFactorX32;
        return factor0X32 > factor1X32 ? factor1X32 : factor0X32; // <---- take the smaller value
    }

It always takes the lesser of collateral factors configured for the position tokens to determine collateralFactorX32 used in calculating the health status of the position. This means that even if a position has enough liquidity in token1 (not disabled) it will not be considered if token2 gets disabled ( 0 is always lower).

This is a desirable behavior when borrowing since the protocol should prevent new loans against collateral tokens that are disabled. However this is not acceptable during liquidation, since it would retroactively affect positions that were considered valid before the token was disabled and it will penalize them regardless of their actual health status.

POC

Here is a simple example of the vulnerability

Alice opens a position that holds WETH/USDC as the underlying tokens
Alice borrows from the Vault
Alice position is healthy and has enough liquidity to cover the loan 5 times over
USDC gets disabled as a collateral - tokenConfigs[USDC ].collateralFactorX32=0
Bob calls liquidate() for Alice position
Since _checkLoanIsHealthy() will return false (collateralFactorX32 is 0) , Alice will be unjustly liquidated

Recommended Mitigation

Consider implementing the following changes in order to handle the collateral token removal more swiftly in the protocol and with less impact on it's users

The fix considers the fact that _checkLoanIsHealthy() is used in multiple functions, each with its own expected behaviour after token is disabled:

V3Vault.transform() - must prevent new transformations after position token is disabled
V3Vault.borrow() - must prevent new loans after position token is disabled
V3Vault.decreaseLiquidityAndCollect() - owner should not be prevented from managing the position delegated to the vault. The position can have plenty of liquidity in the non-disabled token.
V3Vault.liquidate()- healthy positions must not become liquidatable once a token is disabled

First refactor _calculateTokenCollateralFactorX32() so that it checks if one of the tokens is disabled and returns the collateral factor of the other one instead:

function _calculateTokenCollateralFactorX32(uint256 tokenId) internal view returns (uint32) {
        ....
        // if one of the tokens is disabled return the factor for the other one
        if(factor0X32 === 0 ) return factor1X32 
        if(factor1X32 === 0 ) return factor0X32 

         // a default factorx32 can be added in case both tokens are disabled

        return factor0X32 > factor1X32 ? factor1X32 : factor0X32;
    }

This way the health check would still be evaluated based on the current position collateral and acceptable reserve factor set by the protocol.

It will prevent unjust liquidations in liquidate()
It will allow position to be managed in decreaseLiquidityAndCollect()

To handle the cases in transform() & borrow() a new check must be added, since the refactored health check from above won't prevent them from getting called

 function _hasDisabledTokens(uint256 tokenId) internal view returns (uint32) {
        (,, address token0, address token1,,,,,,,,) = nonfungiblePositionManager.positions(tokenId);
        uint32 factor0X32 = tokenConfigs[token0].collateralFactorX32;
        uint32 factor1X32 = tokenConfigs[token1].collateralFactorX32;

        return factor0X32 == 0 || factor1X32 == 0
    }

And then add it to the borrow and transform function

 function borrow(uint256 tokenId, uint256 assets) external override {
         ....
        // Prevent borrows for disabled collateral
        if (_hasDisabledTokens(tokenId)) {
            revert CollateralDisabled();
        }
   }

 ......

  function transform(uint256 tokenId, address transformer, bytes calldata data) external override  {
         ....
        // Prevent transformations for disabled collateral
        if (_hasDisabledTokens(tokenId)) {
            revert CollateralDisabled();
        }
   }

The main idea of the whole change is that it will prevent new loans to be created with the disabled collateral, while allowing the outstanding loans to be handled gracefully.

Disabling tokens should not influence retroactively the positions that were created before the changes, especially when funds will be lost.

Comments from the auditor

Speaking from my experience as a dev I would like to add that I also agree with the recommendation in the original submission:

Don't use the collateralFactor as the common denominator whether a token is accepted as collateral or not, use a method such as whitelisting tokens by address and performing necessary checks to see if the token address matches the whitelist.

I've often been tempted to reuse variables and manage multiple states simultaneously, but 90% of the time I end up shooting myself in the foot. This happens because the number of scenarios to consider increases exponentially with each new state. That's why the proposed mitigation above includes a couple of steps - essentially, we're dividing the various states managed by collateralFactorX32 into separate functions.

This is just an advice I felt the urge to share with the sponsors - it's up to the team to decide how they would approach the case.

Assessed type

Invalid Validation

kalinbas commented 2 months ago

This is a general issue with being able to lower collateralFactor (not only setting it to 0). We are conscious about this, and there will be a timelock controlling this function of course. This is as designed

c4-sponsor commented 2 months ago

kalinbas (sponsor) disputed

BogoCvetkov commented 2 months ago

Agree that a time-lock mechanism would probably remediate the issue.

I would like to say I'm providing the arguments below as info for the judge, so you can decide together the validity of this submission:

I'm basing this finding on the original issue which had the same root case, where problem was liquidations were DOSed because of the division by 0. The issue has been confirmed by the sponsors and all the judges, which creates the assumption that this should be considered as a valid root cause!

This is why I allowed myself to submit the other serious issue related to this root cause that has not been fixed. Also the timelock mechanism does not exist currently in the protocol and has also not been mentioned in the original issue as a safeguarding mechanism that is planned to be implemented to handle the case!

Without the knowledge of this mechanism and considering the confirmed validity of the root cause - this looks like a valid bug!

As promised, I won't be turning this into some exhausting dead-end discussion, so leaving the rest to the judge!

Thanks!

jhsagd76 commented 2 months ago

I will mark this issue as OOS (categorized as a misconfiguration by the admin).

I would like to explain why the original issue is valid, while this issue is marked as OOS.

For issues related to administrator configuration, the most important criterion for judgment is whether the relevant configuration is common and reasonable (not erroneous).

Let's first establish a scenario that is applicable to both issues.

A pair that is being used as collateral, such as Luna UST, is currently depegging.

At this point, the administrator announces that we are discontinuing UST as collateral. However, considering that there are still many positions with UST collateral.

setting the collateralFactorX32 directly to 0 would be extremely unfair to these positions. At this time, we can only rely on normal liquidations to counteract the depegging or even bad debts.

Delay the action of disabling the collateral and set the daily lending limit to zero. This is a normal admin configuration and decision.

For the original issue, even if the administrator fulfills the notification obligation, the remaining UST positions cannot be liquidated after the delay in disabling. This will leave persistent bad debt within the system.

For this issue, TBH, setting collateralFactorX32 to zero directly without notification and delay is more like a misconfiguration by the admin.

c4-judge commented 2 months ago

jhsagd76 marked the issue as unsatisfactory: Out of scope

code-423n4 / 2024-04-revert-mitigation-findings

Healthy position owners can be liquidated immediately once a token is disabled #53