Maia DAO Ecosystem - bin2chen's results

Lines of code

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

Vulnerability details

Impact

Logic error

Proof of Concept

setWeight()Used to set the new weight The code is as follows:

    function setWeight(uint256 poolId, uint8 weight) external nonReentrant onlyOwner {
        if (weight == 0) revert InvalidWeight();

        uint256 poolIndex = destinations[poolId];

        if (poolIndex == 0) revert NotUlyssesLP();

        uint256 oldRebalancingFee;

        for (uint256 i = 1; i < bandwidthStateList.length; i++) {
            uint256 targetBandwidth = totalSupply.mulDiv(bandwidthStateList[i].weight, totalWeights);

            oldRebalancingFee += _calculateRebalancingFee(bandwidthStateList[i].bandwidth, targetBandwidth, false);
        }

        uint256 oldTotalWeights = totalWeights;
        uint256 weightsWithoutPool = oldTotalWeights - bandwidthStateList[poolIndex].weight;
        uint256 newTotalWeights = weightsWithoutPool + weight;
        totalWeights = newTotalWeights;

        if (totalWeights > MAX_TOTAL_WEIGHT || oldTotalWeights == newTotalWeights) {
            revert InvalidWeight();
        }

        uint256 leftOverBandwidth;

        BandwidthState storage poolState = bandwidthStateList[poolIndex];
        poolState.weight = weight;
@>      if (oldTotalWeights > newTotalWeights) {
            for (uint256 i = 1; i < bandwidthStateList.length;) {
                if (i != poolIndex) {
                    uint256 oldBandwidth = bandwidthStateList[i].bandwidth;
                    if (oldBandwidth > 0) {
                        bandwidthStateList[i].bandwidth =
                            oldBandwidth.mulDivUp(oldTotalWeights, newTotalWeights).toUint248();
                        leftOverBandwidth += oldBandwidth - bandwidthStateList[i].bandwidth;
                    }
                }

                unchecked {
                    ++i;
                }
            }
            poolState.bandwidth += leftOverBandwidth.toUint248();
        } else {
            uint256 oldBandwidth = poolState.bandwidth;
            if (oldBandwidth > 0) {
@>              poolState.bandwidth = oldBandwidth.mulDivUp(oldTotalWeights, newTotalWeights).toUint248();

                leftOverBandwidth += oldBandwidth - poolState.bandwidth;
            }

            for (uint256 i = 1; i < bandwidthStateList.length;) {
              

                if (i != poolIndex) {
                     if (i == bandwidthStateList.length - 1) {
@>                       bandwidthStateList[i].bandwidth += leftOverBandwidth.toUint248();
                     } else if (leftOverBandwidth > 0) {
@>                       bandwidthStateList[i].bandwidth +=
@>                          leftOverBandwidth.mulDiv(bandwidthStateList[i].weight, weightsWithoutPool).toUint248();
                     }
                }

                unchecked {
                    ++i;
                }
            }
        }

There are several problems with the above code

1. if (oldTotalWeights > newTotalWeights) should be changed to if (oldTotalWeights < newTotalWeights) because the logic inside the if is to calculate the case of increasing weight

2. poolState.bandwidth = oldBandwidth.mulDivUp(oldTotalWeights , newTotalWeights).toUint248(); should be modified to poolState.bandwidth = oldBandwidth.mulDivUp(newTotalWeights, oldTotalWeights).toUint248(); Because this calculates the extra number

3. leftOverBandwidth has a problem with the processing logic

Tools Used

Recommended Mitigation Steps

    function setWeight(uint256 poolId, uint8 weight) external nonReentrant onlyOwner {
...

-        if (oldTotalWeights > newTotalWeights) {
+        if (oldTotalWeights < newTotalWeights) {
            for (uint256 i = 1; i < bandwidthStateList.length;) {
                if (i != poolIndex) {
                    uint256 oldBandwidth = bandwidthStateList[i].bandwidth;
                    if (oldBandwidth > 0) {
                        bandwidthStateList[i].bandwidth =
                            oldBandwidth.mulDivUp(oldTotalWeights, newTotalWeights).toUint248();
                        leftOverBandwidth += oldBandwidth - bandwidthStateList[i].bandwidth;
                    }
                }

                unchecked {
                    ++i;
                }
            }
            poolState.bandwidth += leftOverBandwidth.toUint248();
        } else {
            uint256 oldBandwidth = poolState.bandwidth;
            if (oldBandwidth > 0) {
-               poolState.bandwidth = oldBandwidth.mulDivUp(oldTotalWeights, newTotalWeights).toUint248();
+               poolState.bandwidth = oldBandwidth.mulDivUp(newTotalWeights, oldTotalWeights).toUint248();

                leftOverBandwidth += oldBandwidth - poolState.bandwidth;
            }

+           uint256 currentGiveWidth = 0;
+           uint256 currentGiveCount = 0;
            for (uint256 i = 1; i < bandwidthStateList.length;) {

+                if (i != poolIndex) {
+                     if(currentGiveCount == bandwidthStateList.length - 2 - 1) { //last
+                         bandwidthStateList[i].bandwidth += leftOverBandwidth - currentGiveWidth;
+                    }
+                     uint256 sharesWidth = leftOverBandwidth.mulDiv(bandwidthStateList[i].weight, weightsWithoutPool).toUint248();
+                     bandwidthStateList[i].bandwidth += sharesWidth;
+                     currentGiveWidth +=sharesWidth;  
+                     currentCount++;
+                 }                

-                if (i != poolIndex) {
-                    if (i == bandwidthStateList.length - 1) {
-                        bandwidthStateList[i].bandwidth += leftOverBandwidth.toUint248();
-                    } else if (leftOverBandwidth > 0) {
-                        bandwidthStateList[i].bandwidth +=
-                            leftOverBandwidth.mulDiv(bandwidthStateList[i].weight, weightsWithoutPool).toUint248();
-                    }
-               }

                unchecked {
                    ++i;
                }
            }
        }
...

Assessed type

Context

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/BranchBridgeAgent.sol#L1341

Vulnerability details

Impact

Wrong decimal place conversion, resulting in wrong quantity

Proof of Concept

in callOutSignedAndBridge() The number of tokens will be converted to 18 decimal when packedData is performed.

    function callOutSignedAndBridge(bytes calldata _params, DepositInput memory _dParams, uint128 _remoteExecutionGas)
        external
        payable
        lock
        requiresFallbackGas
    {
        //Encode Data for cross-chain call.
        bytes memory packedData = abi.encodePacked(
            bytes1(0x05),
            msg.sender,
            depositNonce,
            _dParams.hToken,
            _dParams.token,
            _dParams.amount,
@>          _normalizeDecimals(_dParams.deposit, ERC20(_dParams.token).decimals()),
            _dParams.toChain,
            _params,
            msg.value.toUint128(),
            _remoteExecutionGas
        );

But the method of converting to 18 decimal is wrong

    /**
     * @notice Internal function that normalizes an input to 18 decimal places.
     * @param _amount amount of tokens
     * @param _decimals number of decimal places
     */
    function _normalizeDecimals(uint256 _amount, uint8 _decimals) internal pure returns (uint256) {
@>      return _decimals == 18 ? _amount : _amount * (10 ** _decimals) / 1 ether;
    }

The correct should be _amount * 1 ether / (10 ** _decimals)

Many places have problems using this method

Note: BranchPort._denormalizeDecimals() Have similar questions

Tools Used

Recommended Mitigation Steps

    /**
     * @notice Internal function that normalizes an input to 18 decimal places.
     * @param _amount amount of tokens
     * @param _decimals number of decimal places
     */
    function _normalizeDecimals(uint256 _amount, uint8 _decimals) internal pure returns (uint256) {
-       return _decimals == 18 ? _amount : _amount * (10 ** _decimals) / 1 ether;
+       return _decimals == 18 ? _amount : _amount * 1 ether / (10 ** _decimals);
    }

Assessed type

Context

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/cfed0dfa3bebdac0993b1b42239b4944eb0b196c/src/ulysses-omnichain/BranchBridgeAgent.sol#L866

Vulnerability details

Impact

Wrong decimal place conversion, resulting in wrong quantity

Proof of Concept

in _createDepositSingle() will call IPort(localPortAddress).bridgeOut() The parameter _deposit is not converted to 18 decimal

_createDepositSingle()

    function _createDepositSingle(
        address _user,
        address _hToken,
        address _token,
        uint256 _amount,
        uint256 _deposit,
        uint128 _gasToBridgeOut
    ) internal {
        //Deposit / Lock Tokens into Port
@>      IPort(localPortAddress).bridgeOut(_user, _hToken, _token, _amount, _deposit);

        //Deposit Gas to Port
        _depositGas(_gasToBridgeOut);
...

but in IPort(localPortAddress).bridgeOut() method ，will do _denormalizeDecimals on deposit from 18 decimal to token.decimals

    function bridgeOut(
        address _depositor,
        address _localAddress,
        address _underlyingAddress,
        uint256 _amount,
        uint256 _deposit
    ) external virtual requiresBridgeAgent {
        if (_amount - _deposit > 0) {
            _localAddress.safeTransferFrom(_depositor, address(this), _amount - _deposit);
            ERC20hTokenBranch(_localAddress).burn(_amount - _deposit);
        }
        if (_deposit > 0) {
            _underlyingAddress.safeTransferFrom(
@>              _depositor, address(this), _denormalizeDecimals(_deposit, ERC20(_underlyingAddress).decimals())
            );
        }
    }

So we need to convert deposit to 18 decimal before calling bridgeOut

Note: _createDepositMultiple have similar questions

Tools Used

Recommended Mitigation Steps

    function _createDepositSingle(
        address _user,
        address _hToken,
        address _token,
        uint256 _amount,
        uint256 _deposit,
        uint128 _gasToBridgeOut
    ) internal {
        //Deposit / Lock Tokens into Port
-      IPort(localPortAddress).bridgeOut(_user, _hToken, _token, _amount, _deposit);        
+      IPort(localPortAddress).bridgeOut(_user, _hToken, _token, _amount, _normalizeDecimals(_deposit));

        //Deposit Gas to Port
        _depositGas(_gasToBridgeOut);
...

Assessed type

Context

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/talos/boost-aggregator/BoostAggregator.sol#L159-L161

Vulnerability details

Impact

claimReward() will take all rewards if the amountRequested passed in is 0, which may result in user's rewards lost

Proof of Concept

In BoostAggregator.withdrawProtocolFees(), the owner can take the protocol rewards protocolRewards The code is as follows:

    function withdrawProtocolFees(address to) external onlyOwner {
        uniswapV3Staker.claimReward(to, protocolRewards);
@>      delete protocolRewards;
    }

From the above code we can see that uniswapV3Staker is called to fetch and then clear protocolRewards

Let's look at the implementation of uniswapV3Staker.claimReward().

contract UniswapV3Staker is IUniswapV3Staker, Multicallable {
....
    function claimReward(address to, uint256 amountRequested) external returns (uint256 reward) {
        reward = rewards[msg.sender];
@>      if (amountRequested != 0 && amountRequested < reward) {
            reward = amountRequested;
            rewards[msg.sender] -= reward;
        } else {
            rewards[msg.sender] = 0;
        }

        if (reward > 0) hermes.safeTransfer(to, reward);

        emit RewardClaimed(to, reward);
    }

The current implementation is that if the amountRequested==0 passed in means that all rewards[msg.sender] of this msg.sender are taken

This leads to problems.

1. If a malicious owner calls withdrawProtocolFees() twice in a row, it will definitely take all the rewards of the BoostAggregator.
2. probably didn't realize that withdrawProtocolFees() was called when protocolRewards==0

As a result, the rewards that belong to users in BoostAggregator are lost

Tools Used

Recommended Mitigation Steps

Modify claimReward() to remove amountRequested != 0

contract UniswapV3Staker is IUniswapV3Staker, Multicallable {
....
    function claimReward(address to, uint256 amountRequested) external returns (uint256 reward) {
        reward = rewards[msg.sender];
-       if (amountRequested != 0 && amountRequested < reward) {        
+       if (amountRequested < reward) {
            reward = amountRequested;
            rewards[msg.sender] -= reward;
        } else {
            rewards[msg.sender] = 0;
        }

        if (reward > 0) hermes.safeTransfer(to, reward);

        emit RewardClaimed(to, reward);
    }

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

Wrong owner parameter, causing users to lose rewards

Proof of Concept

In TalosStrategyStaked.sol If the user's shares have changed, we need to do flywheel.accrue() first, which will accrue rewards and update the corresponding userIndex. This way we can ensure the accuracy of rewards. So we will call flywheel.accrue() beforeDeposit/beforeRedeem/transfer etc.

Take redeem() as an example, the code is as follows:

contract TalosStrategyStaked is TalosStrategySimple, ITalosStrategyStaked {
...

    function beforeRedeem(uint256 _tokenId, address _owner) internal override {
        _earnFees(_tokenId);
@>      flywheel.accrue(_owner);
    }

But when beforeRedeem() is called with the wrong owner passed in, the redeem() code is as follows:

    function redeem(uint256 shares, uint256 amount0Min, uint256 amount1Min, address receiver, address _owner)
        public
        virtual
        override
        nonReentrant
        checkDeviation
        returns (uint256 amount0, uint256 amount1)
    {
...
        if (msg.sender != _owner) {
            uint256 allowed = allowance[_owner][msg.sender]; // Saves gas for limited approvals.

            if (allowed != type(uint256).max) allowance[_owner][msg.sender] = allowed - shares;
        }

        if (shares == 0) revert RedeemingZeroShares();
        if (receiver == address(0)) revert ReceiverIsZeroAddress();

        uint256 _tokenId = tokenId;
@>      beforeRedeem(_tokenId, receiver);

        INonfungiblePositionManager _nonfungiblePositionManager = nonfungiblePositionManager; // Saves an extra SLOAD
        {
            uint128 liquidityToDecrease = uint128((liquidity * shares) / totalSupply);

            (amount0, amount1) = _nonfungiblePositionManager.decreaseLiquidity(
                INonfungiblePositionManager.DecreaseLiquidityParams({
                    tokenId: _tokenId,
                    liquidity: liquidityToDecrease,
                    amount0Min: amount0Min,
                    amount1Min: amount1Min,
                    deadline: block.timestamp
                })
            );

            if (amount0 == 0 && amount1 == 0) revert AmountsAreZero();

@>          _burn(_owner, shares);

            liquidity -= liquidityToDecrease;
        }    

From the above code, we see that the parameter is receiver, but the person whose shares are burn is _owner.

We need to accrue _owner, not receiver

This leads to a direct reduction of the user's shares without accrue, and the user loses the corresponding rewards

Tools Used

Recommended Mitigation Steps

    function redeem(uint256 shares, uint256 amount0Min, uint256 amount1Min, address receiver, address _owner)
        public
        virtual
        override
        nonReentrant
        checkDeviation
        returns (uint256 amount0, uint256 amount1)
    {
        if (msg.sender != _owner) {
            uint256 allowed = allowance[_owner][msg.sender]; // Saves gas for limited approvals.

            if (allowed != type(uint256).max) allowance[_owner][msg.sender] = allowed - shares;
        }

        if (shares == 0) revert RedeemingZeroShares();
        if (receiver == address(0)) revert ReceiverIsZeroAddress();

        uint256 _tokenId = tokenId;
-       beforeRedeem(_tokenId, receiver);
+       beforeRedeem(_tokenId, _owner);

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

in removeAsset(), Incorrectly set the assetIndex, resulting in subsequent remove fail,Even remove the wrong asset

Proof of Concept

removeAsset() The code is as follows:

    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;

        if (newAssetsLength == 0) revert CannotRemoveLastAsset();

        totalWeights -= weights[assetIndex];

        address lastAsset = assets[newAssetsLength];
@>      assetId[lastAsset] = assetIndex;
        assets[assetIndex] = lastAsset;
        weights[assetIndex] = weights[newAssetsLength];

        assets.pop();
        weights.pop();
        assetId[asset] = 0;

        emit AssetRemoved(asset);

        updateAssetBalances();

        asset.safeTransfer(msg.sender, asset.balanceOf(address(this)));
    }

When setting assetId[lastAsset] use assetId[lastAsset] = assetIndex

However, the current protocol need use index + 1 for assetId The correct value should be assetId[lastAsset] = assetIndex + 1.

For example, suppose Current: assets = [A,B] assetId[A] = 1 assetId[B] = 2

1. After executing removeAsset(A) it becomes

assets = [B] assetId[B] = 0 (error , correct is 1)

2. If execute addAsset(C), it will become assets = [B,C] assetId[B] = 0 assetId[C] = 2

3. then execute remove(B), it will underflow

    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;

The above example demonstrates the failure to remove Due to the wrong assetIndex is also possible to cause the removal the wrong asset

Tools Used

Recommended Mitigation Steps

    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;

        if (newAssetsLength == 0) revert CannotRemoveLastAsset();

        totalWeights -= weights[assetIndex];

        address lastAsset = assets[newAssetsLength];
-       assetId[lastAsset] = assetIndex;
+       assetId[lastAsset] = assetIndex + 1;


....
    

Assessed type

Decimal

Lines of code

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

Vulnerability details

Impact

missing the first transfer of the asset to router, addLiquidity() unable to work

Proof of Concept

UlyssesRouter.addLiquidityuse for mint LP The code is as follows:

    function addLiquidity(uint256 amount, uint256 minOutput, uint256 poolId) external returns (uint256) {

        UlyssesPool ulysses = getUlyssesLP(poolId);

        amount = ulysses.deposit(amount, msg.sender);

        if (amount < minOutput) revert OutputTooLow();
        return amount;
    }

The above code, missing the first transfer of the asset to router e.g.: ulysses.asset().safeTransferFrom(msg.sender, address(this), amount);

Cannot work

Tools Used

Recommended Mitigation Steps

    function addLiquidity(uint256 amount, uint256 minOutput, uint256 poolId) external returns (uint256) {

        UlyssesPool ulysses = getUlyssesLP(poolId);

+       ulysses.asset().safeTransferFrom(msg.sender, address(this), amount);

        amount = ulysses.deposit(amount, msg.sender);

        if (amount < minOutput) revert OutputTooLow();
        return amount;
    }

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

rerange/rebalance occupied the protocolFees that are temporarily stored in the contract and may cause permanent loss

Proof of Concept

When rerange() is executed, there are two important steps as follows

1. Execute _withdrawAll(_tokenId) to retrieve all the tokens from the LP and put them into the contract

    function _withdrawAll(uint256 _tokenId) internal {
        uint128 _liquidity = liquidity; // Saves an extra SLOAD if totalSupply is non-zero.
        if (_liquidity == 0) return;
        INonfungiblePositionManager _nonfungiblePositionManager = nonfungiblePositionManager;
@>      _nonfungiblePositionManager.decreaseLiquidity(
            INonfungiblePositionManager.DecreaseLiquidityParams({
                tokenId: _tokenId,
                liquidity: _liquidity,
                amount0Min: 0,
                amount1Min: 0,
                deadline: block.timestamp
            })
        );
        liquidity = 0;
        _nonfungiblePositionManager.collect(
            INonfungiblePositionManager.CollectParams({
                tokenId: _tokenId,
@>              recipient: address(this),
                amount0Max: type(uint128).max,
                amount1Max: type(uint128).max
            })
        );
    }

2.Execute rerange() to use all the balance in the contract to deposit to LP again

TalosStrategySimple.doRerange() -> PoolActions.rerange() -> getThisPositionTicks:

    function rerange(
        INonfungiblePositionManager nonfungiblePositionManager,
        ActionParams memory actionParams,
        uint24 poolFee
    )
        internal
        returns (int24 tickLower, int24 tickUpper, uint256 amount0, uint256 amount1, uint256 tokenId, uint128 liquidity)
    {
        int24 baseThreshold = actionParams.tickSpacing * actionParams.optimizer.tickRangeMultiplier();

        uint256 balance0;
        uint256 balance1;
@>      (balance0, balance1, tickLower, tickUpper) = getThisPositionTicks(
            actionParams.pool, actionParams.token0, actionParams.token1, baseThreshold, actionParams.tickSpacing
        );
        emit Snapshot(balance0, balance1);

        (tokenId, liquidity, amount0, amount1) = nonfungiblePositionManager.mint(
            INonfungiblePositionManager.MintParams({
                token0: address(actionParams.token0),
                token1: address(actionParams.token1),
                fee: poolFee,
                tickLower: tickLower,
                tickUpper: tickUpper,
                amount0Desired: balance0,
                amount1Desired: balance1,
                amount0Min: 0,
                amount1Min: 0,
                recipient: address(this),
                deadline: block.timestamp
            })
        );
    }


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

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

In this case, one thing is missing. protocolFees0/_protocolFees1 is temporarily stored in the contract until it is taken away

So when rerange(), it will use up the protocolFees that belong to the protocol.

Problems caused

1. occupied protocolFee may be lost permanently 2.TalosStrategyVanilla may _compoundFees() underflow when calculating protocol fees, resulting in failure to deposit/redeem.

contract TalosStrategyVanilla is TalosStrategySimple {
...
    function _compoundFees(uint256 _tokenId) internal returns (uint256 amount0, uint256 amount1) {
@>      uint256 balance0 = token0.balanceOf(address(this)) - protocolFees0;
@>      uint256 balance1 = token1.balanceOf(address(this)) - protocolFees1;

        emit Snapshot(balance0, balance1);

Tools Used

Recommended Mitigation Steps

Trigger collectProtocolFees() before rerange/rebalance to fetch ProtocolFees

Assessed type

Context

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/maia/tokens/ERC4626PartnerManager.sol#L189

Vulnerability details

Impact

In the migratePartnerVault() method, if vaultId == 0 means illegal address, but the id of the vaults starts from 0, resulting in the first vault being mistaken as an illegal vault address

Proof of Concept

In the migratePartnerVault() method, it will determine whether newPartnerVault is legal or not, by vaultId!=0 of vault

The code is as follows:

    function migratePartnerVault(address newPartnerVault) external onlyOwner {
@>      if (factory.vaultIds(IBaseVault(newPartnerVault)) == 0) revert UnrecognizedVault();

        address oldPartnerVault = partnerVault;
        if (oldPartnerVault != address(0)) IBaseVault(oldPartnerVault).clearAll();
        bHermesToken.claimOutstanding();

But when factory adds vault, the index starts from 0, so the id of the first vault is 0

PartnerManagerFactory.addVault()

contract PartnerManagerFactory is Ownable, IPartnerManagerFactory {
    constructor(ERC20 _bHermes, address _owner) {
        _initializeOwner(_owner);
        bHermes = _bHermes;
        partners.push(PartnerManager(address(0)));
    }

    function addVault(IBaseVault newVault) external onlyOwner {
@>      uint256 id = vaults.length;
        vaults.push(newVault);
        vaultIds[newVault] == id;

        emit AddedVault(newVault, id);
    }

The id of the first vault starts from 0, because in constructor does not add address(0) to the vaults similar to partners

So migratePartnerVault() can't be processed for the first vault

Tools Used

Recommended Mitigation Steps

Similar to partners, in the constructor method, a vault with address(0) is added by default.

contract PartnerManagerFactory is Ownable, IPartnerManagerFactory {
    constructor(ERC20 _bHermes, address _owner) {
        _initializeOwner(_owner);
        bHermes = _bHermes;
        partners.push(PartnerManager(address(0)));
+       vaults.push(IBaseVault(address(0)));
    }

Assessed type

Context

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/maia/vMaia.sol#L91

Vulnerability details

Impact

Lack of override forfeitBoost , when need forfeit will underflow

Proof of Concept

In vMaia, override the claimBoost() code to be empty to avoid fail

The code and comments are as follows.

    /// @dev Boost can't be claimed; does not fail. It is all used by the partner vault.
    function claimBoost(uint256 amount) public override {}

But it does not override the corresponding forfeitBoost() This will still reduce userClaimedBoost when forfeit() is needed, resulting in underflow

UtilityManager.forfeitBoost()

    function forfeitBoost(uint256 amount) public virtual {
        if (amount == 0) return;
@>      userClaimedBoost[msg.sender] -= amount;
        address(gaugeBoost).safeTransferFrom(msg.sender, address(this), amount);

        emit ForfeitBoost(msg.sender, amount);
    }

So you should also override forfeitBoost() and turn it into an empty code to avoid failure when you need to use forfeit

Tools Used

Recommended Mitigation Steps

contract vMaia is ERC4626PartnerManager {

+   /// @dev Boost can't be forfeit; does not fail.
+   function forfeitBoost(uint256 amount) public override {}

...

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

If there is a weekly that has not been taken, it may result in insufficient mint HERMES

Proof of Concept

In updatePeriod(), mint new HERMES every week with a certain percentage weeklyEmission.

The code is as follows.

    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);
            }

            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;
    }

The above code will first determine if the balance of the current contract is less than _required and if it is less, then mint new HERMES, so that there will be enough HERMES for the distribution.

But there is a problem that the current balance of contract may contain the last weekly HERMES,that flywheelGaugeRewards have not yet been taken: (e.g. last week's allocation of weeklyEmission)

Because the gaugeCycle of flywheelGaugeRewards may be greater than one week. So it is possible that the last weekly HERMES has not yet been taken.

So we can't use the current balance to compare with _required directly, we need to Consider the weekly staying in the contract, it hasn't been taken, to avoid not having enough balance when flywheelGaugeRewards comes to take weekly.

Tools Used

Recommended Mitigation Steps

    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) {
+           if (_balanceOf < weekly + _growth + share ) {
-              HERMES(underlying).mint(address(this), _required - _balanceOf);
+              HERMES(underlying).mint(address(this),weekly + _growth + share - _balanceOf);
            }

            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

Context

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/erc-20/ERC20Gauges.sol#L547-L549

Vulnerability details

Impact

the position of i++ is wrong, which may lead to an infinite loop

Proof of Concept

In the loop of the _decrementWeightUntilFree() method, the position of i++ is wrong, which may lead to a infinite loop

    function _decrementWeightUntilFree(address user, uint256 weight) internal nonReentrant {
...
        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++;
                }
            }
        }

In the above code, when userGaugeWeight == 0, i is not incremented, resulting in a infinite loop. The current protocol does not restrict getUserGaugeWeight[user][gauge] == 0.

Tools Used

Recommended Mitigation Steps

    function _decrementWeightUntilFree(address user, uint256 weight) internal nonReentrant {
...
        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++;
-               }
            }
+           unchecked {
+             i++;
+           }            
        }

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

in restakeToken(), wrong use isNotRestake = true, even if the time end, only owner can restake()

Proof of Concept

anyone can call restakeToken if the block time is after the end time of the incentive

    function _unstakeToken(IncentiveKey memory key, uint256 tokenId, bool isNotRestake) private {
        Deposit storage deposit = deposits[tokenId];

        (uint96 endTime, uint256 stakedDuration) =
            IncentiveTime.getEndAndDuration(key.startTime, deposit.stakedTimestamp, block.timestamp);

        address owner = deposit.owner;

        // anyone can call restakeToken if the block time is after the end time of the incentive
@>      if ((isNotRestake || block.timestamp < endTime) && owner != msg.sender) revert NotCalledByOwner();

        {

But restakeToken() is passed the wrong isNotRestake=true, so even if the time end, only owner can restake()

Error passing isNotRestake==true, should pass isNotRestake=false

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

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

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

Tools Used

Recommended Mitigation Steps

    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

Context

Lines of code

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

Vulnerability details

Impact

Wrong judgment condition, resulting in extra weight release, resulting in the user losing the corresponding reward

Proof of Concept

When a user makes a transfer of ERC20Gauges, if the user does not have enough freeWeight will first call _decrementWeightUntilFree() to reduce the weights already assigned to gauges until there is enough freeWeight for the transfer

The _decrementWeightUntilFree() code is as follows:

    function _decrementWeightUntilFree(address user, uint256 weight) internal nonReentrant {
...

        uint112 userFreed;
        uint112 totalFreed;

        // Loop through all user gauges, live and deprecated
        address[] memory gaugeList = _userGauges[user].values();

        // Free gauges through the entire list or until underweight
        uint256 size = gaugeList.length;
@>      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++;
                }
            }
        }    

In the above code, there is a mistake in the loop condition: (userFreeWeight + totalFreed) < weight.

This expression uses totalFreed, which is the variable used for the reduction of _totalWeight (it don't contains userGaugeWeight of _deprecatedGauges ).

But now we are trying to free the user's weight the correct variable to use is userFreed, which is the total weight be free for the user. It contains the userGaugeWeight of _deprecatedGauges, which is also a legitimate weight. So it would be more accurate to use userFreed instead of totalFreed to avoid free too much weight.

Tools Used

Recommended Mitigation Steps

    function _decrementWeightUntilFree(address user, uint256 weight) internal nonReentrant {
...

        uint112 userFreed;
        uint112 totalFreed;

        // Loop through all user gauges, live and deprecated
        address[] memory gaugeList = _userGauges[user].values();

        // Free gauges through the entire list or until underweight
        uint256 size = gaugeList.length;
-       for (uint256 i = 0; i < size && (userFreeWeight + totalFreed) < weight;) {
+       for (uint256 i = 0; i < size && (userFreeWeight + userFreed) < 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

Context

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/RootPort.sol#L406-L410

Vulnerability details

Impact

addBridgeAgentFactory() Does not work properly

Proof of Concept

addBridgeAgentFactory() The implementation code is as follows：

contract RootPort is Ownable, IRootPort {
...
    address[] public bridgeAgentFactories;

    function addBridgeAgentFactory(address _bridgeAgentFactory) external onlyOwner {
@>      bridgeAgentFactories[bridgeAgentsLenght++] = _bridgeAgentFactory;

        emit BridgeAgentFactoryAdded(_bridgeAgentFactory);
    }

The current implementation is that the owner cannot be newly added because it will Index out of bounds.

we need to use bridgeAgentFactories.push(), and set isBridgeAgentFactory[_bridgeAgentFactory] = true;

Tools Used

Recommended Mitigation Steps

contract RootPort is Ownable, IRootPort {
...
    address[] public bridgeAgentFactories;

    function addBridgeAgentFactory(address _bridgeAgentFactory) external onlyOwner {
-       bridgeAgentFactories[bridgeAgentsLenght++] = _bridgeAgentFactory;
+       require(!isBridgeAgentFactory[_bridgeAgentFactory],"exists");
+       isBridgeAgentFactory[_bridgeAgentFactory] = true;
+       bridgeAgentFactories.push(_bridgeAgentFactory);
+       bridgeAgentFactoriesLenght++;


        emit BridgeAgentFactoryAdded(_bridgeAgentFactory);
    }

Assessed type

Context

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/gauges/factories/BribesFactory.sol#L79-L98

Vulnerability details

Impact

Malicious transfer 1 wei rewardToken to address(0), will cause BribesFactory.createBribeFlywheel() to fail forever

Proof of Concept

BribesFactory.createBribeFlywheel() use for create Flywheel The code is as follows:

    function createBribeFlywheel(address bribeToken) public {
        if (address(flywheelTokens[bribeToken]) != address(0)) revert BribeFlywheelAlreadyExists();

        FlywheelCore flywheel = new FlywheelCore(
            bribeToken,
@>          FlywheelBribeRewards(address(0)),
            flywheelGaugeWeightBooster,
            address(this)
        );

        flywheelTokens[bribeToken] = flywheel;

        uint256 id = bribeFlywheels.length;
        bribeFlywheels.push(flywheel);
        bribeFlywheelIds[flywheel] = id;
        activeBribeFlywheels[flywheel] = true;

@>      flywheel.setFlywheelRewards(address(new FlywheelBribeRewards(flywheel, rewardsCycleLength)));

        emit BribeFlywheelCreated(bribeToken, flywheel);
    }

From the above code, we can see that

1. when new FlywheelCore() , it use FlywheelBribeRewards == address(0)
2. then call setFlywheelRewards() to set new FlywheelRewards: flywheel.setFlywheelRewards(address(new FlywheelBribeRewards(flywheel, rewardsCycleLength)));

Let's look at the implementation of setFlywheelRewards():

    function setFlywheelRewards(address newFlywheelRewards) external onlyOwner {
@>      uint256 oldRewardBalance = rewardToken.balanceOf(address(flywheelRewards));
        if (oldRewardBalance > 0) {
@>          rewardToken.safeTransferFrom(address(flywheelRewards), address(newFlywheelRewards), oldRewardBalance);
        }
        flywheelRewards = newFlywheelRewards;

        emit FlywheelRewardsUpdate(address(newFlywheelRewards));
    }