iamandreiski - Deposits/Withdrawals to/from Arrakis Vaults can be sandwiched due to no slippage checks in mint/burn functions

[sherlock-admin3]

iamandreiski

medium

Deposits/Withdrawals to/from Arrakis Vaults can be sandwiched due to no slippage checks in mint/burn functions

Summary

There are two instances in which deposits/withdrawals can be sandwiched:

• When depositing funds through the Router, there is slippage protection, but when directly minting through the vault, no slippage protection is present, making all of the deposits prone to being sandwiched.
• Same goes for withdrawing directly by burning the shares on the vault instead of going through the router's removeLiquidity().
• The above, coupled with the fact that depositing and withdrawing in the same block is possible, makes these user paths prone to sandwich attacks.

Vulnerability Detail

There are two paths that a user can take when depositing or withdrawing liquidity from the vault. The first one is going through the ArrakisPublicVaultRouter, and the second one is directly through the vault's mint and burn functions.

For the purpose of this example, we're going to examine withdrawals, but the same flow is present for deposits as well.

When withdrawing / removing liquidity via the router, removeLiquidity() subsequently calls _removeLIquidity() in which the slippage control logic is located:

function _removeLiquidity(RemoveLiquidityData memory params_)
        internal
        returns (uint256 amount0, uint256 amount1)
    {
        (amount0, amount1) = IArrakisMetaVaultPublic(params_.vault)
            .burn(params_.burnAmount, params_.receiver);

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

The problem is, that a user can call burn() directly on the public vault, without having to go through the router:

function burn(
        uint256 shares_,
        address receiver_
    ) external returns (uint256 amount0, uint256 amount1) {
        if (shares_ == 0) revert BurnZero();
        uint256 supply = totalSupply();
        if (shares_ > supply) revert BurnOverflow();

        uint256 proportion = FullMath.mulDiv(shares_, BASE, supply);

        if (receiver_ == address(0)) revert AddressZero("Receiver");

        _burn(msg.sender, shares_);

        (amount0, amount1) = _withdraw(receiver_, proportion);

        emit LogBurn(shares_, receiver_, amount0, amount1);
    }

The function above will calculate the proportion based on the shares that we want to burn and the total supply. The other problem is that the amount of the two tokens that we will receive is based on the supply of the pools:

 function withdraw(
        address receiver_,
        uint256 proportion_
    )
        public
        virtual
        onlyMetaVault
        nonReentrant
        returns (uint256 amount0, uint256 amount1)
    {

        if (receiver_ == address(0)) revert AddressZero();
        if (proportion_ == 0) revert ProportionZero();
        if (proportion_ > BASE) revert ProportionGtBASE();

        {
            (uint256 _amt0, uint256 _amt1) = pool.getReserves();

            amount0 = FullMath.mulDiv(proportion_, _amt0, BASE);
            amount1 = FullMath.mulDiv(proportion_, _amt1, BASE);
        }

        if (amount0 == 0 && amount1 == 0) revert AmountsZeros();

        alm.withdrawLiquidity(amount0, amount1, receiver_, 0, 0);

        emit LogWithdraw(receiver_, proportion_, amount0, amount1);
    }

Since the amounts are based on the pool reserves, as well as the total supply of the share token, this can be easily manipulated and/or sandwiched in order to maliciously profit on the user's unprotected transactions.

Considering that deposits and withdrawals can be executed in the same block, plus flashloans can be utilized to aid in the sandwich attack.

Impact

No slippage control allows for malicious bots/users to sandwich users who directly mint/burn tokens without going through the router.

Code Snippet

https://github.com/sherlock-audit/2024-03-arrakis/blob/64a7dc6ccb5de2824870474a9f35fd3386669e89/arrakis-modular/src/ArrakisMetaVaultPublic.sol#L51-L74 https://github.com/sherlock-audit/2024-03-arrakis/blob/64a7dc6ccb5de2824870474a9f35fd3386669e89/arrakis-modular/src/ArrakisMetaVaultPublic.sol#L81-L98

Tool used

Manual Review

Recommendation

Make the mint/burn functions only callable from the router OR include slippage control arguments and checks in them as well.

[sherlock-admin3]

escalate This issue is not a valid. There is no specific proof of concept (POC) provided to verify its severity. In practice, both deposits and withdrawals are percentage-based. During deposits, if a sandwich attack causes an increase in the amount of token1, the percentage-based calculation will require more token1 to be provided, potentially leading to transaction failure. Similarly, during withdrawals, users can exploit this to receive more token1.

Please provide an effective coding POC and additional comments to supplement detailed attack paths.

You've deleted an escalation for this issue.

[0xjuaan]

Agreeing with escalation. No attack path or specific impact provided.

[iamandreiski]

escalate This issue is not a valid. There is no specific proof of concept (POC) provided to verify its severity. In practice, both deposits and withdrawals are percentage-based. During deposits, if a sandwich attack causes an increase in the amount of token1, the percentage-based calculation will require more token1 to be provided, potentially leading to transaction failure. Similarly, during withdrawals, users can exploit this to receive more token1.

Please provide an effective coding POC and additional comments to supplement detailed attack paths.

Hi, thank you for your comments, if this was invalid, then no slippage checks would be needed when depositing/withdrawing through the router as well. But that's not the case, and they're utilized.

The protocol's safeguarding mechanisms to always make sure that a certain ratio is kept in the vault is why they're utilizing minimum and maximum amounts when depositing through the router, if the vault has been rebalanced and ratios changed, the user wouldn't be damaged. That's why they're also always using the smaller proportion of the both when adding liquidity:

 uint256 proportion0 = amount0 == 0
            ? type(uint256).max
            : FullMath.mulDiv(maxAmount0_, BASE, amount0);
        uint256 proportion1 = amount1 == 0
            ? type(uint256).max
            : FullMath.mulDiv(maxAmount1_, BASE, amount1);

        uint256 proportion =
            proportion0 < proportion1 ? proportion0 : proportion1;

As well as:

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

None of those are utilized in here, meaning that users will have no actual control over the ratios of the token that they get. Or the ones that they have to "input" into the system. Blindly assuming that "will require more token1 to be provided, potentially leading to transaction failure" and the user doesn't have it OR hasn't approved the token to be used for a larger-than-needed sum isn't a good security practice.

• Total vault shares minted until this point: 25000e18
• When reserves are fetched, there are 25e18(amt0) token A present in the pool, and 1000e18(amt1) token B.
• User A wants to burn 2500e18 shares.
• The proportion will be calculated as this: (2500e18 * 1e18) / 25000e18 = 1e17

Following the calculation of the proportion here is how the amount0 and amount1 would be calculated:

{
            (uint256 _amt0, uint256 _amt1) = pool.getReserves();

            amount0 = FullMath.mulDiv(proportion_, _amt0, BASE);
            amount1 = FullMath.mulDiv(proportion_, _amt1, BASE);
        }

• amount0 = (1e17 * 25e18) / 1e18 = 2.5e18
• amount1 = (1e17 * 1000e18) / 1e18 = 100e18

Now the following scenario takes place, before the user's deposit is executed a larger deposit was made and the pool rebalanced with the pool's new state being:

• Total Supply: 32000e18
• TokenA: 20e18, TokenB: 1600e18

(2500e18 * 1e18) / 32000e18 = 7.8125e16

• amount0 = (7.8125e16 * 20e18) / 1e18 = 1.5625e18
• amount1 = (7.8125e16 * 1600e18) / 1e18 =125e18

2.5e18 vs 100e18 or 1.5625e18 vs 125e18 might not reflect the optimal market ratio and the user might loose a significant portion of their initial investment.

Assumptions made that the even though the ratio remains the same, but the user gets different amount of the tokens take in consideration that the the Token X * Token Y will always have the optimal ratio and can't be skewed, which isn't the case.

It's a basic slippage protection mechanism which is utilized on all major pool-based platforms so that if skewed pool ratios do occur which do not reflect the optimal market ratio between those two tokens, the user's withdrawal of individual tokens (or deposits) aren't maliciously or not even maliciously but just a bad series of events happened - damaged.

[0xjuaan]

Escalate

The watson has made a valid comment here, that the ratio of received tokens can be manipulated via a rebalance. However this is not mentioned in the report at all which vaguely states 'sandwich attack'.

The report also mentions that flash loans can be used to aid in the sandwich, which is false since flash loans have to be repaid in the same transaction.

Also, the report states that the attack can occur on minting shares, but this is not the case, since no more tokens than what they approved to the module can be taken from them. The rebalance attack can only affect burning shares.

Overall, the report states that "there is no slippage protection and there can be a sandwich attack", without explaining the fact that this can only have an impact when burning shares, and that a malicious executor must rebalance the pool ratio in order for the attack to work.

[sherlock-admin3]

Escalate

The watson has made a valid comment here, that the ratio of received tokens can be manipulated via a rebalance. However this is not mentioned in the report at all which vaguely states 'sandwich attack'.

The report also mentions that flash loans can be used to aid in the sandwich, which is false since flash loans have to be repaid in the same transaction.

Also, the report states that the attack can occur on minting shares, but this is not the case, since no more tokens than what they approved to the module can be taken from them. The rebalance attack can only affect burning shares.

Overall, the report states that "there is no slippage protection and there can be a sandwich attack", without explaining the fact that this can only have an impact when burning shares, and that a malicious executor must rebalance the pool ratio in order for the attack to work.

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.

[WangSecurity]

The original report indeed lacks enough information how this slippage issue can arise. There not enough proof that it can actually be manipulated, since no slippage checks and using reserves & balances to get the conversion rate doesn't necessarily mean it can be used exploited to execute a slippage related issue. Moreover, the original report has factual mistakes, which are expressed in the escalation comment.

The comment made by @iamandreiski after the first escalation is indeed valid, but here we have to judge and reward the original report. Hence, planning to accept the escalation and invalidate the report.

[Gevarist]

This is wanted, we are following uniswap V3 style by putting slippage check on router. This is not a valid finding.

[WangSecurity]

Result: Invalid Unique

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• 0xjuaan: accepted