Maia DAO - Ulysses - ast3ros's results

Lines of code

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

Vulnerability details

Impact

The unrestricted access to the payableCall function can result in unauthorized transactions, leading to the loss of funds in the Virtual Account. Additionally, since the Virtual Account is designed to be integrated with various dApps, this vulnerability could also lead to the loss of assets in those dApps.

Proof of Concept

The payableCall function in the VirtualAccount contract allows for multiple calls to be made to other contracts. The function is publicly accessible and does not have any restrictions on who can call it.

function payableCall(PayableCall[] calldata calls) public payable returns (bytes[] memory returnData) {
    uint256 valAccumulator;
    uint256 length = calls.length;
    returnData = new bytes[](length);
    PayableCall calldata _call;
    for (uint256 i = 0; i < length;) {
        _call = calls[i];
        uint256 val = _call.value;
        // Humanity will be a Type V Kardashev Civilization before this overflows - andreas
        // ~ 10^25 Wei in existence << ~ 10^76 size uint fits in a uint256
        unchecked {
            valAccumulator += val;
        }

        bool success;

        if (isContract(_call.target)) (success, returnData[i]) = _call.target.call{value: val}(_call.callData);

        if (!success) revert CallFailed();

        unchecked {
            ++i;
        }
    }

    // Finally, make sure the msg.value = SUM(call[0...i].value)
    if (msg.value != valAccumulator) revert CallFailed();
}

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

The VirtualAccount as a Omnichain Wallet, it stores assets of users. A malicious user can call the payableCall function to drain the funds of the VirtualAccount. The VirtualAccount is used to intergated in different dAPP. It means that anyone can interact with the dApp as the VirtualAccount and drain the funds related to the Virtual Account.

Tools Used

Manual

Recommended Mitigation Steps

Add requiresApprovedCaller modifier to restrict the caller of payableCall function.

- function payableCall(PayableCall[] calldata calls) public payable returns (bytes[] memory returnData) {
+ function payableCall(PayableCall[] calldata calls) public payable requiresApprovedCaller returns (bytes[] memory returnData) {

Assessed type

Access Control

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/ffbe532c6f5224a55ce099b4016bd8806bdbc913/src/BranchBridgeAgent.sol#L578-L584 https://github.com/code-423n4/2023-09-maia/blob/ffbe532c6f5224a55ce099b4016bd8806bdbc913/src/BranchBridgeAgent.sol#L720 https://github.com/code-423n4/2023-09-maia/blob/ffbe532c6f5224a55ce099b4016bd8806bdbc913/src/BranchBridgeAgent.sol#L748-L751

Vulnerability details

Impact

If a transaction fails on the destination chain, the channel between the source and destination chains becomes blocked. This effectively halts the operation of the protocol until the issue is resolved or a force resume message is delivered by the dstUA contract.

Proof of Concept

If a transaction fails in the destination chain, the channel between the source chain and destination chain will be blocked. The source chain cannot send any message to the destination chain until the transaction is success or force resume message delivery, only by the dstUA contract.

https://layerzero.gitbook.io/docs/faq/messaging-properties#message-ordering

    LayerZero provides ordered delivery of messages from a given sender to a destination chain, i.e. srcUA-> dstChain. In other words, the message order nonce is shared by all dstUA on the same dstChain. That's why a STORED message blocks the message pathway from srcUA to all dstUA on the same destination chain.  If it isn't necessary to preserve the sequential nonce property for a particular dstUA the sender must add the nonce into the payload and handle it end-to-end within the UA.  UAs can implement a non-blocking pattern in their contract code. 

LayerZero recommends implementing a non-blocking pattern where all errors and exceptions are caught locally for future retries. This ensures that the channel remains open even if a transaction fails.

    (bool success, bytes memory reason) = address(this).excessivelySafeCall(
        gasleft(),
        150,
        abi.encodeWithSelector(this.nonblockingLzReceive.selector, _srcChainId, _srcAddress, _nonce, _payload)
    );
    // try-catch all errors/exceptions
    if (!success) {
        _storeFailedMessage(_srcChainId, _srcAddress, _nonce, _payload, reason);
    }

https://github.com/LayerZero-Labs/solidity-examples/blob/2116a67dc72e4d13d66e6cc9107096ae705a07b7/contracts/lzApp/NonblockingLzApp.sol#L35-L38

However, in Maia protocol, we can see that not all error, revert is caught

    function lzReceive(uint16, bytes calldata _srcAddress, uint64, bytes calldata _payload) public override {
        address(this).excessivelySafeCall(
            gasleft(),
            150,
            abi.encodeWithSelector(this.lzReceiveNonBlocking.selector, msg.sender, _srcAddress, _payload)
        );
    }

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

In contrast, Maia's current implementation does not catch all errors or reverts. For example, the _execute function can be reverted, but this is not caught.

    if (!success) revert ExecutionFailure();

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

    } else {
        // If no fallback is requested revert allowing for settlement retry.
        revert ExecutionFailure();
    }

https://github.com/code-423n4/2023-09-maia/blob/ffbe532c6f5224a55ce099b4016bd8806bdbc913/src/BranchBridgeAgent.sol#L748-L751

Tools Used

Manual

Recommended Mitigation Steps

Catch all the revert in _execute functions and save for future retry like in code example provided by LayerZero.

Assessed type

Error

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/ffbe532c6f5224a55ce099b4016bd8806bdbc913/src/RootBridgeAgent.sol#L1090 https://github.com/code-423n4/2023-09-maia/blob/ffbe532c6f5224a55ce099b4016bd8806bdbc913/src/BranchBridgeAgent.sol#L638-L660 https://github.com/code-423n4/2023-09-maia/blob/ffbe532c6f5224a55ce099b4016bd8806bdbc913/src/BranchBridgeAgent.sol#L651

Vulnerability details

Impact

The incorrect flag setting prevents the fallback mechanism from executing in the destination chain. As a result, users cannot retrieve their tokens, affecting the reliability of the protocol.

Proof of Concept

When settling multiple assets and perform a remote call to a branch chain, the function RootBridgeAgent._createSettlementMultiple sets the fallback flag based on the _hasFallbackToggled variable. The flag is set to either bytes1(0x02) & 0x0F or bytes1(0x02).

    _hasFallbackToggled ? bytes1(0x02) & 0x0F : bytes1(0x02)

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

The logic error is both flag settings evaluate to 0x02:

0x02 & 0x0F = 0x02 (0000 0010 & 0000 1111 = 0000 0010)

This means that the fallback flag is always set to 0x02, regardless of whether _hasFallbackToggled is true or false.

The BranchBridgeAgent contract checks for a flag value of _payload[0] == 0x82 to trigger the fallback mechanism. Because the flag is incorrectly set to 0x02, the fallback mechanism is never triggered, even when it should be.

        // DEPOSIT FLAG: 2 (Multiple Settlement)
    } else if (flag == 0x02) {
        // Parse recipient
        address payable recipient = payable(address(uint160(bytes20(_payload[PARAMS_START:PARAMS_START_SIGNED]))));

        // Parse deposit nonce
        nonce = uint32(bytes4(_payload[22:26]));

        //Check if tx has already been executed
        if (executionState[nonce] != STATUS_READY) revert AlreadyExecutedTransaction();

        //Try to execute remote request
        // Flag 2 - BranchBridgeAgentExecutor(bridgeAgentExecutorAddress).executeWithSettlementMultiple(recipient, localRouterAddress, _payload)
        _execute(
            _payload[0] == 0x82,
            nonce,
            recipient,
            abi.encodeWithSelector(
                BranchBridgeAgentExecutor.executeWithSettlementMultiple.selector,
                recipient,
                localRouterAddress,
                _payload
            )
        );

https://github.com/code-423n4/2023-09-maia/blob/ffbe532c6f5224a55ce099b4016bd8806bdbc913/src/BranchBridgeAgent.sol#L638-L660

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

Tools Used

Manual

Recommended Mitigation Steps

Correct the fallback flag setting in the _createSettlementMultiple function.

-        _hasFallbackToggled ? bytes1(0x02) & 0x0F : bytes1(0x02)
+        _hasFallbackToggled ? bytes1(0x82) : bytes1(0x02)

Assessed type

Error

[L-1] Nonce is too low

In the protocol the depositNonce and settlementNonce in bridgeAgent are set to uint32. The maximum of those numbers are only 2^32 ~ 4 bil. It is too low for a protocol that is going to have a lot of bridge transactions. The transactions are going to be revert when the nonces are overflow.

https://github.com/code-423n4/2023-09-maia/blob/ffbe532c6f5224a55ce099b4016bd8806bdbc913/src/BranchBridgeAgent.sol#L84 https://github.com/code-423n4/2023-09-maia/blob/ffbe532c6f5224a55ce099b4016bd8806bdbc913/src/RootBridgeAgent.sol#L75

Normally, the nonce should be set to uint64 which is safe.

[L-2] Cannot turn on branch bridge agent factory again

When branch bridge agent is disable, it cannot be enable again, the only option is creating a new one.

    // Check if BridgeAgentFactory is active
    if (IPort(_localPortAddress).isBridgeAgentFactory(_newBridgeAgentFactoryAddress)) {
        // If so, disable it.
        IPort(_localPortAddress).toggleBridgeAgentFactory(_newBridgeAgentFactoryAddress);
    } else {
        // If not, add it.
        IPort(_localPortAddress).addBridgeAgentFactory(_newBridgeAgentFactoryAddress);
    }

https://github.com/code-423n4/2023-09-maia/blob/ffbe532c6f5224a55ce099b4016bd8806bdbc913/src/CoreBranchRouter.sol#L249-L255

[NC-1] Function setCoreRouter in BranchPort cannot be called

The function setCoreRouter can only be called by coreRouter. However the contract CoreBranchRouter has no function to call this function.

    function setCoreRouter(address _newCoreRouter) external override requiresCoreRouter {

https://github.com/code-423n4/2023-09-maia/blob/ffbe532c6f5224a55ce099b4016bd8806bdbc913/src/BranchPort.sol#L331-L335

[NC-2] Wrong natspec

The natspec below is for a different setter function, not the bridgeOut function.

/**
 * @notice Setter function to decrease local hToken supply.
 *   @param _localAddress token address.
 *   @param _amount amount of tokens.
 *   @param _deposit amount of underlying tokens.
 *
 */
function bridgeOut(
    address _depositor,
    address _localAddress,
    address _underlyingAddress,
    uint256 _amount,
    uint256 _deposit
) external;

https://github.com/code-423n4/2023-09-maia/blob/ffbe532c6f5224a55ce099b4016bd8806bdbc913/src/interfaces/IBranchPort.sol#L111-L124