Maia DAO Ecosystem - minhquanym's results

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/uni-v3-staker/libraries/RewardMath.sol#L29

Vulnerability details

Impact

UniswapV3Staker depends on the second per liquidity inside values from the Uniswap V3 Pool to calculate the amount of reward a position should receive. This value represents the amount of second liquidity inside a tick range that is "active" (tickLower < currentTick < tickUpper). The second per liquidity inside a specific tick range is supposed to always increase over time.

In the RewardMath library, the seconds inside are calculated by taking the current timestamp value and subtracting the value at the moment the position is staked. Since this value increases over time, it should be normal. Additionally, this implementation is similar to Uniswap Team's implementation.

function computeBoostedSecondsInsideX128(
    uint256 stakedDuration,
    uint128 liquidity,
    uint128 boostAmount,
    uint128 boostTotalSupply,
    uint160 secondsPerLiquidityInsideInitialX128,
    uint160 secondsPerLiquidityInsideX128
) internal pure returns (uint160 boostedSecondsInsideX128) {
    // this operation is safe, as the difference cannot be greater than 1/stake.liquidity
    uint160 secondsInsideX128 = (secondsPerLiquidityInsideX128 - secondsPerLiquidityInsideInitialX128) * liquidity;
    // @audit secondPerLiquidityInsideX128 could smaller than secondsPerLiquidityInsideInitialX128
    ...
}

However, even though the second per liquidity inside value increases over time, it could overflow uint256, resulting in the calculation reverting. When computeBoostedSecondsInsideX128() reverts, function _unstake() will also revert, locking the LP position in the contract forever.

Proof of Concept

Consider the value of the second per liquidity in three different timestamps: t1 < t2 < t3

secondPerLiquidity_t1 = -10 = 2**256-10
secondPerLiquidity_t2 = 100
secondPerLiquidity_t3 = 300

As we can see, its value always increases over time, but the initial value could be smaller than 0. When calculating computeBoostedSecondsInsideX128() for a period from t1 -> t2, it will revert.

Additionally, as I mentioned earlier, this implementation is similar to the one from Uniswap team. However, please note that Uniswap team used Solidity 0.7, which won't revert on overflow and the formula works as expected while Maia uses Solidity 0.8.

For more information on how a tick is initialized, please refer to this code

if (liquidityGrossBefore == 0) {
    // by convention, we assume that all growth before a tick was initialized happened _below_ the tick
    if (tick <= tickCurrent) {
        info.feeGrowthOutside0X128 = feeGrowthGlobal0X128;
        info.feeGrowthOutside1X128 = feeGrowthGlobal1X128;
        info.secondsPerLiquidityOutsideX128 = secondsPerLiquidityCumulativeX128;
        info.tickCumulativeOutside = tickCumulative;
        info.secondsOutside = time;
    }
    info.initialized = true;
}

The second per liquidity inside a range that has tickLower < currentTick < tickUpper is calculated as:

secondsPerLiquidityCumulativeX128 - tickLower.secondsPerLiquidityOutsideX128 - tickUpper.secondsPerLiquidityOutsideX128

// If lower tick is just init,
// Then: secondsPerLiquidityCumulativeX128 = tickLower.secondsPerLiquidityOutsideX128
// And: tickUpper.secondsPerLiquidityOutsideX128 != 0
// => Result will be overflow

Tools Used

Manual Review

Recommended Mitigation Steps

Consider using unchecked block to calculate this value.

Assessed type

Under/Overflow

struct SwapCallbackData {
    bool zeroForOne;
}

function uniswapV3SwapCallback(int256 amount0, int256 amount1, bytes calldata _data) external {
    require(msg.sender == address(pool), "FP");
    require(amount0 > 0 || amount1 > 0, "LEZ"); // swaps entirely within 0-liquidity regions are not supported
    SwapCallbackData memory data = abi.decode(_data, (SwapCallbackData));
    bool zeroForOne = data.zeroForOne;

    if (zeroForOne) {
        token0.mint(address(this), uint256(amount0));
        token0.transfer(msg.sender, uint256(amount0));
    }
    else {
        token1.mint(address(this), uint256(amount1));
        token1.transfer(msg.sender, uint256(amount1));
    }
}

// Test minting a position and transferring it to Uniswap V3 Staker, after creating a gauge
function testAudit1() public {
    // Create a Uniswap V3 pool
    (pool, poolContract) =
        UniswapV3Assistant.createPool(uniswapV3Factory, address(token0), address(token1), poolFee);

    // Initialize 1:1 0.3% fee pool
    UniswapV3Assistant.initializeBalanced(poolContract);
    hevm.warp(block.timestamp + 100);

    // 3338502497096994491500 to give 1 ether per token with 0.3% fee and -60,60 ticks
    uint256 _tokenId0 = newNFT(-180, 180, 3338502497096994491500);
    uint256 _tokenId1 = newNFT(-60, 60, 3338502497096994491500);

    hevm.warp(block.timestamp + 100);

    // @audit Step 1: Swap to make currentTick go to (60, 180) range
    uint256 amountSpecified = 30 ether;
    bool zeroForOne = false;
    pool.swap(
        address(this),
        zeroForOne,
        int256(amountSpecified),
        1461446703485210103287273052203988822378723970342 - 1, // MAX_SQRT_RATIO - 1
        abi.encode(SwapCallbackData({zeroForOne: zeroForOne}))
    );
    (, int24 _currentTick, , , , ,) = pool.slot0();
    console2.logInt(int256(_currentTick));

    hevm.warp(block.timestamp + 100);

    // @audit Step 2: Swap back to make currentTick go back to (-60, 60) range
    zeroForOne = true;
    pool.swap(
        address(this),
        zeroForOne,
        int256(amountSpecified),
        4295128739 + 1, // MIN_SQRT_RATIO + 1 
        abi.encode(SwapCallbackData({zeroForOne: zeroForOne}))
    );

    (, _currentTick, , , , ,) = pool.slot0();
    console2.logInt(int256(_currentTick));

    hevm.warp(block.timestamp + 100);

    // @audit Step 3: Create normal Incentive
    uint256 minWidth = 10;
    // Create a gauge
    gauge = createGaugeAndAddToGaugeBoost(pool, minWidth);
    // Create a Uniswap V3 Staker incentive
    key = IUniswapV3Staker.IncentiveKey({pool: pool, startTime: IncentiveTime.computeEnd(block.timestamp)});

    uint256 rewardAmount = 1000 ether;
    rewardToken.mint(address(this), rewardAmount);
    rewardToken.approve(address(uniswapV3Staker), rewardAmount);

    createIncentive(key, rewardAmount);

    // @audit Step 4: Now we have secondsPerLiquidity of tick 60 is not equal to 0. 
    //        We just need to create a position with range [-120, 60], 
    //        then secondsPerLiquidityInside of this position will be overflow
    hevm.warp(key.startTime + 1);
    int24 tickLower = -120;
    int24 tickUpper = 60;
    uint256 tokenId = newNFT(tickLower, tickUpper, 3338502497096994491500);
    (, uint160 secondsPerLiquidityInsideX128,) = pool.snapshotCumulativesInside(tickLower, tickUpper);
    console2.logUint(uint256(secondsPerLiquidityInsideX128));

    // @audit Step 5: Stake the position
    // Transfer and stake the position in Uniswap V3 Staker
    nonfungiblePositionManager.safeTransferFrom(address(this), address(uniswapV3Staker), tokenId);
    (address owner,,, uint256 stakedTimestamp) = uniswapV3Staker.deposits(tokenId);

    // @audit Step 6: Increase time to make `secondsPerLiquidity` go from negative to positive value
    //        Then `unstakeToken` will revert
    hevm.warp(block.timestamp + 5 weeks);

    (, secondsPerLiquidityInsideX128,) = pool.snapshotCumulativesInside(tickLower, tickUpper);
    console2.logUint(uint256(secondsPerLiquidityInsideX128));

    uniswapV3Staker.unstakeToken(tokenId);
}

Lines of code

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

Vulnerability details

Impact

In the UlyssesToken contract, each asset has an ID stored in the assetId mapping, and all assets are stored in the assets[] list. If asset A is stored at index i in the list, its ID will be i + 1, as seen in the addAsset() function.

function addAsset(address asset, uint256 _weight) external nonReentrant onlyOwner {
    ...
    assetId[asset] = assets.length + 1;
    ...
}

In the removeAsset() function, the ID of the last asset in the list is set incorrectly when it is swapped with the asset being removed. As a result, two assets may have the same ID, and removing one of them could remove the other instead, locking the asset permanently.

function removeAsset(address asset) external nonReentrant onlyOwner {
    // No need to check if index is 0, it will underflow and revert if it is 0
    uint256 assetIndex = assetId[asset] - 1;

    uint256 newAssetsLength = assets.length - 1;
    ...

    address lastAsset = assets[newAssetsLength];
    // @audit it should be assetIndex + 1 instead
    assetId[lastAsset] = assetIndex;
    assets[assetIndex] = lastAsset;
    ...
}

Proof of Concept

Continuing the scenario in the impact section, after the first removal, there will be two assets with the same assetId.

1. Let's call them asset A and B. They have assetId[A] = assetId[B] = 5 and assets[4] = B.
2. The owner calls removeAsset(A), and assetIndex = 5 - 1 = 4. It will try to remove assets[4] in the list. As a result, asset B will be removed, and all asset A tokens will be transferred out instead of those of asset B, which means asset B is now locked in the contract.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider fixing the issue by setting the assetId[lastAsset] = assetIndex + 1.

Assessed type

Other