Maia DAO - Ulysses - hals's results

Lines of code

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

Vulnerability details

Impact

• Users can create virtual accounts in the root chain to enable them managing their assets and performing calls to other contracts via payableCall function.

• But as can be noticed; this function can be accessed by anyone as it doesn't have an access modifier for authorized callers.

• Given that this contract is designed to receive both native assets and tokens via the root bridge, it becomes susceptible to various attacks, potentially resulting in the loss of funds.

• One possible attack scenario involves an attacker crafting a call from this contract to interact with an ERC20 contract to approve himself of these tokens, and then stealing them.

Proof of Concept

VirtualAccount.payableCall function

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

Tools Used

Manual Testing.

Recommended Mitigation Steps

Add requiresApprovedCaller modifier to the 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/f5ba4de628836b2a29f9b5fff59499690008c463/src/BranchBridgeAgent.sol#L262-L273 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootBridgeAgent.sol#L538-L571

Vulnerability details

Impact

• Users who interact with the protocol with "EOAs" (externally owned accounts) will be using the same address that is created on all evm chains for these accounts.

• But users of account abstraction wallets (which are unique smart contract instances deployed on individual chains) will have different addresses on different chains.

• This will introduce problems for the users who use these wallets; as the protocol assumes in all of its calls (signed calls in particular) that the user address who initiated the L0 call in the branch chain is the same address who will be receiving the result of these calls in the root chain (and later in the other chains); i.e: msg.sender address.

• The problem clearly arises when the user initiates a signed call to the root bridge with his account abstraction wallet (that will be the msg.sender of this call), and the root bridge agent will create a virtual account for the user with the address extracted from the payload of the call (the extracted address here will be the same address of the msg.sender of this call in the branch chain; but this address will not be owned by the user in the root chain as the account abstraction wallet will create a different address for the user in different chains):

              // Get User Virtual Account
              VirtualAccount userAccount = IPort(localPortAddress).fetchVirtualAccount(
                  address(uint160(bytes20(_payload[PARAMS_START:PARAMS_START_SIGNED])))
              );

• This virtual account of the user will be receiving funds and minted global hTokens (virtualized asset representation) in the root chain (Arbitrum).

• But the created virtual account for the user will not be accessible by the same user who initiated the call in the branch chain with his abstract wallet account (different address on different chains), so he will not be able to withdraw the assets sent to the virtual account as he is not the same address who can access this contract; which will lead to loss of user assets as they become inaccessible VirtualAccount:

    /// @inheritdoc IVirtualAccount
    function withdrawNative(uint256 _amount) external override requiresApprovedCaller {
        msg.sender.safeTransferETH(_amount);
    }

    /// @inheritdoc IVirtualAccount
    function withdrawERC20(address _token, uint256 _amount) external override requiresApprovedCaller {
        _token.safeTransfer(msg.sender, _amount);
    }

    /// @inheritdoc IVirtualAccount
    function withdrawERC721(address _token, uint256 _tokenId) external override requiresApprovedCaller {
        ERC721(_token).transferFrom(address(this), msg.sender, _tokenId);
    }
    //.... some code
    modifier requiresApprovedCaller() {
        if (!IRootPort(localPortAddress).isRouterApproved(this, msg.sender)) {
            if (msg.sender != userAddress) {
                revert UnauthorizedCaller();
            }
        }
        _;
    }

Proof of Concept

An example of where the msg.sender address is used to make calls by the branch, where it will be received by the root to create a virtual account for him:

BranchBridgeAgent.callOutSigned

    function callOutSigned(address payable _refundee, bytes calldata _params, GasParams calldata _gParams)
        external
        payable
        override
        lock
    {
        //Encode Data for cross-chain call.
        bytes memory payload = abi.encodePacked(bytes1(0x04), msg.sender, depositNonce++, _params);

        //Perform Signed Call without deposit
        _performCall(_refundee, payload, _gParams);
    }

RootBridgeAgent.lzReceiveNonBlocking

   // DEPOSIT FLAG: 4 (Call without Deposit + msg.sender)
        } else if (_payload[0] == 0x04) {
            // Parse deposit nonce
            nonce = uint32(bytes4(_payload[PARAMS_START_SIGNED:PARAMS_TKN_START_SIGNED]));

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

            // Get User Virtual Account
            VirtualAccount userAccount = IPort(localPortAddress).fetchVirtualAccount(
                address(uint160(bytes20(_payload[PARAMS_START:PARAMS_START_SIGNED])))
            );

            // Toggle Router Virtual Account use for tx execution
            IPort(localPortAddress).toggleVirtualAccountApproved(userAccount, localRouterAddress);

            // Avoid stack too deep
            uint16 srcChainId = _srcChainId;

            // Try to execute remote request
            // Flag 4 - RootBridgeAgentExecutor(bridgeAgentExecutorAddress).executeSignedNoDeposit(address(userAccount), localRouterAddress, data, _srcChainId
            _execute(
                nonce,
                abi.encodeWithSelector(
                    RootBridgeAgentExecutor.executeSignedNoDeposit.selector,
                    address(userAccount),
                    localRouterAddress,
                    _payload,
                    srcChainId
                ),
                srcChainId
            );

            // Toggle Router Virtual Account use for tx execution
            IPort(localPortAddress).toggleVirtualAccountApproved(userAccount, localRouterAddress);

Tools Used

Manual Testing.

Recommended Mitigation Steps

When users initiate a signed call from the BranchBridgeAgent; allow users to pass the address where they would like to interact with the protocol in the root chain (used as their address in the root chain) instead of using the msg.sender address that will be encoded in the call payload.

Assessed type

Other