Zellic - Audit Reports

Category: Business Logic

Price deviation for calm period detection is done by exponentiating, not multiplying

Critical Severity

Critical Impact

Medium Likelihood

Description

A general issue with using the current Uniswap price as a price oracle is that these prices can be manipulated by an attacker during their transaction, possibly using flash loans.

To protect against such price manipulation, deposits are only allowed during a calm period in which prices have not recently moved by much. This is implemented by the onlyCalmPeriods modifier, which should revert when the relative change of the current price compared to the time-weighted average price over the last 60 seconds is too large.

This is the relevant code:

/// @notice Modifier to only allow deposit/harvest actions when current price is within 1% of twap.
modifier onlyCalmPeriods() {
    int24 tick = currentTick();
    int56 twapTick = twap();

    int56 upperBound = deviation + deviationThreshold;
    int56 lowerBound = deviationThreshold - deviation;

    // Calculate if greater than deviation % from twap and revert if it is. 
    if (tick >= 0) {
        if (tick > twapTick * upperBound / deviationThreshold || tick < twapTick * lowerBound / deviationThreshold) revert NotCalm();
    } else if (tick < twapTick * upperBound / deviationThreshold || tick > twapTick * lowerBound / deviationThreshold) revert NotCalm();
    _;
}

This code actually operates at the level of ticks rather than price. In Uniswap V3, the tick associated to a price is defined by $t i c k = lo g_{1.0001} (p r i ce)$ . In onlyCalmPeriods, the variable tick then holds the tick corresponding to the current tick, while twapTick holds the time-weighted arithmetic mean tick over the last 60 seconds. This tick corresponds to the time-weighted geometric mean price over the last 60 seconds, which we will call twapPrice. For more details on this, see the Uniswap V3 white paper↗, section 5.2.

The check in onlyCalmPeriods has two variants to deal with signs. To simplify exposition, let us assume for the moment that tick and twapTick are both positive. Then what is checked (i.e., it reverts if this does not hold) is the following, where we use $d = d e v ia t i o n / d e v ia t i o n T h res h o l d$ (which is 0.01 by default).

⟺ ⟺ ⟺ ⟺ ⟺ ⟺ \frac{d e v ia t i o n T h res h o l d - d e v ia t i o n}{d e v ia t i o n T h res h o l d} \cdot tw a pT i c k \leq t i c k j u s tp a dd in g f or t h e p df v ers i o n \leq \frac{d e v ia t i o n T h res h o l d + d e v ia t i o n}{d e v ia t i o n T h res h o l d} \cdot tw a pT i c k (1 - d) \cdot tw a pT i c k \leq t i c k \leq (1 + d) \cdot tw a pT i c k 1.000 1^{(1 - d) \cdot tw a pT i c k} \leq 1.000 1^{t i c k} \leq 1.000 1^{(1 + d) \cdot tw a pT i c k} (1.000 1^{tw a pT i c k})^{1 - d} \leq p r i ce \leq (1.000 1^{tw a pT i c k})^{1 + d} tw a pP r i c e^{1 - d} \leq p r i ce \leq tw a pP r i c e^{1 + d}

In order to check for the relative change of $p r i ce$ compared to $tw a pP r i ce$ of at most, for example, 1%, using $d = 0.01$ for this, the check should instead have been this:

(1 - d) \cdot tw a pP r i ce \leq p r i ce \leq (1 + d) \cdot tw a pP r i ce

A consequence of exponentiating with $1 - d$ and $1 + d$ instead of multiplying with it is that the relative change allowed changes with $tw a pP r i ce$ . Here is a table with some example values where $d = 0.01$ , the default value, with percentage changes in parentheses:

$tw a pP r i ce$	minimum $p r i ce$ allowed	maximum $p r i ce$ allowed
$1$	$1.0$	(	$0.00$	%)	$1.0$	(	$0.00$	%)
$2$	$1.9862$	(	$- 0.69$	%)	$2.0139$	(	$0.70$	%)
$10$	$9.77$	(	$- 2.28$	%)	$10.23$	(	$2.33$	%)
$0.1$	$0.10233$	(	$2.33$	%)	$0.09772$	(	$- 2.28$	%)
$1, 000, 000, 000, 000$	$758, 577, 575, 029$	(	$- 24.14$	%)	$1, 318, 256, 738, 556$	(	$31.83$	%)

The above table was generated with this Python code:

for twap in (1, 2, 10, 0.1, 10**12):
    min_p = twap**0.99
    max_p = twap**1.01
    min_perc = ((min_p / twap) - 1) * 100
    max_perc = ((max_p / twap) - 1) * 100
    print(f'| `L!${twap}$` | `L!${min_p:,}$` (`L!${min_perc:.2f}$`%) | `L!${max_p:,}$` (`L!${max_perc:.2f}$`%) |')

Note that the relative change allowed in the positive direction can be calculated as follows when $tw a pP r i ce$ is above 1:

\frac{tw a pP r i c e ^{1 + d}}{tw a pP r i ce} - 1 = tw a pP r i c e^{d} - 1 = (tw a pP r i c e^{- 1})^{- d} - 1 = \frac{( tw a pP r i c e ^{- 1} ) ^{1 - d}}{tw a pP r i c e ^{- 1}} - 1

As the latter value is the relative change allowed in the negative direction when the time-weighted average price is $tw a pP r i c e^{- 1}$ , this means the situation is symmetrical (in the multiplicative sense) around 1.

The relative change allowed being zero when $tw a pP r i ce = 1$ , and being bigger the further away $tw a pP r i ce$ is from 1 has two different negative consequences.

Firstly, when $tw a pP r i ce = 1$ or is very close to it, the onlyCalmPeriods modifier will revert on even very tiny deviations from that price. A time-weighted average price of very close to 1 is not unrealistic in pools for two different USD stablecoins, for example. The StrategyPassiveManagerUniswap contract may then disallow deposits a lot more often than desired.

Secondly, when $tw a pP r i ce = 1$ is very large or very small, then the onlyCalmPeriods check will be much weaker than expected, allowing the possibility of an attacker to profit from price manipulations after all. A very large or very small price is not unrealistic, for example the DAI stablecoin has 18 decimals while USDC has 6 decimals, so the price of the DAI/USDC pool will tend to be around $1 0^{- 12}$ ; in which case, with $d = 0.01$ , a relative deviation of up to $31.83$ % is allowed in one direction, as can be read off from the above table.

Note how in the current implementation, the behavior of the onlyCalmPeriods modifier depends significantly on the amount of decimals the tokens have. As decimals should be an implementation detail and not play a role in financial logic, this is an indication that the current checks are incorrect.

Impact

If the time-weighted average price is close to 1, deposits will be disallowed more often than intended.

If the time-weighted average price is far away from 1 on a logarithmic scale, an attacker manipulating the Uniswap pool's price might be able to drain the pool of part of its value within one transaction while making a profit. See ref↗ for a more detailed explanation of this, where we estimate that in the context of a DAI/USDC Uniswap pool with a fee of 0.01%, it is profitable to carry out an attack that drains value from the vault/strategy contract when the strategy's liquidity is at least about 0.001 of the other liquidity providers' liquidity in the manipulated range and as long as the relative deviation exceeds about 25.02%. If the liquidity ratio is 0.01 instead of 0.001, then 2.06% would suffice. Given that in this case we arrived at the relative deviation allowed being up to 31.83%, this leaves ample room for minor inaccuracies in the estimation as well as fees for flash loans.

Recommendations

Consider changing onlyCalmPeriods to check for relative change in the price rather than the ticks. Equivalently, check for absolute change in the ticks. This is equivalent:

⟺ ⟺ ⟺ ⟺ ⟺ (1 - d) \cdot tw a pP r i ce \leq p r i ce \leq (1 + d) \cdot tw a pP r i ce lo g_{1.0001} ((1 - d) \cdot tw a pP r i ce) \leq lo g_{1.0001} (p r i ce) \leq lo g_{1.0001} ((1 + d) \cdot tw a pP r i ce) lo g_{1.0001} (tw a pP r i ce) + lo g_{1.0001} (1 - d) \leq lo g_{1.0001} (p r i ce) j u s tp a dd in g f or t h e p df v ers i o n \leq lo g_{1.0001} (tw a pP r i ce) + lo g_{1.0001} (1 + d) tw a pT i c k + lo g_{1.0001} (1 - d) \leq t i c k \leq tw a pT i c k + lo g_{1.0001} (1 + d)

Note that $lo g_{1.0001} (1 - d)$ and $lo g_{1.0001} (1 + d)$ are constants (they do not depend on twapPrice or tick), so there is no need to calculate logarithms on chain for this check.

Remediation

This issue has been acknowledged by Beefy, and fixes were implemented in the following commits: