Maia DAO Ecosystem - kutugu's results

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/RootBridgeAgent.sol#L674-L681 https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/RootBridgeAgent.sol#L714-L725

Vulnerability details

Impact

_gasSwapIn and _gasSwapOut using the wrong price. There are a couple of questions here:

• TWAP prices can be manipulated, the calculated price is not credible
• The best swap price is to start with the minimum, not the current price

Proof of Concept

First for TWAP price manipulation, here is a good explanation. The proposer can manipulate the TWAP price by manipulating two consecutive blocks, and then use sandwiches, JIT and other means to arbitrage. By the way, since there's no slippage protection, a lot of arbitrage space.
Second, for the swap price, it should be set to zeroForOneOnInflow ? TickMath.MIN_SQRT_RATIO + 1 : TickMath.MAX_SQRT_RATIO - 1, not the current slot0 price. This results in missing out on many low price ranges and losing funds.

Tools Used

Manual review

Recommended Mitigation Steps

Set the price to zeroForOneOnInflow ? TickMath.MIN_SQRT_RATIO + 1 : TickMath.MAX_SQRT_RATIO - 1

Assessed type

Uniswap

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/BranchBridgeAgent.sol#L1340-L1342 https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/BranchPort.sol#L388-L390

Vulnerability details

Impact

According to the documentation, Ulysses Omnichain accounting supports tokens with any decimals and converts them to 18 decimals.
However, its normalization and denormalizeDecimals formula are incorrect and incompatible with low-precision tokens.

Proof of Concept

    function _normalizeDecimals(uint256 _amount, uint8 _decimals) internal pure returns (uint256) {
        return _decimals == 18 ? _amount : _amount * (10 ** _decimals) / 1 ether;
    }

For USDT, the decimal is 6, and amounts less than 1e12 are all 0 after _normalizeDecimals, which obviously prevents most user from interacting with protocol. The same for _denormalizeDecimals.

Tools Used

Manual review

Recommended Mitigation Steps

Use 36 precision scaling

    function _normalizeDecimals(uint256 _amount, uint8 _decimals) internal pure returns (uint256) {
        return _amount * 10 ** (36 - _decimals);
    }

Assessed type

Decimal

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/rewards/rewards/FlywheelAcummulatedRewards.sol#L46-L54

Vulnerability details

Impact

FlywheelAcummulatedRewards/FlywheelBribeRewards gains are instantaneous and can be frontrun.
The user only needs to frontrun the delegate before each incentive is distributed to get the incentive, and there is no way to prevent the user from undelegating after receiving the reward.
The protocal is to mint votes using staking bHermes. For example, the user can staking bHermes to get votes, delegate to receive the incentive, undelegate, and then take out bHermes to do other earnings, and wait until a week later to re-delegate.

Proof of Concept

function testUndelegateAfterAccrue() external {
        MockERC20 token = new MockERC20("test token", "TKN", 18);
        FlywheelCore flywheel = createFlywheel(token);
        gaugeToken.setMaxDelegates(2);

        hermes.mint(address(this), 100e18);
        hermes.approve(address(gaugeToken), 100e18);
        gaugeToken.mint(address(this), 100e18);
        gaugeToken.delegate(address(this));
        gaugeToken.incrementGauge(address(gauge), 100e18);

        address other = address(0xdead);
        hevm.startPrank(other);
        hermes.mint(other, 100e18);
        hermes.approve(address(gaugeToken), 100e18);
        gaugeToken.mint(other, 100e18);
        gaugeToken.delegate(other);
        gaugeToken.incrementGauge(address(gauge), 100e18);
        hevm.stopPrank();

        // addBribeFlywheel
        gauge.addBribeFlywheel(flywheel);
        token.mint(address(depot), 100 ether);

        // claimRewards
        gauge.accrueBribes(address(this));
        flywheel.claimRewards(address(this));
        gauge.accrueBribes(other);
        flywheel.claimRewards(other);
        assert(token.balanceOf(address(this)) == token.balanceOf(other));
        assert(token.balanceOf(address(this)) == 50 ether);

        // Undo delegate
        gaugeToken.decrementGauge(address(gauge), 100e18);
        gaugeToken.undelegate(address(this), 100e18);
        gaugeToken.burn(address(this), 100e18);

        hevm.warp(WEEK - 3 days);

        // frontrun to delegate
        gaugeToken.mint(address(this), 100e18);
        gaugeToken.delegate(address(this));
        gaugeToken.incrementGauge(address(gauge), 100e18);

        // update bribeFlywheel
        token.mint(address(depot), 100 ether);

        // Cycle 1
        hevm.warp(WEEK);

        // claimRewards
        gauge.accrueBribes(address(this));
        flywheel.claimRewards(address(this));
        gauge.accrueBribes(other);
        flywheel.claimRewards(other);
        assert(token.balanceOf(address(this)) == token.balanceOf(other));
        assert(token.balanceOf(address(this)) == 100 ether);

        // Undo delegate
        gaugeToken.decrementGauge(address(gauge), 100e18);
        gaugeToken.undelegate(address(this), 100e18);
        gaugeToken.burn(address(this), 100e18);

        hevm.warp(WEEK * 2 - 3 days);

        // frontrun to delegate
        gaugeToken.mint(address(this), 100e18);
        gaugeToken.delegate(address(this));
        gaugeToken.incrementGauge(address(gauge), 100e18);

        // update bribeFlywheel
        token.mint(address(depot), 100 ether);

        // Cycle 2
        hevm.warp(WEEK * 2);

        // claimRewards
        gauge.accrueBribes(address(this));
        flywheel.claimRewards(address(this));
        gauge.accrueBribes(other);
        flywheel.claimRewards(other);
        assert(token.balanceOf(address(this)) == token.balanceOf(other));
        assert(token.balanceOf(address(this)) == 150 ether);
    }

Add the above test to BaseV2GaugeTest.t.sol, and you will find that the gains from the frontrun and the gains from the continuous delegate are equal.

Tools Used

Foundry

Recommended Mitigation Steps

Assign incentives by delegate duration instead of cycle

Assessed type

Context

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/RootBridgeAgent.sol#L1219-L1222 https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/RootBridgeAgent.sol#L848-L852

Vulnerability details

Impact

DepositGasAnycallConfig can deposit the gas fee externally, but here should not withdraw the nativeToken. This prevents gas from being deposited.

Proof of Concept

There are two ways to store gas in RootBridgeAgent:

// deposit GAS
function _manageGasOut(uint24 _toChain) internal returns (uint128) {
    uint256 amountOut;
    address gasToken;
    uint256 _initialGas = initialGas;

    if (_toChain == localChainId) {
        //Transfer gasToBridgeOut Local Branch Bridge Agent if remote initiated call.
        if (_initialGas > 0) {
            address(wrappedNativeToken).safeTransfer(getBranchBridgeAgent[localChainId], userFeeInfo.gasToBridgeOut);
        }

        return uint128(userFeeInfo.gasToBridgeOut);
    }

    if (_initialGas > 0) {
        if (userFeeInfo.gasToBridgeOut <= MIN_FALLBACK_RESERVE * tx.gasprice) revert InsufficientGasForFees();
        (amountOut, gasToken) = _gasSwapOut(userFeeInfo.gasToBridgeOut, _toChain);
    } else {
        if (msg.value <= MIN_FALLBACK_RESERVE * tx.gasprice) revert InsufficientGasForFees();
        wrappedNativeToken.deposit{value: msg.value}();
        (amountOut, gasToken) = _gasSwapOut(msg.value, _toChain);
    }

    IPort(localPortAddress).burn(address(this), gasToken, amountOut, _toChain);
    return amountOut.toUint128();
}


// pay GAS
if (localAnyCallExecutorAddress == msg.sender) {
    //Save initial gas
    initialGas = _initialGas;
}
//Zero out gas after use if remote call
if (initialGas > 0) {
    _payExecutionGas(userFeeInfo.depositedGas, userFeeInfo.gasToBridgeOut, _initialGas, fromChainId);
}

When localAnyCallExecutorAddress invoke anyExecute, gas fee is stored in nativeToken first, then later withdraw from nativeToken and stored into multichain. That's right

    function depositGasAnycallConfig() external payable {
        //Deposit Gas
        _replenishGas(msg.value);
    }

    function _replenishGas(uint256 _executionGasSpent) internal {
        //Unwrap Gas
        wrappedNativeToken.withdraw(_executionGasSpent);
        IAnycallConfig(IAnycallProxy(localAnyCallAddress).config()).deposit{value: _executionGasSpent}(address(this));
    }

But when deposit gas directly from the outside, there is no need to interact with wrappedNativeToken, and the withdraw prevents the deposit.

Tools Used

Manual review

Recommended Mitigation Steps

Also add a deposit logic to depositGasAnycallConfig, or remove the withdraw logic

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

restakeToken is used to restaking an NFT, which can be invoked by anyone when the current period expires according to code comments. But a problem with the code implementation caused only the owner can invoke restakeToken even after the period expired, which the team admitted was an implementation error.

Proof of Concept

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

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

When restakeToken invoke _unstakeToken, the parameter isNotRestake should be set to false, but in the code is true, which would prevent other users from calling restakeToken.

Tools Used

Manual review

Recommended Mitigation Steps

Set isNotRestake to false

Assessed type

Context

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

UlyssesRouter has no expiration time protection, if the token price changes before tx execution, it will affect user earnings.

Proof of Concept

1. User send a tx for swaping tokenA -> tokenB, who sets the slippage based on the current market price of the token
2. The tx remained in mempool for a long time due to network congestion
3. The tokenA market price rises before tx execution and can be exchanged for more tokenB
4. But the searcher sees this tx and takes it out of mempool to perform a sandwich arbitrage, bringing the gain down to the slippage and the user's earnings are lost

Tools Used

Manual review

Recommended Mitigation Steps

Add expiration time protection for UlyssesRouter

Assessed type

MEV