sherlock-audit / 2024-03-arrakis-judging

0 stars 0 forks source link

whitehair0330 - A malicious rebalancing process can `significantly` alter the ratio between the amounts of `token0` and `token1` held in the pool. #59

Closed sherlock-admin3 closed 1 month ago

sherlock-admin3 commented 1 month ago

whitehair0330

high

A malicious rebalancing process can significantly alter the ratio between the amounts of token0 and token1 held in the pool.

Summary

Malicious vault rebalance executors can substantially manipulate the actual market price of the pool's assets through the rebalancing process.

Vulnerability Detail

The executor of a public vault can call any function of the ValantisHOTModule contract through the rebalancing process. Consider a scenario where the executor calls the ValantisHOTModule.swap() function during rebalancing. The ValantisHOTModule.swap() function has three steps: withdrawing all assets from the pool, swapping the tokens, and depositing the assets back into the pool.

    function swap(
        bool zeroForOne_,
        uint256 expectedMinReturn_,
        uint256 amountIn_,
        address router_,
        uint160 expectedSqrtSpotPriceUpperX96_,
        uint160 expectedSqrtSpotPriceLowerX96_,
        bytes calldata payload_
    ) external onlyManager whenNotPaused {
        [...]

363         alm.withdrawLiquidity(_amt0, _amt1, address(this), 0, 0);

        [...]

376         (bool success,) = router_.call(payload_);

        [...]

406     alm.depositLiquidity(
            balance0,
            balance1,
            expectedSqrtSpotPriceLowerX96_,
            expectedSqrtSpotPriceUpperX96_
        );

        [...]
    }

During the rebalancing, there are two checks in place: the maxDeviation check for the price, and the maxSlippagePIPS check for the total underlying of the vault. However, within the ValantisHOTModule.swap() function, there will be no change in the pool's price, as the withdrawing and depositing operations do not modify the _ammState. Additionally, the maxSlippagePIPS check for the total underlying value will also pass, as all swapped tokens are also deposited back into the pool.

As a result, a malicious executor can execute arbitrary swaps, leading to a significant alteration of the ratio between the amounts of token0 and token1 held in the pool. This imbalance in the pool's token composition effectively changes the exchange rate of the pool's assets. This exchange rate manipulation could ultimately result in a loss of funds for pool participants.

Impact

Malicious executors can substantially manipulate the pool's exchange rate.

Code Snippet

https://github.com/sherlock-audit/2024-03-arrakis/blob/main/arrakis-modular/src/ArrakisStandardManager.sol#L322-L421

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

Tool used

Manual Review

Recommendation

There should be a check to ensure the ratio between the amounts of token0 and token1 held in the pool remains within an acceptable range, in the ValantisHOTModule.swap() function.

whitehair0330 commented 1 month ago

Escalate

This issue should be a valid one. This attack is not based on a malicious router contract, and it significantly alters the ratio between pool reserves.

sherlock-admin3 commented 1 month ago

Escalate

This issue should be a valid one. This attack is not based on a malicious router contract, and it significantly alters the ratio between pool reserves.

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.

0xjuaan commented 1 month ago

How are the funds lost? This is not explained in the report

whitehair0330 commented 1 month ago

@0xjuaan SovereignPool is a swapper. When the ratio between the amounts of token0 and token1 changes significantly, the attacker can then profit by swapping. This is a well-known issue where manipulation of the exchange rate leads to the loss of funds for the Liquidity Providers.

0xjuaan commented 1 month ago

I believe the exchange rate is stored in the _ammState storage slot, and is not affected by the rebalance. Hence the exchange rate used during any swaps after the rebalance is the same. It's just that the available liquidity for the swaps will be lower depending on the reserve ratio.

whitehair0330 commented 1 month ago

When the liquidity providers withdraw their assets, they will receive tokens in proportion to the new ratio between the pool reserves. As a result, some liquidity providers will realize profits, while others may incur losses.

0xjuaan commented 1 month ago

Lets say the pool currently contains 1 ETH and 2000 USDC (Assume each ETH costs 2000 USDC)

Then a rebalance occurs, swapping half of the ETH into USDC.

So now the reserves are 0.5 ETH and 3000 USDC

While the reserve ratio is different, the TVL is the same. So when LPs withdraw, they still receive the same value of tokens, just different amounts of each token. So no loss.

whitehair0330 commented 1 month ago

If the attacker manages to make one of the pool's reserves almost zero, then the pool will lose its swapping ability, which is the core functionality. This is sufficient to demonstrate that this issue can be considered a medium-severity one.

0xjuaan commented 1 month ago

Such a DoS won't last more than 7 days (since the public vault owner can change the executor, and then perform an appropriate rebalance).

Sherlock rules state the the DoS must last more than 7 days to be considered medium, so I believe it is invalid.

Furthermore, such a DoS was not mentioned in the report at all.

WangSecurity commented 1 month ago

Based on the discussion above, I believe the following is true:

LPs won't receive a loss cause TVL is the same. The DOS (caused by swapping almost all token0 into token1 and vice versa) won't last more than 7 days cause the executor can be changed and the number of tokens returned to normal.

Hence, I believe it should remain low. Planning to reject the escalation and leave the issue as it is.

WangSecurity commented 1 month ago

Result: Invalid Unique

sherlock-admin4 commented 1 month ago

Escalations have been resolved successfully!

Escalation status: