c4-submissions commented 1 year ago

Lines of code

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

Vulnerability details

Impact

The source address of LayerZero messages is validated on a wrong part of the calldata, which will cause all cross-chain-messages to fail on a live deployment.

Proof of Concept

The receivers of cross-chain-messages BranchBridgeAgent and RootBridgeAgent both perform validation of the sender in their respective requiresEndpoint modifiers:

// RootBridgeAgent
if (getBranchBridgeAgent[_srcChain] != address(uint160(bytes20(_srcAddress[PARAMS_ADDRESS_SIZE:])))) {
    revert LayerZeroUnauthorizedCaller();
}

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

Both try to extract the sender address from the last 20 bytes of the _srcAddress (which consists of 2 EVM addresses, so 40 bytes in total). The issue is that the address of the sender on the source chain is contained in the first 20 bytes not the last.

During development the assumption was made that whatever path gets send, will also be the path when receiving. Looking at BranchBridgeAgent._performCall, the following parameter is passed:

ILayerZeroEndpoint(lzEndpointAddress).send{value: msg.value}(
    ...
    rootBridgeAgentPath,
    ...
);

Here, rootBridgeAgentPath is the 40 bytes combination of remoteAddress and localAddress mentioned above:

rootBridgeAgentPath = abi.encodePacked(_rootBridgeAgentAddress, address(this));

It is easy to think that since address(this) occupies the last 20 bytes here, that the path given to the receiver will also be in the same order.

However, LayerZero actually swaps the addresses as the path is always semantically meant to be comprised of remoteAddress first and then localAddress, relative to the current chain (see docs: Sending on L0 and Trusted remotes on L0 + Github example)

Proving on a real on-chain transaction that this is what happens:

cross-chain tx on layerzeroscan

This link shows a cross-chain message from Arbitrum to Avalanche. Looking at the decoded transaction traces (using Tenderly, links below), we can see that the path used in lzSend is 0x9d1b1669c73b033dfe47ae5a0164ab96df25b944352d8275aae3e0c2404d9f68f6cee084b5beb3dd while the path received in lzReceive is 0x352d8275aae3e0c2404d9f68f6cee084b5beb3dd9d1b1669c73b033dfe47ae5a0164ab96df25b944 (first and last 20 bytes swapped)

lzSend on Arbitrum

lzReceive on Avalanche

The reason why this has not been caught in the unit tests is because unit tests can't integrate with LayerZero, so the encoding has been done manually, but in the wrong order, as can be seen in CoreRootBridgeAgentTest.t.sol:

function encodeSystemCall(
    address payable _fromBridgeAgent,
    address payable _toBridgeAgent,
    ...
) private {
    ...
    // since we are on the target chain already, _from is the remote and should be packed first
    RootBridgeAgent(_toBridgeAgent).lzReceive{gas: _gasParams.gasLimit}(
            _srcChainIdId, abi.encodePacked(_toBridgeAgent, _fromBridgeAgent), 1, inputCalldata
        );
}

To get an executable POC, swap _toBridgeAgent and _fromBridgeAgent when encoding and run any test that uses encodeSystemCall. The tests will fail, works the same with the other encode* functions. (Executable Github Gist for reference)

Tools Used

Manual Review, Tenderly, Layerzeroscan

Recommended Mitigation Steps

In the requiresEndpoint modifiers, get the sender from the first 20 bytes, not the last:

// RootBridgeAgent
if (getBranchBridgeAgent[_srcChain] != address(uint160(bytes20(_srcAddress[0:PARAMS_ADDRESS_SIZE])))) {
    revert LayerZeroUnauthorizedCaller();
}

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

Also adjust the encode* functions in the tests to reflect that.

Assessed type

Other

c4-pre-sort commented 1 year ago

0xA5DF marked the issue as duplicate of #439

c4-pre-sort commented 1 year ago

0xA5DF marked the issue as sufficient quality report

c4-judge commented 1 year ago

alcueca changed the severity to 2 (Med Risk)