c4-submissions commented 1 year ago

Lines of code

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

Vulnerability details

Impact

Should not hardcode chain id because chain id can change on layerzero side

Proof of Concept

One of the integration guide from layerzero is

https://layerzero.gitbook.io/docs/evm-guides/layerzero-integration-checklist

Do not hardcode LayerZero chain Ids. Use admin restricted setters instead.

but in the current implementation, the code assume chain id can never change

if we take a look at the code, the chain id is set to immutable in BranchBridgeAgent

    /// @notice Chain Id for Root Chain where liquidity is virtualized(e.g. 4).
    uint16 public immutable rootChainId;

    /// @notice Chain Id for Local Chain.
    uint16 public immutable localChainId;

later we _performCall is triggered, the chain id used to send message using layerzero endpoint

this is a issue because if layerzero change chain id, all mesage sending will either be sent to wrong chain or revert

and layerzero does change chain id once last year

confimed by layerzero team

relevant discord discussion is here on layerzero server

https://discord.com/channels/881985666265780274/881992936609435688/1157059327841009664

layerzero team made an annoncment on discord last year as well

https://discord.com/channels/881985666265780274/881992936609435688/1025202074805358622

I put the screen shot here

https://drive.google.com/drive/folders/1ydmqFef48EO5GYP9XTmap1eirJr4IJH_?usp=drive_link

to access the discord link above, one needs to join layerzero discord server first

anyway I just want to prove that chain id can be potentially changed and it was already changed so hard code chain id can cause issue

Tools Used

Manual Review

Recommended Mitigation Steps

. Use admin restricted setters to allow admin update layerzero chain id

Assessed type

DoS

c4-pre-sort commented 1 year ago

0xA5DF marked the issue as primary issue

c4-pre-sort commented 1 year ago

0xA5DF marked the issue as sufficient quality report

0xLightt commented 1 year ago

Duplicate of #412

c4-judge commented 11 months ago

alcueca changed the severity to QA (Quality Assurance)

alcueca commented 11 months ago

Valid QA, despite sponsor dispute on #412