kfx - Liquidity calculation overflows can be weaponized for DoS attacks via token donations

[sherlock-admin4]

kfx

medium

Liquidity calculation overflows can be weaponized for DoS attacks via token donations

Summary

The function _calculateAMMLiquidity() can be forced to revert with a swap that ends moving the AMM price close to the price boundaries, followed by a token donation.

Vulnerability Detail

Function getLiquidityForAmount0 and getLiquidityForAmount1 may revert due to numerical overflow when sqrtRatioAX96 and sqrtRatioBX96 have very similar values.

In the design of HOT, such a similarity of prices can be achieved by swapping most of the reserves to a single token. Such a situation may arise completely naturally, as the price of the asset evolves over time, and the Valantis pool gets arbitraged.

If in this situation the scarce token's reserves get sufficiently increased, liquidity calculations start to revert.

Reserve increases normally happen via the deposit function called by a trusted liquidity provider. However, for rebase tokens the balanceOf function is used to check for reserves. This enables two other ways how reserves can be increased:

1. Via donations to the pool - can be weaponized by an attacker.
2. Over time, as positive rebase tokens grow on its own. They will eventually reach the overflow state, provided that the price stays close to the range bounds for a sufficient time.

The following HOT core functions call _calculateAMMLiquidity():

• depositLiquidity
• withdrawLiquidity
• getReservesAtPrice
• AMM swaps (in the body of the swap)
• HOT swaps (in the callback)

When this state is entered, most of these functions become unuseable. The exceptions are:

• withdrawLiquidity - since the liquidity recalculation is done after moving the tokens out of the pool, withdrawing sufficient liquidity fixes the DoS
• HOT swaps that change the price of the AMM - however, this is not possible if the oracle feeds have not been set, or the signer is inoperational. According to comments in the code, "AMM-only" mode of operation is also envisioned, where the oracle feeds are not set. In any case, setting the feeds is under a timelock and cannot be done immediately.

The DoS can also be fixed by calling setPriceBounds, but it's also under a timelock.

Impact

The DoS is easy to revert, by calling withdrawLiquidity with the right arguments, however, it still does break core contract functionality, requires intervention of a trusted party, and stops time-sensitive functions from execution (the permissionless AMM swaps, for instance). Therefore I believe this issue is a valid Medium.

Code Snippet

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

    function _calculateAMMLiquidity() private view returns (uint128 updatedLiquidity) {
        (uint160 sqrtSpotPriceX96Cache, uint160 sqrtPriceLowX96Cache, uint160 sqrtPriceHighX96Cache) = _getAMMState();

        // Query current pool reserves
        (uint256 reserve0, uint256 reserve1) = ISovereignPool(_pool).getReserves();

        // Calculate liquidity corresponding to each of token's reserves and respective price ranges
        uint128 liquidity0 = LiquidityAmounts.getLiquidityForAmount0(
            sqrtSpotPriceX96Cache,
            sqrtPriceHighX96Cache,
            reserve0 // @audit may revert if sqrtSpotPriceX96Cache and sqrtPriceHighX96Cache are close
        );
        uint128 liquidity1 = LiquidityAmounts.getLiquidityForAmount1(
            sqrtPriceLowX96Cache,
            sqrtSpotPriceX96Cache,
            reserve1 // @audit may revert if sqrtPriceLowX96Cache and sqrtSpotPriceX96Cache are close
        );
     // ...
    }

PoC

Attack PoC

```solidity function test_swap_and_donate_attack() public { // ------- // Setup (not part of the attack) // ------- // deploy rebase token pool SovereignPoolConstructorArgs memory poolArgs = _generateDefaultConstructorArgs(); poolArgs.isToken0Rebase = true; poolArgs.isToken1Rebase = true; pool = this.deploySovereignPool(poolArgs); hot = deployAndSetDefaultHOT(pool); _addToContractsToApprove(address(pool)); _addToContractsToApprove(address(hot)); token0.approve(address(hot), 1e26); token1.approve(address(hot), 1e26); token0.approve(address(pool), 1e26); token1.approve(address(pool), 1e26); hot.depositLiquidity(5e18, 10_000e18 - 1700, 0, 0); vm.prank(address(this)); hot.setMaxOracleDeviationBips(hotImmutableMaxOracleDeviationBound, hotImmutableMaxOracleDeviationBound); // ------- // Attack // ------- // 1. Swap to price bounds using the permissionless AMM swap, clearing almost all token1 from the pool SovereignPoolSwapContextData memory data; SovereignPoolSwapParams memory params = SovereignPoolSwapParams({ isSwapCallback: false, isZeroToOne: true, amountIn: 5.773502691896257884e18, amountOutMin: 0, recipient: address(this), deadline: block.timestamp + 2, swapTokenOut: address(token1), swapContext: data }); pool.swap(params); // 2. Donate some token1 to the pool token1.transfer(address(pool), 1e18); // 3. Check that the AMM is now DoSed vm.expectRevert(); hot.depositLiquidity(1, 1, 0, 0); // fails with "EvmError: Revert" vm.expectRevert(); hot.withdrawLiquidity(1, 1, address(this), 0, 0); // fails with "EvmError: Revert" } ```

Tool used

Manual Review

Recommendation

Do not allow swaps to move the post-swap price close to the AMM price boundaries - leave sufficient safety margin. This will increase the price of the attack.

[0xffff11]

The root cause is correctly identified, but the scenario is not viable:

• The attacker loses a lot of funds to move the price to the boundary

• The honest executor can unlock the situation by calling setPriceBounds and take attacker funds

  This makes for a very costly very short DoS for attacker

[kfx]

Escalate

The attacker loses a lot of funds to move the price to the boundary

Price movements within the range are normally expected, including up to the price boundary. The attackers final swap that moves the price the last 0.01% closer to the boundary to make the donation small.

The honest executor can unlock the situation by calling setPriceBounds and take attacker funds

The attack can be done by a dishonest executor, similar to what is described in #79

[sherlock-admin3]

Escalate

The attacker loses a lot of funds to move the price to the boundary

Price movements within the range are normally expected, including up to the price boundary. The attackers final swap that moves the price the last 0.01% closer to the boundary to make the donation small.

The honest executor can unlock the situation by calling setPriceBounds and take attacker funds

The attack can be done by a dishonest executor, similar to what is described in #79

The escalation could not be created because you are not exceeding the escalation threshold.

You can view the required number of additional valid issues/judging contest payouts in your Profile page, in the Sherlock webapp.