The user will lose his assets when making a signed call to the root chain with an account abstraction wallet

[c4-submissions]

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

[c4-pre-sort]

0xA5DF marked the issue as duplicate of #877

[c4-pre-sort]

0xA5DF marked the issue as sufficient quality report

[c4-judge]

alcueca marked the issue as satisfactory

[c4-judge]

alcueca changed the severity to 3 (High Risk)

[c4-judge]

alcueca marked the issue as duplicate of #351

[c4-judge]

alcueca marked the issue as partial-50

[alcueca]

Downgrading the whole lot to Medium again.

[c4-judge]

alcueca changed the severity to 2 (Med Risk)

[c4-judge]

alcueca marked the issue as satisfactory

[c4-judge]

alcueca changed the severity to 3 (High Risk)