Maia DAO Ecosystem - Emmanuel's results

Lines of code

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

Vulnerability details

Impact

Attacker will cause user's funds to be collected and locked on Branch chain without it being recorded on root chain.

Proof of Concept

Anyone can call BranchBridgeAgent#retrieveDeposit, with an invalid _depositNonce:

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

If for example, global depositNonce is x, attacker can call retrieveDeposit(x+y). RootBridgeAgent#anyExecute will be called, and the executionHistory for the depositNonce that the attacker specified would be updated to true:

    function anyExecute(bytes calldata data){
        ...
    /// 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");
    }
    }
    ...
    }

This means that when a user makes a deposit on that BranchBridgeAgent and his Deposit gets assigned a depositNonce which attacker previously called retrieveDeposit for, his tokens would be collected on that BranchBridgeAgent, but would not succeed on RootBridgeAgent because executionHistory for that depositNonce has already been maliciously set to true.

Attack Scenario

• current global deposit nonce is 50
• attacker calls retrieveDeposit(60), which would update executionHistory of depositNonce 60 to true on Root chain
• when a user tries to call any of the functions(say callOutAndBridge), and gets assigned depositNonce of 60, it won't be executed on root chain because executionHistory for depositNonce 60 is already set to true
• User won't also be able to claim his tokens because anyFallback was not triggered. So he has lost his deposit.

Tools Used

Manual Review

Recommended Mitigation Steps

A very simple and effective solution is to ensure that in the BranchBridgeAgent#retrieveDepoit function, msg.sender==getDeposit[_depositNonce].owner just like it was done in BranchBridgeAgent#retryDeposit

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

A malicious user would make a deposit specifying an hToken of a high value(say hEther), and a depositToken of relatively lower value(say USDC), and for that user, RootBridgeAgent would increment his hToken balance by the amount of depositTokens he sent

Proof of Concept

Here is the checkParams function:

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

The function performs 3 checks:

• _dParams.amount must be less than or equal to _dParams.deposit
• If _dParams.amount>0, _dParams.hToken must be a valid localToken
• If _dParams.deposit>0, _dParams.token must be a valid underlying token.

The PROBLEM is that the check only requires that getLocalTokenFromUnder[_dParams.token]!=address(0), but does not check that getLocalTokenFromUnder[_dParams.token]==_dParams.hToken:

    function isUnderlyingToken(
        address _underlyingToken,
        uint24 _fromChain
    ) external view returns (bool) {
        return
            getLocalTokenFromUnder[_underlyingToken][_fromChain] != address(0);
    }

The checkParams function is used in the RootBridgeAgent#bridgeIn function.

This allows a user to call BranchBridgeAgent#callOutAndBridge with a hToken and token that are not related

ATTACK SCENARIO

• Current price of Ether is 1800USDC
• RootBridgeAgent is deployed on Arbitrum
• BranchBridgeAgent for Ethereum mainnet has two local tokens recorded in RootBridgeAgent:
  • hEther(whose underlying is Ether)
  • hUSDC(whose underlying is USDC)
• Alice calls BranchBridgeAgent#callOutAndBridge on ethereum with the following as DepositInput(_dParams):
  • hToken(address of local hEther)
  • token(address of USDC)
  • amount(0)
  • deposit(10)
  • toChain(42161)
• BranchPort#bridgeOut transfers 10 USDC from user to BranchPort, and anyCall call is made to RootBridgeAgent
• RootBridgeAgent#bridgeIn is called which calls CheckParamsLib.checkParams
  • checkParams verifies that _dParams.amount(0) is less than or equal to _dParams.deposit(10)✅
  • verifies that _dParams.hToken(hEther) is a valid localToken✅
  • verifies that _dParams.token(USDC) is a valid underlying token(i.e. its local token is non zero)✅
• RootBridgeAgent#bridgeIn calls RootPort#bridgeToRoot which mints 10 global hEther to user if (_deposit > 0) mint(_recipient, _hToken, _deposit, _fromChainId);
• With just 10 USDC, user has been able to get 10 ether(18000USDC) worth of funds on root chain.

Execution flow: BranchBridgeAgent#callOutAndBridge->BranchBridgeAgent#_callOutAndBridge->BranchBridgeAgent#_depositAndCall->BranchBridgeAgent#_performCall->RootBridgeAgent#anyExecute->RootBridgeAgentExecutor#executeWithDeposit->RootBridgeAgentExecutor#_bridgeIn->RootBridgeAgent#bridgeIn

Tools Used

Manual Review

Recommended Mitigation Steps

Currently, the protocol only checks if the token is recognized by rootport as an underlying token by checking that the registered local token for _dParams.token is not zero address.

• Instead of that, it would be more effective to check that the registered local token for _dParams.token is equal to _dParams.hToken.
• Some sanity checks may also be done on DepositInput(_dParams) in BranchBridgeAgent. Although this is not necessary.

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/BranchBridgeAgent.sol#L433-L439 https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/BranchBridgeAgent.sol#L1243-L1306

Vulnerability details

Impact

There is a function retrieveDeposit in BranchBridgeAgent. But this function is totally unusable and does not serve its intended purpose because there was an omission in implementing its logic.

Proof of Concept

retrieveDeposit attaches a 0x08 flag:

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

For the 0x08 flag in RootBridgeAgent#anyExecute, function doesn't do anything on Root chain even when depositNonce is valid, but simply returns a "false" success value to trigger BranchBridgeAgent#anyFallback:

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

    //Unrecognized Function Selector
}

But if you check all if statements in BranchBridgeAgent#anyFallback, there is no code that handles 0x08 flag. https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/BranchBridgeAgent.sol#L1243-L1306 Therfore, the call would return (false, "unknown selector");, meaning that retrieveDeposit is unusable.

Tools Used

Manual Review

Recommended Mitigation Steps

According to the sponsor, it was an omission. In BranchBridgeAgent#anyFallback, instead of if ((flag == 0x00) || (flag == 0x01) || (flag == 0x02)), it should be if ((flag == 0x00) || (flag == 0x01) || (flag == 0x02) || (flag == 0x08))

Assessed type

Error

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/BranchBridgeAgent.sol#L1158 https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/BranchBridgeAgent.sol#L1182 https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/BranchBridgeAgent.sol#L1206

Vulnerability details

Impact

It will be impossible to retry a settlement once it fails on ArbitrumBranchBridgeAgent, which could lead to loss of funds.

Proof of Concept

RootBridgeAgent calls ArbitrumBranchBridgeAgent#anyExecute which would bridge in assets. The execution flow for settlement on a BranchBridgeAgent on a chain apart from Arbitrum, goes as follows:

• User makes a call on BranchBridgeAgent to withdraw his tokens on RootBridgeAgent by calling one of the functions e.g. callOut
• User's call is relayed to RootBridgeAgent
• RootBridgeAgent updates the accounting(reduces global hToken balance in case of a withdraw)
• RootBridgeAgent informs BranchBridgeAgent of the settlement that was made on root chain, so that branch chain can also update accounting of the recipient. Multichain's anyCall is used to call BranchBridgeAgent, which would call BranchBridgeAgent#anyExecute.
• If anyExecute reverts or returns a "false" success value, anyFallback would be called on RootBridgeAgent, which would allow user to retry or redeem the settlement.
• Instead of reverting which may expose some attack vectors, protocol wraps the major deposit interaction in a try-catch block, and return false when an error is caught. The assumption is that multichain would also call anyFallback in RootBridgeAgent when it sees that success value is false.

To the best of my understanding, this assumption is correct if BranchBridgeAgent is not ArbitrumBranchBridgeAgent. But for ArbitrumBranchBridgeAgent, RootBridgeAgent calls ArbitrumBranchBridgeAgent#anyExecute directly, and does not use Multichain's anyCall because both of the contracts are on the same chain. Therefore, returning a "false" success value when RootBridgeAgent calls ArbitrumBranchBridgeAgent will not trigger anyFallback, and users settlement will not be reopened for retrying or redemption.

Tools Used

Manual Review

Recommended Mitigation Steps

ArbitrumBranchBridgeAgent should call RootBridgeAgent#anyFallback whenever a settlement is unsuccessful.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/RootBridgeAgent.sol#L981 https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/RootBridgeAgent.sol#L1005 https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/RootBridgeAgent.sol#L1073 https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/RootBridgeAgent.sol#L1108 https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/RootBridgeAgent.sol#L1134

Vulnerability details

Impact

This causes loss of users funds because when they deposit on ArbitrumBranchBridgeAgent but the deposit was unsuccessful on RootBridgeAgent, the call will not revert(because of try-catch), but rether, returns a "false" success value, and since multicall is not the one that called anyExecute, returning a "false" success value will not trigger anyFallback(which clears users' deposit for redemption) on ArbitrumBranchBridgeAgent.

Proof of Concept

The execution flow for making a deposit from a BranchBridgeAgent on a chain apart from Arbitrum, to RootBridgeAgent goes as follows:

• User deposits by calling one of the available functions e.g. callOutAndBridge
• User's tokens are collected on BranchBridgeAgent
• Multichain's anyCall is used to initiate the cross chain request
• Multichain calls anyExecute on the destination contract
• If anyExecute reverts or returns a "false" success value, anyFallback would be called on source contract, which would allow user to claim their deposits
• Instead of reverting which may expose some attack vectors, protocol wraps the major deposit interaction in a try-catch block, and return false when an error is caught. The assumption is that multichain would call anyFallback in BranchBridgeAgent when it sees that success value is false.

To the best of my understanding, this assumption is correct if BranchBridgeAgent is not ArbitrumBranchBridgeAgent. But for ArbitrumBranchBridgeAgent, RootBridgeAgent#anyExecute is called directly by ArbitrumBranchBridgeAgent, and does not use Multichain's anyCall because both of the contracts are on the same chain. Therefore, returning a "false" success value when ArbitrumBranchBridgeAgent calls RootBridgeAgent will not trigger anyFallback, and users' deposit will not be cleared, but locked forever.

Tools Used

Manual Review

Recommended Mitigation Steps

RootBridgeAgent should call ArbitrumBranchBridgeAgent#anyFallback whenever a deposit from Arbitrum branch was unsuccessful.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/RootBridgeAgent.sol#L873-L890 https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/RootBridgeAgent.sol#L922 https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/RootBridgeAgent.sol#L246 https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/RootBridgeAgent.sol#L571

Vulnerability details

Impact

Wrong userFeeInfo will be used when retrySettlement is called directly.

Proof of Concept

Here is retrySettlement function:

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

The assumption here is that if initialGas is not 0, then retrySettlement is being called by RootBridgeAgent#anyExecute, which has already set values for initialGas and userFeeInfo(which would later be deleted at end of the anycall function), but if it is 0, then retrySettlement is being called directly by a user, so user should specify _remoteExecutionGas and send some msg.value with the call, which would make up the userFeeInfo. But this assumption is not completely correct because whenever RootBridgeAgent#anyExecute is called with a depositNonce that has been recorded in executionHistory, the function returns early, which prevents other parts of the anyExecute function from being executed. At the beginning of anyExecute, initialGas and userFeeInfo values are set and at the end of anyExecute call, if initialGas>0, _payExecutionGas sets initialGas and userFeeInfo to 0. So when the function returns earlier, before _payExecutionGas is called, initialGas and userFeeInfo are not updated. If a user calls retrySettlement immediately after that, the call will use a wrong userFeeInfo(i.e. userFeeInfo set when anyExecute was called with a depositNonce that has already been recorded) because initialGas!=0. Whereas, it was meant to use values sent by caller of retrySettlement

Looking at a part of _manageGasOut logic which is called in _retrySettlement,

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

This could cause one of these:

• User's retrySettlement call would revert if userFeeInfo.gasToBridgeOut(which user does not have control over) is less than MIN_FALLBACK_RESERVE * tx.gasprice
• User's call passes without him sending any funds, so he makes a free retrySettlement transaction

Tools Used

Manual Review

Recommended Mitigation Steps

Consider implementing one of these:

• restrict retrySettlement to only be called by AgentExecutor
• or delete initialGas and userFeeInfo before return is called if nonce has been executed before:

//Check if tx has already been executed
if (executionHistory[fromChainId][nonce]) {
    _forceRevert();
    delete initialGas;
    delete userFeeInfo;
    //Return true to avoid triggering anyFallback in case of `_forceRevert()` failure
    return (true, "already executed tx");
}

Assessed type

Error