code-423n4 / 2023-09-maia-findings

24 stars 17 forks source link

Do not hardcode the chain ID #412

Open c4-submissions opened 10 months ago

c4-submissions commented 10 months ago

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BranchBridgeAgent.sol#L52-L56 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/CoreRootRouter.sol#L46-L47 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/factories/BranchBridgeAgentFactory.sol#L20-L24 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/factories/RootBridgeAgentFactory.sol#L12-L13

Vulnerability details

Summary

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

Impact

Hardcoding the chainId value is not a good practice as hard forks might change the chainId for a network

Tools Used

Manual

Recommended Mitigation Steps

Use admin-restricted setters instead.

Assessed type

DoS

c4-pre-sort commented 10 months ago

0xA5DF marked the issue as primary issue

c4-pre-sort commented 10 months ago

0xA5DF marked the issue as sufficient quality report

0xLightt commented 10 months ago

While it is suggested in layerzero's best practices, layerzero won't update a destination's chain ID. This only happened in the past during the first 24h of a new chain being deployed. If they were to change this at any time it would break or put in risk most if not all protocols that use them and would make building decentralized protocols impossible. The description is not accurate, a layerzero chain ID has noting to do with a hard fork, it is handled by the layerzero's contract and it is different from the original network's chain ID. In addition, the address that would have the permission to set the chain ID of destination root/branch chains would essentially have custody of all the funds in the protocol.

c4-sponsor commented 10 months ago

0xLightt (sponsor) disputed

c4-judge commented 10 months ago

alcueca changed the severity to QA (Quality Assurance)

c4-judge commented 10 months ago

alcueca marked the issue as grade-a