Maia DAO Ecosystem - T1MOH's results

Lines of code

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

Vulnerability details

Impact

Modifier checkDeviation is implemented to (qouting) "mitigate price manipulation during rebalance and also prevents placing orders when it's too volatile". However initial deposit through 'init()' can be done on volatile market. I find it illogical, suppose protocol team want to save users from doing so.

Proof of Concept

Note about checkDeviation: https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/talos/base/TalosBaseStrategy.sol#L421-L427

    /// @notice Function modifier that checks if price has not moved a lot recently.
    /// This mitigates price manipulation during rebalance and also prevents placing orders when it's too volatile.
    modifier checkDeviation() {
        ITalosOptimizer _optimizer = optimizer;
        pool.checkDeviation(_optimizer.maxTwapDeviation(), _optimizer.twapDuration());
        _;
    }

Modifier checkDeviation is implemented on all function except init(): https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/talos/base/TalosBaseStrategy.sol#L102-L106

    function init(uint256 amount0Desired, uint256 amount1Desired, address receiver)
        external
        virtual
        nonReentrant
        returns (uint256 shares, uint256 amount0, uint256 amount1)
    {

Tools Used

Manual Review

Recommended Mitigation Steps

Consider adding this modifier to function init()

Assessed type

Oracle

Lines of code

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

Vulnerability details

Impact

Usually router in AMM is stateless, i.e. it isn't supposed to contain any tokens, it is just wrapper of low-level pool functions to perform user-friendly interaction. Current implementation of addLiquidity() assumes that user firstly transfers tokens to router and then router performs deposit to pool. However it is not atomic and requires two transactions. Another user can break in after the first transaction and deposit someone else's tokens.

Proof of Concept

Router calls deposit with msg.sender as receiver of shares: https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-amm/UlyssesRouter.sol#L49-L56

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

In deposit pool transfers tokens from msg.sender, which is router: https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/erc-4626/UlyssesERC4626.sol#L34-L45

    function deposit(uint256 assets, address receiver) public virtual nonReentrant returns (uint256 shares) {
        // Need to transfer before minting or ERC777s could reenter.
        asset.safeTransferFrom(msg.sender, address(this), assets);

        shares = beforeDeposit(assets);

        require(shares != 0, "ZERO_SHARES");

        _mint(receiver, shares);

        emit Deposit(msg.sender, receiver, assets, shares);
    }

First user will lose tokens sent to router, if malicious user calls addLiquidity() after it

Tools Used

Manual Review

Recommended Mitigation Steps

Transfer tokens to router via safeTransferFrom():

    function addLiquidity(uint256 amount, uint256 minOutput, uint256 poolId) external returns (uint256) {
        UlyssesPool ulysses = getUlyssesLP(poolId);
        address(ulysses.asset()).safeTransferFrom(msg.sender, address(this), amount);

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

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

Assessed type

Access Control

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/talos/libraries/PoolActions.sol#L55-L88 https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/talos/libraries/PoolActions.sol#L90-L103

Vulnerability details

Impact

Account of protocol fee is broken, because tokens of protocolFee0 and protocolFee1 are used while rerange/rebalance to add liquidity. At the same time this variables protocolFee0 and protocolFee1 are not updated, and de facto contract doesn't have protocol fee on balance

Proof of Concept

Function rerange is used both in rerange and in rebalance: https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/talos/strategies/TalosStrategySimple.sol#L30-L46

    function doRerange() internal override returns (uint256 amount0, uint256 amount1) {
        (tickLower, tickUpper, amount0, amount1, tokenId, liquidity) = nonfungiblePositionManager.rerange(
            PoolActions.ActionParams(pool, optimizer, token0, token1, tickSpacing), poolFee
        );
    }

    function doRebalance() internal override returns (uint256 amount0, uint256 amount1) {
        int24 baseThreshold = tickSpacing * optimizer.tickRangeMultiplier();

        PoolActions.ActionParams memory actionParams =
            PoolActions.ActionParams(pool, optimizer, token0, token1, tickSpacing);

        PoolActions.swapToEqualAmounts(actionParams, baseThreshold);

        (tickLower, tickUpper, amount0, amount1, tokenId, liquidity) =
            nonfungiblePositionManager.rerange(actionParams, poolFee);
    }

Let's have a look at this function. This function calls getThisPositionTicks to get the amounts balance0 and balance1 of tokens to addLiquidity: https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/talos/libraries/PoolActions.sol#L56-L88

    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),
                amount0Desired: balance0,
                amount1Desired: balance1,
                ...
            })
        );
    }

Mistake is in function getThisPositionTicks() because it returns actual token balance of Strategy contract: https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/talos/libraries/PoolActions.sol#L90-L103

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

Returns actual balance which consists of 2 parts: protocolFee and users' funds. Rerange must use users' funds, but not protocolFee.

Suppose following scenario:

1. User added 1000 tokens of liquidity
2. This liquidity generated 100 tokens of fee, 50 of which is protocolFee
3. Rerange is called. After removing liquidity contract has 1000 + 100 tokens balance. And contract add liquidity of whole balance - 1100 tokens
4. Function collectFee doesn't work because actual balance is less than withdrawing amount. And protocol loses profit: https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/talos/base/TalosBaseStrategy.sol#L394-L415

    function collectProtocolFees(uint256 amount0, uint256 amount1) external nonReentrant onlyOwner {
        uint256 _protocolFees0 = protocolFees0;
        uint256 _protocolFees1 = protocolFees1;

        if (amount0 > _protocolFees0) {
            revert Token0AmountIsBiggerThanProtocolFees();
        }
        if (amount1 > _protocolFees1) {
            revert Token1AmountIsBiggerThanProtocolFees();
        }
        ERC20 _token0 = token0;
        ERC20 _token1 = token1;
        uint256 balance0 = _token0.balanceOf(address(this));
        uint256 balance1 = _token1.balanceOf(address(this));
        require(balance0 >= amount0 && balance1 >= amount1);
        if (amount0 > 0) _token0.transfer(msg.sender, amount0);
        if (amount1 > 0) _token1.transfer(msg.sender, amount1);

        protocolFees0 = _protocolFees0 - amount0;
        protocolFees1 = _protocolFees1 - amount1;
        emit RewardPaid(msg.sender, amount0, amount1);
    }

Tools Used

Manual Review

Recommended Mitigation Steps

I suggest using different address for protocolFee. Transfer all protocolFee tokens away from this contract to not mix it with users' assets. Create a contract like "ProtocolFeeReceiver.sol" and make force transfer of tokens when Strategy gets fee

Also want to note that in forked parent project SorbettoFragola it is implemented via burnExactLiquidity https://github.com/Popsicle-Finance/SorbettoFragola/blob/9fb31b74f19005d86a78abc758553e7914e7ba49/SorbettoFragola.sol#L458-L483

Assessed type

Math

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/main/src/talos/base/TalosBaseStrategy.sol#L144-L145 https://github.com/code-423n4/2023-05-maia/blob/main/src/talos/base/TalosBaseStrategy.sol#L206-L207 https://github.com/code-423n4/2023-05-maia/blob/main/src/talos/base/TalosBaseStrategy.sol#L357-L358 https://github.com/code-423n4/2023-05-maia/blob/main/src/talos/TalosStrategyVanilla.sol#L146-L147 https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/talos/libraries/PoolActions.sol#L82-L83

Vulnerability details

Impact

There is no slippage protection on any of the calls to increase or decrease liquidity, allowing for trades to be subject to MEV-style attacks such as front-running and sandwiching.

Proof of Concept

amount0Min and amount1Min are hardcoded to zero, allowing infinite slippage, which results in loss of funds for liquidity providers. It was also reported by Zellic in paragraph 3.9, but was fixed only in redeem() function

Tools Used

Manual Review

Recommended Mitigation Steps

Add slippage control

Assessed type

Uniswap

Lines of code

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

Vulnerability details

Impact

Malicious user can intentionally not call unstake() or restake and thus his stake will exist in this incentive. endIncentive() reverts when there are unstaked tokens and therefore unclaimed hermes tokens can't be transferred away to minter.

Proof of Concept

Function _unstakeToken() has argument isNotRestake to allow other users to restake nft when endTime of stake is reached. It is to remove all stakes from ended incentive to call endIncentive() and transfer hermes tokens to minter. Because it will revert when there are stakers: https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/uni-v3-staker/UniswapV3Staker.sol#L199

    function endIncentive(IncentiveKey memory key) external returns (uint256 refund) {
        ...
        if (incentive.numberOfStakes > 0) revert EndIncentiveWhileStakesArePresent();
        ...
    }

However function restake sets argument isNotRestake to true, blocking this functionality: https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/uni-v3-staker/UniswapV3Staker.sol#L340-L348

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

Here you can see that function will revert if called from not owner after endTime with isNotRestake = true: https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/uni-v3-staker/UniswapV3Staker.sol#L365

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

Tools Used

Manual Review

Recommended Mitigation Steps

Refactor

    function restakeToken(uint256 tokenId) external {
        IncentiveKey storage incentiveId = stakedIncentiveKey[tokenId];
        //@audit must pass `false` instead of `true`
-        if (incentiveId.startTime != 0) _unstakeToken(incentiveId, tokenId, true);
+        if (incentiveId.startTime != 0) _unstakeToken(incentiveId, tokenId, false);

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/talos/base/TalosBaseStrategy.sol#L269 https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/talos/libraries/PoolActions.sol#L85 https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/talos/TalosStrategyVanilla.sol#L148 https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/talos/base/TalosBaseStrategy.sol#L147 https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/talos/base/TalosBaseStrategy.sol#L208 https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/talos/base/TalosBaseStrategy.sol#L269 https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/talos/base/TalosBaseStrategy.sol#L359

Vulnerability details

Impact

The transaction can be pending in mempool for a long time and can be executed in a long time after the user submit the transaction. This makes the slippage check insufficient, because price can move up and defined tokenMinAmount now is too low for current price, allowing big slippage.

Proof of Concept

In links to affects code I specified all lines in Talos without deadline protection. Despite the fact that there is also no slippage control, there is slippage control in redeem function: https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/talos/base/TalosBaseStrategy.sol#L263-L271

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

This occurence makes issue valid

Tools Used

Manual Review

Recommended Mitigation Steps

Add expiration timestamp check. Check Uniswap Router implemetation. Add deadline as function argument and additionally check it:

modifier ensure(uint deadline) {
	require(deadline >= block.timestamp, 'UniswapV2Router: EXPIRED');
	_;
}

Assessed type

Uniswap

Lines of code

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

Vulnerability details

Impact

addLiquidity(), removeLiquidity(), swap() don't have deadline parameter. It means that transaction can be pending in mempool so long, such that minOutput is too low comparing to current price at execution, and therefore high slippage action is performed.

Proof of Concept

Suppose current scenario:

1. User wants to deposit 1000 tokens. token : share are traded as 1 : 1.
2. User accepts slippage of 1%, therefore specifies minOutput = 1000 * 1 * (100% - 1%) = 990
3. Transaction is pending long time in mempool. token/price share changed a lot, now 1 token is equal to 2 shares.
4. Fair price with slippage 1% should be minOutput = 1000 * 2 * (100% - 1%) = 1980, but user specified 990 and can perform bad trade because of slippage

Tools Used

Manual Review

Recommended Mitigation Steps

Add deadline argument and check it, like UniswapV2Router does:

    function addLiquidityETH(
        address token,
        uint amountTokenDesired,
        uint amountTokenMin,
        uint amountETHMin,
        address to,
        uint deadline
    ) external virtual override payable ensure(deadline) returns (uint amountToken, uint amountETH, uint liquidity) {
        ...
    }

    modifier ensure(uint deadline) {
        require(deadline >= block.timestamp, 'UniswapV2Router: EXPIRED');
        _;
    }

Assessed type

MEV

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/main/src/governance/GovernorBravoDelegateMaia.sol#L17-L27

Vulnerability details

Impact

Actual time of Voting Delay and Voting Period is 20% less than described in docs and comments.

Proof of Concept

Here you can see, that code assumes 15 seconds per block

    /// @notice The minimum setable voting period
    uint256 public constant MIN_VOTING_PERIOD = 80640; // About 2 weeks

    /// @notice The max setable voting period
    uint256 public constant MAX_VOTING_PERIOD = 161280; // About 4 weeks

    /// @notice The min setable voting delay
    uint256 public constant MIN_VOTING_DELAY = 40320; // About 1 weeks

    /// @notice The max setable voting delay
    uint256 public constant MAX_VOTING_DELAY = 80640; // About 2 weeks

And for example in propose this value are used:

function propose(
        address[] memory targets,
        uint256[] memory values,
        string[] memory signatures,
        bytes[] memory calldatas,
        string memory description
    ) public returns (uint256) {
        ...

        uint256 startBlock = add256(block.number, votingDelay);
        uint256 endBlock = add256(startBlock, votingPeriod);

        ...
    }

However block.number in Arbitrum contract will return value of L1 block.number when Sequencer received the transaction On Ethereum block time is 12 seconds, which breaks the initial assumption. And therefore duration is 20% less than intended

Tools Used

Manual Review

Recommended Mitigation Steps

Change values to:

    /// @notice The minimum setable voting period
    uint256 public constant MIN_VOTING_PERIOD = 100800; // About 2 weeks

    /// @notice The max setable voting period
    uint256 public constant MAX_VOTING_PERIOD = 201600; // About 4 weeks

    /// @notice The min setable voting delay
    uint256 public constant MIN_VOTING_DELAY = 50400; // About 1 weeks

    /// @notice The max setable voting delay
    uint256 public constant MAX_VOTING_DELAY = 100800; // About 2 weeks

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

There is unnecessary condition for claiming: pendingRewards > DIVISIONER, otherwise claimRewards won't be called. It makes users to lose his reward if requirement not met

Proof of Concept

It claims rewards only if condition met. But there is no sense in this condition. I think it was implemented prior to rounding, but there are no problems with it:

    function unstakeAndWithdraw(uint256 tokenId) external {
        address user = tokenIdToUser[tokenId];
        if (user != msg.sender) revert NotTokenIdOwner();

        // unstake NFT from Uniswap V3 Staker
        uniswapV3Staker.unstakeToken(tokenId);

        uint256 pendingRewards = uniswapV3Staker.tokenIdRewards(tokenId) - tokenIdRewards[tokenId];

        if (pendingRewards > DIVISIONER) {
            uint256 newProtocolRewards = (pendingRewards * protocolFee) / DIVISIONER;
            /// @dev protocol rewards stay in stake contract
            protocolRewards += newProtocolRewards;
            pendingRewards -= newProtocolRewards;

            address rewardsDepot = userToRewardsDepot[user];
            if (rewardsDepot != address(0)) {
                // claim rewards to user's rewardsDepot
                uniswapV3Staker.claimReward(rewardsDepot, pendingRewards);
            } else {
                // claim rewards to user
                uniswapV3Staker.claimReward(user, pendingRewards);
            }
        }

        // withdraw rewards from Uniswap V3 Staker
        uniswapV3Staker.withdrawToken(tokenId, user, "");
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Remove if statement, claim rewards always

Assessed type

Math

Lines of code

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

Vulnerability details

Impact

ERC4626PartnerManager mints more tokens than needed when bHermesRate increased.

1. I suppose it can break voting in which this token is used. Because totalSupply is increased, more and more tokens are stuck in contract after every increasing of bHermesRate.
2. The second concern is that all this tokens are approved to partnerVault contract and can be extracted. But implementation of partnerVault is out of scope and I don't know is it possible. But this token excess exists in ERC4626PartnerManager.sol

Proof of Concept

Token amount to mint is difference between totalSupply * newRate and balance of this contract.

    function increaseConversionRate(uint256 newRate) external onlyOwner {
        if (newRate < bHermesRate) revert InvalidRate();

        if (newRate > (address(bHermesToken).balanceOf(address(this)) / totalSupply)) {
            revert InsufficientBacking();
        }

        bHermesRate = newRate;

        partnerGovernance.mint(
            address(this), totalSupply * newRate - address(partnerGovernance).balanceOf(address(this))
        );
        bHermesToken.claimOutstanding();
    }

However it is wrong to account balance of address(this) because it decreases every claim. Let me explain.

Suppose bHermesRate = 10, balance of bHermes is 50, totalSupply is 0 (nobody interacted yet)

1. User1 deposits 5 MAIA, therefore mints 5 vMAIA and mints 5 * 10 = 50 govToken

    function _mint(address to, uint256 amount) internal virtual override {
        if (amount > maxMint(to)) revert ExceedsMaxDeposit();
        bHermesToken.claimOutstanding();

        ERC20MultiVotes(partnerGovernance).mint(address(this), amount * bHermesRate);
        super._mint(to, amount);
    }

2. Admin calls increaseConversionRate(11), ie increases rate by 1. This function will mint 5 * 11 - 0 = 55 tokens, but should mint only 5 * (11 - 10) = 5

        partnerGovernance.mint(
            address(this), totalSupply * newRate - address(partnerGovernance).balanceOf(address(this))
        );

Tools Used

Manual Review

Recommended Mitigation Steps

Refactor function to

    function increaseConversionRate(uint256 newRate) external onlyOwner {
        if (newRate < bHermesRate) revert InvalidRate();

        if (newRate > (address(bHermesToken).balanceOf(address(this)) / totalSupply)) {
            revert InsufficientBacking();
        }

        partnerGovernance.mint(
            address(this), totalSupply * (newRate - bHermesRate)
        );

        bHermesRate = newRate;

        bHermesToken.claimOutstanding();
    }

Assessed type

ERC20

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/governance/GovernorBravoDelegateMaia.sol#L87-L93

Vulnerability details

Impact

As people mint bHermes, bHermesVotes' totalSupply grows. And quorumVotesAmount to execute proposal also grows. But it shouldn't, because new people can't vote for it. This behavior adds inconsistency to voting process, because changes threshold after creating proposal.

Proof of Concept

Here you can see that Governance fetches current totalSupply: https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/governance/GovernorBravoDelegateMaia.sol#L87-L93

    function getProposalThresholdAmount() public view returns (uint256) {
        return govToken.totalSupply() * proposalThreshold / DIVISIONER;
    }

    function getQuorumVotesAmount() public view returns (uint256) {
        return govToken.totalSupply() * quorumVotes / DIVISIONER;
    }

bHermes is ERC4626DepositOnly and mints new govToken when user calls deposit() or mint(), thus increasing totalSupply: https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/hermes/bHermes.sol#L123-L133

    function _mint(address to, uint256 amount) internal virtual override {
        gaugeWeight.mint(address(this), amount);
        gaugeBoost.mint(address(this), amount);
        governance.mint(address(this), amount);
        super._mint(to, amount);
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Add parameter totalSupply to Proposal struct and use it instead of current totalSupply in functions getProposalThresholdAmount() and getQuorumVotesAmount()

Assessed type

Governance

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/erc-4626/UlyssesERC4626.sol#L96-L106

Vulnerability details

Impact

According to EIP4626 previewDeposit(), previewRedeem(), previewMint() must include fee in returned value:

1. previewDeposit "MUST be inclusive of deposit fees. Integrators should be aware of the existence of deposit fees."
2. previewRedeem "MUST be inclusive of withdrawal fees. Integrators should be aware of the existence of withdrawal fees."
3. previewMint "MUST be inclusive of deposit fees. Integrators should be aware of the existence of deposit fees."

Proof of Concept

UlyssesPool.sol inherits UlyssesERC4626.sol with default implementation: https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/erc-4626/UlyssesERC4626.sol#L96-L106

    function previewDeposit(uint256 assets) public view virtual returns (uint256) {
        return assets;
    }

    function previewMint(uint256 shares) public view virtual returns (uint256) {
        return shares;
    }

    function previewRedeem(uint256 shares) public view virtual returns (uint256) {
        return shares;
    }

However deposit, redeem, mint in UlyssesPool.sol take fees: https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-amm/UlyssesPool.sol#L1200-L1221

    function beforeDeposit(uint256 assets) internal override returns (uint256 shares) {
        // Update deposit/mint
        shares = ulyssesAddLP(assets, true);
    }

    /**
     * @notice Performs the necessary steps to make after depositing.
     * @param assets to be deposited
     */
    function beforeMint(uint256 shares) internal override returns (uint256 assets) {
        // Update deposit/mint
        assets = ulyssesAddLP(shares, false);
    }

    /**
     * @notice Performs the necessary steps to take before withdrawing assets
     * @param shares to be burned
     */
    function afterRedeem(uint256 shares) internal override returns (uint256 assets) {
        // Update withdraw/redeem
        assets = ulyssesRemoveLP(shares);
    }

Further you can check that functions ulyssesAddLP() and ulyssesRemoveLP() take fee. I consider it overabundant in this submission

Tools Used

Manual Review

Recommended Mitigation Steps

Override preview functions in UlyssesPool.sol to include fee

Assessed type

ERC4626

Lines of code

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

Vulnerability details

Impact

Talos protocol can't be deployed in a right way.

Proof of Concept

TalosBaseStrategy needs TalosManager to be passed in constructor: https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/talos/base/TalosBaseStrategy.sol#L79-L95

    constructor(
        IUniswapV3Pool _pool,
        ITalosOptimizer _optimizer,
        INonfungiblePositionManager _nonfungiblePositionManager,
        address _strategyManager,
        address _owner
    ) ERC20("TALOS LP", "TLP", 18) {
        ...

        strategyManager = _strategyManager;
        
        ...
    }

But TalosManager needs Strategy to be passed in constructor: https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/talos/TalosManager.sol#L44-L56

   constructor(
        address _strategy,
        int24 _ticksFromLowerRebalance,
        int24 _ticksFromUpperRebalance,
        int24 _ticksFromLowerRerange,
        int24 _ticksFromUpperRerange
    ) {
        strategy = ITalosBaseStrategy(_strategy);
        
        ...
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Add setters for complete deploy, or initializing function

Assessed type

DoS