Open c4-submissions opened 11 months ago
0xA5DF marked the issue as duplicate of #412
0xA5DF marked the issue as sufficient quality report
alcueca changed the severity to QA (Quality Assurance)
Hello, Alcueca.
The first point and the first mitigation step are similar to issue 399
Regarding issue 399:
Root causes: Issue A. User can specify any gas params against best-practice guidelines Issue B. The protocol implements a non-blocking implementation pattern for lzReceive and doesn't handle the "in-blocking scenario" Issue C. The protocol doesn't implement the ILayerZeroUserApplicationConfig functions as recommended
I decided to write a comprehensive submission because the issue with the gas value was described in the LZ Best Practices (399 points to the same page) and in similar contests (I added links to them in the report). Also, similar issues, such as hardcoded values of chainId, are present in the code from the contest.
Since these are well-known and "classic" issues, I decided not to flood the contest with separate reports but to consolidate them into one.
I've just noticed your old comment about bulking issues: https://github.com/code-423n4/org/issues/8#issuecomment-1064370194 So here I had a similar idea to bulk several issues in one because all these issues are described on the same page with best practices.
Your report reads like the other QA reports of lack of adherence to LZ's best practices. #399 points out how a DoS can be constructed thanks to one of those failures.
Recommended Mitigation Steps
- Verify that msg.value is greater than the return value of the getFeeEstimate() function. Additionally, consider another discovery where sponsors have opted to introduce a mechanism for computing gas expenses on a per-target chain basis. This approach is intended to prevent transactions from becoming stuck.
My first recommendation reads like a duplicate of the issue https://github.com/code-423n4/2023-09-maia-findings/issues/785 which is a duplicate of the main issue, but without a big story that lays behind this problem.
Also all of these LZ issues are well explained and known in the best-practices that's why I decided not to spam the contest with big separate reports.
If I added some additional problems to this msg.value checking doesn't mean that it's invalid or QA.
Lines of code
https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BranchBridgeAgent.sol#L770-L777 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BranchBridgeAgent.sol#L161-L173 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BranchBridgeAgent.sol#L578-L584
Vulnerability details
Impact
Interactions with Layer Zero contracts require specific considerations to prevent message stalls during transmission and to address potential issues in the protocol's operation in the future.
Proof of Concept
Here is a list of Best Practices for Layer Zero:
https://layerzero.gitbook.io/docs/evm-guides/best-practice
https://layerzero.gitbook.io/docs/evm-guides/layerzero-integration-checklist
Failure to adhere to these requirements may lead to disruptions in the protocol's functionality.
https://github.com/LayerZero-Labs/LayerZero/blob/48c21c3921931798184367fc02d3a8132b041942/contracts/UltraLightNodeV2.sol#L150
Similar medium case:
https://solodit.xyz/issues/m-07-redemptionsender-should-estimate-fees-to-prevent-failed-transactions-code4rena-velodrome-finance-velodrome-finance-git
The lzReceive function does not store information about failed messages.
https://layerzero.gitbook.io/docs/evm-guides/error-messages/fix-a-storedpayload
In the current implementation, the user can't retry to execute his message if it gets stuck for some reason like a lack of gas. Only function retryDeposit from BranchBridgeAgent.sol has similar functionality but messages without tokens will be lost.
Link to a similar case where the user can't retry the execution of his message in case of miscalculation of gas values:
https://solodit.xyz/issues/h-03-layerzeromodule-miscalculates-gas-risking-loss-of-assets-code4rena-holograph-holograph-contest-git_
RootBridgeAgent.sol:
https://github.com/LayerZero-Labs/LayerZero/blob/48c21c3921931798184367fc02d3a8132b041942/contracts/RelayerV2.sol#L139-L143
The lack of this check was already discussed here:
https://solodit.xyz/issues/h-2-malicious-user-can-use-an-excessively-large-_toaddress-in-oftcoresendfrom-to-break-layerzero-communication-sherlock-uxd-uxd-protocol-git
Tools Used
vs
Recommended Mitigation Steps
https://solodit.xyz/issues/h-03-layerzeromodule-miscalculates-gas-risking-loss-of-assets-code4rena-holograph-holograph-contest-git_
https://github.com/LayerZero-Labs/solidity-examples/blob/main/contracts/lzApp/NonblockingLzApp.sol
Consider making hardcoded values that participate in message exchanges with Layer Zero configurable.
Add a require statement to check the size of the payload before processing.
Assessed type
Context