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

0 stars 0 forks source link

Protocol fee is hardcoded to `0` during auto-adjusts #8

Closed c4-bot-9 closed 2 months ago

c4-bot-9 commented 2 months ago

Lines of code

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

Vulnerability details

Proof of Concept

Take a look at https://github.com/code-423n4/2024-06-krystal-defi/blob/f65b381b258290653fa638019a5a134c4ef90ba8/src/V3Automation.sol#L117-L129

    function _execute(ExecuteParams calldata params, address positionOwner) internal {
..
            if (params.protocolFeeX64 > 0) {
                (,,, protocolFeeAmount0, protocolFeeAmount1,) = _deductFees(DeductFeesParams(state.amount0, state.amount1, 0, params.protocolFeeX64, FeeType.PROTOCOL_FEE, address(params.nfpm), params.tokenId, positionOwner, state.token0, state.token1, address(0)), true);
            }
            state.amount0 = state.amount0 - gasFeeAmount0 - protocolFeeAmount0;
            state.amount1 = state.amount1 - gasFeeAmount1 - protocolFeeAmount1;
        }
        if (params.action == Action.AUTO_ADJUST) {
            require(state.tickLower != params.newTickLower || state.tickUpper != params.newTickUpper);
            SwapAndMintResult memory result;
            if (params.targetToken == state.token0) {
                result = _swapAndMint(SwapAndMintParams(params.protocol, params.nfpm, IERC20(state.token0), IERC20(state.token1), state.fee, params.newTickLower, params.newTickUpper, 0, state.amount0, state.amount1, 0, positionOwner, params.deadline, IERC20(state.token1), params.amountIn1, params.amountOut1Min, params.swapData1, 0, 0, bytes(""), params.amountAddMin0, params.amountAddMin1), false);
            } else if (params.targetToken == state.token1) {
                result = _swapAndMint(SwapAndMintParams(params.protocol, params.nfpm, IERC20(state.token0), IERC20(state.token1), state.fee, params.newTickLower, params.newTickUpper, 0, state.amount0, state.amount1, 0, positionOwner, params.deadline, IERC20(state.token0), 0, 0, bytes(""), params.amountIn0, params.amountOut0Min, params.swapData0, params.amountAddMin0, params.amountAddMin1), false);
            } else {
                // Rebalance without swap @audit all three _swapAndMint query under AUTO_ADJUST have the protocol fee set as `0`
                result = _swapAndMint(SwapAndMintParams(params.protocol, params.nfpm, IERC20(state.token0), IERC20(state.token1), state.fee, params.newTickLower, params.newTickUpper, 0, state.amount0, state.amount1, 0, positionOwner, params.deadline, IERC20(address(0)), 0, 0, bytes(""), 0, 0, bytes(""), params.amountAddMin0, params.amountAddMin1), false);
            }
            emit ChangeRange(address(params.nfpm), params.tokenId, result.tokenId, result.liquidity, result.added0, result.added1);
..
       }

This is the internal execute function that gets always gets queried in the V3Automation, and in the case where the action is AUTO_ADJUST the _swapAndMint() gets called, issue however is that all three instances hardcode the protocol fee as 0, regardless of what has been stored in params.protocolFeeX64, now see the implementation of the SwapAndMintParamsstruct here, evidently, there is a need to attach the right protocolFeeX64, see https://github.com/code-423n4/2024-06-krystal-defi/blob/f65b381b258290653fa638019a5a134c4ef90ba8/src/Common.sol#L151-L152

        uint64 protocolFeeX64;

But since this has been hardcoded to 0 and from this snippet in _execute() we can see that is this protocol fee == 0, which would then lead to swap and minting to be done in the wrong pretense considering the wrong (no) fee is attached

Impact

Protocol erroneously hardcodes the protocol fee as 0 for all instances of swapping and minting for the AUTO_ADJUST action in V3Automation, whereas this value could indeed be > 0.

Recommended Mitigation Steps

Consider applying the correct protocol fee when querying _swapAndMint() for the AUTO_ADJUST action.

Assessed type

Context

3docSec commented 2 months ago

Provisionally marking as satisfactory

c4-judge commented 2 months ago

3docSec marked the issue as satisfactory

c4-judge commented 2 months ago

3docSec marked the issue as selected for report

Haupc commented 2 months ago

Fee is already deducted above: https://github.com/code-423n4/2024-06-krystal-defi/blob/f65b381b258290653fa638019a5a134c4ef90ba8/src/V3Automation.sol#L101

c4-judge commented 2 months ago

3docSec marked the issue as unsatisfactory: Invalid

3docSec commented 2 months ago

Invalid as per sponsor comment