Merge Protocol Defense Tasks into Dev #518

koloz193 · 2024-06-09T23:29:05Z

What ❔

The purpose of these tasks is to make the protocol more resilient while increasing the quality of life when it comes to development, interop, and having a correct running system.

Why ❔

Checklist

PR title corresponds to the body of PR (we generate changelog entries from PRs).
Tests for the changes have been added / updated.
Documentation comments have been added / updated.

…LOWED_FAIR_L2_GAS_PRICE for base tokens (#513)

…mented (#505)

l1-contracts/contracts/state-transition/chain-deps/facets/Executor.sol

.codespell/wordlist.txt

l1-contracts/contracts/bridge/L1SharedBridge.sol

l1-contracts/contracts/bridge/interfaces/IL1ERC20Bridge.sol

l1-contracts/contracts/common/L1ContractErrors.sol

l1-contracts/contracts/state-transition/libraries/Diamond.sol

l1-contracts/contracts/upgrades/BaseZkSyncUpgrade.sol

l2-contracts/contracts/bridge/L2StandardERC20.sol

system-contracts/bootloader/bootloader.yul

…r Import (#572)

l1-contracts/contracts/state-transition/chain-deps/facets/Admin.sol

vladbochok · 2024-08-14T09:49:22Z

l1-contracts/contracts/state-transition/chain-deps/facets/Executor.sol

@@ -147,7 +169,7 @@ contract ExecutorFacet is ZkSyncHyperchainBase, IExecutor {

        // Used as bitmap to set/check log processing happens exactly once.
        // See SystemLogKey enum in Constants.sol for ordering.
-        uint256 processedLogs;
+        uint256 processedLogs = 0;


Why change this line?

for all these OZ suggested adding in the default values. i dont have a strong opinion on keeping or removing

l1-contracts/contracts/state-transition/chain-interfaces/IMailbox.sol

l1-contracts/test/foundry/integration/deploy-scripts/script-out/output-deploy-l1.toml

vladbochok · 2024-08-14T10:10:03Z

system-contracts/contracts/L1Messenger.sol

@@ -209,7 +209,7 @@ contract L1Messenger is IL1Messenger, ISystemContract {
        calldataPtr += 4;

        bytes32[] memory l2ToL1LogsTreeArray = new bytes32[](L2_TO_L1_LOGS_MERKLE_TREE_LEAVES);
-        bytes32 reconstructedChainedLogsHash;
+        bytes32 reconstructedChainedLogsHash = bytes32(0);


for all these OZ suggested adding in the default values. i dont have a strong opinion on keeping or removing

vladbochok · 2024-08-14T10:10:08Z

system-contracts/contracts/L1Messenger.sol

@@ -238,7 +238,7 @@ contract L1Messenger is IL1Messenger, ISystemContract {
        /// Check messages
        uint32 numberOfMessages = uint32(bytes4(_totalL2ToL1PubdataAndStateDiffs[calldataPtr:calldataPtr + 4]));
        calldataPtr += 4;
-        bytes32 reconstructedChainedMessagesHash;
+        bytes32 reconstructedChainedMessagesHash = bytes32(0);


for all these OZ suggested adding in the default values. i dont have a strong opinion on keeping or removing

vladbochok · 2024-08-14T10:10:13Z

system-contracts/contracts/L1Messenger.sol

@@ -255,7 +255,7 @@ contract L1Messenger is IL1Messenger, ISystemContract {
        /// Check bytecodes
        uint32 numberOfBytecodes = uint32(bytes4(_totalL2ToL1PubdataAndStateDiffs[calldataPtr:calldataPtr + 4]));
        calldataPtr += 4;
-        bytes32 reconstructedChainedL1BytecodesRevealDataHash;
+        bytes32 reconstructedChainedL1BytecodesRevealDataHash = bytes32(0);


for all these OZ suggested adding in the default values. i dont have a strong opinion on keeping or removing

vladbochok

Please fix merge conflicts

l1-contracts/contracts/dev-contracts/test/DummySharedBridge.sol

…defense

benceharomi · 2024-08-19T17:56:56Z

l1-contracts/deploy-scripts/DecentralizeGovernanceUpgradeScript.s.sol

@@ -1,4 +1,5 @@
 // SPDX-License-Identifier: MIT


ProxyAdmin and ITransparentUpgradeableProxy imports are not updated to use the v4 alias:

import {ProxyAdmin} from "@openzeppelin/contracts/proxy/transparent/ProxyAdmin.sol"; import {ITransparentUpgradeableProxy} from "@openzeppelin/contracts/proxy/transparent/TransparentUpgradeableProxy.sol";

It should be changed to:

import {ProxyAdmin} from "@openzeppelin/contracts-v4/proxy/transparent/ProxyAdmin.sol"; import {ITransparentUpgradeableProxy} from "@openzeppelin/contracts-v4/proxy/transparent/TransparentUpgradeableProxy.sol";

benceharomi · 2024-08-19T18:00:49Z

Nothing is depending on the OZ v5 contracts packages yet, I would remove them from lib and package.json until they are actually needed.

This reverts commit aa83fb3.

github-actions · 2024-08-20T14:54:25Z

Coverage after merging protocol-defense into dev will be

85.88%

Coverage Report

File	Stmts	Branches	Funcs	Lines	Uncovered Lines
contracts/bridge
L1ERC20Bridge.sol	81.82%	80%	75%	83.87%	62, 70, 70–71, 73–74
L1SharedBridge.sol	77.35%	68.66%	79.41%	79.81%	103–104, 111–112, 119, 119–120, 133–137, 153–154, 162–163, 175–176, 178–179, 190, 190, 190–194, 194–195, 197, 202–204, 204–205, 207–209, 209–210, 212, 224, 233, 233–234, 393–394, 396–397, 528–529, 545–546, 556–557, 572–573, 669–670, 911, 916, 95–96
contracts/bridgehub
Bridgehub.sol	89.29%	74.07%	100%	91.49%	100–101, 112–113, 132–133, 155–156, 158–159, 332–333, 49, 63–64
contracts/common
ReentrancyGuard.sol	90%	66.67%	100%	92.86%	78–79
contracts/common/libraries
L2ContractHelper.sol	42.86%	0%	50%	52.63%	25–26, 31–32, 35–36, 50, 52, 52–53, 57, 57–58, 66
SemVer.sol	100%	100%	100%	100%
UncheckedMath.sol	100%	100%	100%	100%
UnsafeBytes.sol	100%	100%	100%	100%
contracts/governance
ChainAdmin.sol	0%	0%	0%	0%	27, 27–28, 30, 32–33, 39–40, 47–48, 56, 56–57, 60, 62–63, 63, 66, 69, 78, 78–79, 81
Governance.sol	91.67%	94.74%	95%	89.86%	44, 44–45, 48, 50–51, 53–54
contracts/state-transition
StateTransitionManager.sol	60.13%	42.86%	50%	65.42%	101, 106–110, 116, 149–150, 152–153, 155–156, 158–159, 201, 203–204, 209, 211, 211–212, 215–217, 219–220, 255, 275, 289, 294, 299, 304, 309, 314, 319, 386, 386–387, 390, 455–456, 92, 92–93
TestnetVerifier.sol	44.44%	33.33%	50%	50%	16, 16, 16, 32
ValidatorTimelock.sol	90.41%	83.33%	94.74%	89.58%	241, 59–61, 82–83
Verifier.sol	89.88%	35.71%	96.30%	90.93%	1673–1674, 287–302, 305–308, 311–318, 321–328, 331–332, 335–336, 339, 384–385, 395–396, 406–407, 417–418, 428–429, 444–445, 454, 454–455, 904–905
contracts/state-transition/chain-deps
DiamondInit.sol	77.55%	45.45%	100%	86.11%	34–35, 37–38, 40–41, 43–44, 46–47, 71
DiamondProxy.sol	80.77%	62.50%	100%	88.24%	16, 16, 16–17, 27
contracts/state-transition/chain-deps/facets
Admin.sol	86.21%	72.73%	92.31%	87.30%	109, 109–110, 112–113, 178, 180, 83–84, 94–95
Executor.sol	82.04%	63.41%	84.38%	87.90%	137–138, 192, 197, 202, 207, 212, 217, 222, 227, 230–231, 235–236, 240–242, 244–245, 260–261, 280, 294–295, 361–362, 425, 447–449, 469, 48, 48–49, 519–520, 528–529, 549, 556–557, 57, 59, 59–60, 619, 62, 620, 63, 646–647, 696–697, 70, 700–701, 71, 74–75, 775
Getters.sol	92.08%	100%	90.24%	93.10%	153, 206, 72, 77
Mailbox.sol	100%	100%	100%	100%
ZkSyncHyperchainBase.sol	92.86%	85.71%	100%	92.86%	55–56
contracts/state-transition/libraries
Diamond.sol	93.33%	80.77%	100%	95.96%	109–110, 113, 115, 117, 120, 198–199, 316
LibMap.sol	100%	100%	100%	100%
Merkle.sol	100%	100%	100%	100%
PriorityQueue.sol	100%	100%	100%	100%
TransactionValidator.sol	94.44%	88.24%	100%	96%	66–67, 69–70
contracts/upgrades
BaseZkSyncUpgrade.sol	58.20%	27.27%	100%	60.23%	104, 104–105, 108, 111, 114–115, 126, 126–127, 130, 133, 136–137, 151–153, 171–173, 212–213, 215, 215–216, 232–233, 249–250, 252–253, 258–259, 259–260, 271–272, 278–279, 285–286, 293–294, 298–299, 308–309, 311–312, 75–76
BaseZkSyncUpgradeGenesis.sol	56.67%	14.29%	100%	68.18%	25, 25–26, 33–34, 40–41, 52–53, 62–63, 65–66
DefaultUpgrade.sol	100%	100%	100%	100%
GenesisUpgrade.sol	100%	100%	100%	100%
contracts/vendor
AddressAliasHelper.sol	100%	100%	100%	100%

koloz193 added 5 commits June 8, 2024 14:47

chore(l1messenger): charge pubdata for sending l2 to l1 log (#516)

a495753

chore(oz): update oz dependency to 5 and alias oz 4.9 (#512)

a749e4b

chore(bootloader): increase MAX_ALLOWED_FAIR_PUBDATA_PRICE and MAX_AL…

4089f20

…LOWED_FAIR_L2_GAS_PRICE for base tokens (#513)

chore(compiler): move interfaces and libraries to fpp (#514)

f99eaf9

feat(contracts): add selectors to error file and update erc20 unimple…

c01f69e

…mented (#505)

koloz193 marked this pull request as ready for review June 10, 2024 03:59

koloz193 requested review from vladbochok and StanislavBreadless as code owners June 10, 2024 03:59

chore(l1-contracts): Migrate L1 Contracts to Custom Errors (#477)

1561905

vladbochok reviewed Jun 10, 2024

View reviewed changes

l1-contracts/contracts/state-transition/chain-deps/facets/Executor.sol Outdated Show resolved Hide resolved

StanislavBreadless reviewed Jun 10, 2024

View reviewed changes

koloz193 added 4 commits June 10, 2024 11:22

updated some custom errors

4e8797e

lint fix

be6176d

updated error

073cf77

address comments

1957571

StanislavBreadless approved these changes Jun 10, 2024

View reviewed changes

koloz193 added 14 commits July 24, 2024 08:38

OZ L-01 Fix Misleading Errors (#569)

62c65be

OZ L-02 Inconsistent Input Validation (#570)

b1c5968

L-03 getAllHyperchains Function Reverts Due to Invalid Key Access (#571)

ce08934

N-01 Standardize Custom Error for Consistency and N-03 Duplicate Erro…

4177161

…r Import (#572)

N-02 Typographical Errors (#573)

f1bc4c1

N-04 Risk with Floating Pragma and Yul Optimizer Bug (#574)

c8c96e0

N-05 Unused Errors (#575)

6176baf

N-06 Improved Organization of Error Files (#576)

a2fb965

N-07 Naming Issues (#577)

1094312

N-08 Unnamed Error Parameters (#578)

acd3704

N-09 Misleading Use of I Prefix in Abstract Contract (#579)

39605ca

N-12 Misplaced Error Declarations (#580)

7701826

N-13 Unused Named Return Variable (#581)

f33087d

N-15 Unnecessary Casts (#582)

def8017

koloz193 requested a review from StanislavBreadless August 6, 2024 18:47

StanislavBreadless approved these changes Aug 8, 2024

View reviewed changes

Merge branch 'dev' into protocol-defense

e995632

vladbochok reviewed Aug 14, 2024

View reviewed changes

koloz193 added 3 commits August 14, 2024 10:08

Merge branch 'dev' into protocol-defense

0fa0906

Merge branch 'dev' into protocol-defense

3d96b69

fix indexed event params and remove address constraint

31864af

koloz193 requested a review from vladbochok August 14, 2024 19:20

benceharomi reviewed Aug 15, 2024

View reviewed changes

l1-contracts/contracts/dev-contracts/test/DummySharedBridge.sol Outdated Show resolved Hide resolved

remove unused imports

c01681e

koloz193 requested a review from benceharomi August 16, 2024 01:54

lint: unused import

57d8551

vladbochok approved these changes Aug 16, 2024

View reviewed changes

koloz193 added 2 commits August 19, 2024 09:00

Merge branch 'dev' into protocol-defense

1ab17ed

Merge remote-tracking branch 'origin/protocol-defense' into protocol-…

9aab5b8

…defense

benceharomi reviewed Aug 19, 2024

View reviewed changes

This was referenced Aug 19, 2024

feat: package revamp matter-labs/v2-testnet-contracts#24

Merged

feat: package revamp part 2 (setup) matter-labs/v2-testnet-contracts#25

Merged

feat: package revamp part 3 (contracts) matter-labs/v2-testnet-contracts#26

Closed

koloz193 and others added 6 commits August 19, 2024 22:35

fix after merge

29136ea

update dep

0778beb

removed oz v5

e92c04a

updated slither config

aa83fb3

chore(foundry.toml): cleanup

07223ae

Revert "updated slither config"

9bdae7c

This reverts commit aa83fb3.

koloz193 merged commit 8be87b2 into dev Aug 20, 2024
20 checks passed

koloz193 deleted the protocol-defense branch August 20, 2024 15:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge Protocol Defense Tasks into Dev #518

Merge Protocol Defense Tasks into Dev #518

koloz193 commented Jun 9, 2024 •

edited

Loading

vladbochok Aug 14, 2024

koloz193 Aug 14, 2024

vladbochok Aug 14, 2024

koloz193 Aug 16, 2024

vladbochok Aug 14, 2024

koloz193 Aug 16, 2024

vladbochok Aug 14, 2024

koloz193 Aug 16, 2024

vladbochok left a comment

benceharomi Aug 19, 2024

benceharomi commented Aug 19, 2024

github-actions bot commented Aug 20, 2024

Merge Protocol Defense Tasks into Dev #518

Merge Protocol Defense Tasks into Dev #518

Conversation

koloz193 commented Jun 9, 2024 • edited Loading

What ❔

Why ❔

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vladbochok left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benceharomi commented Aug 19, 2024

github-actions bot commented Aug 20, 2024

koloz193 commented Jun 9, 2024 •

edited

Loading