Revert Lend - iamandreiski's results

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/V3Vault.sol#L743-L744 https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/V3Vault.sol#L1077-L1083

Vulnerability details

Impact

Malicious users whose positions are underwater but still have a remaining part of the position which needs to be returned to them through _cleanupLoan() during the liquidation, can indefinitely prevent/postpone a liquidation, by refusing to accept the NFT from the Non-Fungible Position Manager and the function always reverting.

Proof of Concept

When a user's position is eligible for liquidation and liquidate() is called, towards the end after the of the liquidation flow, cleanupLoan() is called in order to disarm the loan and send the remaining position to the owner:

        // disarm loan and send remaining position to owner
_cleanupLoan(params.tokenId, state.newDebtExchangeRateX96, state.newLendExchangeRateX96, owner);

owner is the owner of the NFT position which was put as a collateral: address owner = tokenOwner[params.tokenId];

If the owner of the NFT position has any part of the position that needs to be returned to them (meaning the position should be returned in the current state after the liquidation to the owner):

  _removeTokenFromOwner(owner, tokenId);
        _updateAndCheckCollateral(tokenId, debtExchangeRateX96, lendExchangeRateX96, loans[tokenId].debtShares, 0);
        delete loans[tokenId];
        nonfungiblePositionManager.safeTransferFrom(address(this), owner, tokenId);

The owner of that position can indefinitely postpone the liquidation and make sure that liquidate() always reverts, by refusing to accept the NFT position.

As part of the cleanupLoan as shown above, it calls the safeTransferFrom() method on the Non-Fungible Position Manager, this will check the presence of the onERC721Received() on the user's side. If said user is a malicious contract which refuses to accept the ERC721 NFT, meaning the function reverts any time that onERC721Received() is called on the contract, they can indefinitely postpone / DoS the liquidation process for their loan.

This process can continue indefinitely leading to the creation of bad debt.

Tools Used

• Manual Review

Recommended Mitigation Steps

Instead of sending the NFT to the user, employ a "pull" mechanism in which the user should call a function in order for them to get the remaining position back rather than doing it as a "push" mechanism inside of the liquidations.

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/V3Vault.sol#L856-L866 https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/V3Vault.sol#L1197-L1202 https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/V3Vault.sol#L1270-L1278 https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/V3Vault.sol#L702-L703 https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/V3Vault.sol#L1090-L1120

Vulnerability details

Impact

The core mechanism that Revert utilizes to prevent arbitrary tokens to be utilized as collateral is by the default setting of the collateralFactor. Since the collateralFactor for all tokens (besides the ones approved for usage in the system, and set by admins) is 0, that means that no one can borrow against a collateral which hasn't been approved.

The problem arises when admins would want a collateral removed from the system. There would be multiple reasons as to why this might be the case:

• The underlying collateral has become too volatile.
• The DAO in charge of Revert protocol decides to remove it as a collateral.
• There has been some kind of a change in the mechanics of how that token operates and Revert wants it removed (e.g. upgradeable tokens, most-popular examples include USDC/USDT).

The only way in which this could be performed is to set the collateralFactor back to 0, but this would break core mechanics such as liquidations.

Proof of Concept

All approved collateral which admins decided should be utilized inside of the protocol is introduced by increasing the collateralFactor through setTokenConfig():

 function setTokenConfig(address token, uint32 collateralFactorX32, uint32 collateralValueLimitFactorX32)
        external
        onlyOwner
    {
        if (collateralFactorX32 > MAX_COLLATERAL_FACTOR_X32) {
            revert CollateralFactorExceedsMax();
        }
        tokenConfigs[token].collateralFactorX32 = collateralFactorX32;
        tokenConfigs[token].collateralValueLimitFactorX32 = collateralValueLimitFactorX32;
        emit SetTokenConfig(token, collateralFactorX32, collateralValueLimitFactorX32);
    }

Once a token's collateralFactor was set, and removed afterwards due to extraordinary circumstances, all outstanding loans will never be able to be liquidated either due to panic reverts because of overflow/underflow or panic reverts due to division/modulo by 0.

The problem arises when _checkLoanIsHealthy() is called within liquidate() (the same can be tested by calling loanInfo() as well, since it also calls _checkLoanIsHealthy()).

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

This happens because when calculating the collateralValue through _checkLoanIsHealthy as it can be seen here:

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

Since it's needed to calculate the liquidation collateral value as it can be seen in the liauidate() function:

(state.isHealthy, state.fullValue, state.collateralValue, state.feeValue) =
            _checkLoanIsHealthy(params.tokenId, state.debt);

Since the collateral factor will be 0, the collateralValue will also be 0, this will lead to passing 0 as a value in the _calculateLiquidation() function:

 (state.liquidationValue, state.liquidatorCost, state.reserveCost) =
            _calculateLiquidation(state.debt, state.fullValue, state.collateralValue);

Which will always revert due to division with 0:

unction _calculateLiquidation(uint256 debt, uint256 fullValue, uint256 collateralValue)
        internal
        pure
        returns (uint256 liquidationValue, uint256 liquidatorCost, uint256 reserveCost)
    {
            ...
            uint256 startLiquidationValue = debt * fullValue / collateralValue;
            ...

This can be tested via PoC, by inserting the following line in the testLiquidation() test in V3Vault.t.sol:

vault.setTokenConfig(address(USDC), 0, type(uint32).max); vault.setTokenConfig(address(DAI), 0, type(uint32).max)

You can change USDC/DAI one or both with whatever collateral is part of the NFT in question and run testLiquidationTimeBased().

Tools Used

Manual Review

Recommended Mitigation Steps

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.

Assessed type

DoS