Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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 opened this issue Jun 4, 2024 · 12 comments
Labels
Escalation Resolved This issue's escalations have been approved/rejected Excluded Excluded by the judge without consulting the protocol or the senior Non-Reward This issue will not receive a payout

Comments

@sherlock-admin3
Copy link
Contributor

sherlock-admin3 commented Jun 4, 2024

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.

@github-actions github-actions bot closed this as completed Jun 6, 2024
@github-actions github-actions bot added the Excluded Excluded by the judge without consulting the protocol or the senior label Jun 6, 2024
@sherlock-admin2 sherlock-admin2 changed the title Deep Green Ant - A malicious rebalancing process can significantly alter the ratio between the amounts of token0 and token1 held in the pool. whitehair0330 - A malicious rebalancing process can significantly alter the ratio between the amounts of token0 and token1 held in the pool. Jun 12, 2024
@sherlock-admin2 sherlock-admin2 added the Non-Reward This issue will not receive a payout label Jun 12, 2024
@github-actions github-actions bot changed the title whitehair0330 - A malicious rebalancing process can significantly alter the ratio between the amounts of token0 and token1 held in the pool. Deep Green Ant - A malicious rebalancing process can significantly alter the ratio between the amounts of token0 and token1 held in the pool. Jun 13, 2024
@sherlock-admin sherlock-admin changed the title Deep Green Ant - A malicious rebalancing process can significantly alter the ratio between the amounts of token0 and token1 held in the pool. whitehair0330 - A malicious rebalancing process can significantly alter the ratio between the amounts of token0 and token1 held in the pool. Jun 13, 2024
@whitehair0330
Copy link

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
Copy link
Contributor Author

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.

@sherlock-admin4 sherlock-admin4 added the Escalated This issue contains a pending escalation label Jun 14, 2024
@0xjuaan
Copy link

0xjuaan commented Jun 14, 2024

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

@whitehair0330
Copy link

@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
Copy link

0xjuaan commented Jun 15, 2024

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
Copy link

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
Copy link

0xjuaan commented Jun 15, 2024

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
Copy link

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
Copy link

0xjuaan commented Jun 17, 2024

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
Copy link
Contributor

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
Copy link
Contributor

Result:
Invalid
Unique

@sherlock-admin2 sherlock-admin2 removed the Escalated This issue contains a pending escalation label Jun 22, 2024
@sherlock-admin3 sherlock-admin3 added the Escalation Resolved This issue's escalations have been approved/rejected label Jun 22, 2024
@sherlock-admin4
Copy link

Escalations have been resolved successfully!

Escalation status:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Escalation Resolved This issue's escalations have been approved/rejected Excluded Excluded by the judge without consulting the protocol or the senior Non-Reward This issue will not receive a payout
Projects
None yet
Development

No branches or pull requests

6 participants