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

[DO NOT MERGE]: Release v25 protocol defense #774

Open
wants to merge 62 commits into
base: main
Choose a base branch
from

Conversation

koloz193
Copy link
Contributor

@koloz193 koloz193 commented Sep 5, 2024

v25 release changes

Bootloader

  • increase max_pubdata price and max_l2_price from 1M to 2^64

    • This is for chains with custom base tokens with low unit value.
  • removed upgradesystemcontext method

    • It was a temporary method, used in one of the past upgrades.

Contracts

  • Moved to custom errors from string based ones
  • small gas optimisations
  • l2-contracts moved to 1.5.0 compiler and solidity 0.8.24
  • verifier_contract - mention vk_hash in the source code comments

Scripts

  • workflows - switching to forge & foundry from hardhat
  • script for new chain registration
  • moved from zksync-web3 to zksync-ethers
  • improvements to deploy scripts - comare diamond hash
  • improved compilation caching in scripts (don't recompile if not needed)

Testing

  • more bridgehub tests
  • adopting tests to custom errors
  • more mailbox tests

Docs & comments

  • naming - changing from zkSync to ZKsync
  • additional documentation in solidity files
  • better documentation for some methods & arguments
  • changed wording from 'ergs' to 'gas'
  • added missing licenses to YUL files

Dependencies

  • hardhat updated to 2.22 and chai matchers to 0.2.0
  • lower restriction on solidity versions for interfaces (from 0.8.24 to ^0.8.21)
  • explicit dependency on openzeppelin-v4 (to allow users to include other OZ versions)
  • updated forge-sdk & murky

Raid5594 and others added 30 commits April 24, 2024 15:36
Co-authored-by: Raid Ateir <ra@matterlabs.dev>
Co-authored-by: Uacias <filgum0326@gmail.com>
Co-authored-by: Mateusz Zając <matzayonc@gmail.com>
Co-authored-by: Mateusz Zając <60236390+matzayonc@users.noreply.github.com>
chore(contracts): merge release 23 into dev
Co-authored-by: Stanislav Breadless <stanislavbezkor@gmail.com>
Signed-off-by: Danil <deniallugo@gmail.com>
Co-authored-by: Lyova Potyomkin <lyova.potyomkin@gmail.com>
Co-authored-by: kelemeno <kl@matterlabs.dev>
Co-authored-by: Stanislav Bezkorovainyi <stanislavbezkor@gmail.com>
Co-authored-by: Raid Ateir <ra@matterlabs.dev>
Co-authored-by: kelemeno <34402761+kelemeno@users.noreply.github.com>
Co-authored-by: Jmunoz <juanmunoz890@gmail.com>
Co-authored-by: Bence Haromi <bence.haromi@gmail.com>
Co-authored-by: Danil <deniallugo@gmail.com>
Co-authored-by: tommysr <47206288+tommysr@users.noreply.github.com>
CI checks were failing in the earlier branch
Failing CI check is for coverage and for external contributors, they are not permissioned enough to run this check. The reason being Github doesn't allow granting anything except read for tokens, when PR is from fork https://docs.github.com/en/actions/security-guides/automatic-token-authentication#permissions-for-the-github_token
@koloz193 koloz193 changed the title Release v25 protocol defense [DO NOT MERGE]: Release v25 protocol defense Sep 27, 2024
@koloz193 koloz193 marked this pull request as ready for review September 27, 2024 16:00
koloz193 and others added 2 commits September 27, 2024 12:21
Signed-off-by: Danil <deniallugo@gmail.com>
Co-authored-by: Bence Haromi <56651250+benceharomi@users.noreply.github.com>
Co-authored-by: Grzegorz Prusak <pompon.pompon@gmail.com>
Co-authored-by: Moshe Shababo <17073733+moshababo@users.noreply.github.com>
Co-authored-by: Akosh Farkash <aakoshh@gmail.com>
Co-authored-by: Bruno França <bruno@franca.xyz>
Co-authored-by: Vlad Bochok <41153528+vladbochok@users.noreply.github.com>
Co-authored-by: Roman Brodetski <Roman.Brodetski@gmail.com>
Co-authored-by: vladbochok <vladbochok1@gmail.com>
Co-authored-by: Stanislav Bezkorovainyi <stanislavbezkor@gmail.com>
Co-authored-by: Danil <deniallugo@gmail.com>
@koloz193 koloz193 closed this Oct 4, 2024
@koloz193 koloz193 reopened this Oct 4, 2024
koloz193 and others added 3 commits October 4, 2024 10:25
Signed-off-by: Danil <deniallugo@gmail.com>
Co-authored-by: otani <maksym.kryva@gmail.com>
Co-authored-by: Zach Kolodny <zach.kolodny@gmail.com>
Comment on lines +14 to +30
using stdToml for string;

struct Config {
address admin;
address governor;
}

Config internal config;

function initConfig() public {
string memory root = vm.projectRoot();
string memory path = string.concat(root, "/script-config/config-accept-admin.toml");
string memory toml = vm.readFile(path);
config.admin = toml.readAddress("$.target_addr");
config.governor = toml.readAddress("$.governor");
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems it's a merge artifact

@@ -145,7 +158,8 @@ fn insert_residue_elements_and_commitments(
Ok(reg.render_template(
&verifier_contract_template,
&json!({"residue_g2_elements": residue_g2_elements,
"commitments": commitments}),
"commitments": commitments,
"vk_hash": vk_hash}),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@koloz193 - but this vk_hash is not present in the verifier_contract_template.txt ?

Copy link
Contributor Author

@koloz193 koloz193 Oct 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It’s one of the comments at the top

@shahar4
Copy link
Collaborator

shahar4 commented Oct 20, 2024

@koloz193 it's worth calling out explicitly the bootloader change to remove the 10k gwei limitation for the fair_l2_price. I'd say it's a lot more impactful than the pubdata limitation.

Copy link

Coverage after merging release-v25-protocol-defense into main will be

86.48%

Coverage Report
FileStmtsBranchesFuncsLinesUncovered Lines
contracts/bridge
   L1ERC20Bridge.sol81.82%80%75%83.87%62, 70, 70–71, 73–74
   L1SharedBridge.sol79.59%66.23%84.21%83.41%101–102, 109–110, 117–118, 125, 125–126, 133, 177, 190–191, 199–200, 212–213, 215–216, 227, 227, 227–231, 231–232, 234, 239–241, 241–242, 244–246, 246–247, 249, 261, 270, 270–271, 279–280, 290, 444–445, 447–448, 579–580, 596–597, 607–608, 623–624, 720–721, 962, 967
contracts/bridgehub
   Bridgehub.sol89.29%74.07%100%91.49%100–101, 112–113, 132–133, 155–156, 158–159, 332–333, 49, 63–64
contracts/common
   ReentrancyGuard.sol90%66.67%100%92.86%78–79
contracts/common/libraries
   L2ContractHelper.sol42.86%0%50%52.63%25–26, 31–32, 35–36, 50, 52, 52–53, 57, 57–58, 66
   SemVer.sol100%100%100%100%
   UncheckedMath.sol100%100%100%100%
   UnsafeBytes.sol100%100%100%100%
contracts/governance
   ChainAdmin.sol0%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.sol91.67%94.74%95%89.86%44, 44–45, 48, 50–51, 53–54
contracts/state-transition
   StateTransitionManager.sol59.48%35.71%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, 79, 92, 92–93
   TestnetVerifier.sol44.44%33.33%50%50%16, 16, 16, 32
   ValidatorTimelock.sol95.89%83.33%100%95.83%241, 82–83
   Verifier.sol89.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.sol77.55%45.45%100%86.11%34–35, 37–38, 40–41, 43–44, 46–47, 71
   DiamondProxy.sol92.31%75%100%100%16, 27
contracts/state-transition/chain-deps/facets
   Admin.sol86.21%72.73%92.31%87.30%109, 109–110, 112–113, 178, 180, 83–84, 94–95
   Executor.sol82.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.sol92.08%100%90.24%93.10%153, 206, 72, 77
   Mailbox.sol100%100%100%100%
   ZkSyncHyperchainBase.sol92.86%85.71%100%92.86%55–56
contracts/state-transition/libraries
   Diamond.sol93.33%80.77%100%95.96%109–110, 113, 115, 117, 120, 198–199, 316
   LibMap.sol100%100%100%100%
   Merkle.sol100%100%100%100%
   PriorityQueue.sol100%100%100%100%
   TransactionValidator.sol94.44%88.24%100%96%66–67, 69–70
contracts/upgrades
   BaseZkSyncUpgrade.sol58.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.sol56.67%14.29%100%68.18%25, 25–26, 33–34, 40–41, 52–53, 62–63, 65–66
   DefaultUpgrade.sol100%100%100%100%
   GenesisUpgrade.sol100%100%100%100%
contracts/vendor
   AddressAliasHelper.sol100%100%100%100%

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.