Maia DAO Ecosystem - chaduke's results

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/hermes/minters/BaseV2Minter.sol#L162-L167

Vulnerability details

Impact

Detailed description of the impact of this finding. BaseV2Minter.getRewards() will always fail if it is skipped by one week by flywheelGaugeRewards. The main problem is that while getRewards() always assume there is a sufficient balance in BaseV2Minter but such assumption is not true when BaseV2Minter.getRewards() is skipped for one week, thus not all the rewards are claimed. As a result, BaseV2Minter.getRewards() will mint less HERMES for the following week. Then from now one, there is always a deficit, and BaseV2Minter.getRewards() will always fail. Many people will lose such rewards as a result.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

BaseV2Minter.getRewards() allows flywheelGaugeRewards to claim rewards weekly.

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/hermes/minters/BaseV2Minter.sol#L162-L167

However, it the function call is skipped for one week, then some HERMES will be left in the contract of BaseV2Minter. As a result, when BaseV2Minter.updatePeriod() gets called, it will mint less HERMES:

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/hermes/minters/BaseV2Minter.sol#L139-L141

This will create a deficit that can never be filled. The amount of HERMES in the BaseV2Minter contract will always be smaller than the amount in weekly. As a result, BaseV2Minter.getRewards() will always fail from now one.

Our POC code confirms this finding:

1. The first week, 400000120000000000000000 is claimed by flywheelGaugeRewards, and then we call BaseV2Minter.updatePeriod() twice, one for each following week. Since no rewards were claimed in the following two weeks, BaseV2Minter.updatePeriod() will mint less HERMEST then the amount indicated in weekly since it will consider the balance is already sufficient for the new week. However, overall, the amount of HERMES minted is only sufficient for one week, not for two weeks. Therefore, when we call BaseV2Minter.getRewards(), it will always revert. As a matter of fact, it will always fail in the future regardless how many times we call BaseV2Minter.updatePeriod().

function testMissedOneWeek() public
{
    vm.warp(1 weeks);
    _baseV2Minter.updatePeriod();
    vm.prank(address(_flywheelGaugeRewards));
    uint256 rewards = _baseV2Minter.getRewards();
    console2.log("rewards: %d", rewards);
    
    vm.warp(2 weeks);
    _baseV2Minter.updatePeriod();
    
    vm.warp(3 weeks);
    _baseV2Minter.updatePeriod();
    
    // since one week is skipped without claim rewards, now each claim will always faile due to deficit
    vm.prank(address(_flywheelGaugeRewards));
    vm.expectRevert();
    rewards = _baseV2Minter.getRewards();
    console2.log("rewards: %d", rewards);
}

The complete code is here: https://www.dropbox.com/scl/fi/fh9051pjnxp3xgrrmztn3/maia.t.sol?dl=0&rlkey=k3jlrryeyrbrcnfnn9j4ntvl2

Tools Used

VSCode

Recommended Mitigation Steps

Always mint the required HERMES BaseV2Minter.updatePeriod():

function updatePeriod() public returns (uint256) {
        uint256 _period = activePeriod;
        // only trigger if new week
        if (block.timestamp >= _period + week && initializer == address(0)) {
            _period = (block.timestamp / week) * week;
            activePeriod = _period;
            uint256 newWeeklyEmission = weeklyEmission();
            weekly += newWeeklyEmission;
            uint256 _circulatingSupply = circulatingSupply();

            uint256 _growth = calculateGrowth(newWeeklyEmission);
            uint256 _required = _growth + newWeeklyEmission;
            /// @dev share of newWeeklyEmission emissions sent to DAO.
            uint256 share = (_required * daoShare) / base;
            _required += share;
-            uint256 _balanceOf = underlying.balanceOf(address(this));
-            if (_balanceOf < _required) {
-                HERMES(underlying).mint(address(this), _required - _balanceOf);
-            }
+            HERMES(underlying).mint(address(this), _required);

            underlying.safeTransfer(address(vault), _growth);

            if (dao != address(0)) underlying.safeTransfer(dao, share);

            emit Mint(msg.sender, newWeeklyEmission, _circulatingSupply, _growth, share);

            /// @dev queue rewards for the cycle, anyone can call if fails
            ///      queueRewardsForCycle will call this function but won't enter
            ///      here because activePeriod was updated
            try flywheelGaugeRewards.queueRewardsForCycle() {} catch {}
        }
        return _period;
    }

Assessed type

Math

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/talos/base/TalosBaseStrategy.sol#L182-L235

Vulnerability details

Impact

Detailed description of the impact of this finding. TalosBaseStrategy.deposit() lacks slippage control for the users, as a result, a user might loss funds due to price change at Uniswap V3 pools.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

TalosBaseStrategy.deposit() allows a user to submit liquidity to a Uniswap V3 pool and in return the user will receive shares for the TalosBaseStrategy contract:

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/talos/base/TalosBaseStrategy.sol#L182-L235

The amount of return shares is determined in the amount of liquidity, which in turn, is determined by function nonfungiblePositionManager.increaseLiquidity(). Look at the function of increaseLiquidity(), we can see that the amount of liquidity is not only affected by amount0Desired, and amount1Desired, but also the price ration of token0 and token1:

https://github.com/code-423n4/2023-05-maia/blob/main/lib/v3-periphery/contracts/NonfungiblePositionManager.sol

increaseLiquidity() calls addLiquidity():

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/lib/v3-periphery/contracts/base/LiquidityManagement.sol#L51-L92

At L74, the amount of liquidity depends on sqrtPriceX96, the current price of token0 vs token1.

Therefore, the price of Uniswap V3 pool might be manipulated and the deposit() transaction might front run, as a result, the user might get less liquidity, and thus less shares than expected, a loss of funds scenario.

Tools Used

VScode

Recommended Mitigation Steps

There should be a paramter minShares for the deposit() function so that the user can have the slippage control.

Assessed type

Timing

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/uni-v3-staker/UniswapV3Staker.sol#L340-L348

Vulnerability details

Impact

Detailed description of the impact of this finding. UniswapV3Staker.restakeToken() calls _unstakeToke() with isNotRestake flag as true instead of false, as a result, non-owner users CANNNOT call restakeToken even if the block time is after the end time of the incentive.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

UniswapV3Staker.restakeToken() allows a user to restake a token in a new incentive. In particular, a non-owner user can call restakeToken if the block time is after the end time of the incentive.

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/uni-v3-staker/UniswapV3Staker.sol#L340-L348

However, at L342, the flag isNotRestake is set to true instead of false. We need set it to false since we are in the process of Restake.

As a result, non-owner users CANNNOT call restakeToken even if the block time is after the end time of the incentive. To see this, we look at L374 of the _unstakeToken() function.

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/uni-v3-staker/UniswapV3Staker.sol#L365-L459

L374 will always revert for non-owner users since isNotRestake == true even if the block time is after the end time of the incentive. Therefore, the impact is that, non-owner users CANNNOT call restakeToken even if the block time is after the end time of the incentive.

Tools Used

VSCode

Recommended Mitigation Steps

Set isNotRestake to false when calling _unstakeToke() in UniswapV3Staker.restakeToken().

   function restakeToken(uint256 tokenId) external {
        IncentiveKey storage incentiveId = stakedIncentiveKey[tokenId];
-        if (incentiveId.startTime != 0) _unstakeToken(incentiveId, tokenId, true);
+        if (incentiveId.startTime != 0) _unstakeToken(incentiveId, tokenId, false);

        (IUniswapV3Pool pool, int24 tickLower, int24 tickUpper, uint128 liquidity) =
            NFTPositionInfo.getPositionInfo(factory, nonfungiblePositionManager, tokenId);

        _stakeToken(tokenId, pool, tickLower, tickUpper, liquidity);
    }

Assessed type

Error