Several instances of assumptions on LayerZero refundee can lead to refunded tokens being permanently locked

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootBridgeAgent.sol#L713
https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/MulticallRootRouter.sol#L164
https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/MulticallRootRouter.sol#L188
https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/MulticallRootRouter.sol#L255
https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/MulticallRootRouter.sol#L288
https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/MulticallRootRouter.sol#L344
https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/MulticallRootRouter.sol#L377
https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/MulticallRootRouter.sol#L432
https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/MulticallRootRouter.sol#L465

Vulnerability details

The LayerZero has in place a mechanism for refunding extra native tokens provided to their endpoint's send method that relies on the caller (bridge agent contracts in this case) providing a refund address.

The refundee addresses provided the multipla instances of send calls in BranchBridgeAgent and RootBridgeAgent seem sound, except for a few that are problematic.

1. Within the BranchBridgeAgent.retrieveDeposit use case, both messages (retrieve deposit and fallback) are refunded to the retrieveDeposit caller (msg.sender). While this is correct on the branch chain, on the root chain it may not.
2. Within the MulticallRootRouter's multicallSingleOutput and multicallMultipleOutput (signed), native tokens are refunded in the root chain to the owner of the virtual account. While this address is valid on the branch chain that originally created the virtual address, on the root chain it may not.
3. Within the MulticallRootRouter's multicallSingleOutput and multicallMultipleOutput (unsigned), native tokens are refunded in the root chain to the output recipient. While this address can be assumed to be valid on the branch chain where the output is directed, on the root chain it may not.

For example, if the refundee is a contract that is not deployed on the Arbitrum chain, and it will not be i.e. because its creator has already used the nonce that could deploy at its same address, the refund native tokens are lost.

Impact

Users who can't sign transactions from the same address on the branch and root chain may end up overpaying thir calls.

Proof of Concept

The following runnable (foundry) PoC shows how the root chain endpoint sends tokens to an address (contractUser) that is guaranteed to be valid only on the branch chain, proving the impact on the first scenario:

pragma solidity ^0.8.0;

import "forge-std/Test.sol";

import {BranchBridgeAgent} from "src/BranchBridgeAgent.sol";
import {BranchPort} from "src/BranchPort.sol";
import {RootBridgeAgent} from "src/RootBridgeAgent.sol";
import {GasParams, DepositInput} from "src/interfaces/BridgeAgentStructs.sol";

// as taken from here:
// https://github.com/LayerZero-Labs/solidity-examples/blob/main/contracts/mocks/LZEndpointMock.sol
import {LZEndpointMock} from "./ulysses-omnichain/mocks/LZEndpointMock.sol";

contract FallbackRefundLost is Test {
    function testLostFallbackRefund() public {
        uint16 rootChainId = 9;
        uint16 branchChainId = 10;

        // simulate a real L0 bridge in between
        LZEndpointMock rootLzEndpoint = new LZEndpointMock(rootChainId);
        LZEndpointMock branchLzEndpoint = new LZEndpointMock(branchChainId);

        RootBridgeAgent rba = new RootBridgeAgent(
            rootChainId,             // localChainId
            address(rootLzEndpoint), // _lzEndpointAddress
            address(this),           // _localPortAddress
            address(this)            // _localRouterAddress
        );

        BranchPort bp = new BranchPort(address(this));

        BranchBridgeAgent bba = new BranchBridgeAgent(
            rootChainId,               // _rootChainId
            branchChainId,             // _localChainId
            address(rba),              // _rootBridgeAgentAddress
            address(branchLzEndpoint), // _lzEndpointAddress
            address(this),             // _localRouterAddress
            address(bp)                // _localPortAddress
        );

        bp.initialize(address(this), address(this));
        bp.addBridgeAgent(address(bba));

        branchLzEndpoint.setDestLzEndpoint(address(rba), address(rootLzEndpoint));

        // BranchBridgeAgent knows already the address of RootBridgeAgent from its construction,
        // here we tell RootBridgeAgent where BranchBridgeAgent is 
        // (we set rba instead of bba to work around a separately-reported issue)
        rba.syncBranchBridgeAgent(address(rba), branchChainId);
        rootLzEndpoint.setDestLzEndpoint(address(rba), address(branchLzEndpoint));
        
        uint256 price1 = 11552299000000000;
        uint256 price2 = 28822645511000000;

        // we set up a contract on the branch chain, imagining it does not exist on the root chain
        // and we monitor whether it receives funds on the root chain 
        // (that is, from the root L0 endpoint)
        ReceiveLogger contractUser = new ReceiveLogger();
        vm.deal(address(contractUser), price1 + price2);

        vm.startPrank(address(contractUser));

        // configuration is only partial
        // so execution fails remotely
        bba.callOutAndBridge{value: price1}(
            payable(address(contractUser)), // _refundee
            "",                             // _params,
            DepositInput(
                address(0),
                address(0),
                0,
                0
            ),                   // _dParams,
            GasParams(50_000, 0) // _gParams - we don't really need anything to happen remotely
        );

        
        uint256 extra = 10e6;
        uint256 price3 = 13201155000000000;

        // then we set the deposit up for redemption
        bba.retrieveDeposit{value: price2}(
            bba.depositNonce() - 1, 
            GasParams(300_000, price3 + extra)
        );

        // ah ha! our contract received a refund from the root endpoint!
        // in a truly multi-chain operation, these funds would be lost
        assertTrue(contractUser.didReceiveFrom(address(rootLzEndpoint)));
        assertEq(extra, address(contractUser).balance);
    }
}

contract ReceiveLogger {
    mapping(address sender => bool didReceive) public didReceiveFrom;

    receive() payable external {
        didReceiveFrom[msg.sender] = true;
    }
}

Tools Used

Code review, Foundry

Recommended Mitigation Steps

Possible options would be:

• on Arbitrum, to refund VirtualAccounts instead of plain addresses, so users can always access these funds, in the worst case via remote calls
• adding a "remote refundee parameter" to the retrieveDeposit call

or, more accurately:

• extend GasParams to include the address for native tokens refundees, so the refundee can be different at every hop
• adding a proper GasParams argument to the fallback calls too

Assessed type

Token-Transfer

[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 not a duplicate

[c4-judge]

alcueca marked the issue as duplicate of #679

[c4-judge]

alcueca marked the issue as satisfactory

[c4-judge]

alcueca marked the issue as selected for report

[alcueca]

From the sponsor:

Contracts should not use Virtual Accounts and deploying a specific router for contract usage is most likely the safest option.

[c4-judge]

alcueca changed the severity to 2 (Med Risk)

[alcueca]

This issue only talks about native tokens, and not deposits.

[c4-judge]

alcueca marked the issue as not selected for report

[c4-judge]

alcueca marked issue #872 as primary and marked this issue as a duplicate of 872

[JeffCX]

Issue #872 highlight

the same address for multisig in different network can belong to different owner

but don't see this report make such point so don't see this is a duplicate of #872, I wonder if this issue can be duplicate with #679 together

[c4-judge]

alcueca marked the issue as not a duplicate

[c4-judge]

alcueca marked the issue as duplicate of #679

[alcueca]

While not a 100% duplicate of #679, because of pointing at multiple lines with similar errors, the underlying bug is the same, including the same line as #679. The impact described is lower, and I see more appropriate to mark #679 as best.

[c4-judge]

alcueca changed the severity to 3 (High Risk)