Calling IncreaseLiquidity in AutoCompound without Slippage Protection and block.timestamp as Deadline can Cause Loss of Funds.

[c4-bot-8]

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/transformers/AutoCompound.sol#L163-L167

Vulnerability details

Impact

Calling IncreaseLiquidity in AutoCompound without Slippage Protection and block.timestamp as Deadline can Cause Loss of Funds for the user.

Proof of Concept

The execute function from the AutoCompound contract is calling the increaseLiquidity function from uniswap to compound fees generated from the user's position, but the calling to the increaseLiquidity is sending default and unrecommended values as sending 0s for the amount0Min and amount1Min and setting block.timestamp as de deadline, this means that there's no slippage protection and the tx can be executed at any time the node decide, this can cause loss of funds for the users due to mev manipulation as the transaction will accept any tx even if that results in 0 value added for the users without reverting.

As this is a transformer executed autonomously this can happen as many times as the operator bot keeps executing the Autocompound for compounding fees, so generating repeated losses for the users until is detected and corrected.

https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/transformers/AutoCompound.sol#L163-L167

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

This same problem is present in the AutoRange contract when calling the mint function to create a new position the amount0Min and amount1Min are set to 0s which lets the transaction without slippage protection for the new position being minted.

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

            INonfungiblePositionManager.MintParams memory mintParams = INonfungiblePositionManager.MintParams(
                address(state.token0),
                address(state.token1),
                state.fee,
                SafeCast.toInt24(baseTick + config.lowerTickDelta), // reverts if out of valid range
                SafeCast.toInt24(baseTick + config.upperTickDelta), // reverts if out of valid range
                state.maxAddAmount0,
                state.maxAddAmount1,
                0,
                0,
                address(this), // is sent to real recipient aftwards
                params.deadline
            );

Tools Used

Manual Code Review.

Recommended Mitigation Steps

As a recommendation, you can set the minimum values accepted for the slippage as a percentage of the tokens added to the position while compounding, and allow the user to configure a deadline for this AutoCompound transformer.

Assessed type

Other

[c4-pre-sort]

0xEVom marked the issue as sufficient quality report

[c4-pre-sort]

0xEVom marked the issue as duplicate of #198

[c4-judge]

jhsagd76 marked the issue as unsatisfactory: Out of scope

[mivgubel]

The execute function in the AutoCompund does not validate the minimum value that the user can receive when increasing liquidity since it never calls _validateSwap as the AutoRange's execute function does, the execute function in the AutoCompound only calls _hasMaxTWAPTickDifference and _poolSwap If a swap in the tokens is necessary but if it is not required the amount0Min and amount1Min is not validated and is sent as 0, this opens the door for a user to lose their tokens when trying to increase liquidity without doing a swap in the tokens as I described in the report.

Additionally, there is the problem of using block.timestamp as mentioned in the report which allows the Tx to be executed at any time and perhaps by the time a node executes it the user may no longer want its execution because market conditions have changed and is no longer beneficial to the end user.

I agree that the report of the AutoRange function is not valid since it calls the _validateSwap function that validates the minimum amounts to accept from the swap, but the reported AutoCompound function does not call this function that is used to validate the slippage, so it is necessary to add this call to protect the increaseLiquidity operation in any scenario and not only when a swap is made.

[jhsagd76]

The PriceDifference and tickDifference are prepared for slippage (price) checks in different scenarios. AutoCompound is for compounding interest and only needs to concern itself with the current pool's tick TWAP check without worrying about the external price.