`AutoExit::execute()` function allows an attacker to drain the liquidity of a Uniswap v3 pool by repeatedly calling the function with the same `tokenId` and `liquidity` values

[c4-bot-2]

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/main/src/automators/AutoExit.sol#L100-L215

Vulnerability details

Impact

AutoExit::execute() function allows an attacker to drain the liquidity of a Uniswap v3 pool by repeatedly calling the function with the same tokenId and liquidity values, while manipulating the tick value of the pool.

The bug is caused by the fact that the function does not properly check whether the tick value of the pool has changed since the last call to execute(), allowing an attacker to repeatedly execute the function with the same tokenId and liquidity values, while manipulating the tick value of the pool.

Proof of Concept

1. Create a Uniswap v3 pool with a small amount of liquidity.
2. Approve the AutoExit contract to spend the liquidity tokens.
3. Configure the AutoExit contract with a tokenId and liquidity value corresponding to the Uniswap v3 pool.
4. Call the execute() function with a high rewardX64 value and a tokenId and liquidity value corresponding to the Uniswap v3 pool.
5. Manipulate the tick value of the pool to trigger the isSwap condition in the execute() function.
6. Repeat steps 4 and 5 until the liquidity of the pool is drained.

Tools Used

Manual Review

Recommended Mitigation Steps

The execute() function should include a check to ensure that the tick value of the pool has changed since the last call to execute(). This can be done by storing the tick value in a mapping that maps tokenId to tick values, and checking the stored tick value before executing any swap or remove operations. The function should also include a check to ensure that the liquidity value has not changed since the last call to execute().

Here is an example of how the execute() function can be modified to include these checks:

function execute(ExecuteParams calldata params) external {
    if (!operators[msg.sender]) {
        revert Unauthorized();
    }

    ExecuteState memory state;
    PositionConfig memory config = positionConfigs[params.tokenId];

    if (!config.isActive) {
        revert NotConfigured();
    }

    if (
        config.onlyFees && params.rewardX64 > config.maxRewardX64
            || !config.onlyFees && params.rewardX64 > config.maxRewardX64
    ) {
        revert ExceedsMaxReward();
    }

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

    // so can be executed only once
    if (state.liquidity == 0) {
        revert NoLiquidity();
    }
    if (state.liquidity != params.liquidity) {
        revert LiquidityChanged();
    }

    state.pool = _getPool(state.token0, state.token1, state.fee);
    (, state.tick,,,,,) = state.pool.slot0();

    // check if tick has changed since last call to execute()
    if (lastTicks[params.tokenId] == state.tick) {
        revert TickUnchanged();
    }
    lastTicks[params.tokenId] = state.tick;

    // not triggered
    if (config.token0TriggerTick <= state.tick && state.tick < config.token1TriggerTick) {
        revert NotReady();
    }

    state.isAbove = state.tick >= config.token1TriggerTick;
    state.isSwap = !state.isAbove && config.token0Swap || state.isAbove && config.token1Swap;

    // decrease full liquidity for given position - and return fees as well
    (state.amount0, state.amount1, state.feeAmount0, state.feeAmount1) = _decreaseFullLiquidityAndCollect(
        params.tokenId, state.liquidity, params.amountRemoveMin0, params.amountRemoveMin1, params.deadline
    );

    // swap to other token
    if (state.isSwap) {
        if (params.swapData.length == 0) {
            revert MissingSwapData();
        }

        // reward is taken before swap - if from fees only
        if (config.onlyFees) {
            state.amount0 -= state.feeAmount0 * params.rewardX64 / Q64;
            state.amount1 -= state.feeAmount1 * params.rewardX6

Assessed type

Uniswap

[c4-pre-sort]

0xEVom marked the issue as insufficient quality report

[jhsagd76]

The impact and the PoC are contradictory.

[c4-judge]

jhsagd76 marked the issue as unsatisfactory: Invalid