code-423n4 / 2024-06-krystal-defi-findings

0 stars 0 forks source link

Wrong logic in AUTO_COMPOUND doesn't allow for swap to token1 #28

Open c4-bot-3 opened 4 months ago

c4-bot-3 commented 4 months ago

Lines of code

https://github.com/code-423n4/2024-06-krystal-defi/blob/f65b381b258290653fa638019a5a134c4ef90ba8/src/V3Automation.sol#L158

Vulnerability details

Impact

Protocol functionality broken

Proof of Concept

AUTO_COMPOUND action allows for compounding your gains into liquidity. It additionally allows for swaps in the middle. There is a faulty condition though, which is never effective and does not allow to set token1 as targetToken, namely else if (state.token0 == state.token1):

        } else if (params.action == Action.AUTO_COMPOUND) {
            if (params.targetToken == state.token0) {
                _swapAndIncrease(
//[...]

            // @audit it should be params.targetToken == state.token1 Currently it doesn't allow for a swap
@>          } else if (state.token0 == state.token1) {
                _swapAndIncrease(
//[...]

            } else {
                // compound without swap
                _swapAndIncrease(
//[...]
            }

Because pools cannot have the same token0 and token1, there is no possible position that will fulfill this condition. Looking at other parts of the codebase and params passed to _swapAndIncrease() in this code branch, what the protocol wants to achieve is to check if params.targetToken == state.token1 and perform swap similarly to params.targetToken == state.token0 branch.

Tools Used

Manual Review

Recommended Mitigation Steps

        } else if (params.action == Action.AUTO_COMPOUND) {
            if (params.targetToken == state.token0) {
                _swapAndIncrease(SwapAndIncreaseLiquidityParams(params.protocol, params.nfpm, params.tokenId, state.amount0, state.amount1, 0, positionOwner, params.deadline, IERC20(state.token1), params.amountIn1, params.amountOut1Min, params.swapData1, 0, 0, bytes(""), params.amountAddMin0, params.amountAddMin1, 0), IERC20(state.token0), IERC20(state.token1), false);
-           } else if (state.token0 == state.token1) {
+           } else if (params.targetToken == state.token1) {
                _swapAndIncrease(SwapAndIncreaseLiquidityParams(params.protocol, params.nfpm, params.tokenId, state.amount0, state.amount1, 0, positionOwner, params.deadline, IERC20(state.token0), 0, 0, bytes(""), params.amountIn0, params.amountOut0Min, params.swapData0, params.amountAddMin0, params.amountAddMin1, 0), IERC20(state.token0), IERC20(state.token1), false);

Assessed type

Other

3docSec commented 4 months ago

The report is of sufficient quality

c4-judge commented 4 months ago

3docSec marked the issue as satisfactory

c4-judge commented 4 months ago

3docSec marked the issue as selected for report

3docSec commented 4 months ago

The finding completely misses a justification for the H severity. A swap not happening as it should is better categorized as M.

c4-judge commented 4 months ago

3docSec changed the severity to 2 (Med Risk)