Maia DAO Ecosystem - max10afternoon's results

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/talos/base/TalosBaseStrategy.sol#L394 https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/talos/libraries/PoolActions.sol#L56

Vulnerability details

Impact

A part of the fees accumulated by the underlying uniswap v3 position is allocated to the owner of the Talos' strategy. Those tokens are kept in the balance of the strategy and can be withdrawn at any moment by the owner.

Both the rebalance and rerange functionalities of the protocol, will get access to the entire balances of the strategy to perform their operations. Spending the owner's fees as well where necessary. The protocol will keep track of how much fees are owed to the owner. But for as long as there is not enough liquidity in the strategy the owner won't be able to withdraw the full amount.

(Since anyone can call rebalanace and rerange (via TalosManager.performUpkeep()), it is not possible to determine if and when the owner will be able to withdraw the entirety of the fees that they should get)

Also until there is enough liquidity to repay those fees, both Deposits and Redeems will be disabled (due to an arithmetic underflow) which means that the only way to collect fees, in order to accumulate liquidity in the balance of the strategy, is by calling rerange and rebalance. But, for the reasons mentioned bellow, those are going to use all the tokens available if necessary, potentially making things even worst. Functionally disabling all functionalities, dossing the strategy temporarily.

Proof of Concept

The TalosBaseStrategy.sol contract has two global variables, protocolFees0 and protocolFees1, to keep track of how many fees are owed to the owner of the strategy. The owner can call the collectProtocolFees(uint256 amount0, uint256 amount1) function of the same contract to transfer those amounts from the balances of the strategy to them self. (https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/talos/base/TalosBaseStrategy.sol#L394) Both the rerange() function and the rebalance() functions, from the same contract, will internally call the rerange(INonfungiblePositionManager nonfungiblePositionManager,ActionParams memory actionParams, uint24 poolFee) function from the PoolActions library. (https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/talos/libraries/PoolActions.sol#L56) This function will get access to the entire balances of the strategy to mint a new uniswap position.

The rerange(INonfungiblePositionManager nonfungiblePositionManager,ActionParams memory actionParams, uint24 poolFee) function, mentioned above, doesn't keep track of how many tokens are owed to the owner's fees, and will use them to mint the new position if necessary. Preventing the owner from withdrawing their fees until enough liquidity to pay for them is accumulated in the balances of the strategy.

Moreover both the deposit() and redeem() functions, in the TalosStrategyVanilla strategies, are internally calling _compoundFees(uint256 _tokenId) (https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/talos/TalosStrategyVanilla.sol#L129), to reinvest the occurred fees into the position. The first thing that this function does is to subtract the owner's fees from the balances, to avoid spending them:

uint256 balance0 = token0.balanceOf(address(this)) - protocolFees0;
uint256 balance1 = token1.balanceOf(address(this)) - protocolFees1;

If there are not enough tokens in the strategy's balances, to pay the owner, those lines will cause an underflow, preventing users from depositing/redeem until enough fees are accumulated to make both balances greater than both fees owed to the owner (which might never happen if someone calls performUpkeep again).

In order to solve this, the protocol has to accumulate enough fees to repay the owner. Fees are accumulated in the strategy's balance by calling either: deposit (which would underflow), redeem (which would underflow), rerange and rebalance (which can make things worst). Causing a deadlock, and blocking all interactions with the strategy (Dos), until the conditions explained above are met.

Here is a foundry scripts that shows the behaviours mentioned above. Place it in the test/talos directory to preserve dependencies. This script will pass a couple of checks, and than fails due to an arithmetic undeflow, see comment for better explanations:

// SPDX-License-Identifier: AGPL-3.0-only
pragma solidity ^0.8.0;

import {console2} from "forge-std/console2.sol";
import "forge-std/console.sol";

import {Ownable} from "solady/auth/Ownable.sol";

import {ERC20} from "solmate/tokens/ERC20.sol";
import {FixedPointMathLib} from "solmate/utils/FixedPointMathLib.sol";

import {SafeTransferLib} from "solmate/utils/SafeTransferLib.sol";
import {SafeCastLib} from "solmate/utils/SafeCastLib.sol";

import {TalosStrategyVanilla} from "@talos/TalosStrategyVanilla.sol";
import {ITalosBaseStrategy, TalosBaseStrategy} from "@talos/base/TalosBaseStrategy.sol";
import {IUniswapV3Pool, PoolVariables, PoolActions} from "@talos/libraries/PoolActions.sol";

import {TalosTestor} from "./TalosTestor.t.sol";


contract TalosStrategyVanillaTest is TalosTestor {
    using FixedPointMathLib for uint256;
    using FixedPointMathLib for uint160;
    using FixedPointMathLib for uint128;
    using SafeCastLib for uint256;
    using SafeCastLib for int256;
    using PoolVariables for IUniswapV3Pool;
    using PoolActions for IUniswapV3Pool;
    using SafeTransferLib for ERC20;

        
    event CollectFees(uint256 feesFromPool0, uint256 feesFromPool1, uint256 usersFees0, uint256 usersFees1);
    //////////////////////////////////////////////////////////////////
    //                          SET UP
    //////////////////////////////////////////////////////////////////

    function setUp() public {
        init();
    }

    function initializeTalos() internal override {
        talosBaseStrategy = new TalosStrategyVanilla(
            pool,
            strategyOptimizer,
            nonfungiblePositionManager,
            address(this),
            address(this)
        );
    }

    function testRebalance() public {
        uint256 amount0Desired = 10000000000000;

///////////////////// USER1 DEPOSITS IN THE STRATEGY ////////////////////////////////////////////////////
        token0.mint(user1, amount0Desired);
        token1.mint(user1, amount0Desired);
        hevm.prank(user1);
        token0.approve(address(talosBaseStrategy), amount0Desired);
        hevm.prank(user1);
        token1.approve(address(talosBaseStrategy), amount0Desired);
        hevm.prank(user1);
        talosBaseStrategy.deposit(amount0Desired, amount0Desired, user1);

/////////////////////RANDOM USERS SWAPS IN THE POOL GENERATING FEES (USES ADDRESS.THIS FOR SIMPLICITRY)//
        token0.mint(address(this), type(uint128).max);
        token1.mint(address(this), type(uint128).max);

        token0.approve(address(pool), type(uint128).max);
        token1.approve(address(pool), type(uint128).max);

        Swap(1000000000000, true); 
        Swap(1000000000000, false);
        hevm.warp(block.timestamp + 100);

//////////////////////////////////RERANGE AND CHECK FOR OWNER'S FEES////////////////////////////////////////
        
        //Fees belonging to the owenr of the strategy have been accumulated over time
        hevm.expectEmit(true, true, true, true);
        emit CollectFees(599999999, 329023812, 2999999999, 1645119060);
        talosBaseStrategy.rerange();

        //the owner has some fees to collect
        assertEq(talosBaseStrategy.protocolFees1(),329023812);
        //but the protocol has no liquidity to pay for them
        assertEq(token1.balanceOf(address(talosBaseStrategy)),0);

///////////////////////////////////REVERT DUE TO UNDERFLOW////////////////////////////////////////////////////

        hevm.prank(user1);
        //comment/uncomment any of the lines below, to prevent/cause the expected arithmetic underflow 

        talosBaseStrategy.deposit(amount0Desired, amount0Desired, user1);
        //talosBaseStrategy.redeem(talosBaseStrategy.balanceOf(address(user1)),0,0, address(user1), address(user1));
   
        
    }



    //utility function found in ./TalosTestor.t.sol to swap on pool
    //moved here for vivisbility
    function Swap(uint256 amountSpecified, bool zeroForOne) private {
        //Calc base ticks
        (uint160 sqrtPriceX96,,,,,,) = pool.slot0();

        // Calculate Price limit depending on price impact
        uint160 exactSqrtPriceImpact = (sqrtPriceX96 * (10000 / 2)) / GLOBAL_DIVISIONER;

        uint160 sqrtPriceLimitX96 =
            zeroForOne ? sqrtPriceX96 - exactSqrtPriceImpact : sqrtPriceX96 + exactSqrtPriceImpact;

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

}

Recommended Mitigation Steps

Although it is possible to transfer tokens directly to the strategy to resolve this. Doing so will result in the lost of most of them, since they will be redistributed among all share holders, and won't, obviously, mint any new shares to the sender. Making it not a particularly feasible solution. Instead the rerange(INonfungiblePositionManager nonfungiblePositionManager,ActionParams memory actionParams, uint24 poolFee) function from the PoolActions library could use an approach similar to the _compoundFees(uint256 _tokenId) function of the TalosStrategyVanilla.sol contract (https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/talos/TalosStrategyVanilla.sol#L129) which subtract the amounts owed to the owner's fees, before interacting with the uniswap position

uint256 balance0 = token0.balanceOf(address(this)) - protocolFees0;
uint256 balance1 = token1.balanceOf(address(this)) - protocolFees1;

This way the strategy will always have enough tokens to function correctly and the owner would be able to access their full amount at any time.

Assessed type

DoS