Lines of code

https://github.com/code-423n4/2024-06-krystal-defi/blob/f65b381b258290653fa638019a5a134c4ef90ba8/src/V3Automation.sol#L79-L81 https://github.com/code-423n4/2024-06-krystal-defi/blob/f65b381b258290653fa638019a5a134c4ef90ba8/src/StructHash.sol#L274-L296

Vulnerability details

Impact

User signed orders can be replayed

Proof of Concept

All orders managed by Krystal team are signed by the user, executed by operator and the signature is verified on-chain:

src/V3Automation.sol
    function execute(ExecuteParams calldata params) public payable onlyRole(OPERATOR_ROLE) whenNotPaused() {
        require(_isWhitelistedNfpm(address(params.nfpm)));
        address positionOwner = params.nfpm.ownerOf(params.tokenId);
@>      _validateOrder(params.userOrder, params.orderSignature, positionOwner);
        _execute(params, positionOwner);
    }
//[...]
    function _validateOrder(StructHash.Order memory order, bytes memory orderSignature, address actor) internal view {
@>      address userAddress = recover(order, orderSignature);
@>      require(userAddress == actor);
        require(!_cancelledOrder[keccak256(orderSignature)]);
    }

src/EIP712.sol
    function recover(StructHash.Order memory order, bytes memory signature) internal view returns (address) {
@>      bytes32 digest = _hashTypedDataV4(StructHash._hash(order));
@>      return ECDSA.recover(digest, signature);
    }

However, there is no nonce field in the StructHash.Order. That means that the same hashed order can be reused multiple times. What's also important is that the order is used only to verify if it was signed by the user, however it's not checked if the order that user signed is the same as the params passed by the operator, because ExecuteParams sent to V3Automation.execute() use other set of params to indicate the action to perform:

    struct ExecuteParams {
        Action action;
        Protocol protocol;
        INonfungiblePositionManager nfpm;

        uint256 tokenId;
        uint128 liquidity; // liquidity the calculations are based on

        // target token for swaps (if this is address(0) no swaps are executed)
        address targetToken;

        uint256 amountIn0;
        // if token0 needs to be swapped to targetToken - set values
        uint256 amountOut0Min;
        bytes swapData0;
        //[...]
        // user signed config
        StructHash.Order userOrder;
        bytes orderSignature;
    }

And later in the function, params.userOrder is not used, only the params prepared by the operator:

src/V3Automation.sol
@>      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
@>              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);
            }

Actually, this behaviour is shown in test/integration/V3Automation.t.sol:

@>  StructHash.Order emptyUserConfig; // todo: remove this when we fill user configuration

    function setUp() external {
        _setupBase();
    }

    function testAutoAdjustRange() external {
        // add liquidity to existing (empty) position (add 1 DAI / 0 USDC)
        _increaseLiquidity();
        (address userAddress, uint256 privateKey) = makeAddrAndKey("positionOwnerAddress");

        vm.startPrank(TEST_NFT_ACCOUNT);
        NPM.safeTransferFrom(TEST_NFT_ACCOUNT, userAddress, TEST_NFT);
        vm.stopPrank();

@>      bytes memory signature = _signOrder(emptyUserConfig, privateKey);

        uint256 countBefore = NPM.balanceOf(userAddress);

        (, , , , , , , uint128 liquidityBefore, , , , ) = NPM.positions(
            TEST_NFT
        );

        V3Automation.ExecuteParams memory params = V3Automation.ExecuteParams(
            V3Automation.Action.AUTO_ADJUST,
            Common.Protocol.UNI_V3,
            NPM,
            TEST_NFT,
            liquidityBefore,
            address(USDC),
            500000000000000000,
            400000,
            _get05DAIToUSDCSwapData(),
            0,
            0,
            "",
            0,
            0,
            block.timestamp,
            184467440737095520, // 0.01 * 2^64
            0,
            MIN_TICK_500,
            -MIN_TICK_500,
            true,
            0,
            0,
            emptyUserConfig,
            signature
        );

        // using approve / execute pattern
        vm.prank(userAddress);
        NPM.setApprovalForAll(address(v3automation), true);

        vm.prank(TEST_OWNER_ACCOUNT);

        v3automation.execute(params);

        // now we have 2 NFTs (1 empty)
        uint256 countAfter = NPM.balanceOf(userAddress);
        assertGt(countAfter, countBefore);

        (, , , , , , , uint128 liquidityAfter, , , , ) = NPM.positions(
            TEST_NFT
        );
        assertEq(liquidityAfter, 0);
    }

In the test above, emptyUserConfig - being really empty, i.e. consisting of only 0s, because it's not being set - is being signed and the order successfully executes, even though the action adds liquidity. That means that the deadline, liquidity to get, recipient of the funds, amount , etc. - basically all parameters that user signs are all independent from what is being send on-chain, and can be totally different.

To summarize, there are multiple occassions, where no nonce and actually no verification of execute params confirmity to uder signed order can be exploited:

Purposeful action
- private operator key compromise
Accidental action
- backend api breach
- software failure leading to sending other orders than users signed
- sending the same order multiple times due to local database failure

Tools Used

Manual Review

Recommended Mitigation Steps

Introduce

Assessed type

Invalid Validation

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

Replayable order signature #18