Hardcoded `tokenPoolFee` for all tokens is a downside to protocol as it could cause a DOS for attempts to swap `Zeta` to/fro `token`

[c4-bot-2]

Lines of code

https://github.com/code-423n4/2023-11-zetachain/blob/b237708ed5e86f12c4bddabddfd42f001e81941a/repos/protocol-contracts/contracts/evm/tools/ZetaTokenConsumerUniV3.strategy.sol#L32 https://github.com/code-423n4/2023-11-zetachain/blob/b237708ed5e86f12c4bddabddfd42f001e81941a/repos/protocol-contracts/contracts/evm/tools/ZetaTokenConsumerUniV3.strategy.sol#L158-L182

Vulnerability details

Proof of Concept

From ZetaTokenConsumerUniV3.strategy.sol#L32

 uint24 public immutable tokenPoolFee;

Evidently the protocol hardcodes a fee for all token pools

Now take a look at ZetaTokenConsumerUniV3.strategy.sol#L158-L182 as an example case to prove bug

    function getZetaFromToken(
        address destinationAddress,
        uint256 minAmountOut,
        address inputToken,
        uint256 inputTokenAmount
    ) external override returns (uint256) {
        if (destinationAddress == address(0) || inputToken == address(0)) revert ZetaCommonErrors.InvalidAddress();
        if (inputTokenAmount == 0) revert InputCantBeZero();

        IERC20(inputToken).safeTransferFrom(msg.sender, address(this), inputTokenAmount);
        IERC20(inputToken).safeApprove(address(pancakeV3Router), inputTokenAmount);

        ISwapRouterPancake.ExactInputParams memory params = ISwapRouterPancake.ExactInputParams({
            //@audit "tokenPoolFee"
            path: abi.encodePacked(inputToken, tokenPoolFee, WETH9Address, zetaPoolFee, zetaToken),
            recipient: destinationAddress,
            amountIn: inputTokenAmount,
            amountOutMinimum: minAmountOut
        });

        uint256 amountOut = pancakeV3Router.exactInput(params);

        emit TokenExchangedForZeta(inputToken, inputTokenAmount, amountOut);
        return amountOut;
    }

        function getTokenFromZeta(
        address destinationAddress,
        uint256 minAmountOut,
        address outputToken,
        uint256 zetaTokenAmount
    ) external override nonReentrant returns (uint256) {
        if (destinationAddress == address(0) || outputToken == address(0)) revert ZetaCommonErrors.InvalidAddress();
        if (zetaTokenAmount == 0) revert InputCantBeZero();

        IERC20(zetaToken).safeTransferFrom(msg.sender, address(this), zetaTokenAmount);
        IERC20(zetaToken).safeApprove(address(pancakeV3Router), zetaTokenAmount);

        ISwapRouterPancake.ExactInputParams memory params = ISwapRouterPancake.ExactInputParams({
            //@audit
            path: abi.encodePacked(zetaToken, zetaPoolFee, WETH9Address, tokenPoolFee, outputToken),
            recipient: destinationAddress,
            amountIn: zetaTokenAmount,
            amountOutMinimum: minAmountOut
        });

        uint256 amountOut = pancakeV3Router.exactInput(params);

        emit ZetaExchangedForToken(outputToken, zetaTokenAmount, amountOut);
        return amountOut;
    }

The above code snippet or something very similar is present in multiple strategies across the evm contracts and is the implementation used while attempting to swap tokens to and fro Zeta, one thing that would be key to note is that all tokens do not have pools with "the same fee" or to put in a more general term all tokens do not have their best liquidity attached to a specific fee'd pool(for e.g 0.05%) and in some cases some tokens might not even have any liquidity at all with the pool with this specific fee.

Now if fee is set to an "unpopular value", most attempts to swap wouldn't even go through, but we'd assume admins acting rightly would make extensive research to have fee being set as a fee that's readily available for a wide range of tokens, which still opens up bug cases like:

• For any token that does not have a pool with this fee there is a complete DOS that would occur while attempting to swap, note that the liquidity from a pool might diminish which could cause a pool that was active before to now be dormant, effectively leaving users to be stuck with their tokens.
• All tokens that have pools with better liquidity (on different fee range) wouldn't be used, cause the fee has been hardcoded not to be it, though this seem to be subtly fixed by the suggested slippage, but it's still worth to note.

Impact

The above essentially leads to a case where admins would have to chose a fee that's accepted by multiple tokens, or "more popular", note that this is not done before accepting any token to protocol, which causes the tokens that don't have a pool with such fee or enough liquidity in the pool being completely DOS'd from swaps to and fro Zeta, also with the bull market kicking in and the crypto market being more unstable in general this could also lead to user loss in US$ value of their tokens as they would like to do away with one for the other but they can't due to the hardcoded poolfee value, causing them losses if the token is flash dropping in price.

TLDR: This leads to the inability to process swaps due to unavailable path or even not enough liquidity on a specific path due to hardcoding one pool type fee wise for all tokens, the chances of this happening are also high banking on the fact that protocol is going to be deployed widely across different EVM chains where different fee pool type might be more popular / more inconsistencies would be present

Tool Used

Manual review.

Recommended Mitigation Steps

Do not hardcode the fee for all token pools, instead an implementation should be made to allow admin to set the specific tokenpoolFee for each pair, to make this more robust one could leave the current implementation as somewhat default and then the fee could get updated via a new setter function via which admin would update details if the default pool is not available or if it's not the best bet, alternatively allow users to provide what pool they would like to use while attempting to swap a good suggestion might be made to them on the UI/UX but in the end they should decide what pool to use.

Assessed type

Context

[c4-pre-sort]

DadeKuma marked the issue as primary issue

[DadeKuma]

Intended Design / QA at best

[c4-pre-sort]

DadeKuma marked the issue as insufficient quality report

[c4-judge]

0xean changed the severity to QA (Quality Assurance)

[c4-judge]

0xean marked the issue as grade-b

[Bauchibred]

Hi @0xean I think the conclusion to this report is inaccurate, this obviously causes DOS as is hinted in the report, note that when depositing no query is done to ensure that the token has an optimal path with that particular fee and even in the case where it does, the liquidity could diminish from said pool and as such all swaps would fail, which leads to me not understanding how this is “intended design” to the presort too, cause protocol definitely does not intend for their swaps not to go through, also shouldn’t the sponsors be tagged to give their opinion on whether this is intended or not?

[DadeKuma]

It's a good QA suggestion to have a different fee for each pool.

However, the alleged DoS relies on an admin choosing an incompatible sequence of pools/fees, thus, this report was marked as insufficient quality.