QA Report

[c4-submissions]

See the markdown file with the details of this report here.

[bytes032]

2 l 1 r 14 nc

Low 01 - In Executor.sol, commitBatches() has no check on the max size of dynamic array of _newBatchesData() passed, which may cause out-of-gas error. (Note: Instance not found in automated findings) l

Low 02 - In Executor.sol - proveBatches(), if validator pass _proof, the same proof verification will perform twice which is unecessary nc

Low 03 - In Executor.sol, there are incorrect comments in proveBatches() nc

Low 04 - In Mailbox.sol, there are incorrect comments in _parseL2WithdrawalMessage() nc

Low 05 - In TransactionValidator.sol, unnecessary math operation in getOverheadForTransaction() nc

Low 06 - In TransactionValidator.sol, there is an incorrect range check for the system contract address. nc

Low 07 - Diamond.sol is unable to completely remove an old facet address from accounting, the old facet address can still be queried from Getters.sol nc

Low 08 - In DiamondInit.sol, remove unnecessary code irrelevant to the production nc

Low 09 - DiamondProxy's fallback() cannot handle msg.data.length==0 case against the design intention and will revert nc

Low 10 - Unused Interface Functions (Note: Instances listed here not found in automated reports) nc

Low 11 - Missing getter functions for key internal AppStorage state variables. Some important state accounting cannot be queried directly. nc

Low 12 - Merkle.sol library cannot handle an edge case of a large Merkle tree and will revert. r

Low 13 - DefaultUpgrade.sol upgrade() has no access control, which requires an inefficient and potentially risky flow of system upgrade x

Low 14 - Incorrect code comments in L1Messenger.sol nc

Low 15 - Invalid validation - oversized number of logs can be passed to publishPubdataAndClearState() dup of https://github.com/code-423n4/2023-10-zksync-findings/issues/853

NC 01 - Consider adding comments for custom formula implementation, especially if it is optimized or modified from the original formula from the doc. nc

NC 02 - Some long revert strings are not optimized and abbreviated as the rest of the code base (Note: Not submitted in automated findings) nc

NC 03 - Weth deposit might still be submitted through L1ERC20Bridge.sol due to no explicit reverts, resulting in user waste of gas on L1. nc

[lsaudit]

Hey, I've took a quick look at this report and a lot of issues should be in Gas, imho,

Low 01

This is self-DoS. commitBatches() is external, so whenever I call this function, I need to provide _newBatchesData which will overflow. AFAIK, self-DoSed are not accepted as valid issue on C4.

Low 02

This should be in Gas report, not in QA. It does not pose any security threat.

Low 05

This should be in Gas report, not in QA. It does not pose any security threat. E.g., this was in Gas report https://github.com/code-423n4/2023-10-zksync-findings/issues/996 [G-15]

Low 8

Again, this is more Gas issue than QA.

Low 12

Is it possible that so huge merkle tree be passed to calculateRoot() function? It's an edge-case (extremely non-realistic one), imho it sohuld be NC instead of R.

NC 01

Isn't this formula too obvious to be commented? We cannot comment every single line of code. There's no advanced math in here to be commented, imho this is not valid.

NC 02

This is already reported in bot as: [G‑51] - require()/revert() strings longer than 32 bytes cost extra gas. Again, this is more Gas issue than QA. Moreover, it's already reported in bot - so it's OOS.

[GalloDaSballo]

Mostly looks fine, I'm keeping as is

[c4-judge]

GalloDaSballo marked the issue as grade-b