Sorella Labs

Sorella Angstrom

Cantina Security Report

Organization

@SorellaLabs

Engagement Type

Cantina Reviews

Period

-

Repositories

GitHubSorellaLabs/angstrom

Researchers

cergyk

cccz

Findings

Medium Risk

1 findings

1 fixed

0 acknowledged

Low Risk

2 findings

0 fixed

2 acknowledged

Medium Risk1 finding

  1. The protocol charges less for exactOut direction swap

    State

    Fixed

    PR #560

    Severity

    Severity: Medium

    ≈

    Likelihood: Medium

    ×

    Impact: Medium

    Submitted by

    cccz

    Description

    When charging protocol fee, the protocol always calculates the fee using the unspecified amount multiplied by fee_rate_e6. The problem here is that the percentage of the fee charged should be different when the user's swap direction is different.

    int128 target_amount =                exactIn != params.zeroForOne ? swap_delta.amount0() : swap_delta.amount1();            fee = ((target_amount < 0 ? -target_amount : target_amount) * fee_rate_e6) / 1e6;

    Consider a protocol fee of 1%, an swap fee of 2%, and token0:token1 = 1:1.

    1. The user uses 1000 token0 to swap token1, first calculates the unspecified amount as 1000 * (1 - 2%) = 980 token1, then calculate the protocol fee as 980 * 10% = 9.8 token1, the user finally gets 970.2 token1.

    2. The user uses x token0 to swap 970.2 token1, first calculates the unspecified amount as 970.2 / (1 - 2%) = 990 token0, then calculates the protocol fee as 990 * 1% = 9.9 token0, the user pays 999.9 token0.

    Actually when the direction is exactOut, the fee should be 1 / (1 - fee_rate_e6) - 1 instead of fee_rate_e6. Then in case 2 above, the protocol fee should be 990 / (1 - 1%) - 990 = 10 token0, the user pays 1000 token0.

    Taking the 1% protocol fee as an example, the protocol fee for the exactOut direction should be 1.01%, which would actually cause the protocol to lose 0.01% of the total amount swapped.

    Proof of Concept

    The following POC shows that 10e18 token0 is swapped for 9.702e18 token1, and only 9.999e18 token0 is needed to swap to 9.702e18 token1

    function _createPool(uint16 tickSpacing, uint24 unlockedFee, uint248 startLiquidity)        internal        returns (PoolKey memory pk)    {        vm.prank(controller);        angstrom.configurePool(asset0, asset1, tickSpacing, 0, unlockedFee, 0.01e6);        angstrom.initializePool(asset0, asset1, 0, TickMath.getSqrtPriceAtTick(0));        int24 spacing = int24(uint24(tickSpacing));        pk = poolKey(angstrom, asset0, asset1, spacing);        if (startLiquidity > 0) {            actor.modifyLiquidity(                pk, -1 * spacing, 1 * spacing, int256(uint256(startLiquidity)), bytes32(0)            );        }
        return pk;    }    function test_poc()        public    {        vm.roll(1000);
        uint24 unlockedFee = 0.02e6;        uint256 swapAmount = 10e18;
        // ------ PRE SNAPSHOT ------        uint256 snapshotId = vm.snapshot();        uint248 liq = 100_000e21;        PoolKey memory pk = _createPool(60, unlockedFee, liq);
        vm.prank(node);        angstrom.execute("");        int128 FeeOut1 = actor.swap(pk, true, -int256(swapAmount), 4295128740).amount1(); // 9701999049204093178        console.log(FeeOut1);        vm.revertTo(snapshotId);
        // ------ ACTUAL CALL ------        pk = _createPool(60, unlockedFee, liq);
        vm.prank(node);        angstrom.execute("");        int128 FeeOut2 = actor.swap(pk, true, int256(9701999049204093178), 4295128740).amount0(); // -9998999990200980011        console.log(FeeOut2);    }

    Recommendation

    Change to

    --- a/contracts/src/modules/UnlockHook.sol+++ b/contracts/src/modules/UnlockHook.sol@@ -80,11 +80,11 @@ abstract contract UnlockHook is                 StoreKeyLib.keyFromAssetsUnchecked(_addr(key.currency0), _addr(key.currency1));             int24 fee_rate_e6 = int24(_unlockedFees[storeKey].protocolUnlockedFee);             bool exactIn = params.amountSpecified < 0;--            int128 target_amount =-                exactIn != params.zeroForOne ? swap_delta.amount0() : swap_delta.amount1();-            fee = ((target_amount < 0 ? -target_amount : target_amount) * fee_rate_e6) / 1e6;-+            {+                int128 target_amount = exactIn != params.zeroForOne ? swap_delta.amount0() : swap_delta.amount1();+                int128 p_target_amount = target_amount < 0 ? -target_amount : target_amount;+                fee = exactIn ? p_target_amount * fee_rate_e6 / 1e6 : p_target_amount * 1e6 / (1e6 - fee_rate_e6) - p_target_amount;+            }

Low Risk2 findings

  1. Nodes can avoid sharing fees by using a ToB order and setting save to zero

    State

    Acknowledged

    Severity

    Severity: Low

    Submitted by

    cergyk

    Description

    In Angstrom, bundle execution takes three steps:

    1. Uniswap v4 swap
    2. Top of the block orders
    3. User orders

    The fees accumulated through each of the steps (gasUsedAsset0 or extraFeeAsset0, or simply excess due to profitable swap) are supposed to be accounted for in the save field of the asset:

    Asset.sol?lines=20,27:

    /// @dev Size of a single encoded asset (b20:addr ++ b16:save ++ b16:borrow ++ b16:settle)    uint256 internal constant ASSET_CD_BYTES = 68;
    uint256 internal constant ADDR_OFFSET = 0;    uint256 internal constant SAVE_OFFSET = 20; //@audit save field accounts for all fees    uint256 internal constant TAKE_OFFSET = 36;    uint256 internal constant SETTLE_OFFSET = 52;

    Unfortunately, the only constraint checked at the settlement is bundleDelta[asset] == saving + settle:

    Settlement.sol?lines=75,81:

    Asset asset = assets.getUnchecked(i);    address addr = asset.addr();    uint256 saving = asset.save();    uint256 settle = asset.settle();
    int256 delta = bundleDeltas.sub(addr, saving + settle);
    //@audit delta must be zero. bundleDelta[asset] == saving + settle    if (delta != 0) {        revert BundlDeltaUnresolved(addr);    }

    With settle being the amount to be sent back to UniswapV4 singleton for settling the negative delta.

    This means that a node can adapt ToB orders to direct the accumulated fees directly to her, and set save to be zero for every asset.

    Concrete example

    1. Alice sends a user order with a swap of 1000 USDC for 1000 USDT, accepting 10 USDC as extraFeeAsset0
    2. Bob the node bundles it with his own ToB order of 0 WETH for 10 USDC, allowing him to set save for USDC to zero.

    This way Bob is taking a fee for himself, which should be shared among all validators.

    Impact and likelihood

    This issue is reported as low severity because Sorella Labs already has mechanisms for slashing nodes that deviate from canonical behavior.

    Recommendation

    There is no simple way to mitigate this, but a few solutions can be considered:

    1. save for an asset must be at least the sum of all explicit fees for the asset (gasUsedAsset0 and extraFeeAsset0)
    2. Force the price of a ToB order to be in range compared to other prices used for pairs in the bundle

  2. When expectedLiquidity == 0 and currentOnly, but amount != 0, the rewards are lost

    State

    Acknowledged

    Severity

    Severity: Low

    Submitted by

    cergyk

    Description

    There is an early return in the _decodeAndReward logic:

    GrowthOutsideUpdater.sol?lines=46,48:

    function _decodeAndReward(        bool currentOnly,        CalldataReader reader,        PoolRewards storage poolRewards_,        PoolId id,        int24 tickSpacing,        int24 currentTick    ) internal returns (CalldataReader, uint256) {        if (currentOnly) {            uint128 amount;            (reader, amount) = reader.readU128();            uint128 expectedLiquidity;            (reader, expectedLiquidity) = reader.readU128();            // we sometimes have to do empty updates for swap encoding            // purposes            //@audit early return can be triggered when expectedLiquidity == 0, but amount != 0>>          if (amount == 0 || expectedLiquidity == 0) {>>              return (reader, amount);>>          }

    Unfortunately, in the case where expectedLiquidity == 0 but amount != 0, we can see that the caller still reduces the corresponding bundleDelta:

    PoolUpdates.sol?lines=210,213:

    (reader, rewardTotal) = _decodeAndReward(        variantMap.currentOnly(), reader, poolRewards[id], id, swapCall.tickSpacing, currentTick    );    //@audit bundle delta is reduced by the reward amount    bundleDeltas.sub(swapCall.asset0, rewardTotal);

    If the bundle is otherwise well-formed and able to settle, these rewards become stuck, unclaimable by either LPs or validators.

    Recommendation

    the There shouldn't be a case where expectedLiquidity == 0 but amount != 0:

    -   if (amount == 0 || expectedLiquidity == 0) {+       if (expectedLiquidity == 0 && amount != 0)+           revert RewardsLost();        return (reader, amount);    }