Maia DAO - Ulysses - jasonxiale's results

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BaseBranchRouter.sol#L88-L101 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BaseBranchRouter.sol#L104-L116 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BaseBranchRouter.sol#L160-L177

Vulnerability details

Impact

ArbitrumCoreBranchRouter.sol is inherited from CoreBranchRouter.sol, and CoreBranchRouter.sol is inherited from BaseBranchRouter.sol, because there is an difference between ArbitrumCoreBranchRouter and CoreBranchRouter, BaseBranchRouter._transferAndApproveToken doesn't approve the appropriate permission, which causes BaseBranchRouter.callOutAndBridge and BaseBranchRouter.callOutAndBridgeMultiple might not work on ArbitrumCoreBranchRouter

Proof of Concept

Take BaseBranchRouter.callOutAndBridge as an example:

    function callOutAndBridge(bytes calldata _params, DepositInput calldata _dParams, GasParams calldata _gParams)
        external
        payable
        override
        lock
    {
        //Transfer tokens to this contract.
        _transferAndApproveToken(_dParams.hToken, _dParams.token, _dParams.amount, _dParams.deposit);

        //Perform call to bridge agent.
        IBridgeAgent(localBridgeAgentAddress).callOutAndBridge{value: msg.value}(
            payable(msg.sender), _params, _dParams, _gParams
        );
    }

The function first calls BaseBranchRouter._transferAndApproveToken, and then calls callOutAndBridge. Supposed that we're in ArbitrumCoreBranchRouter, ArbitrumCoreBranchRouter.callOutAndBridge will first calls BaseBranchRouter._transferAndApproveToken and then calls BranchBridgeAgent.callOutAndBridge

In BaseBranchRouter._transferAndApproveToken, the code first transfer hToken/_underlyingToken to address(this), which is ArbitrumCoreBranchRouter.sol, and then calls approve for _localPortAddress which is ArbitrumBranchPort.sol

    function _transferAndApproveToken(address _hToken, address _token, uint256 _amount, uint256 _deposit) internal {
        // Cache local port address
        address _localPortAddress = localPortAddress;

        // Check if the local branch tokens are being spent
        if (_amount - _deposit > 0) {
            unchecked {
                _hToken.safeTransferFrom(msg.sender, address(this), _amount - _deposit);
                ERC20(_hToken).approve(_localPortAddress, _amount - _deposit);        <--------- Here _localPortAddress is approved
            }
        }

        // Check if the underlying tokens are being spent
        if (_deposit > 0) {
            _token.safeTransferFrom(msg.sender, address(this), _deposit);
            ERC20(_token).approve(_localPortAddress, _deposit);        <--------- Here _localPortAddress is approved
        }
    }

Then BranchBridgeAgent.callOutAndBridge is called, within BranchBridgeAgent.callOutAndBridge, BranchBridgeAgent._createDeposit is called. and then IPort(localPortAddress).bridgeOut is called. IPort(localPortAddress).bridgeOut is defined in BranchPort.sol#L277-L285, and since we're in ARB, so _bridgeOut will be ArbitrumBranchPort._bridgeOut

    function _bridgeOut(
        address _depositor,
        address _localAddress,
        address _underlyingAddress,
        uint256 _amount,
        uint256 _deposit
    ) internal override {
        //Store Underlying Tokens
        if (_deposit > 0) {
            _underlyingAddress.safeTransferFrom(_depositor, address(this), _deposit);  <-------  Approved in function BaseBranchRouter._transferAndApproveToken, 
        }


        //Burn hTokens if any are being used
        if (_amount - _deposit > 0) {
            unchecked {
                IRootPort(rootPortAddress).bridgeToRootFromLocalBranch(_depositor, _localAddress, _amount - _deposit); 
            }
        }
    }

The function ArbitrumBranchPort._bridgeOut will call IRootPort(rootPortAddress).bridgeToRootFromLocalBranch to transfer hToken, but rootPortAddress is not approved

    function bridgeToRootFromLocalBranch(address _from, address _hToken, uint256 _amount)
        external
        override
        requiresLocalBranchPort
    {
        if (!isGlobalAddress[_hToken]) revert UnrecognizedToken();

        _hToken.safeTransferFrom(_from, address(this), _amount);
    }

Tools Used

VIM

Recommended Mitigation Steps

add approve for rootPortAddress while in ArbitrumCoreBranchRouter

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

RootBridgeAgent._createSettlementMultiple's _hasFallbackToggled will not work because of typo

Proof of Concept

According to BranchBridgeAgent.lzReceiveNonBlocking, 0x82 is supported as __hasFallbackToggled if true, fallback on execution failure is toggled on in function _execute But because of wrong calculating in RootBridgeAgent._createSettlementMultiple, which always set _hasFallbackToggled as 0x02

        // Prepare data for call with settlement of multiple assets
        _payload = abi.encodePacked(
            _hasFallbackToggled ? bytes1(0x02) & 0x0F : bytes1(0x02), <-------------- Here
            _recipient,
            uint8(hTokens.length),
            _settlementNonce,
            hTokens,
            tokens,
            _amounts,
            _deposits,
            _params
        );

run the following code in chisel:

chisel Welcome to Chisel! Type !help to show available commands. ➜ bytes1(bytes1(0x02) & 0x0F) Type: bytes1 └ Data: 0x02 ➜

Tools Used

Recommended Mitigation Steps

Assessed type

Error

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BranchBridgeAgent.sol#L936-L944 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootBridgeAgent.sol#L1204-L1217

Vulnerability details

Impact

Since BranchBridgeAgent._requiresEndpoint and RootBridgeAgent._requiresEndpoint check the srcAddress incorrectly, which will cause the tx will always revert on dst chain.

Proof of Concept

Since BranchBridgeAgent._requiresEndpoint and RootBridgeAgent._requiresEndpoint share the same root of casue, I will use BranchBridgeAgent._requiresEndpoint as an example In BranchBridgeAgent._requiresEndpoint, the function first get the src address by address(uint160(bytes20(_srcAddress[20:])))) and then compares it with rootBridgeAgentAddress in BranchBridgeAgent.sol#L943

    /// @notice Internal function for caller verification. To be overwritten in `ArbitrumBranchBridgeAgent'.
    function _requiresEndpoint(address _endpoint, bytes calldata _srcAddress) internal view virtual {
        //Verify Endpoint
        if (msg.sender != address(this)) revert LayerZeroUnauthorizedEndpoint();
        if (_endpoint != lzEndpointAddress) revert LayerZeroUnauthorizedEndpoint();

        //Verify Remote Caller
        if (_srcAddress.length != 40) revert LayerZeroUnauthorizedCaller();
        if (rootBridgeAgentAddress != address(uint160(bytes20(_srcAddress[20:])))) revert LayerZeroUnauthorizedCaller();  <------- Here
    }

But according to layerZero's document, and example provided by layerzero, the address should be _srcAddress[0:20] instead of _srcAddress[20:]

Quoting from layerzero

• the srcAddress is a trusted known address on the _srcChain. If the application will connect non-evm chains, the UA should use bytes to store addresses.

And also the example code from layerzero

mapping(address => uint) public addrCounter;
mapping(uint16 => bytes) public trustedRemoteLookup;

// override from ILayerZeroReceiver.sol
function lzReceive(uint16 _srcChainId, bytes memory _srcAddress, uint64 _nonce, bytes memory _payload) override external {
    require(msg.sender == address(endpoint));
    require(keccak256(_srcAddress) == keccak256(trustedRemoteLookup[_srcChainId]);
    address fromAddress;
    assembly {
        fromAddress := mload(add(_srcAddress, 20)) <----------- Here the code uses first 20 bytes
    }
    addrCounter[fromAddress] += 1;
}

Tools Used

VIM

Recommended Mitigation Steps

$git diff BranchBridgeAgent.sol diff --git a/src/BranchBridgeAgent.sol b/src/BranchBridgeAgent.sol index b076d2d..236ad35 100644 --- a/src/BranchBridgeAgent.sol +++ b/src/BranchBridgeAgent.sol @@ -940,7 +940,7 @@ contract BranchBridgeAgent is IBranchBridgeAgent, BridgeAgentConstants {

     //Verify Remote Caller
     if (_srcAddress.length != 40) revert LayerZeroUnauthorizedCaller();

•    if (rootBridgeAgentAddress != address(uint160(bytes20(_srcAddress[20:])))) revert LayerZeroUnauthorizedCaller();
  

•    if (rootBridgeAgentAddress != address(uint160(bytes20(_srcAddress[0:20])))) revert LayerZeroUnauthorizedCaller();
  

  }

  /// @notice Modifier that verifies caller is Branch Bridge Agent's Router.

Assessed type

Other

1. RootBridgeAgent.getFeeEstimate and BranchBridgeAgent.getFeeEstimate doesn't comply with layerZero' Checklist

File:

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BranchBridgeAgent.sol#L160-L173
https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootBridgeAgent.sol#L140-L153

Quoting from LayerZero Integration Checklist:

Do not hardcode useZro to false when estimating fees and sending messages. Pass it as a parameter instead.

2. BranchBridgeAgent._createDepositMultiple doesn't check if array.length is greater than zero

File: https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BranchBridgeAgent.sol#L860C14-L888

BranchBridgeAgent._createDepositMultiple doesn't check if the parameter's length is greater than zero, if the input array's length is zero, gas will wasted.

3. BranchPort.setCoreRouter is never called.

File: https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BranchPort.sol#L331-L335 Function BranchPort.setCoreRouter requires the caller to be router, but the function is never called within current codebase

4. RootBridgeAgent._performFallbackCall should add if branch to handle local messages like RootBridgeAgent._performCall and RootBridgeAgent._performRetrySettlementCall

File: https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootBridgeAgent.sol#L938-L948

Both RootBridgeAgent._performCall and RootBridgeAgent._performRetrySettlementCall can handle Root <==> ArbBrach messages without layerZero messaging, and RootBridgeAgent._performFallbackCall missing this kind of code

5. VirtualAccount lacking of withdraw function for ERC1155

File: https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/VirtualAccount.sol#L51-L63 According to VirtualAccount.sol#L17, VirtualAccount.sol is able to receive ERC1155, but the contract doesn't an external function to withdraw ERC1155 like VirtualAccount.withdrawERC20 and VirtualAccount.withdrawERC721

6. ETH might stuck in the contract

There are many functions in the code are defined as payable, especially for ArbitrumCoreBranchRouter.sol, and ArbitrumBranchBridgeAgent.sol. Normally the functions need to be defined as payable because the function need to pay the layerzero gas fee, but for ArbitrumCoreBranchRouter.sol, and ArbitrumBranchBridgeAgent.sol, calls will never be cross-chain, so those function need to pay the gas fee, thus those function need not to be payable. Take ArbitrumCoreBranchRouter.addLocalToken as an example, if users calls the the function with some ETH(because the user need to send ETH as gas fee for layerZero on other branch chains except ARB, and user doesn't know that), the ETH will stuck in the contract

    function addLocalToken(address _underlyingAddress, GasParams calldata) external payable override {
        //Encode Data, no need to create local token since we are already in the global environment
        bytes memory params = abi.encode(
            _underlyingAddress,
            address(0),
            string.concat("Arbitrum Ulysses ", ERC20(_underlyingAddress).name()),
            string.concat("arb-u", ERC20(_underlyingAddress).symbol()),
            ERC20(_underlyingAddress).decimals()
        );

        // Pack FuncId
        bytes memory payload = abi.encodePacked(bytes1(0x02), params);

        //Send Cross-Chain request (System Response/Request)
        IBridgeAgent(localBridgeAgentAddress).callOutSystem(payable(msg.sender), payload, GasParams(0, 0));
    }

block.chainid's type should be uint instead of uint16

Within the code, localChainId and rootChainId those variables' type is uint16, but according to Block and Transaction Properties, block.chainid's type should be uint

block.chainid (uint): current chain id