Maia DAO Ecosystem - xuwinnie's results

Lines of code

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

Vulnerability details

Impact

There is no effective slippage control for _gasSwapIn and _gasSwapOut. The current approach only ensures an abundant level of liquidity within the pool but does not provide protection against sandwich attacks.

Proof of Concept

function _gasSwapIn(uint256 _amount, uint24 _fromChain) internal returns (uint256) {
    ......
    (bool zeroForOneOnInflow, uint24 priceImpactPercentage, address gasTokenGlobalAddress, address poolAddress) = 
        IPort(localPortAddress).getGasPoolInfo(_fromChain);
    (uint160 sqrtPriceX96,,,,,,) = IUniswapV3Pool(poolAddress).slot0();
    uint160 exactSqrtPriceImpact = (sqrtPriceX96 * (priceImpactPercentage / 2)) / GLOBAL_DIVISIONER;
    uint160 sqrtPriceLimitX96 =
        zeroForOneOnInflow ? sqrtPriceX96 - exactSqrtPriceImpact : sqrtPriceX96 + exactSqrtPriceImpact;

    //Swap imbalanced token as long as we haven't used the entire amountSpecified and haven't reached the price limit
    try IUniswapV3Pool(poolAddress).swap(
        address(this),
        zeroForOneOnInflow,
        int256(_amount),
        sqrtPriceLimitX96,
        abi.encode(SwapCallbackData({tokenIn: gasTokenGlobalAddress}))
    ) returns (int256 amount0, int256 amount1) {
        return uint256(zeroForOneOnInflow ? amount1 : amount0);
    } catch (bytes memory) {
        _forceRevert();
        return 0;
    }
}

The sqrtPriceLimitX96 cannot effectively control slippage, as it still relies on sqrtPriceX96, which can be manipulated through sandwich attacks. As long as there is sufficient liquidity within the pool, the price impact will not be excessive, enabling the sandwich attack to suceed.

Tools Used

Manual

Recommended Mitigation Steps

sqrtPriceLimitX96 should be specified via user's input.

Assessed type

Uniswap

Lines of code

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

Vulnerability details

Impact

Attacker can call retrieveDeposit with a future deposit nonce and anyFallback will be triggered to set the deposit status to Failed. Then, the attacker calls redeemDeposit to delete getDeposit[_depositNonce]. When a unlucky depositer makes a deposit with that nonce, everything will go as usual on branch chain, except that executionHistory[fromChainId][nonce] has already been marked as true on root chain. As a result, the root chain transaction will forcerevert() and the depositer has no way to get their assets back.

Proof of Concept

function retrieveDeposit(uint32 _depositNonce) external payable lock requiresFallbackGas {
    //Encode Data for cross-chain call.
    bytes memory packedData = abi.encodePacked(bytes1(0x08), _depositNonce, msg.value.toUint128(), uint128(0));

    //Update State and Perform Call
    _sendRetrieveOrRetry(packedData);
}

function anyExecute(bytes calldata data)
    external
    virtual
    requiresExecutor
    returns (bool success, bytes memory result)
{
    ......
        /// DEPOSIT FLAG: 8 (retrieveDeposit)
    } else if (flag == 0x08) {
        //Get nonce
        uint32 nonce = uint32(bytes4(data[1:5]));

        //Check if tx has already been executed
        if (!executionHistory[fromChainId][uint32(bytes4(data[1:5]))]) {
            //Toggle Nonce as executed
            executionHistory[fromChainId][nonce] = true;

            //Retry failed fallback
            (success, result) = (false, "");
        } else {
            _forceRevert();
            //Return true to avoid triggering anyFallback in case of `_forceRevert()` failure
            return (true, "already executed tx");
        }
    }
    ......
}

function _redeemDeposit(uint32 _depositNonce) internal {
    Deposit storage deposit = getDeposit[_depositNonce];

    // will be skipped since length is zero
    for (uint256 i = 0; i < deposit.hTokens.length;) {
        _clearToken(deposit.owner, deposit.hTokens[i], deposit.tokens[i], deposit.amounts[i], deposit.deposits[i]);

        unchecked {
            ++i;
        }
    }

    //Delete Failed Deposit Token Info
    delete getDeposit[_depositNonce];
}

Tools Used

Manual Review

Recommended Mitigation Steps

Add a check in retrieveDeposit:

require(getDeposit[_depositNonce].owner == msg.sender);

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

Users can bridge a mismatched pair of local hToken and underlying Token from branch chain to root chain, thereby infinitely minting any root hToken.

Proof of Concept

Below is the flow when a user bridges asset from branch chain to root chain:

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

    function bridgeIn(address _recipient, DepositParams memory _dParams, uint24 _fromChain) public requiresAgentExecutor
    {
        //Check Deposit info from Cross Chain Parameters.
        if (!CheckParamsLib.checkParams(localPortAddress, _dParams, _fromChain)) {
            revert InvalidInputParams();
        }

        //Get global address
        address globalAddress = IPort(localPortAddress).getGlobalTokenFromLocal(_dParams.hToken, _fromChain);

        //Check if valid asset
        if (globalAddress == address(0)) revert InvalidInputParams();

        //Move hTokens from Branch to Root + Mint Sufficient hTokens to match new port deposit
        IPort(localPortAddress).bridgeToRoot(_recipient, globalAddress, _dParams.amount, _dParams.deposit, _fromChain);
    }

function checkParams(address _localPortAddress, DepositParams memory _dParams, uint24 _fromChain)
        internal
        view
        returns (bool)
    {
        if (
            (_dParams.amount < _dParams.deposit) //Deposit can't be greater than amount.
                || (_dParams.amount > 0 && !IPort(_localPortAddress).isLocalToken(_dParams.hToken, _fromChain)) //Check local exists.
                || (_dParams.deposit > 0 && !IPort(_localPortAddress).isUnderlyingToken(_dParams.token, _fromChain)) //Check underlying exists.
        ) {
            return false;
        }
        return true;
    }

    function bridgeToRoot(address _recipient, address _hToken, uint256 _amount, uint256 _deposit, uint24 _fromChainId) external requiresBridgeAgent
    {
        if (!isGlobalAddress[_hToken]) revert UnrecognizedToken();

        if (_amount - _deposit > 0) _hToken.safeTransfer(_recipient, _amount - _deposit);
        if (_deposit > 0) mint(_recipient, _hToken, _deposit, _fromChainId);
    }

During the whole process, only _dParams.hToken is LocalToken and _dParams.token is UnderlyingToken are checked. However, no check is performed to ensure that _dParams.hToken corresponds to _dParams.token, which means user can use an arbitary underlying token as input and receive a diffrent (more valuable) hToken. Below are the steps that Alice receives arb-hETH by depositing LIN, a token created by herself:

1. Alice creates a new ERC20 token "LIN" on BNB Chain and mints herself max(uint256)
2. Alice addLocalToken for LIN at CoreBranchRouter
3. Alice callOutAndBridge with _dParams = {hToken: bnb-hETH, Token: LIN, amount: 10e10, deposit: 10e10}
4. bridgeOut at BranchPort charges Alice 10e10 LIN
5. checkParams at root chain passes
6. bridgeToRoot at RootPort mints Alice 10e10 arb-hETH

Tools Used

Manual

Recommended Mitigation Steps

Add a check to ensure that local token and underlying token match:

function checkParams(address _localPortAddress, DepositParams memory _dParams, uint24 _fromChain)
        internal
        view
        returns (bool)
    {
        if (
            (_dParams.amount < _dParams.deposit) //Deposit can't be greater than amount.
                || (_dParams.amount > 0 && !IPort(_localPortAddress).isLocalToken(_dParams.hToken, _fromChain)) //Check local exists.
                || (_dParams.deposit > 0 && IPort(_localPortAddress).getLocalTokenFromUnder(_dParams.token, _fromChain) != _dParams.hToken //Check underlying exists and matches.
        ) {
            return false;
        }
        return true;
    }

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

Excess gas fee is accumulated within root bridge agent as WETH. Attacker can retrySettlement by virtual account to steal the accumulated fee.

Proof of Concept

function retrySettlement(uint32 _settlementNonce, uint128 _remoteExecutionGas) external payable {
    //Update User Gas available.
    if (initialGas == 0) {
        userFeeInfo.depositedGas = uint128(msg.value);
        userFeeInfo.gasToBridgeOut = _remoteExecutionGas;
    }
    //Clear Settlement with updated gas.
    _retrySettlement(_settlementNonce);
}

function _retrySettlement(uint32 _settlementNonce) internal returns (bool) {
    Settlement memory settlement = getSettlement[_settlementNonce];
    if (settlement.owner == address(0)) return false;

    bytes memory newGas = abi.encodePacked(_manageGasOut(settlement.toChain));
    for (uint256 i = 0; i < newGas.length;) {
        settlement.callData[settlement.callData.length - 16 + i] = newGas[i];
        unchecked {
            ++i;
        }
    }
    ......
    _performCall(settlement.callData, settlement.toChain);

    return true;
}

function _manageGasOut(uint24 _toChain) internal returns (uint128) {
    uint256 amountOut;
    address gasToken;
    uint256 _initialGas = initialGas;
    ......
    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();
}

Steps:

1. Prepare some failed settlements by callOutSigned with zero _remoteExecutionGas and non-arb toChain.
2. callOutSigned with nonzero _remoteExecutionGas.
3. Virtual account calls retrySettlement with a failed nonce. initialGas will be greater than zero and userFeeInfo.gasToBridgeOut amount of WETH will be swapped into remote chain gas token.
4. Receive remaining gas token when _payExecutionGas at branch chain.
5. Virtual account repeats 3 until accumulated WETH is drained.

Note: It's better to attack with a larger _remoteExecutionGas so that less gas will be wasted by multiple executions.

function _gasSwapIn(bytes memory gasData) internal virtual returns (uint256 gasAmount) {
    //Cast to uint256
    gasAmount = uint256(uint128(bytes16(gasData)));
    //Move Gas hTokens from Branch to Root / Mint Sufficient hTokens to match new port deposit
    IPort(localPortAddress).withdraw(address(this), address(wrappedNativeToken), gasAmount);
}

function _payExecutionGas(address _recipient, uint256 _initialGas) internal virtual {
    uint256 gasRemaining = wrappedNativeToken.balanceOf(address(this));
    wrappedNativeToken.withdraw(gasRemaining);
    ......
    _replenishGas(minExecCost);
    SafeTransferLib.safeTransferETH(_recipient, gasRemaining - minExecCost);
    ......
}

Tools Used

Manual

Recommended Mitigation Steps

1. Store availableGas at the beginning of anyExecute:

uint256 availableGas = userFeeInfo.depositedGas - userFeeInfo.gasToBridgeOut;

2. Delete userFeeInfo.gasToBridgeOut after _manageGasOut.
3. Use availableGas as input for _payExecutionGas.

Assessed type

Reentrancy

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/RootBridgeAgent.sol#L811 https://github.com/anyswap/multichain-smart-contracts/blob/645d0053d22ed63005b9414b5610879094932304/contracts/anycall/v7/AnycallV7Config.sol#L204

Vulnerability details

Impact

The premium is not taken into account when calculating minExecCost in _payExecutionGas. Transaction may cosume more gas than what has been replenished.

Proof of Concept

function _payExecutionGas(uint128 _depositedGas, uint128 _gasToBridgeOut, uint256 _initialGas, uint24 _fromChain) internal {
    ......
    uint256 minExecCost = tx.gasprice * (MIN_EXECUTION_OVERHEAD + _initialGas - gasleft());
    if (minExecCost > availableGas) {
        _forceRevert();
        return;
    }
    _replenishGas(minExecCost);
    ......
}

The gas price is determined as tx.gasprice in _payExecutionGas, but the actual charge is tx.gasprice + _feeData.premium. A malicious user can consume an arbitrarily large amount of gas by making external calls at virtual account to generate shortfall. By repeating so, the gas budget of bridge agent will be depleted, and the entire system will become inoperable.

function chargeFeeOnDestChain(address _from, uint256 _prevGasLeft) external onlyAnycallContract {
    if (!_isSet(mode, FREE_MODE)) {
        uint256 gasUsed = _prevGasLeft + EXECUTION_OVERHEAD - gasleft();
        uint256 totalCost = gasUsed * (tx.gasprice + _feeData.premium);
        uint256 budget = executionBudget[_from];
        require(budget > totalCost, "no enough budget");
        executionBudget[_from] = budget - totalCost;
        _feeData.accruedFees += uint128(totalCost);
    }
}

Tools Used

Manual

Recommended Mitigation Steps

Replace tx.gasprice by tx.gasprice + premium

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/RootBridgeAgent.sol#L244 https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/ArbitrumBranchPort.sol#L76-L84

Vulnerability details

Impact

When clearing tokens, _underlyingAddress.safeTransfer is called; however, the lock is missing on retrySettlement, making reentrancy possible.

Proof of Concept

All essential entry points of ulysses omnichain are modified with lock to prevent reentrancy. However, the lock is missing on retrySettlement at RootBridgeAgent.

function retrySettlement(uint32 _settlementNonce, uint128 _remoteExecutionGas) external payable {
    //Update User Gas available.
    if (initialGas == 0) {
        userFeeInfo.depositedGas = uint128(msg.value);
        userFeeInfo.gasToBridgeOut = _remoteExecutionGas;
    }
    //Clear Settlement with updated gas.
    _retrySettlement(_settlementNonce);
}

When clearing tokens, an external call to _underlyingAddress is made. However, _underlyingAddress may be an arbitary contract since token is permissionlessly added, which means it could reenter retrySettlement to receive token multiple times.

function withdraw(address _recipient, address _underlyingAddress, uint256 _deposit)
    external
    override(IBranchPort, BranchPort)
    requiresBridgeAgent
{
    _underlyingAddress.safeTransfer(
        _recipient, _denormalizeDecimals(_deposit, ERC20(_underlyingAddress).decimals())
    );
}

Below are the steps of the attack, starting with 1 BNB on BNB Chain:

1. Create a contract "XIN" on Arbitrum (the root chain).

contract XIN {
    string public name = "xiaoxin";
    string public symbol = "XIN";
    uint8 public decimals = 18;

    address private _virtualAccount;
    RootBridgeAgent private _rootBridgeAgent;

    uint256 private countdown = 0;
    uint256 private nonce = 0;

    constructor(address virtualAccount, address rootBridgeAgent) {
        _virtualAccount = virtualAccount;
        _rootBridgeAgent = RootBridgeAgent(rootBridgeAgent);
    }

    function transfer(address, uint256) external {
        if (countdown == 0) {return;}
        else {
            _rootBridgeAgent.retrySettlement(nonce, 0);
            --countdown;
        }
    }

    function initiate() external {
        require(msg.sender == _virtualAccount);
        countdown = 100;
        nonce = _rootBridgeAgent.settlementNonce();
    }
}

2. addLocalToken for XIN at ArbitrumCoreBranchRouter.
3. callOutSignedAndBridge (on Arbitrum) with _dParams = {hToken: arb-hXIN, token: XIN, amount: 1, deposit: 1} and receive 1 arb-hXIN at virtual account.
4. callOutSignedAndBridge (on BNB Chain) with _dParams = {hToken: bnb-hBNB, token: BNB, amount: 1, deposit: 1} and receive 1 arb-hBNB at virtual account.
5. callOut with params = abi.encode(0x03, [{target: XIN, calldata: abi.encode(selector("initiate()")}], {recipient: attacker, outputTokens: [arb-hXIN, arb-hBNB], amountsOut: [1, 1], depositsOut: [1, 0]}, Arb), initiate() will be called by the virtual account to change safeTransfer into reentrancy mode.
6. retrySettlement is reentered 100 times and attacker receives 101 arb-hBNB.

Tools Used

Manual Review

Recommended Mitigation Steps

Add lock for retrySettlement.

Assessed type

Reentrancy

Lines of code

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

Vulnerability details

Impact

Users have two methods to add liquidity to the Ulysses Pool: "mint" and "deposit". However, the latter may return an inaccurate output, which could be exploited to drain the pool.

Proof of Concept

In the process to mint amount of share, the state change is A:(band, supply×weight) -> B:(band+update, (supply+amount)×weight). User pays amount-sum(posFee)+sum(negFee) of underlying to acquire amount of share. This approach is precise.

In the process to deposit amount of underlying, the simulated state change is A:(band, supply×weight) -> B:(band+update, (supply+amount)×weight). Then (posFee, negFee) is derived from the simulation of A->B. The actual state change is A:(band, supply×weight) -> B':(band+update+posFee, (supply+amount+sum(posFee)-sum(negFee))×weight). We denote the actual fee of A->B' as (posFee', negFee'). User pays amount of underlying to acquire amount+sum(posFee)-sum(negFee) of share. This approach would be acceptable if sum(pos')-sum(neg') >= sum(pos), but this inequality doesn't always hold. If sum(pos')-sum(neg') < sum(pos), insolvency occurs; if sum(pos')-sum(neg') < sum(pos)-sum(neg), user could take profit.

An example is given below:

amount = 10000000
supply = 1000000000000000013287555072
weight = [1, 59, 47]
band = [99452334745147595191585509, 4253569467850027815346666, 216725069177793291903286517]

When Alice deposits 10000000 underlying, she will get 36215776 share. However, the pool actually worsens.

oldRebalancingFee = [0, 10519971631761767037843097, 18152377668510835770992]
newRebalancingFee = [0, 10519971631761767000804564, 18152377668510882599904]
oldMinusNew = [0, +37038533, -46828912]

Actually there should be a systemic approach to construct states of sum(pos')-sum(neg') < sum(pos)-sum(neg) for attacks. However, due to limited time, I have only conducted random tests. By continuously searching for profitable states and modifying the pool state accordingly, attackers can eventually drain the pool.

FAQ

Here are several questions that readers may have:

Q: Why there are three diffrent scenarios? Why could insolvency and user loss happen simultaneously? A: Imagine when you deposit 100$ to a bank, the bank increases your balance by 80$ and claims itself has got 120$.

Q: Why can sum(pos')-sum(neg') >= sum(pos) not hold? A: Difficult question! Roughly this could happen when amount is significantly smaller than supply and posFee is excessively large.

Q: How can the pool be modified to a target state? A: There are several methods including "mint", "redeem" and "swap", but the "deposit" method should not be used until we reach the target state because attacker will mostly experience losses from that.

Q: Why can the attacker eventually drain the pool? A：When "mint", "redeem" or "swap", the attacker pays exactly the delta value of _calculateRebalancingFee. However, when making a "deposit", the attacker receives more than what they deserve. At last, by adding liquidity, _calculateRebalancingFee can be reduced, so the pool will be drained.

Q: Why don't you provide a coded POC of attack? A: We know "deposit" is dangerous and we deprecate it, that's enough.

Tools Used

Python

# -*- coding: utf-8 -*-
"""
Created on Mon Jun 19 10:24:56 2023

@author: xuwinnie
"""

from random import *

def getBandwidthUpdateAmounts(roundUp, positiveTransfer, amount, _totalWeights, _totalSupply):
    # Get the bandwidth state list length
    global length, weightArray, bandwithArray

    # Initialize bandwidth update amounts
    bandwidthUpdateAmounts = [0] * length
    # Initialize bandwidth differences from target bandwidth
    diffs = [0] * length

    # Total difference from target bandwidth of all bandwidth states
    totalDiff = 0
    # Total difference from target bandwidth of all bandwidth states
    transfered = 0
    # Total amount to be distributed according to each bandwidth weights
    transferedChange = 0

    for i in range(length):
        # Load bandwidth and weight from storage
        # Bandwidth is the first 248 bits of the slot
        bandwidth = bandwithArray[i]
        # Weight is the last 8 bits of the slot
        weight = weightArray[i]

        # Calculate the target bandwidth
        targetBandwidth = (_totalSupply * weight) // _totalWeights

        # Calculate the difference from the target bandwidth
        if positiveTransfer:
            # If the transfer is positive, calculate deficit from target bandwidth
            if targetBandwidth > bandwidth:
                # Calculate the difference
                diff = targetBandwidth - bandwidth
                # Add the difference to the total difference
                totalDiff += diff
                # Store the difference in the diffs array
                diffs[i] = diff
        else:
            # If the transfer is negative, calculate surplus from target bandwidth
            if bandwidth > targetBandwidth:
                # Calculate the difference
                diff = bandwidth - targetBandwidth
                # Add the difference to the total difference
                totalDiff += diff
                # Store the difference in the diffs array
                diffs[i] = diff

    # Calculate the amount to be distributed according deficit/surplus
    # and/or the amount to be distributed according to each bandwidth weights
    if amount > totalDiff:
        # If the amount is greater than the total deficit/surplus
        # Total deficit/surplus is distributed
        transfered = totalDiff
        # Set rest to be distributed according to each bandwidth weights
        transferedChange = amount - totalDiff
    else:
        # If the amount is less than the total deficit/surplus
        # Amount will be distributed according to deficit/surplus
        transfered = amount

    for i in range(length):
        # Increase/decrease amount of bandwidth for each bandwidth state
        bandwidthUpdate = 0

        # If there is a deficit/surplus, calculate the amount to be distributed
        if transfered > 0:
            # Load the difference from the diffs array
            diff = diffs[i]

            # Calculate the amount to be distributed according to deficit/surplus
            if roundUp:
                bandwidthUpdate = (transfered * diff + totalDiff - 1) // totalDiff
            else:
                bandwidthUpdate = (transfered * diff) // totalDiff
        # If there is a rest, calculate the amount to be distributed according to each bandwidth weights
        if transferedChange > 0:
            # Load weight from storage
            weight = weightArray[i]

            # Calculate the amount to be distributed according to each bandwidth weights
            if roundUp:
                bandwidthUpdate += (transferedChange * weight + _totalWeights - 1) // _totalWeights
            else:
                bandwidthUpdate += (transferedChange * weight) // _totalWeights

        # If there is an update in bandwidth
        if bandwidthUpdate > 0:
            # Store the amount to be updated in the bandwidthUpdateAmounts array
            bandwidthUpdateAmounts[i] = bandwidthUpdate

    return (bandwidthUpdateAmounts, length)

def updateBandwidth(depositFees, positiveTransfer, destinationState, difference, _totalWeights, _totalSupply, _newTotalSupply):
    global weightArray, bandwithArray
    
    print("  updating "+str(destinationState)+" with diffrence "+str(difference))
    bandwidth = bandwithArray[destinationState]
    print("    old bandwith "+str(bandwidth))
    weight = weightArray[destinationState]

    # Get the target bandwidth
    targetBandwidth = (_totalSupply * weight) // _totalWeights

    # Get the rebalancing fee prior to updating the bandwidth
    oldRebalancingFee = calculateRebalancingFee(bandwidth, targetBandwidth, positiveTransfer)

    if positiveTransfer:
        # If the transfer is positive
        # Add the difference to the bandwidth
        bandwidth += difference

    else:
        # If the transfer is negative
        # Subtract the difference from the bandwidth
        bandwidth -= difference

    if _newTotalSupply > 0:
        # True on deposit, mint and redeem
        # Get the new target bandwidth after total supply change
        targetBandwidth = (_newTotalSupply * weight) // _totalWeights

    # Get the rebalancing fee after updating the bandwidth
    newRebalancingFee = calculateRebalancingFee(bandwidth, targetBandwidth, positiveTransfer)

    positiveFee, negativeFee = 0, 0
    if newRebalancingFee < oldRebalancingFee:
        # If new fee is lower than old fee
        # Calculate the positive fee
        positiveFee = oldRebalancingFee - newRebalancingFee
        print("    positiveFee "+str(positiveFee))

        if depositFees:
            # If depositFees is true, add the positive fee to the bandwidth
            bandwidth += positiveFee

    else:
        # If new fee is higher than old fee
        if newRebalancingFee > oldRebalancingFee:
            # Calculate the negative fee
            negativeFee = newRebalancingFee - oldRebalancingFee
            print("    negativeFee "+str(negativeFee))
            #raise Exception("good")
        else: print("    no fee")

    # Update storage with the new bandwidth
    bandwithArray[destinationState] = bandwidth
    print("    new bandwith "+str(bandwidth))

    return (positiveFee, negativeFee)

def calculateRebalancingFee(bandwidth, targetBandwidth, roundDown):
    # If the bandwidth is larger or equal to the target bandwidth, return 0
    if bandwidth >= targetBandwidth:
        return 0

    # Fee tier 1 (fee % divided by 2)
    lambda1 = int(20e14)
    # Fee tier 2 (fee % divided by 2)
    lambda2 = int(4980e14)
    # Get sigma2 from the first 8 bytes of the fee slot
    sigma2 = int(500e14)
    # Get sigma1 from the next 8 bytes of the fee slot
    sigma1 = int(6000e14)
    
    # Calculate the upper bound for the first fee
    upperBound1 = (targetBandwidth * sigma1) // DIVISIONER
    # Calculate the upper bound for the second fee
    upperBound2 = (targetBandwidth * sigma2) // DIVISIONER

    if bandwidth >= upperBound1:
        return 0

    maxWidth = upperBound1 - upperBound2

    # If the bandwidth is smaller than upperBound2
    if bandwidth >= upperBound2:
        # Calculate the fee for the first interval
        fee = calcFee(lambda1, maxWidth, upperBound1, bandwidth, 0, roundDown)
    else:
        # Calculate the fee for the first interval
        fee = calcFee(lambda1, maxWidth, upperBound1, upperBound2, 0, roundDown)

        # offset = lambda1 * 2
        lambda1 *= 2

        # Calculate the fee for the second interval
        fee2 = calcFee(lambda2, upperBound2, upperBound2, bandwidth, lambda1, roundDown)

        # Add the two fees together
        fee += fee2
        
    return fee


def calcFee(feeTier, maxWidth, upperBound, bandwidth, offset, roundDown):
    # Calculate the height of the trapezium
    height = upperBound - bandwidth

    # Calculate the width of the trapezium, rounded up
    width = ((height * feeTier + maxWidth - 1) // maxWidth) + offset

    # Calculate the fee for this tier
    if roundDown:
        fee = (width * height) // DIVISIONER
    else:
        fee = (width * height + DIVISIONER - 1) // DIVISIONER
    return fee

def mint(amount):
    print("minting "+str(amount)+" underlying")
    global LPBalance, UnderBalance, totalWeights, totalSupply, poolBalance
    
    _totalWeights = totalWeights
    _totalSupply = totalSupply
    _newTotalSupply = _totalSupply + amount

    bandwidthUpdateAmounts, length = getBandwidthUpdateAmounts(True, True, amount, _totalWeights, _newTotalSupply)

    output = 0
    negativeFee = 0
    i = 0
    while i < length:
        updateAmount = bandwidthUpdateAmounts[i]
        if updateAmount > 0:
            output += updateAmount

        _positiveFee, _negativeFee = updateBandwidth(False, True, i, updateAmount, _totalWeights, _totalSupply, _newTotalSupply)

        if _positiveFee > 0:
            negativeFee += _positiveFee
        else:
            output += _negativeFee
        i += 1

    if negativeFee > output:
        #raise Exception("Underflow()")
        pass

    output -= negativeFee
    
    LPBalance += output
    
    if output > UnderBalance:
        raise Exception("Underflow()")
    UnderBalance -= output
    
    totalSupply += amount
    poolBalance += output
    print("receiving "+str(output)+" lp")
    print()
    
def deposit(amount):
    print("depositing "+str(amount)+" underlying")
    global LPBalance, UnderBalance, totalWeights, totalSupply, poolBalance
    
    _totalWeights = totalWeights
    _totalSupply = totalSupply
    _newTotalSupply = _totalSupply + amount

    bandwidthUpdateAmounts, length = getBandwidthUpdateAmounts(False, True, amount, _totalWeights, _newTotalSupply)

    output = 0
    negativeFee = 0
    i = 0
    while i < length:
        updateAmount = bandwidthUpdateAmounts[i]
        if updateAmount > 0:
            output += updateAmount

        _positiveFee, _negativeFee = updateBandwidth(True, True, i, updateAmount, _totalWeights, _totalSupply, _newTotalSupply)

        if _positiveFee > 0:
            output += _positiveFee
        else:
            negativeFee += _negativeFee

        i += 1

    if negativeFee > output:
        raise Exception("Underflow()")

    output -= negativeFee
    
    LPBalance += output
    
    if amount > UnderBalance:
        raise Exception("Underflow()")
    UnderBalance -= amount
    
    totalSupply += output
    poolBalance += amount
    print("receiving "+str(output)+" lp")
    print()
    
def redeem(amount):
    print("redeeming "+str(amount)+" lp")
    global LPBalance, UnderBalance, totalWeights, totalSupply, poolBalance
    
    totalSupply -= amount
    
    if amount > LPBalance:
        raise Exception("Underflow()")        
    LPBalance -= amount
    
    _totalWeights = totalWeights
    _newTotalSupply = totalSupply
    _totalSupply = _newTotalSupply + amount

    bandwidthUpdateAmounts, length = getBandwidthUpdateAmounts(False, False, amount, _totalWeights, _totalSupply)

    output = 0
    negativeFee = 0
    i = 0
    while i < length:
        updateAmount = bandwidthUpdateAmounts[i]
        if updateAmount > 0:
            output += updateAmount

        _positiveFee, _negativeFee = updateBandwidth(False, False, i, updateAmount, _totalWeights, _totalSupply, _newTotalSupply)
        #if _positiveFee > 0:
            #raise Exception("nooooo()")
        negativeFee += _negativeFee

        i += 1

    if negativeFee > output:
        raise Exception("Underflow()")
    output -= negativeFee
    
    UnderBalance += output
    poolBalance -= output
    print("receiving "+str(output)+" underlying")
    print()
    
def getIdealPoolBalance():
    global length, bandwithArray, weightArray, totalWeights, totalSupply
    assets = 0

    for i in range(length):
        targetBandwidth = totalSupply * weightArray[i] // totalWeights
        assets += calculateRebalancingFee(bandwithArray[i], targetBandwidth, False)
        #print(calculateRebalancingFee(bandwithArray[i], targetBandwidth, False))
        assets += bandwithArray[i]
        #print(bandwithArray[i])
    
    return assets

def getFeeStatus():
    global length, bandwithArray, weightArray, totalWeights, totalSupply
    assets = 0

    for i in range(length):
        targetBandwidth = totalSupply * weightArray[i] // totalWeights
        assets += calculateRebalancingFee(bandwithArray[i], targetBandwidth, False)
        print(i)
        print(calculateRebalancingFee(bandwithArray[i], targetBandwidth, False))
    
    return assets

'''
cnt = 0
cnttt = 0        
recordinso = []
recordluck = []


for i in range(100000):
    DIVISIONER = int(1e18)
    length = 3
    bandwithArray = [randint(0, int(1e27)) for _ in range(length)]
    weightArray = [1] + [randint(1, 100) for _ in range(length - 1)]
    #weightArray = [1, 1000]
    totalWeights = sum(weightArray)
    totalSupply = int(1e27)
    poolBalance = getIdealPoolBalance()
    
    UnderBalance = 0
    beforeFee = getFeeStatus()
    #amount = randint(0, int(1e25))
    amount = 10000000000
    LPBalance = amount
    redeem(amount)
    afterFee = getFeeStatus()
    if poolBalance < getIdealPoolBalance():
        print(str(poolBalance)+" insolvency! "+str(getIdealPoolBalance()))
        cnt += 1
        recordinso.append(getIdealPoolBalance() - poolBalance)
        raise Exception("Strange()")
        
    if UnderBalance + afterFee > amount + beforeFee :
        print("lucky!")
        recordluck.append(UnderBalance + afterFee - amount - beforeFee)
        cnttt += 1
    print(i)
print(cnt)
print(cnttt)


'''
cnt = 0
cnttt = 0
recordinso = []
recordluck = []
recordcomp = []

for i in range(1):

    DIVISIONER = int(1e18)
    length = 3
    #bandwithArray = [randint(0, int(1e27)) for _ in range(length)]
    bandwithArray = [99452334745147595191585509, 4253569467850027815346666, 216725069177793291903286517]
    #weightArray = [1] + [randint(1, 100) for _ in range(length - 1)]
    weightArray = [1, 59, 47]
    totalWeights = sum(weightArray)
    totalSupply = int(1e27)
    poolBalance = getIdealPoolBalance()
    
    LPBalance = 0
    UnderBalance = int(1e26)
    beforeFee = getFeeStatus()
    #amount = randint(0, int(1e10))
    amount = 10000000

    deposit(amount)
    afterFee = getFeeStatus()

    if poolBalance < getIdealPoolBalance():
        print(str(poolBalance)+" insolvency! "+str(getIdealPoolBalance()))
        cnt += 1
        recordinso.append(getIdealPoolBalance() - poolBalance)
        recordcomp.append(LPBalance + afterFee - amount - beforeFee)
        #raise Exception("good")
        
    if LPBalance + afterFee > amount + beforeFee:
        print("lucky!")
        cnttt += 1
        recordluck.append(LPBalance + afterFee - amount - beforeFee)
        #break
    print(i)
print(cnt)
print(cnttt)

Recommended Mitigation Steps

Deprecate the "deposit" method. It is hard to find a correct way to handle this.

Assessed type

Context

    function setTotalSupply(uint256 _totalSupply) external {
        totalSupply = _totalSupply;
    }

    function addBandwidthTest(uint248 bandwidth, uint8 weight) external {
        totalWeights += weight;
        bandwidthStateList.push(
            BandwidthState({bandwidth: bandwidth, destination: UlyssesPool(address(0)), weight: weight})
        );
    }

    function getRebalancingFee(uint256 index) external view returns (uint256) {
        return _calculateRebalancingFee(
            bandwidthStateList[index].bandwidth, totalSupply.mulDiv(bandwidthStateList[index].weight, totalWeights), false
        );
    }

    function test_435() public {
        setUpHandler();

        vm.startPrank(handler);

        UlyssesPool[] memory pools = createPools(1);
        UlyssesPool pool1 = UlyssesPool(pools[0]);

        MockERC20(pool1.asset()).mint(address(handler), type(uint256).max / 2);
        MockERC20(pool1.asset()).mint(address(pool1), 100000000000 ether);
        MockERC20(pool1.asset()).approve(address(pool1), type(uint256).max);

        pool1.setTotalSupply(1000000000000000013287555072);

        pool1.addBandwidthTest(99452334745147595191585509, 1);
        pool1.addBandwidthTest(4253569467850027815346666, 59);
        pool1.addBandwidthTest(216725069177793291903286517, 47);

        console2.log(pool1.getRebalancingFee(1), pool1.getRebalancingFee(2), pool1.getRebalancingFee(3));
        uint256 feeBefore = pool1.getRebalancingFee(1) + pool1.getRebalancingFee(2) + pool1.getRebalancingFee(3);

        pool1.deposit(10000000, address(handler));

        console2.log(pool1.getRebalancingFee(1), pool1.getRebalancingFee(2), pool1.getRebalancingFee(3));
        uint256 feeAfter = pool1.getRebalancingFee(1) + pool1.getRebalancingFee(2) + pool1.getRebalancingFee(3);

        // Should revert but doesn't
        console2.log(feeAfter - feeBefore);
    }

Lines of code

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

Vulnerability details

Impact

The goal with bandwidths is to have a maximum that can be withdrawn (swapped) from a pool, so in case a specific chain (or token from a chain) is exploited, then it only can partially affect these pools. However, the maximum limit can be bypassed by repeatedly "balancing" the pool to increase bandwidth for the exploited chain.

Introducing "Balancing": A Technique for Redistributing Bandwidth

During ulyssesAddLP or ulyssesAddLP, liquidity is first distributed or taken proportionally to diff (if any exists), then distributed or taken proportionally to weight. Suppose integer t is far smaller than diff (since the action itself can also change diff), after repeatedly adding t LP, removing t lp, adding t LP, removing t Lp ...... the pool will finally reach another stable state where the ratio of diff to weight is a constant among destinations. This implies that the currentBandwidth will be proportional to weight.

Proof of Concept

Suppose Avalanche is down. Unluckily, Alice holds 100 ava-hETH. She wants to swap ava-hETH for bnb-hETH.

Let's take a look at bnb-hETH pool. Suppose weights are mainnet:4, Avalanche:3, Linea:2. Total supply is 90. Target bandwidths are mainnet:40, Avalanche:30, Linea:20. Current bandwidths are mainnet:30, Avalanche:2(few left), Linea:22.

Ideally Alice should only be able to swap for 2 bnb-hETH. However, she swaps for 0.1 bnb-hETH first. Then she uses the 0.1 bnb-hETH to "balance" the pool (as mentioned above). Current bandwidths will become mainnet:24, Avalanche:18, Linea:12. Then Alice swaps for 14 bnb-hETH and "balance" the pool again. By repeating the process, she can acquire nearly all of the available liquidity in pool and LP loss will be unbounded.

Tools Used

Manual Review

Recommended Mitigation Steps

1. During ulyssesAddLP or ulyssesAddLP, always distribute or take liquidity proportionally to weight.
2. When swapping A for B, reduce bandwidth of A in B pool (as is currently done) while add bandwidth of B in A pool (instead of distributing them among all bandwidths).

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

Traders could "balance" the pool at the end of a swap to not pay or pay less rebalancing fees.

Introducing "Balancing": A Technique for Redistributing Bandwidth

During ulyssesAddLP or ulyssesAddLP, liquidity is first distributed or taken proportionally to diff (if any exists), then distributed or taken proportionally to weight. Suppose integer t is far smaller than diff (since the action itself can also change diff), after repeatedly adding t LP, removing t lp, adding t LP, removing t Lp ...... the pool will finally reach another stable state where the ratio of diff to weight is a constant among destinations. This implies that the currentBandwidth will be proportional to weight.

Proof of Concept

function _calculateRebalancingFee(uint256 bandwidth, uint256 targetBandwidth, bool roundDown)
    internal
    view
    returns (uint256 fee)
{
    // If the bandwidth is larger or equal to the target bandwidth, return 0
    if (bandwidth >= targetBandwidth) return 0;

    // Upper bound of the first fee interval
    uint256 upperBound1;
    // Upper bound of the second fee interval
    uint256 upperBound2;
    // Fee tier 1 (fee % divided by 2)
    uint256 lambda1;
    // Fee tier 2 (fee % divided by 2)
    uint256 lambda2;

    // @solidity memory-safe-assembly
    assembly {
        // Load the rebalancing fee slot to get the fee parameters
        let feeSlot := sload(fees.slot)
        // Get sigma2 from the first 8 bytes of the fee slot
        let sigma2 := shr(192, feeSlot)
        // Get sigma1 from the next 8 bytes of the fee slot
        let sigma1 := and(shr(128, feeSlot), 0xffffffffffffffff)
        // Get lambda2 from the next 8 bytes of the fee slot
        lambda2 := and(shr(64, feeSlot), 0xffffffffffffffff)
        // Get lambda1 from the last 8 bytes of the fee slot
        lambda1 := and(feeSlot, 0xffffffffffffffff)

        // Equivalent to require(denominator != 0 && (y == 0 || x <= type(uint256).max / y))
        if mul(sigma1, gt(targetBandwidth, div(not(0), sigma1))) {
            // Store the function selector of `MulDivFailed()`.
            mstore(0x00, 0xad251c27)
            // Revert with (offset, size).
            revert(0x1c, 0x04)
        }

        // Calculate the upper bound for the first fee
        upperBound1 := div(mul(targetBandwidth, sigma1), DIVISIONER)

        // Equivalent to require(denominator != 0 && (y == 0 || x <= type(uint256).max / y))
        if mul(sigma2, gt(targetBandwidth, div(not(0), sigma2))) {
            // Store the function selector of `MulDivFailed()`.
            mstore(0x00, 0xad251c27)
            // Revert with (offset, size).
            revert(0x1c, 0x04)
        }

        // Calculate the upper bound for the second fee
        upperBound2 := div(mul(targetBandwidth, sigma2), DIVISIONER)
    }

    if (bandwidth >= upperBound1) return 0;

    uint256 maxWidth;
    /// @solidity memory-safe-assembly
    assembly {
        // Calculate the maximum width of the trapezium
        maxWidth := sub(upperBound1, upperBound2)
    }

    // If the bandwidth is smaller than upperBound2
    if (bandwidth >= upperBound2) {
        // Calculate the fee for the first interval
        fee = calcFee(lambda1, maxWidth, upperBound1, bandwidth, 0, roundDown);
    } else {
        // Calculate the fee for the first interval
        fee = calcFee(lambda1, maxWidth, upperBound1, upperBound2, 0, roundDown);

        /// @solidity memory-safe-assembly
        assembly {
            // offset = lambda1 * 2
            lambda1 := shl(1, lambda1)
        }

        // Calculate the fee for the second interval
        uint256 fee2 = calcFee(lambda2, upperBound2, upperBound2, bandwidth, lambda1, roundDown);

        /// @solidity memory-safe-assembly
        assembly {
            // Add the two fees together
            fee := add(fee, fee2)
        }
    }
}

The marginal fee of liquidity is a function of the ratio between the current bandwidth and the target bandwidth. Therefore, when we "balance" it, we also achieve the global optimal state for rebalancing fee. In other words, if a trader performs a swap and then "balance" the pool, they can reclaim a portion of the fees they paid. Additionally, if a pool is not "balanced", one can also "balance" it to profit from arbitrage.

Tools Used

Manual Review

Recommended Mitigation Steps

Incentivize current assets (the sum of current bandwidths) instead of incentivizing current bandwidths separately.

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

Excess gas are accumulated as WETH. However, sweep transfers ETH.

Proof of Concept

function sweep() external {
    if (msg.sender != daoAddress) revert NotDao();
    uint256 _accumulatedFees = accumulatedFees - 1;
    accumulatedFees = 1;
    SafeTransferLib.safeTransferETH(daoAddress, _accumulatedFees);
}

As below, the contract receive WETH at _gasSwapIn. Then, WETH is partially unwrapped at _replenishGas. Excess gas are stored as WETH.

function _gasSwapIn(uint256 _amount, uint24 _fromChain) internal returns (uint256) {
    //Get fromChain's Gas Pool Info
    (bool zeroForOneOnInflow, uint24 priceImpactPercentage, address gasTokenGlobalAddress, address poolAddress) =
        IPort(localPortAddress).getGasPoolInfo(_fromChain);
    ......
    //Swap imbalanced token as long as we haven't used the entire amountSpecified and haven't reached the price limit
    try IUniswapV3Pool(poolAddress).swap(
        address(this),
        zeroForOneOnInflow,
        int256(_amount),
        sqrtPriceLimitX96,
        abi.encode(SwapCallbackData({tokenIn: gasTokenGlobalAddress}))
    ) returns (int256 amount0, int256 amount1) {
        return uint256(zeroForOneOnInflow ? amount1 : amount0);
    } catch (bytes memory) {
        _forceRevert();
        return 0;
    }
}

function _payExecutionGas(uint128 _depositedGas, uint128 _gasToBridgeOut, uint256 _initialGas, uint24 _fromChain) internal {
    ......
    //Replenish Gas
    _replenishGas(minExecCost);

    //Account for excess gas
    accumulatedFees += availableGas - minExecCost;
}

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

Tools Used

Manual

Recommended Mitigation Steps

Transfer WETH in sweep.

Assessed type

Token-Transfer

Lines of code

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

Vulnerability details

Impact

forceRevert() withdraws all deposited gas budget of Root Bridge Agent to ensure that failed AnyCall execution will not be charged. However, if forceRevert() took place during a call made by virtual account, gas can be replenished later manually. As a result, the AnyCall execution will succeed but all withdrawn gas will be frozen.

Proof of Concept

function anyExecute(bytes calldata data)
    external
    virtual
    requiresExecutor
    returns (bool success, bytes memory result)
{
    uint256 _initialGas = gasleft();
    uint24 fromChainId;
    UserFeeInfo memory _userFeeInfo;

    if (localAnyCallExecutorAddress == msg.sender) {
        initialGas = _initialGas;
        (, uint256 _fromChainId) = _getContext();
        fromChainId = _fromChainId.toUint24();

        _userFeeInfo.depositedGas = _gasSwapIn(
            uint256(uint128(bytes16(data[data.length - PARAMS_GAS_IN:data.length - PARAMS_GAS_OUT]))), fromChainId).toUint128();
        _userFeeInfo.gasToBridgeOut = uint128(bytes16(data[data.length - PARAMS_GAS_OUT:data.length]));
    } else {
        fromChainId = localChainId;
        _userFeeInfo.depositedGas = uint128(bytes16(data[data.length - 32:data.length - 16]));
        _userFeeInfo.gasToBridgeOut = _userFeeInfo.depositedGas;
    }

    if (_userFeeInfo.depositedGas < _userFeeInfo.gasToBridgeOut) {
        _forceRevert();
        return (true, "Not enough gas to bridge out");
    }

    userFeeInfo = _userFeeInfo;

    // execution part
    ............

    if (initialGas > 0) {
        _payExecutionGas(userFeeInfo.depositedGas, userFeeInfo.gasToBridgeOut, _initialGas, fromChainId);
    }
}

To implement the attack, attacker callOutSigned on a branch chain to bypass lock. On the root chain, virtual account makes three external calls.

1. retryDeposit at Arbitrum Branch Bridge Agent with an already executed nonce. The call will forceRevert() and initialGas will be nonzero since it has not been modified by reentering. As a result, all execution gas budget will be withdrawn.

function _forceRevert() internal {
    if (initialGas == 0) revert GasErrorOrRepeatedTx();

    IAnycallConfig anycallConfig = IAnycallConfig(IAnycallProxy(localAnyCallAddress).config());
    uint256 executionBudget = anycallConfig.executionBudget(address(this));

    // Withdraw all execution gas budget from anycall for tx to revert with "no enough budget"
    if (executionBudget > 0) try anycallConfig.withdraw(executionBudget) {} catch {}
}

2. callOut at Arbitrum Branch Bridge Agent. The call should succeed and initialGas is deleted.

function _payExecutionGas(uint128 _depositedGas, uint128 _gasToBridgeOut, uint256 _initialGas, uint24 _fromChain) internal {
    delete(initialGas);
    delete(userFeeInfo);

    if (_fromChain == localChainId) return;

3. Directly deposit a small amount of gas at Anycall Config to ensure the success of the transaction.

function deposit(address _account) external payable {
    executionBudget[_account] += msg.value;
    emit Deposit(_account, msg.value);
}

Then, the original call proceeds and _payExecutionGas will be skipped. The call will succeed, with all withdrawn gas budget permanently frozen. (In current implementation, ETH can be sweeped to dao address, but this is another mistake, sweep should transfer WETH instead.)

Tools Used

Manual

Recommended Mitigation Steps

Add a msg.sender check in _forceRevert to ensure local call will be directly reverted.

Assessed type

Reentrancy

Lines of code

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

Vulnerability details

Impact

Malicious user can deliberately set a irrelevant (or even poisonous) local hToken for an underlying token, as anyone can directly access _addLocalToken at root chain without calling addLocalToken at branch chain first.

Proof of Concept

function addLocalToken(address _underlyingAddress) external payable virtual {
    //Get Token Info
    string memory name = ERC20(_underlyingAddress).name();
    string memory symbol = ERC20(_underlyingAddress).symbol();

    //Create Token
    ERC20hToken newToken = ITokenFactory(hTokenFactoryAddress).createToken(name, symbol);

    //Encode Data
    bytes memory data = abi.encode(_underlyingAddress, newToken, name, symbol);

    //Pack FuncId
    bytes memory packedData = abi.encodePacked(bytes1(0x02), data);

    //Send Cross-Chain request (System Response/Request)
    IBridgeAgent(localBridgeAgentAddress).performCallOut{value: msg.value}(msg.sender, packedData, 0, 0);
}

The intended method to add a new local token for an underlying is by calling function addLocalToken at the branch chain. However, it appears that the last line of code, IBridgeAgent(localBridgeAgentAddress).performCallOut{value: msg.value}(msg.sender, packedData, 0, 0); uses performCallOut instead of performSystemCallOut. This means that users can directly callOut at branch bridge agent with _params = abi.encodePacked(bytes1(0x02), abi.encode(_underlyingAddress, anyContract, name, symbol)) to invoke _addLocalToken at the root chain without calling addLocalToken first. As a result, they may set an arbitrary contract as the local token. It's worth noting that the impact is irreversible as there is no mechanism to modify or delete local tokens, meaning that the underlying token can never be properly bridged in the future.

The branch hToken is called by function bridgeIn when redeemDeposit or clearToken:

function bridgeIn(address _recipient, address _localAddress, uint256 _amount)
    external
    virtual
    requiresBridgeAgent
{
    ERC20hTokenBranch(_localAddress).mint(_recipient, _amount);
}

Below are several potetial exploiting methods:

1. If a regular ERC20 contract with admin minting permissions is set, exploiter can mint unlimited amount of local token for himself. By bridging them, he can receive an arbitrary amount of global token at the root chain.
2. If an unrelated contract with empty mint function is set, the underlying asset would be unable to be bridged in from the root chain, and users who attempt to do so could lose their assets.
3. If a malicious contract is set, gas griefing is possible.
4. This contract may serve as an intermediary for reentrancy. (I haven't found a concrete way so far but there is a potential risk)

Tools Used

Manual

Recommended Mitigation Steps

Use performSystemCallOut and executeSystemRequest to send Cross-Chain request for adding local token.

Assessed type

Access Control

Lines of code

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

Vulnerability details

Impact

assetId of lastAsset is incorrectly updated in function removeAsset, leading to data coruption.

Proof of Concept

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

    // Id should be index + 1
    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 removing an asset, the algorithm moves the last asset to the position of the asset to be removed. However, the Id of the last asset is updated as assetId[lastAsset] = assetIndex; breaking the relationship id = index + 1. In future operations, unexpected outcomes may occur due to this error.

Tools Used

Manual review

Recommended Mitigation Steps

assetId[lastAsset] = assetIndex + 1;

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

requiresFallbackGas cannot guarantee adequate gas for anyFallback. If fallback execution _forceRevert(), the user can no longer redeemDeposit and their asset will be permanently lost.

Proof of Concept

function _payFallbackGas(uint32 _depositNonce, uint256 _initialGas) internal virtual {
    uint256 gasLeft = gasleft();
    uint256 minExecCost = tx.gasprice * (MIN_FALLBACK_RESERVE + _initialGas - gasLeft);

    if (minExecCost > getDeposit[_depositNonce].depositedGas) {
        _forceRevert();
        return;
    }
    ......
}

function _requiresFallbackGas() internal view virtual {
    if (msg.value <= MIN_FALLBACK_RESERVE * tx.gasprice) revert InsufficientGas();
}

There are two possibilities that deposited gas would be insufficient for fallback execution:

1. msg.value is only slightly larger than tx.gasprice * MIN_FALLBACK_RESERVE but smaller than tx.gasprice * (MIN_FALLBACK_RESERVE + _initialGas - gasLeft)
2. tx.gasprice rises during two transactions.

Tools Used

Manual review

Recommended Mitigation Steps

Charge and replenish fallback gas during the initial deposit call and never forceRevert() fallback execution (consequence is severe).

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

Wrong AnycallFlag FLAG_ALLOW_FALLBACK is used in _performCall of BranchBridgeAgent. Gas are credited locally instead of remotely, despite gas has already been deposited to bridge out.

Proof of Concept

function _performCall(bytes memory _calldata) internal virtual {
    //Sends message to AnycallProxy
    IAnycallProxy(localAnyCallAddress).anyCall(
        rootBridgeAgentAddress, _calldata, rootChainId, AnycallFlags.FLAG_ALLOW_FALLBACK, ""
    );
}

According to the doc, Execution gas fees are credited to the recipient contract (Bridge Agent) deducting the gas spent from this contract's executionBudget kept in the AnycallConfig contract.

Tools Used

Manual

Recommended Mitigation Steps

Use flag FLAG_ALLOW_FALLBACK_DST

function _performCall(bytes memory _calldata) internal virtual {
    //Sends message to AnycallProxy
    IAnycallProxy(localAnyCallAddress).anyCall(
        rootBridgeAgentAddress, _calldata, rootChainId, AnycallFlags.FLAG_ALLOW_FALLBACK_DST, ""
    );
}

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

When a remote call with deposit(settlement) fails at bridge agent while it is not force reverted, anyFallback will be triggered to set the deposit(settlement) status to Failed. Then, the user should call redeemDeposit(Settlement) to get their assets back. However, if someone calls retryDeposit(Settlement), the status will be set to Success. The remote call will _forceRevert() and anyFallback will not be triggered again, which means the assets will be permanently lost.

function anyExecute(bytes calldata data)
    external
    virtual
    requiresExecutor
    returns (bool success, bytes memory result)
{
......
    if (executionHistory[fromChainId][nonce]) {
            _forceRevert();
            //Return true to avoid triggering anyFallback in case of `_forceRevert()` failure
            return (true, "already executed tx");
        }
......
}

Proof of Concept

function redeemDeposit(uint32 _depositNonce) external lock {
    if (getDeposit[_depositNonce].status != DepositStatus.Failed) {
        revert DepositRedeemUnavailable();
    }
    _redeemDeposit(_depositNonce);
}

function retryDeposit(
    bool _isSigned,
    uint32 _depositNonce,
    bytes calldata _params,
    uint128 _remoteExecutionGas,
    uint24 _toChain
) external payable lock requiresFallbackGas {
    if (getDeposit[_depositNonce].owner != msg.sender) revert NotDepositOwner();
    ......
    getDeposit[_depositNonce].status = DepositStatus.Success;
    _performCall(packedData);
}

Thanks to the owner check, this situation can only occur as a result of user mistake when depositing.

function redeemSettlement(uint32 _depositNonce) external lock {
    address depositOwner = getSettlement[_depositNonce].owner;

    if (getSettlement[_depositNonce].status != SettlementStatus.Failed || depositOwner == address(0)) {
        revert SettlementRedeemUnavailable();
    } else if (
        msg.sender != depositOwner && msg.sender != address(IPort(localPortAddress).getUserAccount(depositOwner))
    ) {
        revert NotSettlementOwner();
    }
    _redeemSettlement(_depositNonce);
}

function _retrySettlement(uint32 _settlementNonce) internal returns (bool) {
    Settlement memory settlement = getSettlement[_settlementNonce];
    if (settlement.owner == address(0)) return false;

    bytes memory newGas = abi.encodePacked(_manageGasOut(settlement.toChain));
    for (uint256 i = 0; i < newGas.length;) {
        settlement.callData[settlement.callData.length - 16 + i] = newGas[i];
        unchecked {
            ++i;
        }
    }

    Settlement storage settlementReference = getSettlement[_settlementNonce];
    settlementReference.gasToBridgeOut = userFeeInfo.gasToBridgeOut;
    settlementReference.callData = settlement.callData;
    settlementReference.status = SettlementStatus.Success;

    _performCall(settlement.callData, settlement.toChain);
    return true;
}

However, when it comes to settlement, there is no owner check in retrySettlement. As a result, any malicious user can call retrySettlement to freeze the assets of innocent users whenever they detect a failed settlement.

Tools Used

Manual

Recommended Mitigation Steps

Add a check in retryDeposit(Settlement):

require(getDeposit[_depositNonce].status != DepositStatus.Failed);

Assessed type

Invalid Validation

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 will fail because an extra wrappedNativeToken.withdraw is made.

Proof of Concept

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

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

When an EOA calls depositGasAnycallConfig, ether is sent to the contract. However, an unnecessary wrappedNativeToken.withdraw is performed.

Tools Used

Manual

Recommended Mitigation Steps

Move wrappedNativeToken.withdraw(_executionGasSpent); outside _replenishGas to Line 1172

function anyExecute(bytes calldata data) {
    ......
    if (initialGas > 0) {
++      wrappedNativeToken.withdraw(_executionGasSpent);
        _payExecutionGas(userFeeInfo.depositedGas, userFeeInfo.gasToBridgeOut, _initialGas, fromChainId);
    }
}

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

Incorrect parameter is used for _unstakeToken() in function "restakeToken". This mistake prevents users from restaking other users' token after incentive ends.

Proof of Concept

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

The third parameter of _unstakeToken is set to true. However, based on the context in the "restakeToken" function, it should be false to enable anyone to call restakeToken if the block time is after the end time of incentive.

function _unstakeToken(IncentiveKey memory key, uint256 tokenId, bool isNotRestake) private {
    ......
    // 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();
    ......
}

Recommended Mitigation Steps

Set the third parameter to false.

if (incentiveId.startTime != 0) _unstakeToken(incentiveId, tokenId, false);

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

Virtual account can perform external calls during root chain execution process. If it callOut at Arbitrum Branch Bridge Agent, anyExecute in Root Bridge Agent will be reentered. lock will not work if user initiated the process on another branch chain. _payExecutionGas will not charge gas for the reentrancy call, Meanwhile, storage variable initialGas and userFeeInfo will be deleted. As a result, no gas will be charged for the original call.

Proof of Concept

function anyExecute(bytes calldata data)
    external
    virtual
    requiresExecutor
    returns (bool success, bytes memory result)
{
    uint256 _initialGas = gasleft();
    uint24 fromChainId;
    UserFeeInfo memory _userFeeInfo;

    if (localAnyCallExecutorAddress == msg.sender) {
        initialGas = _initialGas;
        (, uint256 _fromChainId) = _getContext();
        fromChainId = _fromChainId.toUint24();

        _userFeeInfo.depositedGas = _gasSwapIn(
            uint256(uint128(bytes16(data[data.length - PARAMS_GAS_IN:data.length - PARAMS_GAS_OUT]))), fromChainId).toUint128();
        _userFeeInfo.gasToBridgeOut = uint128(bytes16(data[data.length - PARAMS_GAS_OUT:data.length]));
    } else {
        fromChainId = localChainId;
        _userFeeInfo.depositedGas = uint128(bytes16(data[data.length - 32:data.length - 16]));
        _userFeeInfo.gasToBridgeOut = _userFeeInfo.depositedGas;
    }

    if (_userFeeInfo.depositedGas < _userFeeInfo.gasToBridgeOut) {
        _forceRevert();
        return (true, "Not enough gas to bridge out");
    }

    userFeeInfo = _userFeeInfo;

    // execution part
    ............

    if (initialGas > 0) {
        _payExecutionGas(userFeeInfo.depositedGas, userFeeInfo.gasToBridgeOut, _initialGas, fromChainId);
    }
}

function _payExecutionGas(uint128 _depositedGas, uint128 _gasToBridgeOut, uint256 _initialGas, uint24 _fromChain) internal {
    delete(initialGas);
    delete(userFeeInfo);

    if (_fromChain == localChainId) return;

    uint256 availableGas = _depositedGas - _gasToBridgeOut;
    uint256 minExecCost = tx.gasprice * (MIN_EXECUTION_OVERHEAD + _initialGas - gasleft());
    if (minExecCost > availableGas) {
        _forceRevert();
        return;
    }

    _replenishGas(minExecCost);

    accumulatedFees += availableGas - minExecCost;
}

During the reentrancy call, initialGas will not be modified before the execution part; _payExecutionGas will be invoked, but it will directly return after deleting initialGas and userFeeInfo. As a result, after the execution part of original call, _payExecutionGas will be passed as initialGas is now zero.

Tools Used

Manual

Recommended Mitigation Steps

Store initialGas and userFeeInfo in memory as local variable inside anyExecute.

Assessed type

Reentrancy

Lines of code

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

Vulnerability details

Impact

Incorrect accounting logic for fallback gas will lead to insolvency.

Proof of Concept

// on root chain
function _payExecutionGas(uint128 _depositedGas, uint128 _gasToBridgeOut, uint256 _initialGas, uint24 _fromChain) internal {
    ......
    uint256 availableGas = _depositedGas - _gasToBridgeOut;
    uint256 minExecCost = tx.gasprice * (MIN_EXECUTION_OVERHEAD + _initialGas - gasleft());

    if (minExecCost > availableGas) {
        _forceRevert();
        return;
    }

    _replenishGas(minExecCost);

    //Account for excess gas
    accumulatedFees += availableGas - minExecCost;
}

// on branch chain
function _payFallbackGas(uint32 _depositNonce, uint256 _initialGas) internal virtual {
    ......
    IPort(localPortAddress).withdraw(address(this), address(wrappedNativeToken), minExecCost);
    wrappedNativeToken.withdraw(minExecCost);
    _replenishGas(minExecCost);
}

As above, when paying execution gas on the root chain, the excessive gas is added to accumulatedFees. So theoretically all deposited gas is used up and no gas has been reserved for anyFallback on the branch chain. The withdrawl in _payFallbackGas on branch chain will cause insolvency.

// on branch chain
function _payExecutionGas(address _recipient, uint256 _initialGas) internal virtual {
    ......
    uint256 gasLeft = gasleft();
    uint256 minExecCost = tx.gasprice * (MIN_EXECUTION_OVERHEAD + _initialGas - gasLeft);

    if (minExecCost > gasRemaining) {
        _forceRevert();
        return;
    }

    _replenishGas(minExecCost);

    //Transfer gas remaining to recipient
    SafeTransferLib.safeTransferETH(_recipient, gasRemaining - minExecCost);
    ......
    }
}

// on root chain
function _payFallbackGas(uint32 _settlementNonce, uint256 _initialGas) internal virtual {
    uint256 gasLeft = gasleft();
    uint256 minExecCost = tx.gasprice * (MIN_FALLBACK_RESERVE + _initialGas - gasLeft);

    if (minExecCost > getSettlement[_settlementNonce].gasToBridgeOut) {
        _forceRevert();
        return;
    }

    getSettlement[_settlementNonce].gasToBridgeOut -= minExecCost.toUint128();
}

As above, when paying execution gas on the branch chain, the excessive gas has be sent to the recipent. So therotically all deposited gas is used up and no gas has been reserved for anyFallback on the root chain. _payFallbackGas does not _replenishGas, which will cause insolvency of gas budget in AnycallConfig.

Tools Used

Manual

Recommended Mitigation Steps

Deduct fallback gas from deposited gas.

Assessed type

Context