code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/talos/base/TalosBaseStrategy.sol#L353-L361

Vulnerability details

Impact

Undesired swaps / mints / withdrawals will occur.

Proof of Concept

Only when redeeming in TalosBaseStrategy.sol is the slippage set as amount0Min and amount1Min:

            (amount0, amount1) = _nonfungiblePositionManager.decreaseLiquidity(
                INonfungiblePositionManager.DecreaseLiquidityParams({
                    tokenId: _tokenId,
                    liquidity: liquidityToDecrease,
                    amount0Min: amount0Min,
                    amount1Min: amount1Min,
                    deadline: block.timestamp
                })
            );

For the rest of the functions like init, _withdrawAll, deposit, both amount0Min and amount1Min is set to zero.

        _nonfungiblePositionManager.decreaseLiquidity(
            INonfungiblePositionManager.DecreaseLiquidityParams({
                tokenId: _tokenId,
                liquidity: _liquidity,
                amount0Min: 0,
                amount1Min: 0,
                deadline: block.timestamp
            })
        );

Whereas the protocol refunds the unused amounts back to the user, there is no slippage protection when engaging in Uniswap v3 functions like mint, increase liquidity and decrease liquidity.

In Uniswapv3 Docs, it is mentioned that:

In production, amount0Min and amount1Min should be adjusted to create slippage protections.

For mint, the issue with 0 slippage is that a function calling mint with no slippage protection would be vulnerable to a frontrunning attack designed to execute the mint call at an inaccurate price.

Reference: https://docs.uniswap.org/contracts/v3/guides/providing-liquidity/mint-a-position

Tools Used

VSCode

Recommended Mitigation Steps

Having the input parameters amount0Min and amount1Min instead of setting amount0Min: 0 and amount1Min: 0 will be good for slippage protection.