search

sherlock-audit / 2024-03-arrakis-judging

2 stars 2 forks source link

kfx - Insufficient swap price validation means that solvers can their use signed quotes as free options, causing losses to the LP #69

Closed sherlock-admin4 closed 3 months ago

sherlock-admin4 commented 4 months ago

kfx

medium

Insufficient swap price validation means that solvers can their use signed quotes as free options, causing losses to the LP

Summary

Under conditions where only the first HOT swap in a block gets a discount, there's little incentive to submit the remaining signed quotes for inclusion immediately. Instead, rational solvers can be expected to delay as long as possible, and exercise their free "option" only when it's profitable to them, causing systemic losses to the LP. The losses can be quite big even for a single swap, as the price a non-discounted HOT swap is executed is not directly validated against the current price of the AMM.

Vulnerability Detail

The protocol is designed to support multiple active HOT quotes, as evidenced by the fact the "non-discounted HOT swap" concept exists. Depending on the order the quotes land on the chain, the first is expected to be performed as a discounted swap, the others as non-discounted HOT swaps. For discounted swaps, the execution price can be "better" than the fair price of the asset, by design. In return, these swaps perform AMMs price synchronization with external venues, as a service. Sponsor's comment from Discord:

we need solvers in HOT for landing flashswap on chain, flashswap has market datas to change liquidity and amm price to the market price. [..] In exchange of this service, they will get a deterministic competitive fixed price for doing a swap

Non-discounted swaps do not perform this service, and therefore should be executed at a worse price, at or close to the fair trading price.

Discounted swaps are expected to be included as soon as possible, as long as there's at least two competing solvers, because delaying them let's the other solver to be first and get the profit from the swap. However, once solver has got frontrun by another solver, they have little incentive to include their non-discounted HOT swap immediately. (It's possible to detect such a frontrun by e.g. enclosing the swap in a transaction that checks the amountOut and reverts if the discount was not applied - using private RPCs normally ensures that reverting transactions are not included in blocks.) Instead, a profitable strategy for the solver is to wait as long as possible, and only include the HOT swap if has a good price relative to the market price at the end of the waiting period.

The signed quote that the solver has essentially becomes a free option, given out by the protocol at no cost. The default expiry time for quotes can be quite long, with maxDelay expected to be set to 10 min (according to the docs) or 20 min (value in the deployment script). The asset's price can easily change by several % during these minutes. The solver can wait for expiry - epsilon seconds (where epsilon is a small number, depending on the chain's block time), and check the new asset's price on a CEX, and make a non-atomic arbitrage between the CEX and the Valantis pool.

Even if the expiry is set to a very small value by the signer, it cannot fully prevent the problem, although it does reduce it greatly! Even expiry time of zero is still theoretically arbitrageable on some L2s, including Arbitrum, that can have multiple blocks per second. On Arbitrum a block is generated whenever there are outstanding transactions, with min 250 ms interval. As result, the unfilled quote is still a free option, albeit with expiry time of less than a second. Delaying the choice whether to submit the quote is still expected to give some profit at the expense of the LP if the CEX price rapidly moves in one direction.

The signer can detect this behavior and blacklist such a solver. However, it may not easy to do so, as the solvers can pretend to have honest delays due to some setup issues, and/or perform this attack only occasionally.

Notably, the execution price of the HOT swap is not validated against the current AMM price in the validatePriceConsistency function, enabling quotes that have significantly worse price than the current AMM price to be executed, even if they are not classified as discounted.

Impact

The LP is going to suffer systemic losses if multiple solvers are permitted to get signed quotes at the same time. (If only one solver is permitted to get a signed quote at a time, the system is not going to be competitive.)

Code Snippet

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

In a HOT swap, hot.sqrtSpotPriceX96New is always used as argument for validatePriceConsistency even though it's only applied if isDiscountedHot is true:

    function _hotSwap(
        ALMLiquidityQuoteInput memory almLiquidityQuoteInput,
        bytes memory externalContext,
        ALMLiquidityQuote memory liquidityQuote
    ) internal {
        // ...
        HOTParams.validatePriceConsistency(
            _ammState,
            sqrtHotPriceX96,
            hot.sqrtSpotPriceX96New,  // @audit
            getSqrtOraclePriceX96(),
            hotReadSlot.maxOracleDeviationBipsLower,
            hotReadSlot.maxOracleDeviationBipsUpper,
            _hotMaxDiscountBipsLower,
            _hotMaxDiscountBipsUpper
        );
        // ...
        // Only update the pool state, if this is a discounted hot quote
        if (isDiscountedHot) {
            // ...
            // Update AMM sqrt spot price
            _ammState.setSqrtSpotPriceX96(hot.sqrtSpotPriceX96New);
        }
    }

PoC

Let's say that at timestamp T, the "true" price of the asset is P, and the AMM's price is 0.98*P. The signer may issue multiple quotes with that timestamp, Q1 and Q2, to two competing solvers S1 and S2:

Q1.sqrtHotPriceX96Discounted = 0.99*P
Q1.sqrtHotPriceX96Base       = 0.995*P
Q1.sqrtSpotPriceX96New       = P
Q1.nonce                     = 1
Q1.signatureTimestamp        = T
Q1.expiry                    = 60

Q2.sqrtHotPriceX96Discounted = 0.99*P
Q2.sqrtHotPriceX96Base       = 0.995*P
Q2.sqrtSpotPriceX96New       = P
Q2.nonce                     = 2
Q2.signatureTimestamp        = T
Q2.expiry                    = 60

The intention is that both quotes can be executed, so their nonces are different. Let's assume that in the block with timestamp T, a swap with the quote Q1 gets included, as a discounted swap, with execution price 0.99 P, and after-swap price of P.

S2 has the initiative to delay the execution of Q2. S2 waits for 60 - 12 seconds, and checks CEX price of the asset. Let's assume that its 1.05 P.

S2 now submits their swap with Q2 for inclusion. The prices are validated in validatePriceConsistency function.

First sqrtHotPriceX96Base is compared with sqrtSpotPriceNewX96. This obviously passes (because we know that Q1 was valid, and the price difference is smaller for Q2).

Then the AMM price sqrtSpotPriceX96 is compared with sqrtOraclePriceX96. Let's say the oracle is lagging slightly, at 1.03 P, but the maxOracleDeviationBipsUpper is set such that the check passes (e.g. to 200).

Then sqrtSpotPriceNewX96 (P) is compared with sqrtOraclePriceX96 (1.03 P). Let's say again the maxOracleDeviationBipsLower is set such that the check passes (e.g. to 300).

As a result, the swap is considered valid, even though the AMM price is almost 6% higher than the HOT execution price (1.05 P vs. 0.995 P).

Nowhere in the process is the execution price is directly compared with either the oracle price, or the current AMM price.

Tool used

Manual Review

Recommendation

0xffff11 commented 3 months ago

Sounds like an non-issue, deviation bound checks are respected. The team has already specified that parameters would be set safely.

IWildSniperI commented 3 months ago

image SIGNER is trusted and is supposed to do the right thing

quoting this from sherlock rules image

so signer shouldn't sign millions of quote for solvers and the provided scenario is protected against by mxDelay

leaving this issue at best Low/Informational for being design improvement issue

0xjuaan commented 3 months ago

Escalate

Signer is expected to set the correct expiry (based on the price volatility) to prevent such losses, so this submission should be invalidated

sherlock-admin3 commented 3 months ago

Escalate

Signer is expected to set the correct expiry (based on the price volatility) to prevent such losses, so this submission should be invalidated

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.

kfx commented 3 months ago

Hi,

There is a protocol design side of the issue (which clearly is out of scope), but the part relevant to judging is the price validation part, which can be fixed at the smart contract level.

To respond to 0xffff11:

deviation bound checks are respected

I believe it is an oversight of the team to apply the same price validation checks for non-discounted HOT swaps as for discounted HOT swaps, and in the issue I describe in detail why. In any case, the maximum allowed price difference between the AMM price and the swap's execution price clearly goes beyond the oracle deviation, being the sum of maxOracleDeviationBipsLower + maxOracleDeviationBipsUpper + hotMaxDiscountBipsLower/Upper.

The team has already specified that parameters would be set safely

1) The team has confirmed that the PoC describes a valid pair of quotes by a properly functioning signer. 2) As argued in this issue's text, it's not possible to select parameters that will be safe in all market situations. This is a distinct situation from selecting unsafe parameters due to a mistake or malicious intent.

WangSecurity commented 3 months ago

This is a distinct situation from selecting unsafe parameters due to a mistake or malicious intent.

Could you please elaborate on who's mistake or malicious intent it's going to be, the signer or solver?

kfx commented 3 months ago

Could you please elaborate on who's mistake or malicious intent it's going to be, the signer or solver?

To be clear, I meant that it's NOT going to be mistake or malicious intent on the signers part. Signing orders that are, at the time of the execution, causing losses to the LP, is unavoidable. Otherwise we're assuming a signer operated by an entity which can accurately predict the volatility in the next block(s). This is not realistic, as it requires ability to forecast external events that impact price. What if the true identity of Satoshi is suddenly revealed? A quantum computer that breaks ETH signatures announced? etc. - there's plenty of more minor unpredictable events

I wouldn't even says it's malice on the solver's side, because normally solvers are assumed to be self-interested entities. They are expected to exploit such bad prices, unless the protocol adds additional price validation in the smart contract level.

The issue cannot be exploited deterministically, because it depends on market conditions, but so do other classes of issues, such as liquidation-related issues in lending protocols.

To re-iterate the argument. In discounted HOT swaps that set a new spot price in the pool, the expected loss is the cost the LP pays for this service, and is bounded by a single block (due to signer competition). Due to these reasons, I believe validatePriceConsistency is adequate for them. In non-discounted HOT swaps the loss can grow much larger (bounded by the expiry time), and there's no service being provided by the solver to the protocol, so the cost is paid for nothing.

A support for this issue being valid is this comment in validatePriceConsistency: https://github.com/sherlock-audit/2024-03-arrakis/blob/main/valantis-hot/src/libraries/HOTParams.sol#L127-L132

    // sqrt hot and new AMM spot price cannot differ beyond allowed bounds
    if (
        !checkPriceDeviation(sqrtHotPriceX96, sqrtSpotPriceNewX96, hotMaxDiscountBipsLower, hotMaxDiscountBipsUpper)
    ) {
        revert HOTParams__validatePriceConsistency_hotAndSpotPriceNewExcessiveDeviation();
    }

To me, the comment implies that the developers already had the intention to compare the HOT swap's execution price (sqrtHotPriceX96) with the new AMM spot price, and revert swaps that deviate too far from that. The problem is that the code does not match the comment. For discounted swaps, sqrtSpotPriceNewX96 is not the new spot AMM price, sqrtSpotPriceX96 is! (See the "Code Snippet" in the issue.)

WangSecurity commented 3 months ago

Excuse me, I find it a bit hard when reading the report and the comments to understand what the issue is exactly. Can you provide the link to docs about signers and solvers and discounted Hot swaps, or explain them briefly (or both)?

kfx commented 3 months ago

@WangSecurity sure

From more specific to more general:

  1. Solver request docs https://docs.valantis.xyz/hot/solver-guide-subpages/request
  2. HoT swap docs (solver is referred as authorizedSender in code) https://docs.valantis.xyz/hot/hot-smart-contracts-subpages/hot
  3. HoT overview https://docs.valantis.xyz/hot/understanding-hot-a-graphical-overview

Comments in the discord clarifying that the solvers aren't trusted (I hope I'm allowed to share here, if not I'll delete) image

Sponsor agreeing on the validity of the quotes in the PoC (to be clear, without the context of the rest of the issue): image

WangSecurity commented 3 months ago

Thank you, I see that this situation with two solvers with the identical quotes is indeed possible. But there's another problem, firstly, it's the expiration, but it can be 10 mins, so it doesn't restrict the attack. But there're two problems here:

Then the AMM price sqrtSpotPriceX96 is compared with sqrtOraclePriceX96. Let's say the oracle is lagging slightly, at 1.03 P, but the maxOracleDeviationBipsUpper is set such that the check passes (e.g. to 200).

Then sqrtSpotPriceNewX96 (P) is compared with sqrtOraclePriceX96 (1.03 P). Let's say again the maxOracleDeviationBipsLower is set such that the check passes (e.g. to 300).

These are based on the fact that the oracle is lagging, but we should assume it would return the correct price always, based on README:

Are the admins of the protocols your contracts integrate with (if any) TRUSTED or RESTRICTED? If these integrations are trusted, should auditors also assume they are always responsive, for example, are oracles trusted to provide non-stale information, or VRF providers to respond within a designated timeframe? TRUSTED

Secondly, for these checks to pass, the TRUSTED Owner/Admin has to set specific values (maxOracleDeviationBipsLower and maxOracleDeviationBipsUpper), hence, even if they're set in that way, this behaviour would be acceptable and intended. Hence, since this issue is dependant on the Chainlink lagging and TRUSTED Owner/Admin setting values in a specific way, I believe it's indeed low severity.

Planning to accept the escalation and invalidate the report.

WangSecurity commented 3 months ago

Result: Invalid Unique

sherlock-admin2 commented 3 months ago

Escalations have been resolved successfully!

Escalation status: