No setter function available for ValantisHOTModule.sol.maxSlippage()

Summary

There is no setter function available in ValantisHOTModule.sol or in the contracts that inherit it, to set the maxSlippage at times of varying volatility .

Vulnerability Detail

As we can see the protocol has used maxSlippage in the deploy script as : https://github.com/sherlock-audit/2024-03-arrakis/blob/main/arrakis-modular/script/valantisVaultOne.s.sol#L26C1-L26C41

uint24 constant maxSlippage = PIPS / 50;

which is 2%. Let's consider the case where maxSlippage is set to 2% . It will cause revert at high volatility times during rebalance . The simple contract logic flow for rebalance == ArrakisStandardManager#rebalance() -> ValantisHOTModule#swap() .

Here let's say , the executor is using Uniswap v3 swap router & the trades in Uniswap V3 has a general slippage of ~2-3% at all times , so the executor sets the slippage to about ~3% . Then the executor set's the expectedMinReturn_ parameter of ValantisHOTModule#swap() accordingly & calls the ArrakisStandardManager#rebalance() function . But the ValantisHOTModule#swap() has a check _checkMinReturn in it .

ValantisHOTModule#_checkMinReturn checks compares expectedMinReturn_ to the maxSlippage allowed by the protocol as shown below :

https://github.com/sherlock-audit/2024-03-arrakis/blob/main/arrakis-modular/src/abstracts/ValantisHOTModule.sol#L535C3-L563C6

  // #region internal functions.

    function _checkMinReturn(
        bool zeroForOne_,
        uint256 expectedMinReturn_,
        uint256 amountIn_,
        uint8 decimals0_,
        uint8 decimals1_
    ) internal view {
        if (zeroForOne_) {
            if (
                FullMath.mulDiv(
                    expectedMinReturn_, 10 ** decimals0_, amountIn_
                )
                    < FullMath.mulDiv(
                        oracle.getPrice0(), PIPS - maxSlippage, PIPS
                    )
            ) revert ExpectedMinReturnTooLow();
        } else {
            if (
                FullMath.mulDiv(
                    expectedMinReturn_, 10 ** decimals1_, amountIn_
                )
                    < FullMath.mulDiv(
                        oracle.getPrice1(), PIPS - maxSlippage, PIPS
                    )
            ) revert ExpectedMinReturnTooLow();
        }
    }

and will cause revert as we know the expectedMinReturn_ was set with a slippage of ~3% .

Impact

Revertion at times of High Volatility, due to hardcoded maxSlippage values. There is no setter function to update the maxSlippage variable during high volatility times unless an upgrade is made . This method of using Hard Coded maxSlippage can cause revert at unexpected scenarios .

Code Snippet

https://github.com/sherlock-audit/2024-03-arrakis/blob/main/arrakis-modular/src/abstracts/ValantisHOTModule.sol#L535C3-L563C6

Tool used

Manual Review

Recommendation

Add a setter function to update the maxSlippage variable .

sherlock-admin3 commented 5 months ago

Escalate This is Valid . Maybe the judge invalidated it , thinking it is a no setter function case . But there is more to it . It is a classic Hardcoded Slippage case , read more about it here. It would cause unexpected reverts .

You've deleted an escalation for this issue.

0xjuaan commented 5 months ago

@ADK0010, you have provided a link to an issue regarding 'hardcoded fee tier' but I don't see how that relates to this submission at all

ADK0010 commented 5 months ago

@0xjuaan Sorry , I used the wrong link . Check now I have updated it .

0xjuaan commented 5 months ago

I would say that this is low severity since if the conditions change and the chosen slippage is no longer feasible, the module can simply be changed via setModule()

A new module can be created in 2 days, so there is no DoS that lasts more than 7 days

Based on the above evidence, I'm pretty sure it's an intentional design choice to keep the maxSlippage unchanged.