QA Report

[c4-submissions]

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

[bytes032]

11 l 6 r 9 nc

| QA‑01 | extendedAccountVersion() should not return AccountAbstractionVersion.Version1 for addresses in the kernel space | r

| QA‑02 | storeBatchHash should in no instance return reverted batches | l

| QA‑03 | The additional check for mainnet in proveBatches() should consider that after a hard fork assert block.chaindId != 1 would no longer be a valid protection. | l

| QA‑04 | executeInstant() would fail for operations scheduled in the future due to _checkPredecessorDone() | l

| QA‑05 | Setting a pending admin in AdminFacets actually emits an event for setting a governor instead which leads to issues for any one listening off-chain | l

| QA‑06 | forceDeployOnAddressshould be modified to ensure that the newAddress always receives the funds allocated to it regardles of callConstructor's value | l

| QA‑07 | Allowlist is not being implemented as is supposed to after the Alpha period. | r

| QA‑08 | Forced deployments should be restricted | l

| QA‑09 | Processing of L2 to L1 withdrawal messages with additional Data should be efficiently done and correctly documented | nc

| QA‑10 | Make the fallback a payable prefixed function to ensure that calls don't fail from the bootloader | r

| QA‑11 | Add transaction.signature to _encodeHashEIP712Transaction to ensure compliance with EIP712 | l

| QA‑12 | maxFeePerGas been hardcoded to 0 which leads to not providing any incentives for validators | l

| QA‑13 | Interface specifications mismatch with actual functions | r

| QA‑14 | Multiple fixes for typos/Docs & wrong naming | nc

| QA‑15 | Deviation from Solidity best styling practices in scoped contracts | nc

| QA‑16 | L2EthToken::transferFromTo should be stopped from emmiting dummy events and also protect users from wasting unnecessary gas | nc

| QA‑17 | System hang if validators neglect cross-chain transactions | dup of https://github.com/code-423n4/2023-10-zksync-findings/issues/923

| QA‑18 | Multiple issue attached with _Isvalidsignature does not follow the specification in the EIP | l

| QA‑19 | Setters should always have equality checkers | bot

| QA‑20 | BOOTLOADER_MEMORY_FOR_TXS allocation reduction is risky | l

| QA‑21 | OperationState.Waiting should be considered the case even when timestamp == block.timestamp | l

| QA‑22 | Compatibility issues could arise for pure ERC20 | r

| QA‑23 | AllowList could use a blacklisting mechanism | nc

| QA‑24 | The interface ILegacyGetters only provides availability of deprecated functions instead of the currently accepted ones | r

| QA‑25 | If Changes are made to L2ContractHelper/Config.sol then redeployments of facets should be hastily done | nc

| QA‑26 | Presence of non-production-ready code | nc

| QA‑27 | Add the verifyingContract to the TYPEHASH for EIP-712 | nc

| QA‑28 | Redundant v == 27 & v == 28 check | nc

[lsaudit]

Hey, I've read some issues and some of them are, imho, invalid

QA‑02 - this is exactly the same issue as QA-13 in my report - https://github.com/code-423n4/2023-10-zksync-findings/issues/544 which was evaluated as R. It should either be evaluated as R here, or increased to L in my report (I stick with increasing it to L in my report :P).

QA‑03 - this had been disputed by the sponsor, should not be considered as Low, please check my similar issue which was rejected: https://github.com/code-423n4/2023-10-zksync-findings/issues/543

QA-05 - aren't incorrect event emissions considered as r/nc? Project does not describe any additional off-chain infrastructure, which is related to this event, so the whole description is much more an assumption than a real threat? Thus it should be classified as Refactoring instead of Low.

QA-06, QA-08 - shoudn't these two be merged into one issue?

QA-10 - this affects a mock contract, shoudn't it be evaluated as 'nc'?

QA-15 - already reported in bot as [N‑85] Style guide: Lines are too long

QA-16 - this is invalid, according to EIP, ERC-20: "Transfers of 0 values MUST be treated as normal transfers and fire the Transfer event.". Your recommendation violates EIP-20!

QA-21 - comments state: Note that a "pending" operation may also be "ready"., thus this is not valid

QA‑23 - the current implementation uses whitelist approach. Whitelist approach is much more restrictive then the blacklist approach, I don't see any reason how suggesting a change from more restrictive to less restrictive access control, may improve the security of the contract, imho it's invalid

QA‑24 - this is incorrect, as name Legacy suggests, this is the interface for all deprecated functions, ILegacyGetters.sol contains all deprecated functions. The non-deprecated ones are in IGetters.sol. This should be consider as an invalid issue.

QA-28 - this should be in a gas report, instead of QA Report

[Bauchibred]

Hi @GalloDaSballo, thanks for judging, I'd like to draw your attention to a few findings that might have been missed by the lookouts from this QA report and would appreciate if you could give this another look.

QA-02 should be considered a duplicate of #739, now depending on the outcome of this comment made by @HE1M. which should be validated since a similar issue was validated for med severity in this same contest.

QA-04 is a duplicate of #439, since you've relayed to the warden that you'd double check this issue via this comment, I'd appreciate if you could also take this report into consideration before making a final decision, thanks.

QA-07 seems to be worthy of a re-evaluation in my opinion, being that sponsors have confirmed previously that this setting is only allowed in the alpha release period and that this would change in the future.

QA-18 is a duplicate to #504, under this report I explained to potential issues of this implementation and even clarifying the second case to be pure QA but otherwise should be said for the first case, where I stated that the second section of impact could be considered as a refactoring case, but the first part and a mixture of both could stand out as a medium pending final decision from judge. Now following the disucussion that's been had on #504 and this comment by @VagnerAndrei26 and the fact that this bug case is about in-compliance to an EIP it should be validated as a medium severity finding following Code4rena's rules.

Apologies if the comments on this report is too much, I guess this was cause unlike other contests Code4rena tried something new on this one that led to two lookouts judging QA reports which I only found out during the PJQA period for this contest, this essentially means that, it'd be right to guess, you the judge didn't look at these files, i.e the QA reports due to that you might have missed some of the borderline issues I submitted and requested to look if possible to make an upgrade so as not to spam the judging repo.(If I knew this would be done, I'd have just submitted them as H/M issues pending your validations on them), I'd appreciate if you could give a quick glance at them to see if any is upgradeable, but would really appreciate if you could actually give the quick glance to all issues in the report here as something might be worth upgrading that I've not specified in the table and also do due diligence to what you think these comments from the warden on the issues should be finalised as, would also be important to note that most of the issues discussed above to be duplicate of others are from these borderline issues, which still include other interesting ones as QA-10 & QA-11, do help with a quick check.

Thank you for your time.

[c4-judge]

GalloDaSballo marked the issue as grade-a