-
Notifications
You must be signed in to change notification settings - Fork 387
feat: bundles #1491
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
base: draft-v30
Are you sure you want to change the base?
feat: bundles #1491
Conversation
l1-contracts/test/foundry/l1/integration/l2-tests-abstract/L2Erc20TestAbstract.t.sol
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some initial comments including 2 Critical access control vulnerabilities (if confirmed). #1552 is also a part of this. I'm going to continue tomorrow.
todo: add library to initiate token bridging, maybe @0xValera |
l1-contracts/contracts/state-transition/chain-deps/facets/Mailbox.sol
Outdated
Show resolved
Hide resolved
l1-contracts/contracts/state-transition/chain-deps/facets/Mailbox.sol
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leaving high-level review related to AssetTracker
and #1544
a reminder is that EN will have to check this value for correctness (not a blocker for audit, but a thing to remember) |
l1-contracts/contracts/state-transition/chain-deps/facets/Executor.sol
Outdated
Show resolved
Hide resolved
l1-contracts/contracts/state-transition/chain-deps/facets/Executor.sol
Outdated
Show resolved
Hide resolved
l1-contracts/contracts/state-transition/chain-deps/facets/ZKChainBase.sol
Show resolved
Hide resolved
l1-contracts/contracts/state-transition/chain-deps/facets/Mailbox.sol
Outdated
Show resolved
Hide resolved
l1-contracts/contracts/state-transition/chain-deps/facets/Mailbox.sol
Outdated
Show resolved
Hide resolved
l1-contracts/contracts/state-transition/chain-deps/facets/Mailbox.sol
Outdated
Show resolved
Hide resolved
l1-contracts/contracts/state-transition/chain-deps/facets/Mailbox.sol
Outdated
Show resolved
Hide resolved
uint256 msgCount = 0; | ||
uint256 logsLength = _processLogsInputs.logs.length; | ||
bytes32 baseTokenAssetId = _bridgehub().baseTokenAssetId(_processLogsInputs.chainId); | ||
for (uint256 logCount = 0; logCount < logsLength; ++logCount) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A malicious user can spam with L2 to L1 user logs thus preventing the chain from finalizing on the GW because this loop runs out of gas (due to the number of logs). A way to mitigate this is to limit the number of user logs per batch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Conceptually, each chain batch can fit into a gw batch, since the batches are the same size. We need to make sure there is no txs gas limit, that is true.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cost profile at the moment is a bit different. Chains' logs typically dont need to get published in full to L1, but they do cause storage slot changes inside GWAT that do need to get published on L1...
…kl-medium-interop Additional comments for kl medium interop
chore: avoid avoidViaIrStruct
…contracts into kl/06-10-25/1
Kl/06-10-25/1
delete received interop messages
add back assetMigratin check
Co-authored-by: 0xValera <[email protected]> Co-authored-by: Stanislav Breadless <[email protected]>
What ❔
Note: from V30 onwards, the Anvil debug options are integrated in draft branches, they should be removed before going to audit.
Anvil branch: #1625
Why ❔
Checklist