Bauchibred - Using `slot0` to determine deviation is not ideal

[sherlock-admin]

Bauchibred

medium

Using slot0 to determine deviation is not ideal

Summary

The rebalance function in the SimpleManager contract utilizes the most recent price point slot0 to determine the current pool price and subsequently, the deviation from the oracle price. However, slot0 is the most recent data point and is therefore extremely easy to manipulate, meaning that the price deviation might be inaccurate, which could potentially lead to incorrect rebalancing and Denial of Service (DoS) due to failure of deviation checks.

require(deviation <= maxDeviation_, "maxDeviation");

Vulnerability Detail

In the SimpleManager.sol contract, the rebalance function retrieves the current pool price using slot0 that represents the most recent price point. The function _checkDeviation then calculates the deviation of this current price from the oracle price.

Given slot0's susceptibility to manipulation, the deviation calculation might be skewed. This is particularly crucial because the deviation is extensively used in the protocol to maintain balance and perform vital operations.

For example, if the deviation is larger than the maxDeviation_ parameter in _checkDeviation function, the function fails, potentially causing a DoS in the contract. This is due to the line require(deviation <= maxDeviation_, "maxDeviation"); in the _checkDeviation function.

Impact

The usage of slot0 to determine deviation could potentially allow malicious actors to manipulate the deviation calculation by altering the most recent price. As a consequence, this might lead to incorrect rebalancing operations, resulting in an inaccurate state of the contract, if the deviation check fails due to the manipulated deviation exceeding the maximum allowed deviation.

Code Snippet

rebalance() and _checkDeviation()

function rebalance(
    address vault_,
    Rebalance calldata rebalanceParams_
) external {
    ...
    IUniswapV3Pool pool = IUniswapV3Pool(
        _getPool(
            token0,
            token1,
            rebalanceParams_.mints[i].range.feeTier
        )
    );
    uint256 sqrtPriceX96;
    (sqrtPriceX96, , , , , , ) = pool.slot0();
    uint256 poolPrice = FullMath.mulDiv(
        sqrtPriceX96 * sqrtPriceX96,
        10 ** token0Decimals,
        2 ** 192
    );
    _checkDeviation(
        poolPrice,
        oraclePrice,
        vaultInfo.maxDeviation,
        token1Decimals
    );
    ...ommited for brevity
}
function _checkDeviation(
    uint256 currentPrice_,
    uint256 oraclePrice_,
    uint24 maxDeviation_,
    uint8 priceDecimals_
) internal pure {
    ...ommited for brevity
    require(deviation <= maxDeviation_, "maxDeviation");
}

Tool used

Manual Audit

Recommendation

Considering the potential risks associated with using slot0 to calculate deviation, implementing a Time-Weighted Average Price (TWAP) to determine the price is recommended. By providing a more accurate and harder to manipulate price point, TWAP would yield a more accurate deviation calculation. This would reduce the possibility of incorrect rebalancing and the risk of DoS attacks.

NB: As the Uniswap team have warned here there are issues if TWAP is going to be implemented on an L2 and these should be taken into account.

[Gevarist]

How DoS attack can be profitable to the attacker? By comparing current price of the pool against oracle price, we can be sure that the pool price is in an acceptable range. TWAP can be manipulated to go through the deviation check, and having in the same time a current pool price outside of the acceptable range.

[ctf-sec]

This report and all duplicate actually does not show the impact besides saying using slot0 can be manipulated, downgrade to medium for now

[SergeKireev]

Escalate

This should be invalid, this deviation measurement is exactly in place to prevent slot0 manipulation.

If slot0 is manipulated, the call would revert as expected.

The proposed remediation would actually be introducing a critical vulnerability because checking twap value against chainlink value would revert a lot less, even if slot0 is manipulated and expose rebalance to unbounded slippage

[sherlock-admin]

Escalate

This should be invalid, this deviation measurement is exactly in place to prevent slot0 manipulation.

If slot0 is manipulated, the call would revert as expected.

The proposed remediation would actually be introducing a critical vulnerability because checking twap value against chainlink value would revert a lot less, even if slot0 is manipulated and expose rebalance to unbounded slippage

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.

[0xbepresent]

Escalate

If the slot0 is manipulated by a malicious actor the operators can't execute their strategies as mention this report https://github.com/sherlock-audit/2023-06-arrakis-judging/issues/132 additionally this reports https://github.com/sherlock-audit/2023-06-arrakis-judging/issues/173 mentions that preventing rebalances during volatile conditions can cause vault users to lose out on swap profits.

So the malicious actor can arbitrary block operator rebalance in certain market conditions.

[sherlock-admin]

Escalate

If the slot0 is manipulated by a malicious actor the operators can't execute their strategies as mention this report https://github.com/sherlock-audit/2023-06-arrakis-judging/issues/132 additionally this reports https://github.com/sherlock-audit/2023-06-arrakis-judging/issues/173 mentions that preventing rebalances during volatile conditions can cause vault users to lose out on swap profits.

So the malicious actor can arbitrary block operator rebalance in certain market conditions.

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.

[Jeiwan]

Escalate

This is an invalid finding. The slot0 price is expected to be manipulated and this is why there's the price deviation check that compares the slot0 price with a Chainlink price. The protocol was deliberately designed to work like that and to use the slot0 price.

[sherlock-admin]

Escalate

This is an invalid finding. The slot0 price is expected to be manipulated and this is why there's the price deviation check that compares the slot0 price with a Chainlink price. The protocol was deliberately designed to work like that and to use the slot0 price.

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.

[ctf-sec]

After further check, I agree with jeiwan's escalation and sponsor's original comments

How DoS attack can be profitable to the attacker? By comparing current price of the pool against oracle price, we can be sure that the pool price is in an acceptable range. TWAP can be manipulated to go through the deviation check, and having in the same time a current pool price outside of the acceptable range.

We would consider that manipulating TWAP price is very difficult and the risk for attacker is too high to do.

So recommend closing this as a invalid issue.

[bizzyvinci]

I recommend reading the dup #144

Assuming there's a max deviation of 0.5% and chainlink price has deviated by 1% which is possible and could be within chainlink threshold. The operator could sandwich rebalance swaps by

• frontrunning to deviate uniswap sqrtPrice to the same price as chainlink (which is 1% off)
• rebalance would execute trade at a worse price and suffer from further slippage of 0.5%
• backrun to return Uniswap real sqrtPrice and profit

[SergeKireev]

I recommend reading the dup #144

Assuming there's a max deviation of 0.5% and chainlink price has deviated by 1% which is possible and could be within chainlink threshold. The operator could sandwich rebalance swaps by

* frontrunning to deviate uniswap sqrtPrice to the same price as chainlink (which is 1% off)

* rebalance would execute trade at a worse price and suffer from further slippage of 0.5%

* backrun to return Uniswap real sqrtPrice and profit

Providing liquidity is a losing operation for the LP when price in the pool is deviated from a real price. Here the design chosen by the team considers that the real price is provided by chainlink.

[bizzyvinci]

Providing liquidity is a losing operation for the LP when price in the pool is deviated from a real price.

The attack is not targeting the addition or removal of liquidity. Sandwich and slippage attacks are on swaps. This is an article about sandwich.

Here the design chosen by the team considers that the real price is provided by chainlink.

If and only if it is within the max deviation to Uniswap price. Else transaction should revert.

[SergeKireev]

The attack is not targeting the addition or removal of liquidity. Sandwich and slippage attacks are on swaps. This is an article about sandwich.

Here's one about sandwiching adding liquidity: https://eigenphi.substack.com/p/a-brand-new-sandwich-bot-that-could

in this case _checkDeviation is intended for minting (checked for all minting feeTiers), and check _checkMinReturn is intended for the swapping

frontrunning to deviate uniswap sqrtPrice to the same price as chainlink (which is 1% off)

What I'm trying to say is, how can you tell if chainlink price is off, and not uniswap price is off?

[bizzyvinci]

Here's one about sandwiching adding liquidity: https://eigenphi.substack.com/p/a-brand-new-sandwich-bot-that-could in this case _checkDeviation is intended for minting (checked for all minting feeTiers), and check _checkMinReturn is intended for the swapping

👍 Good point. I didn't know about that before and would need time to study it. I think Sherlock can decide on both points and discuss with team and judge.

What I'm trying to say is, how can you tell if chainlink price is off, and not uniswap price is off?

TWAP. And I'll recommend about 5 to 10 minute TWAP. It is long enough to make manipulation difficult but not too long for it to lag against real market price. Uniswap price can't be off for a long time because arbitrageurs are always looking to take any small profit on every block. Also, TWAP would be hard to manipulate cause we're certain there's some liquidity in the pool which was provided by the vault. However, chainlink price could deviate for a while because time or price deviation has to reach a certain threshold before they update price.

[SergeKireev]

Here's one about sandwiching adding liquidity: https://eigenphi.substack.com/p/a-brand-new-sandwich-bot-that-could in this case _checkDeviation is intended for minting (checked for all minting feeTiers), and check _checkMinReturn is intended for the swapping

+1 Good point. I didn't know about that before and would need time to study it. I think Sherlock can decide on both points and discuss with team and judge.

What I'm trying to say is, how can you tell if chainlink price is off, and not uniswap price is off?

TWAP. And I'll recommend about 5 to 10 minute TWAP. It is long enough to make manipulation difficult but not too long for it to lag against real market price. Uniswap price can't be off for a long time because arbitrageurs are always looking to take any small profit on every block. Also, TWAP would be hard to manipulate cause we're certain there's some liquidity in the pool which was provided by the vault. However, chainlink price could deviate for a while because time or price deviation has to reach a certain threshold before they update price.

If I understand you correctly, you are suggesting to compare TWAP price against chainlink price, instead of checking spot price (slot0) against chainlink price.

This solution introduces a critical vulnerability since any malicious user can imbalance the pool as far as they want in current block, which will leave chainlink price and TWAP price at the same value. If chainlink and TWAP were in range before manipulation, the check passes, and the operation is executed with disastrous slippage

If the solution suggested is to do a threeway check between (slot0, TWAP, chainlink), then it will give more guarantees but is more of a design choice. It is not related to the initial issue stating slot0 is manipulatable

[bizzyvinci]

If I understand you correctly, you are suggesting to compare TWAP price against chainlink price

Yes

any malicious user can imbalance the pool as far as they want in current block

TWAP can NOT be manipulated in current block, but slot0 can be manipulated in current block. You'll have to manipulate at least one previous block to manipulate TWAP. And TWAP can be configured to require more blocks.

[SergeKireev]

If I understand you correctly, you are suggesting to compare TWAP price against chainlink price

Yes

any malicious user can imbalance the pool as far as they want in current block

TWAP can NOT be manipulated in current block, but slot0 can be manipulated in current block. You'll have to manipulate at least one previous block to manipulate TWAP. And TWAP can be configured to require more blocks.

Please read again my previous answer, what you are suggesting is introducing a critical vulnerability

[bizzyvinci]

I guess you mean

If chainlink and TWAP were in range before manipulation, the check passes, and the operation is executed with disastrous slippage

If chainlink and TWAP are in range and slot0 is out of range. It is very likely that slot0 has been manipulated. If chainlink and slot0 are in range and TWAP is out of range. It is very likely that chainlink has deviated AND slot0 has been manipulated.

This is because you can manipulate slot0 in the same block or even transaction as the rebalance call. But it is more difficult to manipulate TWAP, because it has liquidity (at least the one provided by Arrakis) and the attacker would have to influence each block over the TWAP range.

Chainlink price could deviate by as much as 0.25% for stablecoins (which is a lot for stables) or 2% for non stable.

Uniswap price would hardly deviate over several blocks because of MEV searchers that perform arbitrage.

[bizzyvinci]

My comments are based on my own duplicate #144 which has a different angle from this one.

[bizzyvinci]

After some discussion and deliberation with @SergeKireev, I do agree with him.

[hrishibhat]

Result: Invalid Has duplicates Considering this a non-issue based on the above discussion. The code in question is working as expected.

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• SergeKireev: accepted
• Jeiwan: accepted
• 0xbepresent: rejected