Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BranchBridgeAgent is missing source chain checking. #855

Open
c4-submissions opened this issue Oct 6, 2023 · 7 comments
Open

BranchBridgeAgent is missing source chain checking. #855

c4-submissions opened this issue Oct 6, 2023 · 7 comments
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue grade-a primary issue Highest quality submission among a set of duplicates Q-08 QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") sufficient quality report This report is of sufficient quality

Comments

@c4-submissions
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BranchBridgeAgent.sol#L578-L584

Vulnerability details

Impact

According to the LayerZero docs, UAs should check that messages from trusted chain and known address.
https://layerzero.gitbook.io/docs/evm-guides/master/receive-messages

However, BranchBridgeAgent only checks the address and does not verify the source chain. The source chain here should be the root chain (Arbitrum). If there exists a contract on another network with the same address as the RootBridgeAgent on the Arbitrum network, BranchBridgeAgent may consider messages from this contract as legitimate and process them.
https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BranchBridgeAgent.sol#L578-L584

    function lzReceive(uint16, bytes calldata _srcAddress, uint64, bytes calldata _payload) public override {
        address(this).excessivelySafeCall(
            gasleft(),
            150,
            abi.encodeWithSelector(this.lzReceiveNonBlocking.selector, msg.sender, _srcAddress, _payload)
        );
    }

Deploying a contract on another network with the same address as the RootBridgeAgent on the Arbitrum network is a highly challenging task. However, if a hard fork occurs on Arbitrum, and LayerZero decides to support both networks, then there would be a situation where two contracts have identical addresses.

Adding a source chain check can prevent such situations from occurring.

Proof of Concept

In the following test code, we manually modified the ID of the Arbitrum network to simulate a hard fork on the Arbitrum network. Note that this is not a complete testing scripts.

    function testSrcChainId() public {
        switchToLzChain(rootChainId);
        vm.chainId(31337);
        vm.deal(address(this), 200 ether);
        coreRootRouter.removeBranchBridgeAgent(address(avaxMulticallBridgeAgent), address(this), avaxChainId,  GasParams(800_000, 0.03 ether));
        switchToLzChain(avaxChainId);
    }

Tools Used

Foundry

Recommended Mitigation Steps

    function lzReceive(uint16 _srcChainId, bytes calldata _srcAddress, uint64, bytes calldata _payload) public override {
>>>        require(_srcChainId == rootChainId, "source chain error.");
        address(this).excessivelySafeCall(
            gasleft(),
            150,
            abi.encodeWithSelector(this.lzReceiveNonBlocking.selector, msg.sender, _srcAddress, _payload)
        );
    }

Assessed type

Invalid Validation

@c4-submissions c4-submissions added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Oct 6, 2023
c4-submissions added a commit that referenced this issue Oct 6, 2023
@c4-pre-sort
Copy link

0xA5DF marked the issue as sufficient quality report

@c4-pre-sort c4-pre-sort added the sufficient quality report This report is of sufficient quality label Oct 12, 2023
@c4-pre-sort
Copy link

0xA5DF marked the issue as primary issue

@c4-pre-sort c4-pre-sort added the primary issue Highest quality submission among a set of duplicates label Oct 12, 2023
@0xA5DF
Copy link

0xA5DF commented Oct 12, 2023

I doubt this qualifies as med since the likelihood of this being exploited is extremely low
But leaving open for sponsor to comment

@c4-sponsor
Copy link

0xLightt (sponsor) confirmed

@alcueca
Copy link

alcueca commented Oct 25, 2023

The preconditions for this vulnerability are too restrictive to warrant a Medium severity.

@c4-judge
Copy link
Contributor

alcueca changed the severity to QA (Quality Assurance)

@c4-judge c4-judge added downgraded by judge Judge downgraded the risk level of this issue QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax and removed 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value labels Oct 25, 2023
@c4-judge
Copy link
Contributor

alcueca marked the issue as grade-a

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue grade-a primary issue Highest quality submission among a set of duplicates Q-08 QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") sufficient quality report This report is of sufficient quality
Projects
None yet
Development

No branches or pull requests

7 participants