Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

QA Report #914

Open
c4-submissions opened this issue Oct 23, 2023 · 5 comments
Open

QA Report #914

c4-submissions opened this issue Oct 23, 2023 · 5 comments
Labels
bug Something isn't working grade-b Q-06 QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax

Comments

@c4-submissions
Copy link
Contributor

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

@c4-submissions c4-submissions added bug Something isn't working QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax labels Oct 23, 2023
jacobheun pushed a commit that referenced this issue Oct 24, 2023
jacobheun pushed a commit that referenced this issue Oct 24, 2023
@bytes032
Copy link

bytes032 commented Oct 30, 2023

(l-1-extra-notifyexecutionresult-gas-cost-is-charged-to-users-on-l1-l2-transactions-processing) Extra notifyExecutionResult gas cost is charged to users on L1->L2 transactions processing
L
(l-2-if-dummy-verifier-true-block-in-executor-introduces-risk-of-halting-batch-proving-in-production) if DUMMY_VERIFIER == true block in Executor introduces risk of halting batch proving in production
NC
(l-3-refund-recipient-addresses-not-aliased-if-transactions-are-requested-on-constructor) Refund recipient addresses not aliased if transactions are requested on constructor
NC
(l-4-double-alias-conversion-in-l1-bridge-deposits) Double alias conversion in L1 bridge deposits
L
(l-5-wrong-validation-on-max-number-of-numberofl2tol1logs) Wrong validation on max number of numberOfL2ToL1Logs
L
(l-6-isfacetfreezable-returns-false-for-non-existing-facets-instead-of-reverting) isFacetFreezable() returns false for non-existing facets instead of reverting
NC
(l-7-sending-many-logs-via-a-single-l1-l2-transactions-can-halt-the-publishing-of-pubdata) Sending many logs via a single L1->L2 transactions can halt the publishing of pubdata
L
(l-8-generating-a-large-pubdata-via-a-single-l1-l2-transaction-can-halt-the-publishing-to-l1-operation) Generating a large pubdata via a single L1->L2 transaction can halt the publishing to L1 operation
L
(l-9-upgrade-openzeppelin-dependencies) Upgrade OpenZeppelin dependencies
NC
(l-10-missing-check-for-address0-for-the-allowlist-in-diamondinitinitialize) Missing check for address(0) for the allowList in DiamondInit::initialize()
NC
(l-11-missing-getters-in-getters-sol) Missing getters in Getters.sol
NC
(l-12-prevent-weth-deposits-in-l1erc20bridge) Prevent WETH deposits in L1ERC20Bridge
L
(l-13-facet-state-is-not-entirely-cleared-when-last-selector-is-removed) Facet state is not entirely cleared when last selector is removed
L
(l-14-shadow-operations-will-ultimately-need-to-be-revealed-on-chain) Shadow operations will ultimately need to be revealed on-chain
L
(l-15-potential-gas-bomb-in-governance-execution) Potential gas bomb in Governance execution
-3
(l-16-verifier-params-can-be-accidentally-skipped-during-upgrades) Verifier params can be accidentally skipped during upgrades
NC
(l-17-governance-contract-does-not-implement-erc721-or-erc1155-receivers) Governance contract does not implement ERC721 or ERC1155 receivers
-3
(l-18-potential-missing-validation-in-systemcontextpublishtimestampdatatol1) Potential missing validation in SystemContext::publishTimestampDataToL1()
NC
(nc-1-compressor-uses-hardcoded-values-instead-of-designated-constants) Compressor uses hardcoded values instead of designated constants
NC
(nc-2-wrong-documentation-in-parsel2withdrawalmessage) Wrong documentation in _parseL2WithdrawalMessage
NC
(nc-3-inconsistencies-in-state-diffs-header-docs) Inconsistencies in "State Diffs Header" Docs
NC
(nc-4-rename-block-batch-as-part-of-the-codebase-refactor) Rename block -> batch as part of the codebase refactor
NC
(nc-5-update-forcedeployonaddresses-natspec-to-reflect-its-current-permissions) Update forceDeployOnAddresses() NatSpec to reflect its current permissions
NC

@miladpiri
Copy link

Regarding L-7 and L-8
https://github.com/code-423n4/2023-10-zksync-findings/blob/main/data/zero-idea-Q.md#l-7-sending-many-logs-via-a-si[…]-halt-the-publishing-of-pubdata

It is not possible (implicitly). For L1->L2 transactions, the limit is 72kk, while gas per pubdata is 800. It means that 90000 bytes can be sent.
With each L2->L1 message the users pay for 88 bytes of pubdata. 88 * 2048 = 180224

So yeah, the bound is implicit, but it is in the place.

@c4-judge
Copy link
Contributor

GalloDaSballo marked the issue as grade-b

@c4-judge c4-judge added grade-c unsatisfactory does not satisfy C4 submission criteria; not eligible for awards and removed grade-b labels Dec 10, 2023
@c4-judge
Copy link
Contributor

GalloDaSballo marked the issue as grade-c

@c4-judge c4-judge reopened this Dec 13, 2023
@c4-judge
Copy link
Contributor

GalloDaSballo marked the issue as grade-b

@c4-judge c4-judge added grade-b and removed grade-c unsatisfactory does not satisfy C4 submission criteria; not eligible for awards labels Dec 13, 2023
@C4-Staff C4-Staff added the Q-06 label Jan 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working grade-b Q-06 QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax
Projects
None yet
Development

No branches or pull requests

5 participants