Maia DAO Ecosystem - ltyu's results

Lines of code

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

Vulnerability details

Impact

Increasing weights adds more weight than expected, which breaks rebalancing functionality down the line.

Proof of Concept

A pool weight can be increased with setWeight of UlyssesPool. Once the weight is increased, bandwidth is redistributed to the target poolId first. Afterward, any leftOverBandwidth should be distributed to the other pools, pro rata. However, this is not done correctly.

According to the code, the bandwidthStateList is looped over and the leftOverBandwidth is distributed proportional to their respective weights, which works as intended. Once it reaches the last pool on the list, the entire leftOverBandwidth is added to it. This leads to extra bandwidth to the last pool.

Here is the gist that tries to distribute the leftOverBandwidth.

function setWeight(uint256 poolId, uint8 weight) external nonReentrant onlyOwner {
...
	for (uint256 i = 1; i < bandwidthStateList.length;) {
		if (i != poolIndex) {
			if (i == bandwidthStateList.length - 1) {
				// @audit Last pool always receives entire leftOverBandwidth
				bandwidthStateList[i].bandwidth += leftOverBandwidth.toUint248();
			} else if (leftOverBandwidth > 0) {
				bandwidthStateList[i].bandwidth +=
					leftOverBandwidth.mulDiv(bandwidthStateList[i].weight, weightsWithoutPool).toUint248();
			}
		}
	
		unchecked {
			++i;
		}
	}
}

Tools Used

Recommended Mitigation Steps

Consider subtracting from leftOverBandwidth after each loop iteration. Once the loop gets to the last item, it should be the correct amount.

Assessed type

Math

Lines of code

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

Vulnerability details

Impact

Decreasing weights causes an underflow error, which breaks functionality.

Proof of Concept

A pool weight can be decreased with setWeight of UlyssesPool. Once the weight is decreased for a target poolId, the other pool bandwidths are increased, and then leftOverBandwidth is calculated.

if (oldTotalWeights > newTotalWeights) {
	for (uint256 i = 1; i < bandwidthStateList.length;) {
		if (i != poolIndex) {
			uint256 oldBandwidth = bandwidthStateList[i].bandwidth;
			if (oldBandwidth > 0) {
				// @audit This will increase bandwidth to an amount higher than oldBandwidth
				bandwidthStateList[i].bandwidth =
					oldBandwidth.mulDivUp(oldTotalWeights, newTotalWeights).toUint248();
				// @audit if oldbandwidth > the new bandwidthStateList[i].bandwidth, this will cause an underflow
				leftOverBandwidth += oldBandwidth - bandwidthStateList[i].bandwidth;
			}
		}

		unchecked {
			++i;
		}
	}

	poolState.bandwidth += leftOverBandwidth.toUint248();

// @audit-info increasing weight (newTotalWeights > oldTotalWeights)
}

However, this will not work correctly as calculating leftOverBandwidth will cause an underflow. Since oldTotalWeights is greater than newTotalWeights, the new bandwidthStateList[i].bandwidth will always be greater than oldBandwidth.

This is exemplified below:

Given:

• oldTotalWeights: 20
• newTotalWeights: 10
• oldBandwidth: 100

The calculation for bandwidthStateList[i].bandwidth will be 100 x 20 / 10 = 200. Afterwards, leftOverBandwidth will be calculated as 100 - 200 = -100. An underflow will occur.

Tools Used

Manual

Recommended Mitigation Steps

Consider changing this line to:

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

Assessed type

Under/Overflow

Lines of code

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

Vulnerability details

Impact

Target pool bandwidth calculation is incorrect, which leads to incorrect pool balancing.

Proof of Concept

In setWeights of UlyssesPool.sol, the leftOverBandwidth gets added to the target poolState.bandwidth. As discussed with the Sponsor, this is incorrect because when weight is decreased, the bandwidth should decrease to distribute to the other pools.

This PoC uses a new test file: UlyssesFactoryTest.t.sol. Run the test by calling forge test --match-test testReduceWeightIncreasesBandwidth

import {Test, StdUtils} from "forge-std/Test.sol";
import {UlyssesFactory} from "@ulysses-amm/factories/UlyssesFactory.sol";
import {UlyssesPool} from "@ulysses-amm/UlyssesPool.sol";
import {ERC20} from "solmate/tokens/ERC20.sol";
import {MockERC20} from "solmate/test/utils/mocks/MockERC20.sol";

contract InvariantUlyssesFactory is Test {
    struct BandwidthState {
        uint248 bandwidth;
        uint8 weight;
        UlyssesPool destination;
    }
    UlyssesFactory ulyssesFactory;
    ERC20 token1;
    ERC20 token2;
    address alice = address(1);
    function setUp() public {
        ulyssesFactory = new UlyssesFactory(address(this));
        token1 = new MockERC20("Token1", "TK1", 18);
        token2 = new MockERC20("Token2", "TK2", 18);
        deal(address(token1), alice, 1 ether);
    }
    function testReduceWeightIncreasesBandwidth() public {
        ERC20[] memory assets = new ERC20[](4);
        assets[0] = token1;
        assets[1] = token1;
        assets[2] = token1;
        assets[3] = token1;

        uint8[][] memory weights = new uint8[][](4); // outer
        // Initialize the inner arrays
        weights[0] = new uint8[](4);
        weights[1] = new uint8[](4);
        weights[2] = new uint8[](4);
        weights[3] = new uint8[](4);

        // Pool1 weights
        weights[0][0] = 5;
        weights[0][1] = 10;
        weights[0][2] = 10;
        weights[0][3] = 10;

        // Pool2 weights
        weights[1][0] = 5;
        weights[1][1] = 10;
        weights[1][2] = 10;
        weights[1][3] = 10;

        // Pool3 weights
        weights[2][0] = 5;
        weights[2][1] = 10;
        weights[2][2] = 10;
        weights[2][3] = 10;

        // Pool4 weights
        weights[3][0] = 5;
        weights[3][1] = 10;
        weights[3][2] = 10;
        weights[3][3] = 10;
        
        
        uint256[] memory poolIds = ulyssesFactory.createPools(assets, weights, address(this));
        UlyssesPool pool1 = ulyssesFactory.pools(poolIds[0]); // id = 2

        vm.startPrank(alice);
        token1.approve(address(pool1), 0.1 ether);
        pool1.deposit(0.1 ether, alice);
        vm.stopPrank();
        
        uint256 pool1BandwidthBefore = pool1.getBandwidth(3);

        // Set weight to lower
        pool1.setWeight(3, 5);

        uint256 pool1BandwidthAfter = pool1.getBandwidth(3);
        assertEq(pool1BandwidthAfter > pool1BandwidthBefore, true);
    }
}

Tools Used

Manual

Recommended Mitigation Steps

Consider changing this line to:

poolState.bandwidth -= leftOverBandwidth.toUint248();

Assessed type

Math

Lines of code

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

Vulnerability details

Impact

depositToPort() mints incorrect hToken amounts because token mints are inflated/deflated depending on the underlying decimal.

Proof of Concept

In depositToPort() of ArbitrumBranchPort.sol, the BridgeAgent transfers an underlying _deposit amount from the depositor, and subsequently mints the same _deposit amount. This is fine when the underlying decimal is 18, but is incorrect with decimals not equal to 18. The amount that is transferred and minted should not use the same _decimal variable. Instead, the mint amount should be a denormalized amount i.e., converted to 1e18 equivalent amount.

For example, given an underlying token with a decimal of 6, and the _deposit amount of 1e6. Currently, the amount transferred will be 1e6, which is correct. But, the amount minted will also be 1e6. This means that tokens with higher decimals will mint more hTokens. As discussed with the Sponsor, the amount should be 1e18 in this case.

Tools Used

Manual review

Recommended Mitigation Steps

Consider denormalization of the _deposit amount to 1e18 for the mintToLocalBranch() amount.

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

Assessed type

Math

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/BranchBridgeAgent.sol#L1340-L1342 https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/BranchBridgeAgent.sol#L687 https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/BranchBridgeAgent.sol#L252 https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/BranchBridgeAgent.sol#L284 https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/BranchBridgeAgent.sol#L341-L343

Vulnerability details

Impact

Functions that rely on _normalizeDecimals() does not work as intended. It does not handle decimals other than 18 correctly. This results in incorrect accounting down the line.

Proof of Concept

Throughout BranchBridgeAgent.sol, _normalizeDecimals() is used to convert an underlying deposit amount to 18 decimal places. After discussing with the Sponsor, _normalizeDecimals() is intended to convert an amount to 1e18. However, it does not work correctly for decimals other than 18.

Take for example, when a user calls BaseBranchRouter.callOutAndBridge() with dParams.deposits of 1e6 (token decimal 6), the subsequent call of BranchBridgeAgent._callOutAndBridge() will 1) deposit 1e6 tokens on behalf of the user, and 2) pack the value of _normalizeDecimals(1e6, 6). In this case, it will be 1e6 * (10 ** 6) / 1 ether, which results in 0.

When the data reaches the Root chain, no RootTokens will be minted despite the user depositing 1e6 underlying tokens.

A similar, but opposite, scenario can happen with token decimals larger than 18.

Tools Used

Manual

Recommended Mitigation Steps

Consider fixing _normalizeDecimals() to handle decimals other than 18.

Assessed type

Math

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/RootPort.sol#L282 https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/BranchPort.sol#L248 https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/BranchPort.sol#L275

Vulnerability details

Impact

Root hTokens will be minted based on the decimals of the underlying deposit, which causes incorrect accounting.

Proof of Concept

In bridgeToRoot() of RootPort.sol, the root hToken is transferred to the _recipient. The amount that is transferred is the difference between _amount and _deposit. As discussed with the Sponsor, _amount is an amount of hTokens, which is decimal 18. The _deposit is the deposit amount of underlying, which can have varying decimals. This results in incorrect accounting, because _deposit amounts using decimals lower than 18 will result in more tokens transferred. Tokens with decimals greater than 18, can result in an underflow because _deposit is greater than _amount.

There are other places where amount - deposit is used, but does not take into account the deposit decimals, such as:

• BranchPort.bridgeOut()
• BranchPort.bridgeOutMultiple()
• BranchBridgeAgent._clearToken()

Tools Used

Manual

Recommended Mitigation Steps

Consider converting _deposit to 18 decimals to match the hToken decimals.

Assessed type

Decimal

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/BranchBridgeAgent.sol#L948-L952 https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/BranchBridgeAgent.sol#L984 https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/BranchPort.sol#L212

Vulnerability details

Impact

Underlying token amounts are incorrect when withdrawing deposits.

Proof of Concept

In _redeemDeposit() of BranchBridgeAgent.sol, a user can withdraw their failed deposit. First, the deposits amounts are fetched by the _depositNonce. Next, clearToken() is called to mint and withdraw the local port and underlying tokens, respectively. When withdraw() is called, the _deposit amount is first converted using _denormalizeDecimals(). However, the result of the conversion is incorrect when token decimals are not 18.

Here are some examples when _denormalizeDecimals() is called:

• _deposit of 1e6, decimal 6: 1e6 * 1 ether / (10 ** 6) = 1e18
• _deposit of 1e20, decimal 20: 1e20 * 1 ether / (10 ** 20) = 1e18

In both cases, the result is 1e18 which is incorrect because the underlying amount should be in their respective decimals. In both cases, using _deposit amount would be correct.

Tools Used

Manual

Recommended Mitigation Steps

Consider using the stored _deposit amount to transfer the underlying amount.

Assessed type

Decimal

Lines of code

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

Vulnerability details

Impact

Deposit amount of tokens of varying decimals are incorrectly converted, leading to incorrect accounting.

Proof of Concept

In bridgeOut() of BranchPort.sol, the underlying token is transferred from the _depositor to the BranchPort. The _deposit amount is first converted using _denormalizeDecimals(). However, the result of the conversion is incorrect when token decimals are not 18.

Here are some examples when _denormalizeDecimals() is called::

• _deposit of 1e6, decimal 6: 1e6 * 1 ether / (10 ** 6) = 1e18
• _deposit of 1e20, decimal 20: 1e20 * 1 ether / (10 ** 20) = 1e18

In both cases, the result is 1e18 which is incorrect because the underlying amount should be in their respective decimals. In both cases, using _deposit amount would be correct.

Tools Used

Manual

Recommended Mitigation Steps

Consider directly using _deposit since it is already in the correct decimal places.

Assessed type

Decimal

Lines of code

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

Vulnerability details

Impact

Cross-chain calls will fail since source-fee is not supplied to Anycall

Proof of Concept

In _performCall() of BranchBridgeAgent.sol, a cross-chain called is made using anyCall() with the _flag of 4. According to the Anycall V7 documentation and code, when using gas _flag of 4, the gas fee must be paid on the source chain. This means anyCall() must be called and sent gas.

However, this is not the case, and the result is _performCall will always revert. This will impact many functions that rely on this function such as callOut(), callOutSigned(), retryDeposit(), and etc.

Tools Used

Manual

Recommended Mitigation Steps

After discussing with the Sponsor, it is expected that the fee be paid on the destination chain, specifically the rootBridgeAgent. Consider refactoring the code to change the _flag to use pay on destination.

Alternatively, if pay on source is the intention, consider refactoring the code to include fees, starting with _performCall. Additional refactoring will be required.

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

Assessed type

Library

Lines of code

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

Vulnerability details

Impact

Certain functions require native tokens to be sent. These functions will revert.

Proof of Concept

According to the Sponsor, VirtualAccounts can "call any of the dApps present in the Root Chain (Arbitrum) e.g. Maia, Hermes, Ulysses AMM,Uniswap." However, this is not the case as call() is not payable and thus cannot send native tokens to other contracts. This is problematic because certain functions require native token transfers and will fail.

Tools Used

Manual

Recommended Mitigation Steps

Consider creating a single call() function that has a payable modifier and {value: msg.value}. Be aware that since calls[i].target.call() is in a loop, it is not advisable to add payable to the existing call(). This is because msg.value may be used multiple times, and is unsafe.

Assessed type

Payable

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/ccc9a39240dbd8eab22299737370996b2b833efd/src/ulysses-amm/factories/UlyssesFactory.sol#L93

Vulnerability details

Impact

In createPools of UlyssesFactory.sol, the return parameter poolIds is used to store new pool ids after creation. However, it has not been initialized. This causes an index out of bounds error when createPools is called.

Proof of Concept

Any test that calls ulyssesFactory.createPools(...); will cause index out of bounds

Tools Used

Manual review

Recommended Mitigation Steps

Consider adding this line:

poolIds = new uint256[](length);

Assessed type

Invalid Validation

As there was only so much time for such a large codebase, this audit was partially completed and the analysis is provided in the context of the completed modules. The modules that were completed are: Ulysses (AMM and Omnichain), and Hermes (with its dependencies).

Analysis of the codebase (What's unique? What's using existing patterns?)

Overall, the codebase is clean, with a lot of comments, and supporting documentation. The use of natspec when needed was not distracting, but was helpful where a little extra color was needed, such as when assembly was used.

Whether the devs are aware or not, I would like to gently remind that the use of SafeTransferLib may only give the disguise of being safe. The Library is intended to make token transfers safer by handling missing return values. However, as noted in the natspec, the Library does not check if the token has code. Keep this in mind in the future if return values are relied on.

Architecture feedback

Documentation I think the documentation could be more technical. The documentation has a lot of high level concepts that were hard to connect to the code without clarification.

For example, in the first paragraph of "Virtual Liquidity"

Virtual Liquidity refers to the mirroring of assets locked in different chains within a single liquidity management environment. This opens up doors to unmatched composability.

To someone uninitiated, it's unclear what the mirror of assets locked in different chains means, how this "opens up doors to unmatched composability".

There are also a few discrepancies between the code and documentation. For example, the first condition in the rebalancing fee table is incorrect. Although mistakes in inevitable, many mistakes add additional understanding time to auditors and other technical-minded readers.

I also appreciated that there were videos that walked through the code.

Centralization risks

One of the ethos I got from this project, is that decentralization is the developer's minds. Although there are a lot of onlyOwner type of functions, as noted by the Sponsor, a lot of the contracts' ownership can either be renounced as needed, or would be given to Governance.

Systemic risks

Anycall is one of the key components of making all this work. There is a lack of integration testing for this component.

Other recommendations

Test Coverage I think the biggest weakness of this codebase was that the coverage was unclear. As mentioned in the discord discussion, forge coverage does not work. From an auditor perspective, this created a lot of blindspots on what to focus on. As a developer, it's unclear what code remains untested. After reviewing some of the tests, I believe there are missing tests (such as UlyssesFactory). This resulted in many of the issues found.

Another minor nitpick, but related, is that of the reuse of custom errors. Some custom errors, such as DelegationError is used in multiple functions (see ERC20MultiVotes). This made debugging harder when I was running my own tests.

Time spent:

60 hours