Maia DAO - Ulysses - QiuhaoLi's results

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/main/src/VirtualAccount.sol#L85

Vulnerability details

Impact

Anyone can use a user's virtual account payableCall and calls functions like retrySettlement/redeemSettlement/retrieveSettlement that require the caller to be the virtual account, which can lead to economic loss to users.

Proof of Concept

In VirtualAccount.sol, we have two functions which aggregate calls ensuring each call is successful: call and payableCall. payableCall is added later since https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/VirtualAccount.sol#L41.

To prevent anyone use the aggregate call in VirtualAccount, we use the requiresApprovedCaller modifier which only let port approved address call:

    modifier requiresApprovedCaller() {
        if (!IRootPort(localPortAddress).isRouterApproved(this, msg.sender)) {
            if (msg.sender != userAddress) {
                revert UnauthorizedCaller();
            }
        }
        _;
    }

However, the payableCall didn't use requiresApprovedCaller modifier, so anyone can call this function:

    /// @inheritdoc IVirtualAccount
    function payableCall(PayableCall[] calldata calls) public payable returns (bytes[] memory returnData) {

So, anyone can call a user's virtual account payableCall and trigger privileged functions of that user. For example:

1. Alice triggers a settlement through the root agent bridge using her virtual account.
2. The settlement failed on the dst chain.
3. Time passed, and based on the current situation, Alice decides to retrieve the settlement.
4. However, Bob calls retrySettlement function first using Alice's virtual account payableCall, and the settlement is executed success on the dst chain.
5. Alice suffers from the settlement.

Tools Used

Manual Review.

Recommended Mitigation Steps

Add requiresApprovedCaller modifier to payableCall.

Assessed type

Access Control

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/main/src/ArbitrumBranchBridgeAgent.sol#L112

Vulnerability details

Impact

When users on root call ArbitrumBranchBridgeAgent deposit with _hasFallbackToggled set or Retrieve Settlement (flag = 0x03), the native gas sent by them (may not needed, but users can do that) can left in the contract and lost later.

Proof of Concept

When users call the branch agents through the root bridge agent, it will send received native gas tokens to the branch bridge agent, e.g., in the _performCall of src/RootBridgeAgent.sol:

    function _performCall() internal {
        } else {
            //Send Gas to Local Branch Bridge Agent
            callee.call{value: msg.value}("");  // @audit <-------- ArbitrumBranchBridgeAgent situaion
            //Execute locally
            IBranchBridgeAgent(callee).lzReceive(0, "", 0, _payload);
        }

And in the branch bridge agent, if it receives 0x03 (Retrieve Settlement) or 0x82 (Multiple Settlement with _hasFallbackToggled set), the branch bridge agent may call ILayerZeroEndpoint(lzEndpointAddress).send using the native tokens it received (exceed gas tokens will send to the refundee, which the original sender controls), so the sender won't lose his native tokens:

    function _performFallbackCall(address payable _refundee, uint32 _settlementNonce) internal virtual {
        //Sends message to LayerZero messaging layer
        ILayerZeroEndpoint(lzEndpointAddress).send{value: address(this).balance}( // @audit <----
            rootChainId,
            rootBridgeAgentPath,
            abi.encodePacked(bytes1(0x09), _settlementNonce),
            _refundee,
            address(0),

However, in ArbitrumBranchBridgeAgent.sol:_performFallbackCall(), which overrides the function above, it directly calls the IRootBridgeAgent(rootBridgeAgentAddress).lzReceive and doesn't send the received eth back (also it ignores the refundee parameter):

    function _performFallbackCall(address payable, uint32 _settlementNonce) internal override {
        //Sends message to Root Bridge Agent
        IRootBridgeAgent(rootBridgeAgentAddress).lzReceive(
            rootChainId, "", 0, abi.encodePacked(bytes1(0x09), _settlementNonce)
        );
    }

This can lead:

1. User A calls a root bridge agent with native tokens. The root bridge agent sends native tokens to ArbitrumBranchBridgeAgent directly and triggers the deposit with _hasFallbackToggled set or Retrieve Settlement (flag = 0x03).
2. ArbitrumBranchBridgeAgent's _performFallbackCall calls the root bridge agent and make the received eth left in it.
3. When ArbitrumBranchBridgeAgent receives 0x00, 0x01, etc. later, all the native balance will be transferred to a router, and the user A sent assets will be lost.

Tools Used

Manual Review.

Recommended Mitigation Steps

The ArbitrumBranchBridgeAgent.sol:_performFallbackCall() should transfer the native tokens to the refudee first and then call the root bridge agent's lzreceive:

    import {SafeTransferLib} from "solady/utils/SafeTransferLib.sol";
    function _performFallbackCall(address payable _refundee, uint32 _settlementNonce) internal override {
        address(_refundee).safeTransferETH(address(this).balance); // <----
        //Sends message to Root Bridge Agent
        IRootBridgeAgent(rootBridgeAgentAddress).lzReceive(
            rootChainId, "", 0, abi.encodePacked(bytes1(0x09), _settlementNonce)
        );
    }

Assessed type

ETH-Transfer

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/main/src/BranchBridgeAgent.sol#L578

Vulnerability details

Impact

By calculating gas usage and modifying _gParams.gasLimit, attacker can make lzReceive() revert with out-of-gas. Since we don't use LayerZero's last-resort interface to force resume message delivery, the blocking message can lead to DoS.

Proof of Concept

According to layerzero's Doc: dstUA is expected to gracefully handle all errors/exceptions when receiving a message, and any uncaught errors/exceptions (including out-of-gas) will cause the message to transition into STORED. A STORED message will block the delivery of any future message from srcUA to all dstUA on the same destination chain and can be retried until the message becomes SUCCESS. dstUA should implement a handler to transition the stored message from STORED to SUCCESS. If a bug in dstUA contract results in an unrecoverable error/exception, LayerZero provides a last-resort interface to force resume message delivery, only by the dstUA contract.

However, in our implementation, the lzReceive() is revertable and we lack forceResumeReceive implementation:

1. The caller controls the callout*'s GasParams (_gParams.gasLimit, _gParams.remoteBranchExecutionGas), so the caller can decide how much gas is used when lzReceive() is called.

2. address(this).excessivelySafeCall() is an internal call that includes some instructions before and after the call instruction. The root cause is that excessivelySafeCall is to prevent reversion in the external call, but itself can revert for out-of-gas since the sender controls gas. For example, the out-of-gas revert may happen after the out-of-gas in the external call return (1/64 gas is not enough):

    function excessivelySafeCall(
        address _target,
        uint256 _gas,
        uint16 _maxCopy,
        bytes memory _calldata
    ) internal returns (bool, bytes memory) {
        // set up for assembly call
        uint256 _toCopy;
        bool _success;
        bytes memory _returnData = new bytes(_maxCopy);
        // dispatch message to recipient
        // by assembly calling "handle" function
        // we call via assembly to avoid memcopying a very large returndata
        // returned by a malicious contract
        assembly {
            _success := call( // <---- out-of-gas, not revert
            _gas, // gas
            _target, // recipient
            0, // ether value
            add(_calldata, 0x20), // inloc
            mload(_calldata), // inlen
            0, // outloc
            0 // outlen
            )
        // limit our copy to 256 bytes
            _toCopy := returndatasize()
            if gt(_toCopy, _maxCopy) {
                _toCopy := _maxCopy
            }
        // Store the length of the copied bytes
            mstore(_returnData, _toCopy)
        // copy the bytes from returndata[0:_toCopy]
            returndatacopy(add(_returnData, 0x20), 0, _toCopy)  // <---- out-of-gas here, revert!
        }

3. We didn't implement the forceResumeReceive according to layerzero's best practices: https://layerzero.gitbook.io/docs/evm-guides/best-practice. So, the reverted call will keep blocking messages.

Tools Used

Manual Review.

Recommended Mitigation Steps

1. Use try-catch for lzReceiveNonBlocking:

    function lzReceive(uint16, bytes calldata _srcAddress, uint64, bytes calldata _payload) public override {
        try address(this).lzReceiveNonBlocking(msg.sender, _srcAddress, _payload) () {}
        catch{}
    }

2. Implement the forceResumeReceive: https://github.com/LayerZero-Labs/solidity-examples/blob/main/contracts/lzApp/LzApp.sol.

Assessed type

Context

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/main/src/RootBridgeAgent.sol#L1090

Vulnerability details

Impact

Users won't get a fallback call even if the _hasFallbackToggled is set in router/callOutAndBridgeMultiple, which can lead to the wrong assumption that the call has successfully executed on the destination chain without retry/redeem, resulting in assets lost.

Proof of Concept

In RootBridgeAgent:_createSettlementMultiple, we encode the DEPOSIT FLAG as:

        // Prepare data for call with settlement of multiple assets
        _payload = abi.encodePacked(
            _hasFallbackToggled ? bytes1(0x02) & 0x0F : bytes1(0x02),
            _recipient,

Since bytes1(0x02) & 0x0F == 0x02, no matter if _hasFallbackToggled is true or false, the deposit flag is always 0x02, making the fallback won't happen.

So, if a user is using a router that calls callOutAndBridgeMultiple with _hasFallbackToggled set, the fallback won't happen when the call fails on the dst chain.

Tools Used

Manual Review.

Recommended Mitigation Steps

Change to:

        // Prepare data for call with settlement of multiple assets
        _payload = abi.encodePacked(
            _hasFallbackToggled ? bytes1(0x82) : bytes1(0x02),

Assessed type

Context

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/main/src/RootBridgeAgentExecutor.sol#L100 https://github.com/code-423n4/2023-09-maia/blob/main/src/RootBridgeAgentExecutor.sol#L134 https://github.com/code-423n4/2023-09-maia/blob/main/src/RootBridgeAgentExecutor.sol#L185 https://github.com/code-423n4/2023-09-maia/blob/main/src/RootBridgeAgentExecutor.sol#L225

Vulnerability details

Impact

If users trigger RootBridgeAgentExecutor executeWithDeposit/executeWithDepositMultiple or executeSignedWithDeposit/executeSignedWithDepositMultiple with msg.value > 0 (airdrop native gas tokens) and there is no calldata to the router, the msg.value will be left in RootBridgeAgentExecutor and may lost later.

Proof of Concept

In RootBridgeAgentExecutor executeWithDeposit/executeWithDepositMultiple or executeSignedWithDeposit/executeSignedWithDepositMultiple, we only send the msg.value to the router if there is calldata. For example in the executeWithDeposit:

        // Bridge In Assets
        _bridgeIn(_router, dParams, _srcChainId);

        // Check if there is additional calldata in the payload
        if (_payload.length > PARAMS_TKN_SET_SIZE) {
            //Execute remote request
            IRouter(_router).executeDepositSingle{value: msg.value}(
                _payload[PARAMS_TKN_SET_SIZE:], dParams, _srcChainId
            );
        }
        // <--------- if msg.value > 0, left here
    }

However, users can call these functions with msg.value > 0 and only trigger _bridgeIn/_bridgeInMultiple (they may want to send to the router with native tokens also). In that situation, native gas tokens will be left in RootBridgeAgentExecutor, and later be used by other executes from other senders.

Tools Used

Manual Review.

Recommended Mitigation Steps

Add an else block for executeWith* and executeSignedWith* to transfer native gas tokens to routers if msg.value > 0. E.g., for executeWithDeposit:

        // Check if there is additional calldata in the payload
        if (_payload.length > PARAMS_TKN_SET_SIZE) {
            //Execute remote request
            IRouter(_router).executeDepositSingle{value: msg.value}(
                _payload[PARAMS_TKN_SET_SIZE:], dParams, _srcChainId
            );
        } else if (msg.value > 0) {
            _router.safeTransferETH(msg.value);
        }

Assessed type

ETH-Transfer