juaan - `setPriceBounds` is not timelocked, so malicious executor can steal funds

[sherlock-admin3]

juaan

high

setPriceBounds is not timelocked, so malicious executor can steal funds

Summary

The Valantis docs state the following:

Liquidity Providers should implement a Timelock before calling the setPriceBounds function in the HOT. Without proper Timelocks, malicious price-bound attacks could become possible.

A malicious executor can atomically set the price bounds of the HOT, allowing them to profit via an atomic sandwich attack.

Vulnerability Detail

The ArrakisStandardManager's rebalance() function allows the executor role to call arbitrary payloads_ on the ValantisHOTModule linked to a specific meta-vault.

The executor is a restricted role, so is not trusted.

The executor can use the ArrakisStandardManager's rebalance() function to call setPriceBounds() on the module, which changes the price bounds of the HOT contract. This occurs without any timelock, while the docs state that a timelock should be used to prevent attacks.

Attack Steps:

Since the price bounds can be changed atomically, by a malicious executor, they can perform the following attack:

1. Manipulate the spot price of the pool by swapping a significant amount in 1 direction.
2. Set the price bounds very tightly around this manipulated spot price.
3. Swap back in the other direction.

Using this sequence of events atomically, the liquidity provider can buy the pool's tokens at a significant discount.

Impact

Loss of funds for LPs, stolen funds by malicious executor

Code Snippet

https://github.com/sherlock-audit/2024-03-arrakis/blob/64a7dc6ccb5de2824870474a9f35fd3386669e89/arrakis-modular/src/abstracts/ValantisHOTModule.sol#L303-L315

Tool used

Manual Review

Recommendation

Consider timelocking the access to setPriceBounds() from the executor, since they are a restricted role.

Note that the malicious executor could still sandwich the timelock execution to carry out the same exploit, so further protection is likely needed.

[0xjuaan]

Escalate

This submission clearly demonstrates that a requirement stated in the valantis docs is not met by the arrakis contracts which integrate with valantis pools.

Liquidity Providers should implement a Timelock before calling the setPriceBounds function in the HOT. Without proper Timelocks, malicious price-bound attacks could become possible.

As a result, a malicious executor can steal funds via the attack described (taken from the docs)

Hence, this should be a valid high

[sherlock-admin3]

Escalate

This submission clearly demonstrates that a requirement stated in the valantis docs is not met by the arrakis contracts which integrate with valantis pools.

Liquidity Providers should implement a Timelock before calling the setPriceBounds function in the HOT. Without proper Timelocks, malicious price-bound attacks could become possible.

As a result, a malicious executor can steal funds via the attack described (taken from the docs)

Hence, this should be a valid high

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[cu5t0mPeo]

this is a known issue

[0xjuaan]

@cu5t0mPeo where is it specified as a known issue?

[cu5t0mPeo]

Sorry, I was mistaken; it is indeed inconsistent with the documentation.

[WangSecurity]

Agree with the escalation, planning to accept it and validate with high severity, are there any duplicates of it?

[0xjuaan]

no dupes that I could find

[ADK0010]

@WangSecurity I think this issue is invalid . The report states :

Since the price bounds can be changed atomically, by a malicious executor, they can perform the following attack:

1) Manipulate the spot price of the pool by swapping a significant amount in 1 direction.
2) Set the price bounds very tightly around this manipulated spot price.
3) Swap back in the other direction.

After "Manipulating the spot price of the pool" as mentioned in report , when executor calls set price bounds it follows the path - ArrakisStandardManager.sol#rebalance() -> ValantisHOTModule.sol#setPriceBounds -> HOT.sol#setPriceBounds where the spot price is checked with max deviation possible at "checkPriceDeviation" -

  if (address(feedToken0) != address(0)) {
            // Feeds have been set, oracle deviation should be checked.
            // If feeds are not set, then HOT is in AMM-only mode, and oracle deviation check is not required.
            if (
                !HOTParams.checkPriceDeviation(
                    sqrtSpotPriceX96Cache,
                    getSqrtOraclePriceX96(),
                    hotReadSlotCache.maxOracleDeviationBipsLower,
                    hotReadSlotCache.maxOracleDeviationBipsUpper
                )
            ) {
                revert HOT__setPriceBounds_spotPriceAndOracleDeviation();
            }
        }

causing revert in Manipulated case where "a swapping significant amount in 1 direction" is done as mentioned in the report . Thereby making the attack invalid .
@0xjuaan do look into this.

[0xjuaan]

@ADK0010 is correct, there is a check for the deviation between spot price and oracle price.

However I believe the issue is valid for the following reasons:

1. The attack can still occur, the price manipulation just has to be less than the max deviation that was set.
2. Furthermore, the attack is atomic so can be repeated a large number of times
3. The oracle checks are not mandatory, they only occur in some cases (when the HOT is not in AMM-only mode)
4. The code clearly deviates from the specifications provided by Valantis docs, which are required to be implemented in Arrakis contracts (the stated requirement is to timelock setPriceBounds to prevent manipulation).

Will respect the judge's decision based on the info provided

[ADK0010]

Thanks @0xjuaan for the comments . Sorry But I forgot to mention one point which was after swap & manipulation (as mentioned in the report) , there is 2 more checks in ArrakisStandardManager.sol#rebalance() - 1) https://github.com/sherlock-audit/2024-03-arrakis/blob/64a7dc6ccb5de2824870474a9f35fd3386669e89/arrakis-modular/src/ArrakisStandardManager.sol#L383C1-L386C66

        // check if the underlying protocol price has not been
        // manipulated during rebalance.
        // that can indicate a sandwich attack.
        module.validateRebalance(info.oracle, info.maxDeviation);

Here unlike in HOT.sol the info.maxDeviation is set by Vault Owner 2) https://github.com/sherlock-audit/2024-03-arrakis/blob/64a7dc6ccb5de2824870474a9f35fd3386669e89/arrakis-modular/src/ArrakisStandardManager.sol#L391C1-L413C14

        {
            uint256 vaultInToken1AfterRebalance = FullMath.mulDiv(
                amount0, price0, 10 ** token0Decimals
            ) + amount1;

            uint256 currentSlippage = vaultInToken1BeforeRebalance
                > vaultInToken1AfterRebalance
                ? FullMath.mulDiv(
                    vaultInToken1BeforeRebalance
                        - vaultInToken1AfterRebalance,
                    PIPS,
                    vaultInToken1BeforeRebalance
                )
                : FullMath.mulDiv(
                    vaultInToken1AfterRebalance
                        - vaultInToken1BeforeRebalance,
                    PIPS,
                    vaultInToken1BeforeRebalance
                );

            if (currentSlippage > info.maxSlippagePIPS) {
                revert OverMaxSlippage();
            }

here slippage is checked by converting the entire underlying token amounts used into token1 , and comparing it with slippage . So if the attack is done more number of times in a single loop as mentioned above by staying inside maxDeviation of HOT.sol & somehow drains more funds , even then the slippage factors set here would cause it to revert . For the points @0xjuaan mentioned above : 1) Considering the attack involves swap & setting price bounds state(which uses storage opcodes) , the gas fees can be significantly high to conduct the attack multiple times in a single loop of rebalance() thereby reducing profits for malicious executor . 2) My views mentioned above on it 3) But they are done anyway after every rebalance external call as mentioned above in validateRebalance() 4) this might be true but can it be considered a valid H/M , I don't know I leave it to the judges .

[0xjuaan]

I think that since the fund loss is constrained (has to be less than maxSlippagePIPS- which can potentially be even 10% of TVL), and the arrakis code does not follow the requirement stated in the Valantis docs, this should be a medium severity issue.

[WangSecurity]

To clarify, @0xjuaan @ADK0010 the executor in that case steals the funds, correct?

[WangSecurity]

And another question, rebalance function inside ASM has cooldown period between executions, correct?

[0xjuaan]

To clarify, @0xjuaan @ADK0010 the executor in that case steals the funds, correct?

Yes, executor steals funds since the average price for their buy is lower than the average price for their sell.

And another question, rebalance function inside ASM has cooldown period between executions, correct?

Yes

[WangSecurity]

Since the loss is limited to maxSlippagePIPS (set by a TRUSTED admin) and after the attack, the Owner can change the executor and return price bounds to the appropriate state, I believe this is the intended behaviour and the slippage check here is indeed to prevent such attack from stealing too many funds.

Planning to reject the escalation and leave the issue as it is.

[WangSecurity]

Result: Invalid Unique

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• 0xjuaan: rejected