Maia DAO Ecosystem - SpicyMeatball's results

Lines of code

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

Vulnerability details

Impact

When calling removeAsset in UlyssesToken.sol we update our arrays of assets and weights, as well as the assetId mapping, where each asset address is assigned it's own id, if id = 0 we assume that asset isn't a vault asset. Unfortunately there is a bug in the assignment of assetId which cause the last asset to be given the wrong id.

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

At worst case (when we remove asset with id = 1) this can result in inability to remove the last asset, because of underflow.

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

Proof of Concept

Test case

// SPDX-License-Identifier: AGPL-3.0
pragma solidity >=0.8.0 <0.9.0;

import {MockERC20} from "solmate/test/utils/mocks/MockERC20.sol";
import {UlyssesToken} from "@ulysses-amm/UlyssesToken.sol";
import "forge-std/Test.sol";

import "forge-std/console.sol";

contract UlyssesTokenTestC4 is Test {
    uint256 constant NUM_ASSETS = 4;
    address[] underlyings;
    UlyssesToken vault;

    function setUp() public {
        for (uint256 i = 0; i < NUM_ASSETS; i++) {
            underlyings.push(address(new MockERC20("Mock ERC20", "MERC20", 18)));
        }

        address[] memory assets = new address[](NUM_ASSETS);
        for (uint256 i = 0; i < NUM_ASSETS; i++) {
            assets[i] = underlyings[i];
        }

        uint8[4] memory exampleWeights = [10, 20, 30, 40];

        uint256[] memory weights = new uint256[](NUM_ASSETS);
        for (uint256 i = 0; i < NUM_ASSETS; i++) {
            weights[i] = exampleWeights[i];
        }

        vault = new UlyssesToken(1, assets, weights, "Mock ERC4626", "MERC4626", address(this));
    }

    function getAssets() private view {
        address[] memory assets = vault.getAssets();
        console.log("\n---ASSETS & WEIGHTS---\n");
        for(uint256 i=0; i<assets.length; i++) {
            console.log(assets[i], vault.weights(i));
        }
    }

    function test_removeAsset() public {
        console.log("\n---BEFORE ASSET REMOVAL---\n");
        console.log("ID TOKEN0: ", vault.assetId(underlyings[0]));
        console.log("ID TOKEN1: ", vault.assetId(underlyings[1]));
        console.log("ID TOKEN2: ", vault.assetId(underlyings[2]));
        console.log("ID TOKEN3: ", vault.assetId(underlyings[3]));
        getAssets();

        vault.removeAsset(address(underlyings[0]));

        console.log("\n---AFTER ASSET REMOVAL---\n");
        console.log("ID TOKEN0: ", vault.assetId(underlyings[0]));
        console.log("ID TOKEN1: ", vault.assetId(underlyings[1]));
        console.log("ID TOKEN2: ", vault.assetId(underlyings[2]));
        console.log("ID TOKEN3: ", vault.assetId(underlyings[3]));
        getAssets();
        //can't remove asset #3 because of underflow
        vm.expectRevert();
        vault.removeAsset(underlyings[3]);
    }
}

logs if removing first asset

---BEFORE ASSET REMOVAL---                                                                                                     
                                                                                                                               
  ID TOKEN0:  1                                                                                                                
  ID TOKEN1:  2                                                                                                                
  ID TOKEN2:  3                                                                                                                
  ID TOKEN3:  4                                                                                                                

---AFTER ASSET REMOVAL---

  ID TOKEN0:  0
  ID TOKEN1:  2
  ID TOKEN2:  3
  ID TOKEN3:  0

logs if removing second asset

---BEFORE ASSET REMOVAL---                                                                                                     
                                                                                                                               
  ID TOKEN0:  1                                                                                                                
  ID TOKEN1:  2                                                                                                                
  ID TOKEN2:  3                                                                                                                
  ID TOKEN3:  4                                                                                                                

---AFTER ASSET REMOVAL---

  ID TOKEN0:  1
  ID TOKEN1:  0
  ID TOKEN2:  3
  ID TOKEN3:  1

Tools Used

Forge

Recommended Mitigation Steps

Change this line https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-amm/UlyssesToken.sol#L72

+ assetId[lastAsset] = assetIndex + 1;

Assessed type

Error

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/main/src/talos/libraries/PoolActions.sol#L98-L99 https://github.com/code-423n4/2023-05-maia/blob/main/src/talos/libraries/PoolActions.sol#L80-L81 https://github.com/code-423n4/2023-05-maia/blob/main/src/talos/TalosStrategyVanilla.sol#L130-L131 https://github.com/code-423n4/2023-05-maia/blob/main/src/talos/base/TalosBaseStrategy.sol#L408

Vulnerability details

Impact

In the PoolActions library we assume that all tokens at the strategy contract are available for use. During rerange operations we withdraw all liquidity from a position and then mint a new position with all the tokens we hold including tokens that are stored as fees.

https://github.com/code-423n4/2023-05-maia/blob/main/src/talos/libraries/PoolActions.sol#L98-L99

This can lead to a situation where tokenX.balanceOf(address(this)) < protocolFeesX

TalosStrategyVanilla.sol is especially vulnerable because we subtract fees amount in the _compoundFees function, which can cause underflow. Thus all user transactions (deposit, redeem) will revert.

https://github.com/code-423n4/2023-05-maia/blob/main/src/talos/TalosStrategyVanilla.sol#L130-L131

Also both TalosStrategyVanilla.sol and TalosStrategyStaked are affected in the same way where the owner cannot collect earned fees from the protocol due to their loss after rerange/rebalance.

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

Proof of Concept

Test case in StrategyVanillaTest.t.sol

    function testBreakFeesAccountingWithRerange()
        public
    {
        uint256 amount0Desired = 1e18;
        console2.log("TEST_BREAK");
        token0.mint(user1, 1e18);
        token1.mint(user1, 1e18);
        (uint256 totalSharesUser1,,) = deposit(amount0Desired, amount0Desired, user1);
        deposit(amount0Desired, amount0Desired, user2);

        TalosStrategyVanilla secondTalosStrategyVanilla = new TalosStrategyVanilla(
            pool,
            strategyOptimizer,
            nonfungiblePositionManager,
            address(this),
            address(this)
        );
        initTalosStrategy(secondTalosStrategyVanilla);

        _deposit(amount0Desired, amount0Desired, user1, secondTalosStrategyVanilla);
        _deposit(amount0Desired, amount0Desired, user2, secondTalosStrategyVanilla);

        ISwapRouter.ExactInputSingleParams memory params0 =
            ISwapRouter.ExactInputSingleParams({
            tokenIn: address(token0),
            tokenOut: address(token1),
            fee: pool.fee(),
            recipient: user1,
            deadline: block.timestamp,
            amountIn: 1e15,
            amountOutMinimum: 0,
            sqrtPriceLimitX96: 0
        });

        ISwapRouter.ExactInputSingleParams memory params1 =
            ISwapRouter.ExactInputSingleParams({
            tokenIn: address(token1),
            tokenOut: address(token0),
            fee: pool.fee(),
            recipient: user1,
            deadline: block.timestamp,
            amountIn: 1e15,
            amountOutMinimum: 0,
            sqrtPriceLimitX96: 0
        });

        hevm.startPrank(user1);
        console2.log("\nSWAPS\n");
        // Do a few swaps to earn some fees
        token0.approve(address(swapRouter), type(uint256).max);
        token1.approve(address(swapRouter), type(uint256).max);
        swapRouter.exactInputSingle(params0);
        swapRouter.exactInputSingle(params1);

        hevm.stopPrank();
        console2.log("\nREBALANCE\n");
        talosBaseStrategy.rebalance();
        console2.log("\nTRY_DEPOSIT_OR_REDEEM\n");
        // This functions will fail
        deposit(amount0Desired, amount0Desired, user1);
        //withdraw(totalSharesUser1, user1);
    }

Here we can see that after collecting fees from the swap operations and position rebalancing deposit and redeem will be unavailable because of underflow.

Logs

REBALANCE                                                                                                                                   
                                                                                                                                            
  BEFORERERANGE BAL0: 0 FEES0: 0                                                                                                             
  BEFORERERANGE BAL1: 0 FEES1: 0                                                                                                             

  AFTERWITHDRAW BAL0: 1999503000000128338 FEES0: 299999999999
  AFTERWITHDRAW BAL1: 1059256884072339381 FEES1: 299999999999
  
  AFTERCALLBACK BAL0: 1999503703228476177 FEES0: 299999999999
  AFTERCALLBACK BAL1: 1059256531392140369 FEES1: 299999999999

  RERANGE AMOUNT0 DESIRED: 1999503703228476177 AMOUNT1 DESIRED: 1059256531392140369
  RERANGE AMOUNT0 ACTUAL: 1999502298767962064 AMOUNT1 ACTUAL: 1059256531392140369

  AFTERRERANGE BAL0: 1404460514113 FEES0: 299999999999
  AFTERRERANGE BAL1: 0 FEES1: 299999999999 //!!!

Owner is unable to collect fees from TalosStrategyStaked, case for StrategyStakedTest.t.sol

    function testBreakFeesAccountingWithRerange()
        public
    {
        uint256 amount0Desired = 1e18;
        // make ERC721 hook fail to collect fees directly from pool
        boostAggregator.removeWhitelistedAddress(address(talosBaseStrategy));
        console2.log("TEST_BREAK");
        (uint256 totalSharesUser1,,) = deposit(amount0Desired, amount0Desired, user1);
        deposit(amount0Desired, amount0Desired, user2);

        TalosStrategyStaked secondTalosStrategyStaked = new TalosStrategyStaked(
            pool,
            strategyOptimizer,
            boostAggregator,
            address(this),
            flywheel,
            address(this)
        );

        initTalosStrategy(secondTalosStrategyStaked);

        _deposit(amount0Desired, amount0Desired, user1, secondTalosStrategyStaked);
        _deposit(amount0Desired, amount0Desired, user2, secondTalosStrategyStaked);

        console2.log("\nREBALANCE\n");
        // earn some fees
        poolDisbalancer(30);
        talosBaseStrategy.rebalance();

        console2.log("\nTRY_COLLECT_FEES\n");
        uint256 fees0 = talosBaseStrategy.protocolFees0();
        uint256 fees1 = talosBaseStrategy.protocolFees1();
        console2.log("FEES: ", fees0, fees1);
        // This will fail
        talosBaseStrategy.collectProtocolFees(fees0, fees1);
    }

Tools Used

Foundry

Recommended Mitigation Steps

Subtract acquired fees in getThisPositionTicks

 function getThisPositionTicks(
        IUniswapV3Pool pool,
        ERC20 token0,
        ERC20 token1,
        int24 baseThreshold,
        int24 tickSpacing,
        uint256 _protocolFees0,
        uint256 _protocolFees1
    ) private view returns (uint256 balance0, uint256 balance1, int24 tickLower, int24 tickUpper) {
        // Emit snapshot to record balances
        balance0 = token0.balanceOf(address(this)) - _protocolFees0;
        balance1 = token1.balanceOf(address(this)) - _protocolFees1;

        //Get exact ticks depending on Optimizer's balances
        (tickLower, tickUpper) = pool.getPositionTicks(balance0, balance1, baseThreshold, tickSpacing);
    }

Assessed type

Under/Overflow

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/main/src/erc-20/ERC20Gauges.sol#L548

Vulnerability details

Impact

When a user transfers or burns his tokens, the algorithm is freeing weights from the gauges that belong to him. This is done inside a for loop that iterates over the list of user's gauges _userGauges[user].values(), if the gauge has a non-zero weight we remove it, unfortunately loop counter is placed inside the if block if (userGaugeWeight != 0) making it impossible to proceed if gauge weight is zero, therefore transfer/burn will fail out of gas.

I'm putting this bug in medium because the only way for this to happen is when the user increments his gauge with a zero value.

Proof of Concept

Test case in ERC20GaugesTest.t.sol

    function testIncrementGauges() public {
        token.mint(address(this), 100e18);
        token.setMaxDelegates(1);
        token.delegate(address(this));

        token.setMaxGauges(2);
        token.addGauge(gauge1);
        token.addGauge(gauge2);

        token.incrementGauge(gauge1, 0);
        token.incrementGauge(gauge2, 100e18);

        address to = hevm.addr(100);
        token.transfer(to, 100e18);
    }

Tools Used

Forge

Recommended Mitigation Steps

Put i++ outside of the if block

        for (uint256 i = 0; i < size && (userFreeWeight + totalFreed) < weight;) {
            address gauge = gaugeList[i];
            uint112 userGaugeWeight = getUserGaugeWeight[user][gauge];
            if (userGaugeWeight != 0) {
                // If the gauge is live (not deprecated), include its weight in the total to remove
                if (!_deprecatedGauges.contains(gauge)) {
                    totalFreed += userGaugeWeight;
                }
                userFreed += userGaugeWeight;
                _decrementGaugeWeight(user, gauge, userGaugeWeight, currentCycle);

            }
            unchecked {
                i++;
            }
        }

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

Usually DEXs add a deadline parameter to their swap functions to protect users whose transaction got stuck on the blockchain and is completed when the price is below/above that what they expected at the time the swap was submitted.

Proof of Concept

Let's say Alice wants to swap 1000 Token0 for 500 Token1, she sends the transaction via our router, but pays a small amount of gas, so her transaction was placed last, by the time it is completed price for Token1 has risen making it 1000 Tokens0 for 400 Token1, which results in the loss of tokens on unfavorable trade.

Tools Used

Manual review

Recommended Mitigation Steps

Add a deadline check

function swap(uint256 amount, uint256 minOutput, Route[] calldata routes, uint256 deadline) external returns (uint256) {
        require(deadline >= block.timestamp, 'Expired')

Assessed type

Timing