Maia DAO - Ulysses - 0xStalin's results

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/main/src/RootPort.sol#L350-L353

Vulnerability details

Impact

• Attackers can gain control of User's Virtual Accounts and steal all the assets these accounts hold in the Root environment

Proof of Concept

• When sending signed messages from a Branch to Root, the RootBridgeAgent contract calls the RootPort::fetchVirtualAccount() to get the virtual account that is assigned in the Root environment to the address who initiated the call in the SrcBranch, and if that address doesn't have assigned a virtual account yet, it proceeds to create one and assign it.
• The problem is that the fetchVirtualAccount() function solely relies on the address of the caller in the SrcBranch, but it doesn't take into account from which Branch the call comes.

BranchBridgeAgent.sol

function callOutSignedAndBridge(
    ...
) external payable override lock {
  ...
  //Encode Data for cross-chain call.
  bytes memory payload = abi.encodePacked(
      _hasFallbackToggled ? bytes1(0x85) : bytes1(0x05),
      //@audit-info => Encodes the address of the caller in the Branch and sends it to the RootBridgeAgent
      //@audit-info => This address will be used to fetch the VirtualAccount assigned to it!
      msg.sender,
      _depositNonce,
      _dParams.hToken,
      _dParams.token,
      _dParams.amount,
      _dParams.deposit,
      _params
  );
}

RootBridgeAgent.sol

function lzReceiveNonBlocking(
  ...

) public override requiresEndpoint(_endpoint, _srcChainId, _srcAddress) {
  ...
  ...
  ...
  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();
      }

      //@audit-info => Reads the address of the msg.sender in the BranchBridgeAgent and forwards that address to the RootPort::fetchVirtualAccount()
      // 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);

    ...
    ...
  }
  ...
  ...
}

RootPort.sol

//@audit-info => Receives from the RootBridgeAgent contract the address of the caller in the BranchBridgeAgent contract
//@audit-info => Fetches the VirtualAccount assigned to the _user address regardless from what Branch the call came from
function fetchVirtualAccount(address _user) external override returns (VirtualAccount account) {
    account = getUserAccount[_user];
    if (address(account) == address(0)) account = addVirtualAccount(_user);
}

• Like Example, Let's suppose that a user uses a MultiSigWallet contract to deposit tokens from Avax to Root, in the RootBridgeAgent contract, the address of the MultisigWallet will be used to create a Virtual Account, and all the globalTokens that were bridged will be deposited in this Virtual Account.

  • Now, the problem is that the address of the MultisigWallet, might not be controlled by the same user on a different chain, For example, in Polygon, an attacker could gain control of the address of the same address of the MultisigWallet that was used to deposit tokens from Avax in the Root environment, an attacker can send a signed message from Polygon using the same address of the MultisigWallet that deposited tokens from Avax, to the Root environment, requesting to withdraw the assets that the Virtual Account is holding in the Root environment to the Polygon Branch.
    • When the message is processed by the Root environment, the address that will be used to obtain the Virtual Account will be the address that initiated the call in Polygon, which will be the same address of the user's MultisigWallet contract who deposited the assets from Avax, but the Root environment, when fetching the virtual account, makes no distinctions between the branches, thus, it will give access to the Virtual Account of the attacker's caller address and process the message in the Root environment.
      • As a result, an attacker can gain control of the Virtual Account of an account contract that was used to deposit assets from a chain into Root, by gaining control of the same address of the account contract that deposited the assets in a different chain.

• As explained in detail on this article written by Rekt, it is possible to gain control of the same address for contract accounts in a different chain, especially for those contract accounts that are deployed using the Gnosis Safe contracts:

Tools Used

Manual Audit & Article wrote by Rekt

Recommended Mitigation Steps

• The recommendation is to add some logic that validates if the caller address in the BranchBridgeAgent is a contract account or an EOA, and if it's a contract account, send a special flag as part of the crosschain message, so that the RootBridgeAgent contract can know if the caller in the SrcBranch it's a contract or an EOA.
  • If the caller is an EOA, the caller's address can be assigned as the Virtual Account owner on all the chains, for EOAs there are no problems.
  • But, if the caller is a Contract Account, when fetching the virtual account, forward the SrcChain, and if a Virtual Account is created, just authorize the caller address on the SrcBranch as the owner for that Virtual Account, in this way, only the contract account in the SrcBranch can access the Virtual Account in the Root environment.
    • Make sure to use the srcChainId to validate if the caller is an owner of the Virtual Account!

Assessed type

Access Control

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/main/src/RootBridgeAgent.sol#L756-L757 https://github.com/code-423n4/2023-09-maia/blob/main/src/RootBridgeAgent.sol#L789-L792

Vulnerability details

Impact

• Users won't get back the native token they paid to execute cross-chain txs if the txs are reverted in the RootBridgeAgent contract.

Proof of Concept

When txs execution fails in the RootBridgeAgent contract, the current implementation in the RootBridgeAgent contract is to revert the tx (if a fallback was not set) that is initiated by the LayerZeroEndpoint contract in the Root environment (Arbitrum).

• If the tx is initiated by the LayerZeroEndpoint contract and is reverted in the RootBridgeAgent contract (and fallback was not defined), the native token will be sent back to the LayerZeroEndpoint contract and will be left there, it is not refunded to the refundee user.
• The execution flow in the RootBridgeAgent contract goes from the lzReceive() function to the lzReceiveNonBlocking() function to the _execute() function where if the tx fails and no fallback is set, the whole tx will be reverted if the call to the BridgeAgentExecutor contract fails.

Let's do a walkthrough the code of the LayerZeroEndpoint contract to visualize what happens to the txs when they fail.

Endpoint.sol (LayerZero contract)

    function receivePayload(uint16 _srcChainId, bytes calldata _srcAddress, address _dstAddress, uint64 _nonce, uint _gasLimit, bytes calldata _payload) external override receiveNonReentrant {
        ...

        //@audit-info => LayerZeroEndpoint contract sends the specified _gasLimit to the dstContract (RootBridgeAgent contract)
        try ILayerZeroReceiver(_dstAddress).lzReceive{gas: _gasLimit}(_srcChainId, _srcAddress, _nonce, _payload) {
            // success, do nothing, end of the message delivery
        } catch (bytes memory reason) {
            //@audit-info => If the execution of the lzReceive() function in the dstContract fails, the Endpoint contract doesn't refund the native tokens to the refundee address
            // revert nonce if any uncaught errors/exceptions if the ua chooses the blocking mode
            storedPayload[_srcChainId][_srcAddress] = StoredPayload(uint64(_payload.length), _dstAddress, keccak256(_payload));
            emit PayloadStored(_srcChainId, _srcAddress, _dstAddress, _nonce, _payload, reason);
        }
    }

Let's now visualize the _execute() functions in the RootBridgeAgent contract

  function _execute(uint256 _depositNonce, bytes memory _calldata, uint16 _srcChainId) private {
      //Update tx state as executed
      executionState[_srcChainId][_depositNonce] = STATUS_DONE;

      //Try to execute the remote request
      (bool success,) = bridgeAgentExecutorAddress.call{value: address(this).balance}(_calldata);

      //@audit-info => If execution fails in the RootBridgeAgentExecutor contract, the whole tx is reverted, thus, the native token is sent back to the LayerZeroEndpoint contract, but it is not sent back to the refundee address
      // No fallback is requested revert allowing for retry.
      if (!success) revert ExecutionFailure();
  }

function _execute(
    bool _hasFallbackToggled,
    uint32 _depositNonce,
    address _refundee,
    bytes memory _calldata,
    uint16 _srcChainId
) private {
    //Update tx state as executed
    executionState[_srcChainId][_depositNonce] = STATUS_DONE;

    //Try to execute the remote request
    (bool success,) = bridgeAgentExecutorAddress.call{value: address(this).balance}(_calldata);

    //Update tx state if execution failed
    if (!success) {
        //Read the fallback flag.
        if (_hasFallbackToggled) {
            // Update tx state as retrieve only
            executionState[_srcChainId][_depositNonce] = STATUS_RETRIEVE;
            // Perform the fallback call
            _performFallbackCall(payable(_refundee), _depositNonce, _srcChainId);
        } else {
            //@audit-info => If execution fails in the RootBridgeAgentExecutor contract and no fallback was set, the whole tx is reverted, thus, the native token is sent back to the LayerZeroEndpoint contract, but it is not sent back to the refundee address
            // No fallback is requested revert allowing for retry.
            revert ExecutionFailure();
        }
    }
}

• The purpose of reverting the tx is to prevent the nonce from the srcChain from being marked as STATUS_DONE in the executionState mapping, so the tx can be retried or retrieved and redeemed from the Source Branch
  • But, the current implementation is flawless and will cause users to never get back the unspent native token.

Coded PoC

• I coded a PoC to demonstrate the problem I'm reporting, using the RootForkTest.t.sol test file as the base to reproduce this PoC:

  • Make sure to add the new testAddGlobalTokenNoReturnsGasPoC() function.

• For this PoC I'm forcefully causing the tx to be reverted by trying to add a globalToken that has already been added, but the core issue is the same regardless of what causes the tx to be reverted, the user won't get back the unspent native token that was paid for the execution of the crosschain message.

function testAddGlobalTokenNoReturnsGasPoC() public {
    //Add Local Token from Avax
    testAddLocalToken();

    //Switch Chain and Execute Incoming Packets
    switchToLzChain(avaxChainId);

    vm.deal(address(this), 1000 ether);

    GasParams[3] memory gasParams =
        [GasParams(15_000_000, 0.1 ether), GasParams(2_000_000, 3 ether), GasParams(200_000, 0)];
        // [GasParams(15_000_000, 0.1 ether), GasParams(1000, 3 ether), GasParams(200_000, 0)];

    //@audit-info => User1 adds the avaxGlobalToken first
    avaxCoreRouter.addGlobalToken{value: 1000 ether}(newAvaxAssetGlobalAddress, ftmChainId, gasParams);

    //Switch Chain and Execute Incoming Packets
    switchToLzChain(rootChainId);
    //Switch Chain and Execute Incoming Packets
    switchToLzChain(ftmChainId);
    //Switch Chain and Execute Incoming Packets
    switchToLzChain(rootChainId);

    newAvaxAssetFtmLocalToken = rootPort.getLocalTokenFromGlobal(newAvaxAssetGlobalAddress, ftmChainId);

    require(newAvaxAssetFtmLocalToken != address(0), "Failed is zero");

    console2.log("New Local: ", newAvaxAssetFtmLocalToken);

    require(
        rootPort.getLocalTokenFromGlobal(newAvaxAssetGlobalAddress, ftmChainId) == newAvaxAssetFtmLocalToken,
        "Token should be added"
    );

    require(
        rootPort.getUnderlyingTokenFromLocal(newAvaxAssetFtmLocalToken, ftmChainId) == address(0),
        "Underlying should not be added"
    );

    uint256 rootBridgeAgentBalanceBefore = address(coreRootBridgeAgent).balance;
    console2.log("RootBridge balance before: ", rootBridgeAgentBalanceBefore);

    //Switch Chain and Execute Incoming Packets
    switchToLzChain(avaxChainId);

    //@audit-info => User2 adds the avaxGlobalToken after, tx is reverted and native tokens is not refunded
    address user2 = vm.addr(10);
    vm.label(user2, "User2");
    vm.deal(user2, 1000 ether);
    vm.prank(user2);
    console2.log("user2 balance before: ", user2.balance);
    avaxCoreRouter.addGlobalToken{value: 1000 ether}(newAvaxAssetGlobalAddress, ftmChainId, [GasParams(15_000_000, 0.1 ether), GasParams(2_000_000, 3 ether), GasParams(200_000, 0)]);
    //Switch Chain and Execute Incoming Packets
    switchToLzChain(rootChainId);
    //Switch Chain and Execute Incoming Packets
    switchToLzChain(avaxChainId);
    
    console2.log("user2 balance after: ", user2.balance);

    switchToLzChain(rootChainId);
    uint256 rootBridgeAgentBalanceAfter = address(coreRootBridgeAgent).balance;
    console2.log("RootBridge balance after: ", rootBridgeAgentBalanceAfter);

    assertTrue(rootBridgeAgentBalanceAfter > rootBridgeAgentBalanceBefore);
}

• Now everything is ready to run the test and analyze the output:

forge test --mc RootForkTest --match-test testAddGlobalTokenNoReturnsGasPoC -vvvv

• By analyzing the output below, we can see that the unspent native tokens are left in the dst contract instead of being returned back to the user (See the next paragraph below the output to understand this behavior and a comparisson against what will happen in production, where the LayerZeroEndpoint contract uses the UltraLightNodeV2 library)

    ├─ [0] console::log(Sending native token airdrop...) [staticcall]
    │   └─ ← ()
    ├─ [0] VM::deal(RootForkTest: [0xBb2180ebd78ce97360503434eD37fcf4a1Df61c3], 1000000000000000000000 [1e21])
    │   └─ ← ()
    ├─ [55] RootBridgeAgent::receive{value: 1000000000000000000000}()
    │   └─ ← ()
    ├─ [0] VM::deal(0x4D73AdB72bC3DD368966edD0f0b2148401A178E2, 150000000000 [1.5e11])
    │   └─ ← ()
    ├─ [0] VM::prank(0x4D73AdB72bC3DD368966edD0f0b2148401A178E2)
    │   └─ ← ()
    ├─ [51462] 0x3c2269811836af69497E5F486A85D7316753cf62::receivePayload(106, 0xcb3b263f002b8888f712ba46ebf1302e1294608cd5f23f71c860c0c261da56585d3a24c6dbfacaa1, RootBridgeAgent: [0xCB3b263f002b8888f712BA46EbF1302e1294608c], 4, 15000000 [1.5e7], 0x0100000004010000000000000000000000004cceba2d7d2b4fdce4304d3e09a1fea9fbeb1528000000000000000000000000a475b806ead7ebb851589f958fd1038fcbeec1c1000000000000000000000000000000000000000000000000000000000000007000000000000000000000000000000000000000000000000000000000001e848000000000000000000000000000000000000000000000000029a2241af62c00000000000000000000000000000000000000000000000000000000000000030d400000000000000000000000000000000000000000000000000000000000000000)
    │   ├─ [48205] RootBridgeAgent::lzReceive(106, 0xcb3b263f002b8888f712ba46ebf1302e1294608cd5f23f71c860c0c261da56585d3a24c6dbfacaa1, 4, 0x0100000004010000000000000000000000004cceba2d7d2b4fdce4304d3e09a1fea9fbeb1528000000000000000000000000a475b806ead7ebb851589f958fd1038fcbeec1c1000000000000000000000000000000000000000000000000000000000000007000000000000000000000000000000000000000000000000000000000001e848000000000000000000000000000000000000000000000000029a2241af62c00000000000000000000000000000000000000000000000000000000000000030d400000000000000000000000000000000000000000000000000000000000000000)
    │   │   ├─ [45896] RootBridgeAgent::lzReceiveNonBlocking(0x3c2269811836af69497E5F486A85D7316753cf62, 106, 0xcb3b263f002b8888f712ba46ebf1302e1294608cd5f23f71c860c0c261da56585d3a24c6dbfacaa1, 0x0100000004010000000000000000000000004cceba2d7d2b4fdce4304d3e09a1fea9fbeb1528000000000000000000000000a475b806ead7ebb851589f958fd1038fcbeec1c1000000000000000000000000000000000000000000000000000000000000007000000000000000000000000000000000000000000000000000000000001e848000000000000000000000000000000000000000000000000029a2241af62c00000000000000000000000000000000000000000000000000000000000000030d400000000000000000000000000000000000000000000000000000000000000000)
    │   │   │   ├─ [12638] RootBridgeAgentExecutor::executeNoDeposit{value: 1000000000000000000000}(CoreRootRouter: [0x32e03c6b1b446C7a4381852A82F7Cd4BEB00B17d], 0x0100000004010000000000000000000000004cceba2d7d2b4fdce4304d3e09a1fea9fbeb1528000000000000000000000000a475b806ead7ebb851589f958fd1038fcbeec1c1000000000000000000000000000000000000000000000000000000000000007000000000000000000000000000000000000000000000000000000000001e848000000000000000000000000000000000000000000000000029a2241af62c00000000000000000000000000000000000000000000000000000000000000030d400000000000000000000000000000000000000000000000000000000000000000, 106)
    │   │   │   │   ├─ [4395] CoreRootRouter::execute{value: 1000000000000000000000}(0x010000000000000000000000004cceba2d7d2b4fdce4304d3e09a1fea9fbeb1528000000000000000000000000a475b806ead7ebb851589f958fd1038fcbeec1c1000000000000000000000000000000000000000000000000000000000000007000000000000000000000000000000000000000000000000000000000001e848000000000000000000000000000000000000000000000000029a2241af62c00000000000000000000000000000000000000000000000000000000000000030d400000000000000000000000000000000000000000000000000000000000000000, 106)
    │   │   │   │   │   ├─ [605] RootPort::isGlobalAddress(ERC20hTokenRoot: [0xa475b806eaD7ebB851589f958fD1038fcbEEC1c1]) [staticcall]
    │   │   │   │   │   │   └─ ← true
    │   │   │   │   │   ├─ [742] RootPort::isGlobalToken(ERC20hTokenRoot: [0xa475b806eaD7ebB851589f958fD1038fcbEEC1c1], 112) [staticcall]
    │   │   │   │   │   │   └─ ← true
    │   │   │   │   │   └─ ← "TokenAlreadyAdded()"
    │   │   │   │   └─ ← "TokenAlreadyAdded()"
    │   │   │   └─ ← "ExecutionFailure()"
    │   │   └─ ← ()
    │   └─ ← ()
    ├─ [0] VM::selectFork(0)
    │   └─ ← ()
    ├─ [0] VM::getRecordedLogs()
    │   └─ ← []
    ├─ [0] console::log(Events caugth:, 0) [staticcall]
    │   └─ ← ()
    ├─ [0] VM::resumeGasMetering()
    │   └─ ← ()
    ├─ [0] console::log(user2 balance after: , 978780807362276784517 [9.787e20]) [staticcall]
    │   └─ ← ()
    ├─ [0] VM::selectFork(1)
    │   └─ ← ()
    ├─ [0] VM::getRecordedLogs()
    │   └─ ← []
    ├─ [0] console::log(Events caugth:, 0) [staticcall]
    │   └─ ← ()
    ├─ [0] VM::resumeGasMetering()
    │   └─ ← ()
    ├─ [0] console::log(RootBridge balance after: , 1000000000000000000000 [1e21]) [staticcall]
    │   └─ ← ()
    └─ ← ()

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 2.37s

• The difference between the logic implemented on the LzForkTest.t.sol file that is executed in these tests and the actual library that is used by the LayerZeroEndpoint contract is that, in the LzForkTest.t.sol implementation, the native tokens are airdroped in a separate tx before actually running the payload on the dstContract.lzReceive() function, that's why if the tx is reverted in the dstContract, the native tokens are left in the dstContract itself, but in production, the tokens will be left in the LayerZero contract, regardless of this difference, the user is not getting back any of the unspent native tokens.

LzForkTest.t.sol contract

function executePacket(Packet memory packet) public {
    ...
    //@audit-info => handleAdapterParams() airdrops the native tokens to the dstContract in a separate tx
    uint256 gasLimit = handleAdapterParams(adapterParams);

    // Acquire gas, Prank into Library and Mock LayerZeroEndpoint.receivePayload call
    vm.deal(receivingLibrary, gasLimit * tx.gasprice);
    vm.prank(receivingLibrary);
    ILayerZeroEndpoint(receivingEndpoint).receivePayload(
        packet.originLzChainId,
        abi.encodePacked(packet.destinationUA, packet.originUA), //original
        // abi.encodePacked(packet.originUA, packet.destinationUA), //fixed
        // abi.encodePacked(address(1), address(1)),
        packet.destinationUA,
        packet.nonce,
        gasLimit,
        packet.payload
    );
}

function handleAdapterParams(AdapterParams memory params) internal returns (uint256 gasLimit) {
    ...
    ...
    ...
            //@audit-info => Tokens are airdroped to the dstContract!
            console2.log("Sending native token airdrop...");
            deal(address(this), nativeForDst * tx.gasprice);
            addressOnDst.call{value: nativeForDst * tx.gasprice}("");
    ...
    ...
    ...
}

Tools Used

Manual Audit & LayerZeroEndpoint Message Library

Recommended Mitigation Steps

• The recommendation is to implement a mechanism to refund the received ETH from the LayerZeroEndpoint contract to the refundee user in case the execution in the RootBridgeAgent executor fails, and, instead of forcefully reverting the tx in the RootBridgeAgent contract, first, either refund or credit the total of the unspent ETH to the refundee, and secondly, mark the nonce on the srcChain in the executionState mapping as STATUS_READY, so the nonce can be retried or retrieved and retried.

• By making sure that the nonce is set as STATUS_READY in case the tx execution in the RootBridgeAgent executor fails and no fallback mechanism is set, the contracts will allow that users can retry the same nonce or retrieve and redeem them, and the users will get back the unspent native token of the tx that failed.

• Apply the below changes to the _execute() functions in the RootBridgeAgent contract

function _execute(uint256 _depositNonce, bytes memory _calldata, uint16 _srcChainId) private {
    //Update tx state as executed
    executionState[_srcChainId][_depositNonce] = STATUS_DONE;

    //Try to execute the remote request
    (bool success,) = bridgeAgentExecutorAddress.call{value: address(this).balance}(_calldata);

    // No fallback is requested revert allowing for retry.
-     if (!success) revert ExecutionFailure();
+     if (!success) {
+     executionState[_srcChainId][_depositNonce] = STATUS_READY;
      //@audit-info => Make sure that the refundee gets back the unspent ETH
+     <decodeRefundeeFromCalldata>.call{value: address(this).balance}();
    }
}

function _execute(
    bool _hasFallbackToggled,
    uint32 _depositNonce,
    address _refundee,
    bytes memory _calldata,
    uint16 _srcChainId
) private {
    //@audit-ok => Sets the [_srcChainId][nonce] as STATUS_DONE, it can't be re-executed!
    //Update tx state as executed
    executionState[_srcChainId][_depositNonce] = STATUS_DONE;

    //Try to execute the remote request
    (bool success,) = bridgeAgentExecutorAddress.call{value: address(this).balance}(_calldata);

    //Update tx state if execution failed
    if (!success) {
        //Read the fallback flag.
        if (_hasFallbackToggled) {
            // Update tx state as retrieve only
            executionState[_srcChainId][_depositNonce] = STATUS_RETRIEVE;
            // Perform the fallback call
            _performFallbackCall(payable(_refundee), _depositNonce, _srcChainId);
        } else {
-           // No fallback is requested revert allowing for retry.
-           revert ExecutionFailure();
+           executionState[_srcChainId][_depositNonce] = STATUS_READY;
            //@audit-info => Make sure that the refundee gets back the unspent ETH
+           <decodeRefundeeFromCalldata>.call{value: address(this).balance}();
        }
    }
}

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

• All user deposited assets, both, hTokens and underlyingTokens are at risk of getting stuck in a BranchPort if the address of the depositor gets blacklisted in one of all the deposited underlyingTokens

Proof of Concept

The problem is caused by the fact that the redemption process works by sending back all the tokens that were deposited and that tokens can only be sent back to the same address from where they were deposited.

• Users can deposit/bridgeOut multiple tokens at once (underlyingTokens and hTokens) from a Branch to Root. The system has a mechanism to prevent users from losing their tokens in case something fails with the execution of the crosschain message in the Root environment.

  • If something fails with the execution in Root, the users can retry the deposit, and as a last resource, they can retrieve and redeem their deposit from Root and get their tokens back in the Branch where they were deposited.

• When redeeming deposits, the redemption is made atomically, in the sense that it redeems all the tokens that were deposited at once, it doesn't redeem one or two specific tokens, it redeems all of them.

  • The problem is that the function to redeem the tokens sets the recipient address to be the caller (msg.sender), and the caller is enforced to be only the owner of the depositor (i.e. the account from where the tokens were taken from).
    • The fact that the depositor's address gets blacklisted in one of the underlying tokens should not cause that all the rest of the tokens to get stuck in the BranchPort.

function redeemDeposit(uint32 _depositNonce) external override lock {
    //@audit-info => Loads the deposit's information based on the _depositNonce
    // Get storage reference
    Deposit storage deposit = getDeposit[_depositNonce];

    // Check Deposit
    if (deposit.status == STATUS_SUCCESS) revert DepositRedeemUnavailable();
    if (deposit.owner == address(0)) revert DepositRedeemUnavailable();
    if (deposit.owner != msg.sender) revert NotDepositOwner();

    // Zero out owner
    deposit.owner = address(0);

    //@audit-issue => Sending back tokens to the deposit.owner. Depositors can't specify the address where they'd like to receive their tokens
    // Transfer token to depositor / user
    for (uint256 i = 0; i < deposit.tokens.length;) {
        _clearToken(msg.sender, deposit.hTokens[i], deposit.tokens[i], deposit.amounts[i], deposit.deposits[i]);

        unchecked {
            ++i;
        }
    }

Coded PoC

• I coded a PoC to demonstrate the problem I'm reporting, using the RootForkTest.t.sol test file as the base to reproduce this PoC:
  • Make sure to import the below Mock a Blacklisted token under the test/ulysses-omnichain/helpers/ folder, and also add the global variables and the 3 below functions in the RootForkTest file

// SPDX-License-Identifier: AGPL-3.0-only
pragma solidity >=0.8.0;

import {MockERC20} from "solmate/test/utils/mocks/MockERC20.sol";

contract BlacklistedToken is MockERC20 {

  mapping (address => bool) public blacklistedUsers;

  constructor(
    string memory _name,
    string memory _symbol,
    uint8 _decimals
  ) MockERC20(_name, _symbol, _decimals) {}


  function blacklistAddress(address _user) external returns (bool) {
    blacklistedUsers[_user] = true;
  }

  function transfer(address to, uint256 amount) public override returns (bool) {
    if(blacklistedUsers[to]) revert("Blacklisted User");
    super.transfer(to,amount);
    return true;
  }

  function mint(address to, uint256 value) public override {
    super._mint(to, value);
  }

  function burn(address from, uint256 value) public override {
    super._burn(from, value);
  }

}

...

+//@audit-info => Blacklisted Token
+import "./helpers/BlacklistedToken.sol";

...

contract RootForkTest is LzForkTest {
    ...

    // ERC20s from different chains.

    address avaxMockAssethToken;

    MockERC20 avaxMockAssetToken;

    //@audit-info => underlyingTokens for PoC
+   MockERC20 underToken0;
+   MockERC20 underToken1;
    //@audit-info => Create a new token using a contract that allows to Blacklist users!
+   BlacklistedToken underBlacklistToken;

    ...

    function _deployUnderlyingTokensAndMocks() internal {
        //Switch Chain and Execute Incoming Packets
        switchToLzChain(avaxChainId);
        vm.prank(address(1));
        // avaxMockAssethToken = new MockERC20("hTOKEN-AVAX", "LOCAL hTOKEN FOR TOKEN IN AVAX", 18);
        avaxMockAssetToken = new MockERC20("underlying token", "UNDER", 18);
        
        //@audit-info => Deploying underlyingTokens for PoC
+       underToken0 = new MockERC20("u0 token", "U0", 18);
+       underToken1 = new MockERC20("u0 token", "U0", 18);
+       underBlacklistToken = new BlacklistedToken("u2 BlaclistedToken", "U2", 18);

        ...
    }

    ...

    //@audit => Variables required for the PoC
+   address[] public hTokens;
+   address[] public tokens;
+   uint256[] public amounts;
+   uint256[] public deposits;
+
+   address public localTokenUnder0;
+   address public localTokenUnder1;
+   address public localBlacklistedToken;
+
+   address public globalTokenUnder0;
+   address public globalTokenUnder1;
+   address public globalBlacklistedToken;
+
+   address public _recipient;

    //@audit-info => First function required for the PoC, will create a Deposit in a Branch that will fail its execution in Root
    function testDepositBlocklistedTokenWithNotEnoughGasForRootFallbackModePoC() public {

        //Switch Chain and Execute Incoming Packets
        switchToLzChain(avaxChainId);

        vm.deal(address(this), 10 ether);

        avaxCoreRouter.addLocalToken{value: 1 ether}(address(underToken0), GasParams(2_000_000, 0));
        avaxCoreRouter.addLocalToken{value: 1 ether}(address(underToken1), GasParams(2_000_000, 0));
        avaxCoreRouter.addLocalToken{value: 1 ether}(address(underBlacklistToken), GasParams(2_000_000, 0));
        
        
        //Switch Chain and Execute Incoming Packets
        switchToLzChain(rootChainId);

         //Switch Chain and Execute Incoming Packets
        switchToLzChain(avaxChainId);

        //Switch Chain and Execute Incoming Packets
        switchToLzChain(rootChainId);
        prevNonceRoot = multicallRootBridgeAgent.settlementNonce();

        localTokenUnder0 = rootPort.getLocalTokenFromUnderlying(address(underToken0), avaxChainId);
        localTokenUnder1 = rootPort.getLocalTokenFromUnderlying(address(underToken1), avaxChainId);
        localBlacklistedToken = rootPort.getLocalTokenFromUnderlying(address(underBlacklistToken), avaxChainId);

        switchToLzChain(avaxChainId);
        prevNonceBranch = avaxMulticallBridgeAgent.depositNonce();

        vm.deal(address(this), 50 ether);

        uint256 _amount0 = 1 ether;
        uint256 _amount1 = 1 ether;
        uint256 _amount2 = 1 ether;

        uint256 _deposit0 = 1 ether;
        uint256 _deposit1 = 1 ether;
        uint256 _deposit2 = 1 ether;


        //GasParams
        GasParams memory gasParams = GasParams(100_000 , 0 ether);

        _recipient = address(this);

        vm.startPrank(address(avaxPort));

        ERC20hTokenBranch(localTokenUnder0).mint(_recipient, _amount0 - _deposit0);
        ERC20hTokenBranch(localTokenUnder1).mint(_recipient, _amount1 - _deposit1);
        ERC20hTokenBranch(localBlacklistedToken).mint(_recipient, _amount2 - _deposit2);

        underToken0.mint(_recipient, _deposit0);
        underToken1.mint(_recipient, _deposit1);
        underBlacklistToken.mint(_recipient, _deposit2);

        vm.stopPrank();

        // Cast to Dynamic
        hTokens.push(address(localTokenUnder0));
        hTokens.push(address(localTokenUnder1));
        hTokens.push(address(localBlacklistedToken));

        tokens.push(address(underToken0));
        tokens.push(address(underToken1));
        tokens.push(address(underBlacklistToken));

        amounts.push(_amount0);
        amounts.push(_amount1);
        amounts.push(_amount2);

        deposits.push(_deposit0);
        deposits.push(_deposit1);
        deposits.push(_deposit2);


        //@audit-info => Prepare deposit info
        DepositMultipleInput memory depositInput =
            DepositMultipleInput({hTokens: hTokens, tokens: tokens, amounts: amounts, deposits: deposits});

    
        // Approve AvaxPort to spend
        MockERC20(hTokens[0]).approve(address(avaxPort), amounts[0] - deposits[0]);
        MockERC20(tokens[0]).approve(address(avaxPort), deposits[0]);
        MockERC20(hTokens[1]).approve(address(avaxPort), amounts[1] - deposits[1]);
        MockERC20(tokens[1]).approve(address(avaxPort), deposits[1]);
        MockERC20(hTokens[2]).approve(address(avaxPort), amounts[2] - deposits[2]);
        BlacklistedToken(tokens[2]).approve(address(avaxPort), deposits[2]);
        

        //@audit-info => deposit multiple assets from Avax branch to Root
        //@audit-info => Attempting to deposit two hTokens and two underlyingTokens
        avaxMulticallBridgeAgent.callOutSignedAndBridgeMultiple{value: 1 ether}(
            payable(address(this)),bytes(""), depositInput, gasParams, true
        );

        require(prevNonceBranch == avaxMulticallBridgeAgent.depositNonce() - 1, "Branch should be updated");

        // avaxMulticallRouter.callOutAndBridgeMultiple{value: 1 ether}(bytes(""), depositInput, gasParams);

        console2.log("GOING ROOT AFTER BRIDGE REQUEST FROM AVAX");
        //Switch Chain and Execute Incoming Packets
        switchToLzChain(rootChainId);
        require(prevNonceRoot == multicallRootBridgeAgent.settlementNonce(), "Root should not be updated");

    }

    //@audit-info => Calls the function above and retrieves the deposit in the Branch
    function testRetrieveDepositPoC() public {
        //Set up
        testDepositBlocklistedTokenWithNotEnoughGasForRootFallbackModePoC();

        switchToLzChain(avaxChainId);

        //Get some ether.
        vm.deal(address(this), 10 ether);

        //Call Deposit function
        console2.log("retrieving");
        avaxMulticallBridgeAgent.retrieveDeposit{value: 10 ether}(prevNonceRoot, GasParams(1_000_000, 0.01 ether));

        require(
            avaxMulticallBridgeAgent.getDepositEntry(prevNonceRoot).status == 0, "Deposit status should be success."
        );

        console2.log("Going ROOT to retrieve Deposit");
        switchToLzChain(rootChainId);
        console2.log("Triggered Fallback");

        console2.log("Returning to Avax");
        switchToLzChain(avaxChainId);
        console2.log("Done ROOT");

        require(
            avaxMulticallBridgeAgent.getDepositEntry(prevNonceRoot).status == 1,
            "Deposit status should be ready for redemption."
        );
    }


    //@audit-info => The _recipient/depositor of the Deposit is blacklisted before redeeming the deposit from the Branch
    function testRedeemBlocklistedTokenPoC() public {
        //Set up
        testRetrieveDepositPoC();

        //Get some ether.
        vm.deal(address(this), 10 ether);

        uint256 balanceBeforeUnderToken0 = underToken0.balanceOf(_recipient);
        uint256 balanceBeforeUnderToken1 = underToken1.balanceOf(_recipient);
        uint256 balanceBeforeBlaclistedToken = underBlacklistToken.balanceOf(_recipient);

        uint256 balanceBeforeUnderToken0BranchPort = underToken0.balanceOf(address(avaxPort));
        uint256 balanceBeforeUnderToken1BranchPort = underToken1.balanceOf(address(avaxPort));
        uint256 balanceBeforeBlaclistedTokenBranchPort = underBlacklistToken.balanceOf(address(avaxPort));

        //@audit-info => receiver get's blacklisted before redeeming its deposit
        underBlacklistToken.blacklistAddress(_recipient);

        //Call Deposit function
        console2.log("redeeming");
        vm.expectRevert();
        avaxMulticallBridgeAgent.redeemDeposit(prevNonceRoot);

        assertFalse(
            avaxMulticallBridgeAgent.getDepositEntry(prevNonceRoot).owner == address(0),
            "Deposit status should not have deleted because the redemption can't be executed"
        );

        assertFalse(underToken0.balanceOf(_recipient) == balanceBeforeUnderToken0 + 1 ether, "Balance should not be increased because tokens can't be redeemed");
        assertFalse(underToken1.balanceOf(_recipient) == balanceBeforeUnderToken1 + 1 ether, "Balance should not be increased because tokens can't be redeemed");
        assertFalse(underBlacklistToken.balanceOf(_recipient) == balanceBeforeBlaclistedToken + 1 ether, "Balance should not be increased because tokens can't be redeemed");
    }

• Now everything is ready to run the test and analyze the output:

forge test --mc RootForkTest --match-test testRedeemBlocklistedTokenPoC -vvvv

• As we can see in the Output, the depositor can't redeem its deposit because his address was blacklisted in one of the 3 deposited underlyingTokens.
  • As a consequence, the depositor's tokens are stuck in the BranchPort

├─ [0] console::log(redeeming) [staticcall]
    │   └─ ← ()
    ├─ [0] VM::expectRevert()
    │   └─ ← ()
    ├─ [45957] BranchBridgeAgent::redeemDeposit(1)
    │   ├─ [19384] BranchPort::withdraw(RootForkTest: [0xBb2180ebd78ce97360503434eD37fcf4a1Df61c3], MockERC20: [0x32Fa025409e66A35F3C95B04a195b4517f479dCF], 1000000000000000000 [1e18])
    │   │   ├─ [18308] MockERC20::transfer(RootForkTest: [0xBb2180ebd78ce97360503434eD37fcf4a1Df61c3], 1000000000000000000 [1e18])
    │   │   │   ├─ emit Transfer(from: BranchPort: [0x369Ff55AD83475B07d7FF2F893128A93da9bC79d], to: RootForkTest: [0xBb2180ebd78ce97360503434eD37fcf4a1Df61c3], amount: 1000000000000000000 [1e18])
    │   │   │   └─ ← true
    │   │   └─ ← ()
    │   ├─ [19384] BranchPort::withdraw(RootForkTest: [0xBb2180ebd78ce97360503434eD37fcf4a1Df61c3], MockERC20: [0x541dC483Eb43cf8F9969baF71BF783193e5C5B1A], 1000000000000000000 [1e18])
    │   │   ├─ [18308] MockERC20::transfer(RootForkTest: [0xBb2180ebd78ce97360503434eD37fcf4a1Df61c3], 1000000000000000000 [1e18])
    │   │   │   ├─ emit Transfer(from: BranchPort: [0x369Ff55AD83475B07d7FF2F893128A93da9bC79d], to: RootForkTest: [0xBb2180ebd78ce97360503434eD37fcf4a1Df61c3], amount: 1000000000000000000 [1e18])
    │   │   │   └─ ← true
    │   │   └─ ← ()
    │   ├─ [1874] BranchPort::withdraw(RootForkTest: [0xBb2180ebd78ce97360503434eD37fcf4a1Df61c3], BlacklistedToken: [0x56723b40D167976C402fBfe901cDD81fA5584dc4], 1000000000000000000 [1e18])
    │   │   ├─ [660] BlacklistedToken::transfer(RootForkTest: [0xBb2180ebd78ce97360503434eD37fcf4a1Df61c3], 1000000000000000000 [1e18])
    │   │   │   └─ ← "Blacklisted User"
    │   │   └─ ← 0x90b8ec18
    │   └─ ← 0x90b8ec18
    ├─ [6276] BranchBridgeAgent::getDepositEntry(1) [staticcall]
    │   └─ ← (1, 0xBb2180ebd78ce97360503434eD37fcf4a1Df61c3, [0xabb4Cf532dC72dFDe5a18c67AF3fD3359Cb87055, 0x2B8A2bb23C66976322B20B6ceD182b1157B92862, 0x6079330AaAC5ca228ade7a78CF588F67a23Fe815], [0x32Fa025409e66A35F3C95B04a195b4517f479dCF, 0x541dC483Eb43cf8F9969baF71BF783193e5C5B1A, 0x56723b40D167976C402fBfe901cDD81fA5584dc4], [1000000000000000000 [1e18], 1000000000000000000 [1e18], 1000000000000000000 [1e18]], [1000000000000000000 [1e18], 1000000000000000000 [1e18], 1000000000000000000 [1e18]])
    ├─ [542] MockERC20::balanceOf(RootForkTest: [0xBb2180ebd78ce97360503434eD37fcf4a1Df61c3]) [staticcall]
    │   └─ ← 0
    ├─ [542] MockERC20::balanceOf(RootForkTest: [0xBb2180ebd78ce97360503434eD37fcf4a1Df61c3]) [staticcall]
    │   └─ ← 0
    ├─ [542] BlacklistedToken::balanceOf(RootForkTest: [0xBb2180ebd78ce97360503434eD37fcf4a1Df61c3]) [staticcall]
    │   └─ ← 0
    ├─ [542] MockERC20::balanceOf(BranchPort: [0x369Ff55AD83475B07d7FF2F893128A93da9bC79d]) [staticcall]
    │   └─ ← 1000000000000000000 [1e18]
    ├─ [542] MockERC20::balanceOf(BranchPort: [0x369Ff55AD83475B07d7FF2F893128A93da9bC79d]) [staticcall]
    │   └─ ← 1000000000000000000 [1e18]
    ├─ [542] BlacklistedToken::balanceOf(BranchPort: [0x369Ff55AD83475B07d7FF2F893128A93da9bC79d]) [staticcall]
    │   └─ ← 1000000000000000000 [1e18]
    └─ ← ()

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 3.58s

Tools Used

Manual Audit

Recommended Mitigation Steps

• When redeeming the failed deposits, the easiest and most straightforward solution is to allow the depositor to pass an address where it would like to receive all the deposited tokens.

- function redeemDeposit(uint32 _depositNonce) external override lock {
+ function redeemDeposit(uint32 _depositNonce, address _receiver) external override lock {  
        ...

        
        // Transfer token to depositor / user
        for (uint256 i = 0; i < deposit.tokens.length;) {
-           _clearToken(msg.sender, deposit.hTokens[i], deposit.tokens[i], deposit.amounts[i], deposit.deposits[i]);
+           _clearToken(_receiver, deposit.hTokens[i], deposit.tokens[i], deposit.amounts[i], deposit.deposits[i]);

            unchecked {
                ++i;
            }
        }

        ...
    }

Assessed type

Token-Transfer

Lines of code

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

Vulnerability details

Impact

• The impact of reading the destAddr instead of the srcAddr in the requiresEndpoint() modifier can be categorized in two big problems:
  1. BridgeAgents (both, Branch and Root) won't be able to process the calls that are sent from their counterparties in the different branches/chains
  2. Attackers can gain direct access to the BridgeAgent::lzReceive() by calling the LayerZero endpoint using a Fake Contract and sending any arbitrary data at will.

Proof of Concept

Before explaining each of the two problems mentioned in the previous sections, first, let's understand how and why the issue occurs in the first place.

• The Branch contracts are meant to be deployed on different chains, and the Root contracts are meant to be deployed on Arbitrum (Arbitrum is also a Branch chain at the same time)
• Most of the interactions with the protocol must go through the Root branch, even though the interaction was started in Mainnet or Optimism, the execution flow needs to go through the Root contracts to update the virtual balances and reflect the changes in the state of the Root Branch.

  • To achieve crosschain communication, the contracts are using the LayerZero crosschain message protocol, which in a few words works like this:

    • Branch contracts receives an user request, the branch contracts prepares the calldata that will be sent to the Root branch (Deployed in Arbitrum), the BranchBridgeAgent will call the send() function of the LayerZeroEndpoint contract that is deployed in the same chain, the LayerZero protocol will receive the call, validate it and will run the lzReceive() of the RootBranchBridge contract (deployed in Arbitrum), then, the lzReceive() calls the lzReceiveNonBlocking() in the same contract, and prior to execute anything there is the requiresEndpoint() modifier that is in charge of validating that the caller is either the LayerZeroEndpoint contract or the BranchBridgeAgent that is deployed on Arbitrum, if the caller is not the BranchBridgeAgent and it's the LayerZeroEndpoint, then it proceeds to validate that the address that actually sent the message to LayerZero (from the srcAddress) is the BranchBridgeAgent contract of the SourceChain, and if that check passes, then the modifier will allow the lzReceiveNonBlocking() to be executed, otherwise, the tx will be reverted.

  • Apparently, everything is fine, but, there is a problem, a very well-hidden problem, the address that is used to validate if the caller that sent the message to LayerZero is extracted from the last 20 bytes of the _srcAddress parameter, and the first 20 bytes are ignored.

    • To understand why reading the last 20 bytes instead of the first 20 bytes it's a problem, it is required to take a look at how LayerZero encodes that data, that's why we are gonna take a look at the contracts of the LayerZero.
      • The execution flow in the LayerZero contracts look like this: Endpoint::send() => UltraLightNodeV2::send() => UltraLightNodeV2::validateTransactionProof() => Endpoint::receivePayload() => ILayerZeroReceiver(_dstAddress).lzReceive()
      • As for this report we are mostly interested in the interaction from the UltraLightNodeV2::validateTransactionProof() => Endpoint::receivePayload() => ILayerZeroReceiver(_dstAddress).lzReceive()
        • By looking at the UltraLightNodeV2::validateTransactionProof() function we can see that the srcAddress (The one that called the Endpoint) is encoded in the first 20 bytes of the pathData, and the dstAddress (The contract that will receive the message) is encoded in the last 20 bytes.

    function validateTransactionProof(uint16 _srcChainId, address _dstAddress, uint _gasLimit, bytes32 _lookupHash, bytes32 _blockData, bytes calldata _transactionProof) external override {
        ...

        //@audit-info => pathData will be sent to `endpoint:receivePayload` and there will be received as the _srcAddress, and that exact same value is forwarded to the DestinationContract::lzReceive()
        bytes memory pathData = abi.encodePacked(_packet.srcAddress, _packet.dstAddress);
        emit PacketReceived(_packet.srcChainId, _packet.srcAddress, _packet.dstAddress, _packet.nonce, keccak256(_packet.payload));
        endpoint.receivePayload(_srcChainId, pathData, _dstAddress, _packet.nonce, _gasLimit, _packet.payload);
    }

    function receivePayload(uint16 _srcChainId, bytes calldata _srcAddress, address _dstAddress, uint64 _nonce, uint _gasLimit, bytes calldata _payload) external override receiveNonReentrant {
       ...

        //@audit-info => In the `Endpoint::receivePayload()`, the pathData from the `UltraLightNodeV2::validateTransactionProof()` is received as the _srcAddress parameter, which is then forwarded as it is to the `DestinationContract.lzReceive()`
        try ILayerZeroReceiver(_dstAddress).lzReceive{gas: _gasLimit}(_srcChainId, _srcAddress, _nonce, _payload) {
            // success, do nothing, end of the message delivery
        } catch (bytes memory reason) {
           ...
        }
    }

• In the case of the contracts for the Ulysses system, the DestinationContract in the LayerZero will be a BridgeAgent, this means that the value of the _srcAddress parameter that is received in the BridgeAgent::lzReceive() will be encoded exactly how the UltraLightNodeV2::validateTransactionProof() encoded it, the first 20 bytes containing the srcAddress (The one that called the Endpoint), and the last 20 bytes the dstAddress (The contract that will receive the message)

  • So, when the BridgeAgent::lzReceive() function receives the call from LayerZero and it calls the lzReceiveNonBlocking() it will call the modifier requiresEndpoint() to validate that the caller is the LayerZeroEndpoint or the LocalBranchBridgeAgent, and if the LayerZeroEndpoint is the caller, it must validate that the address that sent the message is, in reality, the BranchBridgeAgent of the SourceChain.
    • RootBridgeAgent::lzReceive() => RootBridgeAgent::lzReceiveNonBlocking() => RootBridgeAgent::requiresEndpoint()

function lzReceive(uint16 _srcChainId, bytes calldata _srcAddress, uint64, bytes calldata _payload) public {
    (bool success,) = address(this).excessivelySafeCall(
        gasleft(),
        150,
        //@audit-info => lzReceive() forwards the _srcAddress parameter as it is received from the LayerZeroEndpoint
        //@audit-info => As shown before, the UltraLightNodeV2 library encodes in the first 20 bytes the srcAddress and in the last 20 bytes the destinationAddress
        abi.encodeWithSelector(this.lzReceiveNonBlocking.selector, msg.sender, _srcChainId, _srcAddress, _payload)
    );

    if (!success) if (msg.sender == getBranchBridgeAgent[localChainId]) revert ExecutionFailure();
}

```solidity
function lzReceiveNonBlocking(
        //@audit-info => The _endpoint is the msg.sender of the lzReceive()
        address _endpoint,
        uint16 _srcChainId,
        bytes calldata _srcAddress,
        bytes calldata _payload
        //@audit-info => _endpoint can only be the LocalBranchBridgeAgent or the LayerZero Endpoint!
        //@audit-info => _srcAddress is encoded exactly as how the UltraLightNodeV2 library encoded it
            //@audit-info => in the first 20 bytes the srcAddress and in the last 20 bytes the destinationAddress
    ) public override requiresEndpoint(_endpoint, _srcChainId, _srcAddress) {
        ...
    }

• As we can see, the requiresEndpoint() modifier reads from the _srcAddress parameter, the offset being read is from the PARAMS_ADDRESS_SIZE, which its value is 20, to the last byte, that means, the offset being read is from the byte 20 to the byte 40, that means, it is reading the bytes corresponding to the DestinationAddress, instead of the reading the bytes corresponding to the SourceAddress

modifier requiresEndpoint(address _endpoint, uint16 _srcChain, bytes calldata _srcAddress) virtual {
    if (msg.sender != address(this)) revert LayerZeroUnauthorizedEndpoint();

    //@audit-info => _endpoint can be the LocalBranchBridgeAgent (The BranchBridgeAgent deployed in Arbitrum!)
    if (_endpoint != getBranchBridgeAgent[localChainId]) {
        //@audit-info => If _endpoint is not the LocalBranchBridgeAgent, it can only be the LayerZero Endpoint!
        if (_endpoint != lzEndpointAddress) revert LayerZeroUnauthorizedEndpoint();

        if (_srcAddress.length != 40) revert LayerZeroUnauthorizedCaller();

        //@audit-info => Checks if the `_srcAddres` is the BranchBridgeAgent of the sourceChain!
        if (getBranchBridgeAgent[_srcChain] != address(uint160(bytes20(_srcAddress[PARAMS_ADDRESS_SIZE:])))) {
            revert LayerZeroUnauthorizedCaller();
        }
    }
    _;
}

• At this point we've already covered why the problem exists in the first place, to summarize all of the above, the problem is that the requiresEndpoint() in the BridgeAgent contracts is reading an incorrect offset to validate if the caller of the message that was sent to the LayerZero is a valid BranchBridgeAgent, instead of reading the offset corresponding to the srcAddress (The caller) [The first 20 bytes], it is reading the offset corresponding to the dstAddress (The destination) [The last 20 bytes]

• Now, it is time to explain how this problem/bug/vulnerability can cause problems to the protocol, as I mentioned in the beginning, this problem can cause two major problems:

1. BridgeAgents (both, Branch and Root) won't be able to process the calls that are sent from their counterparties in the different branches/chains:
   • The BridgeAgents won't be able to process the calls because of the condition in the requiresEndpoint() modifier that validates if the srcAddress (The Caller) who sent the message to the LayerZero is the BridgeAgent of the Source Chain, the bytes that are being read corresponds to the Destination, instead of the Source, that means, the address that will be used to validate the caller it will be the address of the Destination Contract (This address is really the same address of the contract where the check is being executed), instead of the address of the actual caller, this will cause the tx to be reverted and never executed (To demonstrate this first point I coded a PoC)

modifier requiresEndpoint(address _endpoint, uint16 _srcChain, bytes calldata _srcAddress) virtual {
        ...
        ...
            ...
            //@audit-info => Check if the `_srcAddres` is the BranchBridgeAgent of the sourceChain!
            //@audit-info => Currently is reading the last 20 bytes, those bytes corresponds to the Destination Address of the message, which is the address of the contract where the execution is currently running.
            //@audit-info => requiresEndpoint() compares if the sourceAddress is different than the BranchBridgeAgent of the Source Chain, and because the address being read is the Destination instead of the Source, this check will always revert for calls between BridgeAgents!
            if (getBranchBridgeAgent[_srcChain] != address(uint160(bytes20(_srcAddress[PARAMS_ADDRESS_SIZE:])))) {
                revert LayerZeroUnauthorizedCaller();
            }
        ...
    ...
}

2. Attackers can gain direct access to the BridgeAgent::lzReceive() by calling the LayerZero endpoint using a Fake Contract and sending any arbitrary data at will.
   • Same logic as in Point 1, but this time an attacker can exploit the vulnerability that the only check to verify the authenticity of the caller of the LayerZeroEndpoint is wrong, the modifier is currently ignoring who was the caller of the message that is received from the LayerZeroEndpoint, this opens the doors for attacker to create Malicious contracts to send arbitrary data through a message call using the LayerZeroEndpoint and gain access to all the functionalities of the lzReceiveNonBlocking() function.
     • For this scenario, I was unable to code a PoC because it would've been necessary to modify all the setUp of the testing suite
     • An idea of a Malicious Contract could be something like this one, it is preparing the calldata as the BridgeAgent expects, but this allows to set all the values at will, and this also gives access to Admin functionalities to the attackers

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

import {ILayerZeroEndpoint} from "./interfaces/ILayerZeroEndpoint.sol";

import {
    GasParams
} from "./interfaces/IRootBridgeAgent.sol";

interface ICoreRootRouter {
  function bridgeAgentAddress() external view returns (address);
}

interface IBridgeAgent {
  function getBranchBridgeAgentPath(uint256 chainId) external view returns (bytes memory);
  function lzEndpointAddress() external view returns (address);
}

contract MaliciousContract {

  uint32 settlementNonce = 1;
  bytes destinationPath;
  address lzEndpointAddress;

  function setDestinationPath(address CoreRootRouter, uint256 dstChainId) public {
    address bridgeAgentAddress = ICoreRootRouter(CoreRootRouter).bridgeAgentAddress();
    destinationPath = IBridgeAgent(bridgeAgentAddress).getBranchBridgeAgentPath(dstChainId);
    lzEndpointAddress = IBridgeAgent(bridgeAgentAddress).lzEndpointAddress();
  }

  function hijackedPortStrategy(
    address _portStrategy,
    address _underlyingToken,
    uint256 _dailyManagementLimit,
    bool _isUpdateDailyLimit,
    address _refundee,
    uint16 _dstChainId,
    GasParams calldata _gParams,
    address CoreRootRouter
  ) external payable {
    // Encode CallData
    bytes memory params = abi.encode(_portStrategy, _underlyingToken, _dailyManagementLimit, _isUpdateDailyLimit);

    // Pack funcId into data
    bytes memory _payload = abi.encodePacked(bytes1(0x06), params);

    //Encode Data for call.
    bytes memory payload = abi.encodePacked(bytes1(0x00), _refundee, settlementNonce++, _payload);

    setDestinationPath(CoreRootRouter, _dstChainId);

    _performCall(_dstChainId, payable(_refundee), payload, _gParams, lzEndpointAddress);
  }

  function _performCall(
    uint16 _dstChainId,
    address payable _refundee,
    bytes memory _payload,
    GasParams calldata _gParams,
    address lzEndpointAddress
  ) internal {

    ILayerZeroEndpoint(lzEndpointAddress).send{value: msg.value}(
        _dstChainId,
        // getBranchBridgeAgentPath[_dstChainId],
        destinationPath,
        _payload,
        _refundee,
        address(0),
        // abi.encodePacked(uint16(2), _gParams.gasLimit, _gParams.remoteBranchExecutionGas, callee)
        abi.encodePacked(uint16(2), _gParams.gasLimit, _gParams.remoteBranchExecutionGas, destinationPath)
    );

  }

}

Coded a Poc

• To reproduce the PoC for the first scenario is necessary to make a couple of changes, the first change needs to be done in the LzForkTest.t.sol test file, it's required to update the order in which the executePacket() function orders the srcAddress and destAddress, line 466.

    function executePacket(Packet memory packet) public {
        ...
        ILayerZeroEndpoint(receivingEndpoint).receivePayload(
            packet.originLzChainId,
-           // abi.encodePacked(packet.destinationUA, packet.originUA), //original, wrong => encoding destAddress in the first 20 bytes and srcAddr in the last 20 bytes (Inversed order as per the UltraLightNodeV2 library)
+           abi.encodePacked(packet.originUA, packet.destinationUA), //fixed, correct => Encoding srcAddr in the first 20 bytes and destAddr in the last 20 bytes, (Exact order as per the UltraLightNodeV2 library)
            packet.destinationUA,
            packet.nonce,
            gasLimit,
            packet.payload
        );
    }

• Now, we can use the RootForkTest.t.sol test file to add the below test:

function testWrongDecodingPoC() public {
    // Add strategy token
    testAddStrategyToken();

    // Deploy Mock Strategy
    switchToLzChainWithoutExecutePendingOrPacketUpdate(ftmChainId);
    mockFtmPortStrategyAddress = address(new MockPortStartegy());
    switchToLzChainWithoutExecutePendingOrPacketUpdate(rootChainId);

    // Get some gas
    vm.deal(address(this), 1 ether);

    coreRootRouter.managePortStrategy{value: 1 ether}(
        mockFtmPortStrategyAddress,
        address(mockFtmPortToken),
        250 ether,
        false,
        address(this),
        ftmChainId,
        GasParams(300_000, 0)
    );

    // Switch Chain and Execute Incoming Packets
    switchToLzChain(ftmChainId);

    require(ftmPort.isPortStrategy(mockFtmPortStrategyAddress, address(mockFtmPortToken)), "Should be added");

    // Switch Chain and Execute Incoming Packets
    switchToLzChainWithoutExecutePendingOrPacketUpdate(rootChainId);
}

• Run the test with the following command (At this point don't make any other changes to the rest of the files, this first test is expected to fail because the requiresEndpoint() will revert the tx due to the problem described on this report, after running this test we will apply the fix to the requiresEndpoint() modifier and we'll re-run the test to verify that everything is working as expected!)

forge test --mc RootForkTest --match-test testWrongDecodingPoC -vvvv

- Expected output after running the first test:

03], 0x), ([0x8be0079c531659141344cd1fd0a4f28419497f9722a3daafe3b4186f6b6457e0, 0x000000000000000000000000bb2180ebd78ce97360503434ed37fcf4a1df61c3, 0x0000000000000000000000000000000000000000000000000000000000000000], 0x)]
    ├─ [0] console::log(Events caugth:, 5) [staticcall]
    │   └─ ← ()
    ├─ [0] VM::resumeGasMetering()
    │   └─ ← ()
    ├─ [501] BranchPort::bridgeAgents(1) [staticcall]
    │   └─ ← "EvmError: Revert"
    └─ ← "EvmError: Revert"

Test result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 3.13s

Ran 1 test suites: 0 tests passed, 1 failed, 0 skipped (1 total tests)

Failing tests:
Encountered 1 failing test in test/ulysses-omnichain/RootForkTest.t.sol:RootForkTest
[FAIL. Reason: Setup failed: EvmError: Revert] setUp() (gas: 0)

Encountered a total of 1 failing tests, 0 tests succeeded

• Now, let's apply the fix to the requiresEndpoint(), instead of reading the last 20 bytes, let's update it so that it correctly reads the first 20 bytes (The offset where srcAddress is encoded as per the UltraLightNodeV2 library)
  • Make sure to apply this fix to the RootBridgeAgent and the BranchBridgeAgent contracts

modifier requiresEndpoint(address _endpoint, uint16 _srcChain, bytes calldata _srcAddress) virtual {
    ...


            //@audit-info => The correct offset of the srcAddr is in the first 20 bytes!
-           // if (getBranchBridgeAgent[_srcChain] != address(uint160(bytes20(_srcAddress[PARAMS_ADDRESS_SIZE:])))) {
+           if (getBranchBridgeAgent[_srcChain] != address(uint160(bytes20(_srcAddress[0 : PARAMS_ADDRESS_SIZE])))) {
    
    ...

• After applying the changes to the requiresEndpoint() modifier, re-run the test using the same command and check the output to verify that everything is working as expected

    ├─ [128424] 0xb6319cC6c8c27A8F5dAF0dD3DF91EA35C4720dd7::receivePayload(110, 0xcb3b263f002b8888f712ba46ebf1302e1294608c2aa5ae54ddbc0f80caed4effc308ba50a20e86e3, BranchBridgeAgent: [0x2Aa5aE54DdbC0F80caED4efFC308ba50A20E86e3], 3, 300000 [3e5], 0x00bb2180ebd78ce97360503434ed37fcf4a1df61c30000000506000000000000000000000000a4c93df56036aa1a74a40ccd353438fa5eed86380000000000000000000000006dae6e4368ce05b6d6ad22876d3372ab5428686400000000000000000000000000000000000000000000000d8d726b7177a800000000000000000000000000000000000000000000000000000000000000000000)
    │   ├─ [125185] BranchBridgeAgent::lzReceive(110, 0xcb3b263f002b8888f712ba46ebf1302e1294608c2aa5ae54ddbc0f80caed4effc308ba50a20e86e3, 3, 0x00bb2180ebd78ce97360503434ed37fcf4a1df61c30000000506000000000000000000000000a4c93df56036aa1a74a40ccd353438fa5eed86380000000000000000000000006dae6e4368ce05b6d6ad22876d3372ab5428686400000000000000000000000000000000000000000000000d8d726b7177a800000000000000000000000000000000000000000000000000000000000000000000)
    │   │   ├─ [123206] BranchBridgeAgent::lzReceiveNonBlocking(0xb6319cC6c8c27A8F5dAF0dD3DF91EA35C4720dd7, 0xcb3b263f002b8888f712ba46ebf1302e1294608c2aa5ae54ddbc0f80caed4effc308ba50a20e86e3, 0x00bb2180ebd78ce97360503434ed37fcf4a1df61c30000000506000000000000000000000000a4c93df56036aa1a74a40ccd353438fa5eed86380000000000000000000000006dae6e4368ce05b6d6ad22876d3372ab5428686400000000000000000000000000000000000000000000000d8d726b7177a800000000000000000000000000000000000000000000000000000000000000000000)
    │   │   │   ├─ [96551] BranchBridgeAgentExecutor::executeNoSettlement(CoreBranchRouter: [0x315023AA8fd423494967Fe294D05BD4B01169A6e], 0x00bb2180ebd78ce97360503434ed37fcf4a1df61c30000000506000000000000000000000000a4c93df56036aa1a74a40ccd353438fa5eed86380000000000000000000000006dae6e4368ce05b6d6ad22876d3372ab5428686400000000000000000000000000000000000000000000000d8d726b7177a800000000000000000000000000000000000000000000000000000000000000000000)
    │   │   │   │   ├─ [95108] CoreBranchRouter::executeNoSettlement(0x06000000000000000000000000a4c93df56036aa1a74a40ccd353438fa5eed86380000000000000000000000006dae6e4368ce05b6d6ad22876d3372ab5428686400000000000000000000000000000000000000000000000d8d726b7177a800000000000000000000000000000000000000000000000000000000000000000000)
    │   │   │   │   │   ├─ [2795] BranchPort::isPortStrategy(MockPortStartegy: [0xa4c93Df56036Aa1a74a40Ccd353438FA5Eed8638], MockERC20: [0x6dae6e4368ce05B6D6aD22876d3372aB54286864]) [staticcall]
    │   │   │   │   │   │   └─ ← false
    │   │   │   │   │   ├─ [89529] BranchPort::addPortStrategy(MockPortStartegy: [0xa4c93Df56036Aa1a74a40Ccd353438FA5Eed8638], MockERC20: [0x6dae6e4368ce05B6D6aD22876d3372aB54286864], 250000000000000000000 [2.5e20])
    │   │   │   │   │   │   ├─ emit PortStrategyAdded(_portStrategy: MockPortStartegy: [0xa4c93Df56036Aa1a74a40Ccd353438FA5Eed8638], _token: MockERC20: [0x6dae6e4368ce05B6D6aD22876d3372aB54286864], _dailyManagementLimit: 250000000000000000000 [2.5e20])
    │   │   │   │   │   │   └─ ← ()
    │   │   │   │   │   └─ ← ()
    │   │   │   │   └─ ← ()
    │   │   │   ├─ emit LogExecute(nonce: 5)
    │   │   │   └─ ← ()
    │   │   └─ ← ()
    │   └─ ← ()
    ├─ [795] BranchPort::isPortStrategy(MockPortStartegy: [0xa4c93Df56036Aa1a74a40Ccd353438FA5Eed8638], MockERC20: [0x6dae6e4368ce05B6D6aD22876d3372aB54286864]) [staticcall]
    │   └─ ← true
    ├─ [0] VM::selectFork(1)
    │   └─ ← ()
    └─ ← ()

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 5.50s

Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

Manual Audit & LayerZeroEndpoint Message Library

Recommended Mitigation Steps

• The recommendation is to update the offset that is read from the _srcAddress parameter in the requiresEndpoint() modifier for the RootBridgeAgent and the BranchBridgeAgent contracts, instead of reading the last 20 bytes, make sure to read the first 20 bytes, in this way, the contracts will be able to decode the data correctly as how it is sent from the LayerZeroEndpoint.

modifier requiresEndpoint(address _endpoint, uint16 _srcChain, bytes calldata _srcAddress) virtual {
    ...

            //@audit-info => The correct offset of the srcAddr is in the first 20 bytes!
-           // if (getBranchBridgeAgent[_srcChain] != address(uint160(bytes20(_srcAddress[PARAMS_ADDRESS_SIZE:])))) {
+           if (getBranchBridgeAgent[_srcChain] != address(uint160(bytes20(_srcAddress[0 : PARAMS_ADDRESS_SIZE])))) {
    
    ...

Assessed type

en/de-code