Maia DAO Ecosystem - Breeje's results

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/main/src/talos/strategies/TalosStrategySimple.sol#L42 https://github.com/code-423n4/2023-05-maia/blob/main/src/talos/libraries/PoolActions.sol#L50

Vulnerability details

Impact

Loss of funds due to MEV Sandwich attacks.

Proof of Concept

Rebalancing is done using doRebalance method in TalosStrategySimple.

File: TalosStrategySimple.sol

  function doRebalance() internal override returns (uint256 amount0, uint256 amount1) {
      int24 baseThreshold = tickSpacing * optimizer.tickRangeMultiplier();

      PoolActions.ActionParams memory actionParams =
          PoolActions.ActionParams(pool, optimizer, token0, token1, tickSpacing);

@->   PoolActions.swapToEqualAmounts(actionParams, baseThreshold);

      (tickLower, tickUpper, amount0, amount1, tokenId, liquidity) =
          nonfungiblePositionManager.rerange(actionParams, poolFee);
  }

Link to code

Line in focus here is:

    PoolActions.swapToEqualAmounts(actionParams, baseThreshold);

swapToEqualAmounts method in PoolActions.sol library:

File: PoolActions.sol

  function swapToEqualAmounts(ActionParams memory actionParams, int24 baseThreshold) internal {
@->   (bool zeroForOne, int256 amountSpecified, uint160 sqrtPriceLimitX96) = actionParams
          .pool
          .getSwapToEqualAmountsParams(
          actionParams.optimizer, actionParams.tickSpacing, baseThreshold, actionParams.token0, actionParams.token1
      );

      //Swap imbalanced token as long as we haven't used the entire amountSpecified and haven't reached the price limit
      actionParams.pool.swap(
          address(this),
          zeroForOne,
          amountSpecified,
 @->      sqrtPriceLimitX96,
          abi.encode(SwapCallbackData({zeroForOne: zeroForOne}))
      );
  }

Link to Code

Here, the value of sqrtPriceLimitX96 is calculated through getSwapToEqualAmountsParams method in PoolVariables.sol library. It basically represents the square root of the lowest or highest price that you are willing to perform the trade at. So basically it is a slippage control parameter.

File: PoolVariables.sol

232:    (uint160 sqrtPriceX96, int24 currentTick,,,,,) = _pool.slot0();

251:    uint160 exactSqrtPriceImpact = (sqrtPriceX96 * (_strategy.priceImpactPercentage() / 2)) / GLOBAL_DIVISIONER;
252:    sqrtPriceLimitX96 = zeroForOne ? sqrtPriceX96 - exactSqrtPriceImpact : sqrtPriceX96 + exactSqrtPriceImpact;

getSwapToEqualAmountsParams method uses the _pool.slot0 to determine The current price of the pool as a sqrt. slot0 is the most recent data point and can easily be manipulated using Flash Loans.

Because of this, the value of sqrtPriceLimitX96 can be manipulated and that value will be passed in swap method.

With this knowledge, a malicious MEV bot could watch for these transactions in the mempool. When it sees such a transaction, it could perform a "sandwich attack", trading massively in the same direction as the trade in advance of it to push the price out of whack, and then trading back after us, so that they end up pocketing a profit at our expense.

Tools Used

VS Code

Recommended Mitigation Steps

Use TWAP rather than slot0.

Assessed type

MEV

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/main/src/talos/TalosStrategyStaked.sol#L28

Vulnerability details

Proof of Concept

There are many instance of this, but to understand things better, taking the example of createTalosV3Strategy method.

The createTalosV3Strategy function deploys a new TalosStrategyStaked contract using the create, where the address derivation depends only on the arguments passed.

At the same time, some of the chains like Arbitrum and Polygon are suspicious of the reorg attack.

File: TalosStrategyStaked.sol

  function createTalosV3Strategy(
        IUniswapV3Pool pool,
        ITalosOptimizer optimizer,
        BoostAggregator boostAggregator,
        address strategyManager,
        FlywheelCoreInstant flywheel,
        address owner
    ) public returns (TalosBaseStrategy) {
        return new TalosStrategyStaked( // @audit-issue Reorg Attack
                pool,
                optimizer,
                boostAggregator,
                strategyManager,
                flywheel,
                owner
            );
    }

Link to Code

Even more, the reorg can be couple of minutes long. So, it is quite enough to create the TalosStrategyStaked and transfer funds to that address using deposit method, especially when someone uses a script, and not doing it by hand.

Optimistic rollups (Optimism/Arbitrum) are also suspect to reorgs since if someone finds a fraud the blocks will be reverted, even though the user receives a confirmation.

Same issue can affect factory contracts in Ulysses omnichain contracts as well with more severe consequences.

Can refer this an issue previously report here to have more understanding about it.

Impact

Exploits involving Stealing of funds.

Tools Used

VS Code

Recommended Mitigation Steps

Deploy such contracts via create2 with salt.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/main/src/talos/base/TalosBaseStrategy.sol#L144-L145

Vulnerability details

Impact

Loss of funds due to Allowing Maximum Slippage.

Proof of Concept

There are many instances throughout the codebase where slippage protection has not been used and 0 has been passed to slippage control instead.

Eg: deposit method in TalosBaseStrategy:

File: TalosBaseStrategy.sol

  (liquidityDifference, amount0, amount1) = nonfungiblePositionManager.increaseLiquidity(
        INonfungiblePositionManager.IncreaseLiquidityParams({
            tokenId: _tokenId,
            amount0Desired: amount0Desired,
            amount1Desired: amount1Desired,
@->         amount0Min: 0,      // @audit-issue No Slippage Protection
@->         amount1Min: 0,
            deadline: block.timestamp        // @audit-issue Miner controlled Deadline
        })
    );

Link to code

In Uniswap's example docs here, it is clearly stated that:

We cannot change the boundaries of a given liquidity position using the Uniswap v3 protocol; increaseLiquidity can only increase the liquidity of a position.

In production, amount0Min and amount1Min should be adjusted to create slippage protections.

It is worth noting the second point that amount0Min and amount1Min acts as a slippage protection and it should be adjusted in the production in order to protect the users from High Slippage.

But in the following instance, this slippage parameter is set to zero which is exposing the transaction to maximum slippage:

• TalosStrategyVanilla.sol
• TalosBaseStrategy.sol 1
• TalosBaseStrategy.sol 2
• TalosBaseStrategy.sol 3
• PoolActions.sol

Tools Used

VS Code

Recommended Mitigation Steps

Allow users to pass the value of amountXMin as parameter.

Assessed type

Uniswap

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-amm/UlyssesRouter.sol#L73

Vulnerability details

Proof of Concept

AMMs provide their users with an option to limit the execution of their pending actions, such as swaps or adding and removing liquidity. The most common solution is to include a deadline timestamp as a parameter (for example: Uniswap V2 and Uniswap V3). If such an option is not present, users can unknowingly perform bad trades.

Currently there are 3 external methods in UlyssesRouter:

1. addLiquidity
2. removeLiquidity
3. swap

2 things are most important while interacting with AMMs:

1. Slippage parameter: Which is controlled here by user by passing the value of minOutput as a function parameter.
2. Deadline of Execution: In case, the transaction is submitted to the mempool and transaction fee is too low for miners to be interested in including the transaction in a block immediately, then the transaction can stay in the mempool for some time. If it is executed at a later time, there is high chance that the slippage parameter earlier passed might result in higher slippage than what user originally anticipated. This can result in loss of funds.

A MEV bot can detect the pending transactions. Since the outdated maximum slippage value now allows for high slippage, the bot sandwiches the transaction, resulting in significant profit for the bot and significant loss for user.

This second part is not handled in UlyssesRouter.

Impact

Potential loss of funds

Tools Used

VS Code

Recommended Mitigation Steps

Add a deadline check exactly how uniswap does in functions related to adding liquidity, removing liquidity and swapping.

Assessed type

MEV

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/main/src/talos/base/TalosBaseStrategy.sol#L208

Vulnerability details

Impact

Miner can potentially hold the transaction which results in loss of funds for users.

Proof of Concept

File: TalosBaseStrategy.sol

    (liquidityDifference, amount0, amount1) = nonfungiblePositionManager.increaseLiquidity(
        INonfungiblePositionManager.IncreaseLiquidityParams({
            tokenId: _tokenId,
            amount0Desired: amount0Desired,
            amount1Desired: amount1Desired,
            amount0Min: 0,     
            amount1Min: 0,
 @->        deadline: block.timestamp        
        })
    );

Link to code

AMMs provide their users with an option to limit the execution of their pending actions, such as swaps or adding and removing liquidity. The most common solution is to include a deadline timestamp as a parameter (for example in Uniswap V2 and Uniswap V3). If such an option is not present, users can unknowingly perform bad trades as interaction with the pools is very time sensitive, if transaction gets hold for sometime than it can lead to bad trades unknowingly by the Users and can potentially result in loss of funds.

All the functions in the codebase that interact with uniswap pools have a deadline parameter passed as block.timestamp, which means that whenever the miner decides to include the transaction in a block, it will be valid at that time, since block.timestamp will be the current timestamp at the time of mining. but here, there is a possibility of a malicious miner to hold the transaction how whatever amount he/she wants.

This is tbe reason, instead of block.timestamp, an extra arguments should be passed with the functions and user should be allowed to pass a valid deadline for it so that in case a malicious miner tries to hold it, then the transaction reverts as per the deadline specified by the user. Having slippage is an issue for any AMMs but having outdated Slippage protection can also be a dangerous attack vector.

Link to a similar issue reported in Paraspace Audit by Code4rena.

Tools Used

VS Code

Recommended Mitigation Steps

Add deadline arguments in all functions that interact with uniswap pools, and pass it along to these calls.

Assessed type

Uniswap