Angry_Mustache_Man - Loss of funds caused by edge case in Deposit & Withdraw functions of Private & Public Vaults

[sherlock-admin4]

Angry_Mustache_Man

medium

Loss of funds caused by edge case in Deposit & Withdraw functions of Private & Public Vaults

Summary

Edge case found in the deposit and withdraw functions of Private & Public vaults leads to loss of funds for the Users .

Vulnerability Detail

Arrakis Meta Vault is composed of two types of Vaults : One type is Public & the other is Private . The Public ones have slippage controls set up in Router while adding & removing Liquidity as shown below :

https://github.com/sherlock-audit/2024-03-arrakis/blob/main/arrakis-modular/src/ArrakisPublicVaultRouter.sol#L145C9-L151C10

 if (
            amount0 < params_.amount0Min
                || amount1 < params_.amount1Min
                || sharesReceived < params_.amountSharesMin
        ) {
            revert BelowMinAmounts();
        }

&&

https://github.com/sherlock-audit/2024-03-arrakis/blob/main/arrakis-modular/src/ArrakisPublicVaultRouter.sol#L1032C8-L1037C10

 if (
            amount0 < params_.amount0Min
                || amount1 < params_.amount1Min
        ) {
            revert ReceivedBelowMinimum();
        }

but the same is not implemented in deposit & withdraw of Private Vaults . Now let's see how it causes problem in those functions . Firstly let's look at deposit functionality of Private Vaults . Here the contract flow is as follows : ArrakisMetaVaultPrivate.sol#deposit -> ArrakisMetaVaultPrivate.sol#_deposit -> ValantisHOTModulePrivate.sol#fund -> HOT#depositLiquidity The HOT#depositLiquidity function has a check checkPriceDeviation. It compares the spot price with oracle price and the deviation between those is compared with a fixed max deviation allowed by HOT . Let's take an example , where price of token0/token1 in real time is X . The oracle used here is Chainlink , which has usual deviation of about ~1-3% from it's original value . Let's say the oracle price has a deviation of 2% to the upper side , i.e., price reported by Oracle is 1.02X & the spot price was manipulated by a Malicious user to cause a deviation of 5% to upper side i.e., spot price reported is 1.05X . The max deviation set by Protocol in deploy script is 5% as shown below :

https://github.com/sherlock-audit/2024-03-arrakis/blob/main/valantis-hot/scripts/deploy/02_HOTDeploy.s.sol#L83C1-L84C54

            // Set HOT Parameters
            hot.setMaxOracleDeviationBips(500); // 5%

If the max deviation is set to 5% , the given above condition will have a deviation of 3% upper side & will bypass the checkPriceDeviation condition in HOT#depositLiquidity . In this case the amount of tokens deposited by the Victim User would be recorded as 5% less than what he should have deposited .

https://github.com/sherlock-audit/2024-03-arrakis/blob/main/valantis-hot/src/libraries/HOTParams.sol#L183C5-L196C6

    function checkPriceDeviation(
        uint256 sqrtPriceAX96,
        uint256 sqrtPriceBX96,
        uint256 maxDeviationInBipsLower,
        uint256 maxDeviationInBipsUpper
    ) internal pure returns (bool) {
        uint256 diff = sqrtPriceAX96 > sqrtPriceBX96 ? sqrtPriceAX96 - sqrtPriceBX96 : sqrtPriceBX96 - sqrtPriceAX96;
        uint256 maxDeviationInBips = sqrtPriceAX96 < sqrtPriceBX96 ? maxDeviationInBipsLower : maxDeviationInBipsUpper;

        if (diff * HOTConstants.BIPS > maxDeviationInBips * sqrtPriceBX96) {
            return false;
        }
        return true;
    }

Similarly while withdrawing in Private Vaults , if the spot is Manipulated to 5% down side & the oracle price is also showing a deviation of 2% from the original price downwards , then the withdrawed amount by Victim User will be 5% less than what it should actually be withdrawing . For example - If the Victim User was trying to withdraw 1000e6 of USDC & 0.5e18 of WETH from a USDC/WETH Pool , then considering above condition he would be receiving 950e6 of USDC & 0.475e18 of WETH , causing loss of funds for the User .

And there is a 3rd case ,this time let's consider the case of Public Vaults. Let's say the Oracle price has deviation of 3% upwards & The spot price is manipulated to 5% upwards . Now when a User tries to withdraw 1000e6 USDC & 0.5e18 of WETH , he will receive 1050e6 USDC & 0.525e18 of Weth ,causing losses to protocol & stealing other User's funds in a Public Vault .

All these are caused due to the way the deviation checks are done( i.e., by taking difference between Oracle price & Spot price) & due to the fact that no upper or lower limits can be set by the User to prevent these .

Impact

-> Unintended Loss of funds for the User in Private Vaults . -> Unintended Profits given to User in Public Vaults .

Code Snippet

https://github.com/sherlock-audit/2024-03-arrakis/blob/main/arrakis-modular/src/ArrakisPublicVaultRouter.sol#L145C9-L151C10

https://github.com/sherlock-audit/2024-03-arrakis/blob/main/arrakis-modular/src/ArrakisPublicVaultRouter.sol#L1032C8-L1037C10

https://github.com/sherlock-audit/2024-03-arrakis/blob/main/valantis-hot/scripts/deploy/02_HOTDeploy.s.sol#L83C1-L84C54

https://github.com/sherlock-audit/2024-03-arrakis/blob/main/valantis-hot/src/libraries/HOTParams.sol#L183C5-L196C6

Tool used

Manual Review

Recommendation

-> Add slippage controls to deposit & withdraw functions of ArrakisMetaVaultPrivate.sol . -> Add a check in removing liquidity functions to check whether the withdrawed amount is greater than the requested amount or not .

[Gevarist]

Hi, can you explain me what is this "original value"? can we get it on chain to do slippage controls?

I think you mean by original value the average market price, if so that's not available on chain. So we take oracle price as market price/ original value. That means we cannot add more checks on slippage other than the check already done by HOT. And the slippage check on HOT will be lower than 5%, for stables pair the max slippage should be few bips.

On withdrawals you are saying the price is manipulated can you explain how the overall attack is profitable, front running tx + withdrawal + back running tx?

[ADK0010]

@Gevarist for pairs in Chainlink , the values doesn't change until either the acceptable deviation is reached or the heartbeat time has reached .

here the maximum acceptable deviation for this Arb/Usd pair is 2% , so the pair ARB/USD stays in [-2% 2%] . Also look into this report for more reference - Solodit_Report

[ADK0010]

Escalate Dup of #37

[sherlock-admin3]

Escalate Dup of #37

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]

The premise of this report is that manipulating the AMM spot price leads to users withdrawing less funds than they deserve. However the withdrawal amount is calculated as simply a proportion of the pool reserves, it does not use the AMM price.

For example - If the Victim User was trying to withdraw 1000e6 of USDC & 0.5e18 of WETH from a USDC/WETH Pool , then considering above condition he would be receiving 950e6 of USDC & 0.475e18 of WETH , causing loss of funds for the User .

This example does not seem to be true, lowering price of one token relative to the other by 5% does not mean that withdrawing will output 5% less tokens.

[WangSecurity]

About the examples, @ADK0010 could you please forward me to functions where the calculation happens, so these 5% are subtracted or added to the amount the user wants to receive? It seems that the report only shows the check, but not the actual change of the amount expressed in the report.

[ADK0010]

@WangSecurity @0xjuaan Thanks for looking into it , My understanding regarding the issue was wrong . As @0xjuaan said , withdrawal amount is calculated as simply a proportion of the pool reserves, it does not use the AMM price . No more discussions needed on this one . Issue is Invalid . Escalation should be rejected .

[WangSecurity]

Planning to reject the escalation and leave the issue as it is (if #80 is going to be valid in the end, planning to de-dup this issue).

[WangSecurity]

Result: Invalid Unique

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• ADK0010: rejected