Better permanent restriction protection #820

StanislavBreadless · 2024-09-25T10:28:44Z

What ❔

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.

Sync head with base

github-actions · 2024-09-25T10:57:14Z

Changes to gas cost

Generated at commit: ac32611edf84142281497145d848199abd0a6289, compared to commit: 9aa740122a29b5ff4974c566856811d686468b65

🧾 Summary (100% most significant diffs)

Contract	Method	Avg (+/-)	%
PermanentRestriction	allowAdminImplementation setAllowedData setSelectorIsValidated validateCall	-21,546 ✅ -22,033 ✅ -21,359 ✅ -3,037 ✅	-45.13% -44.74% -44.86% -10.75%
DummyBridgehubSetter	createNewChain requestL2TransactionDirect requestL2TransactionTwoBridges setCTM setZKChain	-327 ✅ -1,160 ✅ -119 ✅ -1 ✅ -1 ✅	-0.30% -1.20% -0.08% -0.00% -0.00%
ChainAdmin	setUpgradeTimestamp	+149 ❌	+0.33%
DiamondProxy	finalizeEthWithdrawal requestL2Transaction	+114 ❌ +7 ❌	+0.15% +0.01%
TestnetERC20Token	transfer	+10 ❌	+0.02%
DummyChainTypeManagerWBH	setZKChain	-1 ✅	-0.00%

Full diff report 👇

Contract	Deployment Cost (+/-)	Method	Min (+/-)	%	Avg (+/-)	%	Median (+/-)	%	Max (+/-)	%	# Calls (+/-)
PermanentRestriction	1,998,755 (+851,050)	allowAdminImplementation setAllowedData setSelectorIsValidated tryCompareAdminOfAChain validateCall	26,196 (-21,298) 26,762 (-21,894) 26,253 (-21,330) 453 (0) 722 (-25,199)	-44.84% -45.00% -44.83% 0.00% -97.21%	26,196 (-21,546) 27,217 (-22,033) 26,253 (-21,359) 16,668 (-2,449) 25,217 (-3,037)	-45.13% -44.74% -44.86% -12.81% -10.75%	26,196 (-21,670) 27,033 (-21,863) 26,253 (-21,378) 24,579 (+409) 30,170 (+1,619)	-45.27% -44.71% -44.88% +1.69% +5.67%	26,196 (-21,670) 27,846 (-23,183) 26,253 (-21,378) 24,579 (+409) 31,793 (+1,840)	-45.27% -45.43% -44.88% +1.69% +6.14%	258 (0) 257 (0) 258 (0) 13 (+2) 10 (+3)
ChainTypeManager	4,547,000 (0)	createNewChain initialize	1,053 (0) 22,745 (0)	0.00% 0.00%	3,365,878 (+111,687) 113,757 (+388)	+3.43% +0.34%	3,614,070 (0) 22,745 (0)	0.00% 0.00%	3,614,070 (0) 207,230 (0)	0.00% 0.00%	29 (+9) 75 (+18)
DummyBridgehubSetter	5,364,532 (0)	addChainTypeManager admin createNewChain proveL1ToL2TransactionStatus proveL2LogInclusion proveL2MessageInclusion removeChainTypeManager requestL2TransactionDirect requestL2TransactionTwoBridges setAddresses setCTM setPendingAdmin setZKChain	23,865 (0) 411 (0) 28,727 (+204) 0 (0) 0 (0) 0 (0) 23,802 (0) 32,962 (+15) 30,875 (0) 24,282 (0) 44,193 (0) 25,880 (0) 111,315 (0)	0.00% 0.00% +0.72% +∞% +∞% +∞% 0.00% +0.05% 0.00% 0.00% 0.00% 0.00% 0.00%	44,090 (-1) 1,523 (+1) 107,393 (-327) 1,369 (+19) 1,491 (+36) 1,566 (+22) 25,416 (-2) 95,702 (-1,160) 144,049 (-119) 65,961 (+13) 44,230 (-1) 47,592 (+3) 111,352 (-1)	-0.00% +0.07% -0.30% +1.41% +2.47% +1.42% -0.01% -1.20% -0.08% +0.02% -0.00% +0.01% -0.00%	47,558 (0) 2,411 (0) 36,343 (0) 958 (0) 1,063 (+3) 1,144 (0) 25,774 (0) 75,390 (-68) 46,590 (-10,172) 71,130 (0) 44,241 (-12) 49,745 (0) 111,363 (-12)	0.00% 0.00% 0.00% 0.00% +0.28% 0.00% 0.00% -0.09% -17.92% 0.00% -0.03% 0.00% -0.01%	47,558 (0) 2,411 (0) 871,220 (-8,337) 3,585 (0) 3,764 (-14) 3,969 (+7) 26,190 (0) 221,852 (-648) 399,797 (0) 91,270 (0) 44,253 (0) 49,757 (0) 111,375 (0)	0.00% 0.00% -0.95% 0.00% -0.37% +0.18% 0.00% -0.29% 0.00% 0.00% 0.00% 0.00% 0.00%	6,656 (0) 2,302 (-1) 2,560 (0) 512 (0) 512 (0) 512 (0) 2,048 (0) 1,024 (0) 1,536 (0) 3,328 (0) 2,816 (0) 4,862 (-1) 2,816 (0)
DiamondInit	625,064 (-12)	initialize	22,569 (0)	0.00%	397,824 (+1,558)	+0.39%	400,257 (0)	0.00%	420,157 (0)	0.00%	138 (+9)
ChainAdmin	909,847 (0)	setUpgradeTimestamp	25,361 (0)	0.00%	45,007 (+149)	+0.33%	45,327 (-42)	-0.09%	45,645 (0)	0.00%	256 (0)
MailboxFacet	3,287,412 (0)	finalizeEthWithdrawal	8,101 (0)	0.00%	49,205 (+123)	+0.25%	49,490 (0)	0.00%	49,490 (0)	0.00%	257 (0)
DiamondProxy	2,475,597 (0)	finalizeEthWithdrawal requestL2Transaction util_setChainId	37,590 (0) 33,146 (0) 28,906 (0)	0.00% 0.00% 0.00%	76,496 (+114) 128,885 (+7) 33,702 (-47)	+0.15% +0.01% -0.14%	76,719 (-36) 166,332 (+12) 33,718 (0)	-0.05% +0.01% 0.00%	77,007 (0) 188,247 (0) 34,090 (0)	0.00% 0.00% 0.00%	257 (0) 771 (0) 519 (0)
AccessControlRestriction	1,759,703 (0)	grantRole setRequiredRoleForCall setRequiredRoleForFallback	51,036 (0) 48,605 (0) 47,940 (0)	0.00% 0.00% 0.00%	51,287 (+19) 49,424 (+7) 48,852 (+4)	+0.04% +0.01% +0.01%	51,408 (0) 48,977 (0) 48,312 (0)	0.00% 0.00% 0.00%	51,408 (0) 51,877 (0) 51,658 (0)	0.00% 0.00% 0.00%	1,024 (0) 1,280 (0) 1,280 (0)
Bridgehub	5,327,424 (0)	getZKChain	0 (0)	+∞%	2,930 (-1)	-0.03%	2,758 (0)	0.00%	5,039 (0)	0.00%	60 (+15)
TestnetERC20Token	800,383 (0)	approve mint transfer	24,247 (0) 34,018 (0) 46,273 (0)	0.00% 0.00% 0.00%	46,236 (-1) 61,787 (-2) 46,636 (+10)	-0.00% -0.00% +0.02%	46,207 (0) 68,242 (0) 46,609 (+48)	0.00% 0.00% +0.10%	46,543 (0) 68,602 (0) 46,873 (0)	0.00% 0.00% 0.00%	1,527 (0) 1,704 (0) 257 (0)
L1NativeTokenVault	3,907,758 (-12)	bridgeBurn	12,535 (0)	0.00%	84,633 (+7)	+0.01%	99,679 (0)	0.00%	99,679 (0)	0.00%	1,299 (0)
MerkleTreeNoSort	583,114 (0)	getProof	2,608 (0)	0.00%	32,750 (-2)	-0.01%	33,207 (0)	0.00%	33,229 (0)	0.00%	277 (0)
DummyChainTypeManagerWBH	4,576,779 (0)	setZKChain	44,085 (0)	0.00%	44,122 (-1)	-0.00%	44,133 (-6)	-0.01%	44,145 (0)	0.00%	256 (0)
L1Nullifier	3,325,017 (+12)
MessageRoot	1,508,699 (+24)
DummySharedBridge	1,594,144 (-12)
ReenterL1ERC20Bridge	406,592 (+12)
TestExecutor	3,472,725 (-12)
ValidatorTimelock	987,801 (+12)
GettersFacet	1,152,473 (+12)
TransparentUpgradeableProxy	787,868 (-200,437)

vladbochok · 2024-09-25T11:27:13Z

l1-contracts/contracts/governance/PermanentRestriction.sol

+    /// @param _call The call data.
+    /// @dev This function reverts if the provided call was not a migration call.
+    function tryGetNewAdminFromMigration(Call calldata _call) external view returns (address) {
+        if (_call.target != address(BRIDGE_HUB)) {


Right now we use ChainAdmin contract as an admin in BridgeHub, SharedBridge, and other contracts. The logic is fine, just need to remember that ChainAdmin can be used only for Chains adminship

I think we will need to deploy a different chainadmin contract for those ones

(without the permanent rollup restriction)

l1-contracts/contracts/governance/PermanentRestriction.sol

github-actions · 2024-09-26T07:50:50Z

Coverage after merging sb-better-governance-protection into oz-audit-sep-head will be

85.09%

Coverage Report

File	Stmts	Branches	Funcs	Lines	Uncovered Lines
../da-contracts/contracts
RollupL1DAValidator.sol	64.94%	37.50%	83.33%	70.91%	145, 148, 148, 148, 150, 183–184, 187–188, 27, 27–28, 30, 30–31, 34, 36–37, 41–42, 65, 67, 67, 67–68, 70
contracts/bridge
BridgeHelper.sol	93.33%	50%	100%	100%	22
BridgedStandardERC20.sol	73.33%	25%	92.31%	75.93%	107–108, 113–114, 126–127, 151–152, 193, 193, 200, 200, 207, 207, 218, 54–55, 81–82
L1ERC20Bridge.sol	93.18%	80%	100%	93.75%	188–189, 264
L1Nullifier.sol	76.36%	56%	80%	82.86%	111–112, 127, 127–128, 135, 135–136, 143, 143–144, 173–174, 197, 233–234, 236–237, 246–247, 255–256, 258, 424, 426–427, 427, 427, 429–430, 430, 430, 441–442, 455–456, 477–478, 517, 611, 697, 699, 701, 714, 728, 733
contracts/bridge/asset-router
AssetRouterBase.sol	83.78%	40%	100%	88%	138–139, 57–58, 85–86
L1AssetRouter.sol	90.20%	72%	92%	94.17%	204–205, 241, 250, 252, 255, 57, 576, 58, 73–74, 81–82
contracts/bridge/ntv
L1NativeTokenVault.sol	95.74%	95%	92.31%	96.72%	215, 215–216
NativeTokenVault.sol	87.76%	68.18%	90.48%	91.35%	192, 194, 212–213, 220–221, 254–255, 380, 382, 394–395, 447, 452, 64–65
contracts/bridgehub
Bridgehub.sol	80%	48.48%	93.33%	86.61%	111, 111–112, 118–119, 126–127, 133–134, 140, 140–141, 175–176, 222–223, 223, 223–224, 231–232, 234–235, 238–239, 249–250, 264–265, 314–315, 317–318, 375–376, 391–392, 422–423, 506–507, 588, 687, 690–691, 695–696, 729–730, 743, 786–787, 789–790, 792–793, 827–828, 831–832, 834–835, 870, 875
CTMDeploymentTracker.sol	79.07%	50%	90%	94.74%	115, 119, 34, 41, 64, 91, 94, 96
MessageRoot.sol	91.07%	63.64%	100%	96.97%	116–117, 148, 69, 87
contracts/common
ReentrancyGuard.sol	90%	66.67%	100%	92.86%	78–79
contracts/common/libraries
DataEncoding.sol	71.43%	37.50%	100%	75%	108, 112, 119, 129, 129–131, 134, 75, 83
DynamicIncrementalMerkle.sol	74.42%	100%	80%	72.22%	67–70, 72–74, 76–78
FullMerkle.sol	100%	100%	100%	100%
L2ContractHelper.sol	50%	0%	50%	60%	100, 100–101, 109, 56, 68–69, 74–75, 78–79, 93, 95, 95–96
Merkle.sol	96.61%	90.91%	100%	97.67%	80–81
MessageHashing.sol	100%	100%	100%	100%
SemVer.sol	100%	100%	100%	100%
SystemContractsCaller.sol	0%	0%	0%	0%	114, 122–125, 135–138, 138–139, 141, 141–142, 33, 33–34, 37, 45, 47, 49, 51, 53, 66, 66, 66, 69, 72, 75, 78, 89, 91, 93, 96, 98
UncheckedMath.sol	100%	100%	100%	100%
UnsafeBytes.sol	84.21%	100%	83.33%	84.62%	35–36
contracts/governance
AccessControlRestriction.sol	100%	100%	100%	100%
ChainAdmin.sol	95.12%	80%	100%	96.15%	27–28
Governance.sol	98.15%	94.74%	100%	98.55%	45–46
PermanentRestriction.sol	89.43%	82.61%	100%	89.41%	110, 110–111, 138, 201, 201–202, 205, 207, 207–208, 248–249
contracts/state-transition
ChainTypeManager.sol	66.67%	33.33%	57.14%	77.67%	108, 135–136, 138–139, 141–142, 144–145, 200–201, 245, 252, 270, 276, 283, 295, 302, 309, 317, 324, 332, 339, 357, 359, 424, 443, 443, 443, 446, 446, 446, 448, 461, 466, 491, 74, 87–88
TestnetVerifier.sol	77.78%	66.67%	100%	75%	16, 28
ValidatorTimelock.sol	95.08%	83.33%	100%	95.24%	200, 82–83
Verifier.sol	89.90%	40%	96.30%	90.93%	1674–1675, 287–302, 305–308, 311–318, 321–328, 331–332, 335–336, 339, 383–384, 394–395, 405–406, 416–417, 427–428, 443–444, 453, 453–454, 905–906
contracts/state-transition/chain-deps
DiamondInit.sol	78%	45.45%	100%	86.49%	39–40, 42–43, 45–46, 48–49, 51–52, 77
DiamondProxy.sol	92.31%	75%	100%	100%	16, 27
contracts/state-transition/chain-deps/facets
Admin.sol	72.69%	36.21%	90.91%	85.29%	104–105, 115–116, 130, 130–131, 133–134, 157, 157, 157–158, 158, 158, 160, 239, 241, 254–255, 261, 263, 266, 266, 266, 284, 295–296, 301, 313, 313, 315, 315, 315, 321, 321, 321–322, 322, 322–324, 324, 324–325, 325, 325–327, 354, 356, 360, 369, 379, 383, 40, 40
Executor.sol	76.13%	57.14%	92%	81.25%	120–121, 173, 178, 183, 188, 193, 198, 202–203, 208, 208–209, 209–210, 212, 212–213, 223, 227, 227–228, 246–247, 268, 271, 317,

StanislavBreadless added 7 commits September 10, 2024 17:51

just merge

94b18c1

resolve most of the conflicts

9615d90

sync head with base

1673186

delete unneeded files

eb3583a

Sync audit head with base (#797)

e5727a0

Merge pull request #798 from matter-labs/sb-sync-head-base

ef318e2

Sync head with base

better governance protection

6a8dc33

StanislavBreadless requested a review from vladbochok as a code owner September 25, 2024 10:28

StanislavBreadless added 2 commits September 25, 2024 12:43

fix old tests

82a2639

fix lint

54f3c8b

vladbochok reviewed Sep 25, 2024

View reviewed changes

l1-contracts/contracts/governance/PermanentRestriction.sol Show resolved Hide resolved

vladbochok reviewed Sep 25, 2024

View reviewed changes

l1-contracts/contracts/governance/PermanentRestriction.sol Show resolved Hide resolved

StanislavBreadless added 2 commits September 25, 2024 13:50

tests wip

c2c3dbe

min gas for fallable call

6ea2aed

vladbochok reviewed Sep 25, 2024

View reviewed changes

l1-contracts/contracts/governance/PermanentRestriction.sol Outdated Show resolved Hide resolved

l1-contracts/contracts/governance/PermanentRestriction.sol Outdated Show resolved Hide resolved

StanislavBreadless added 10 commits September 25, 2024 15:14

full test cover for tryGetNewAdminFromMigration

4be4c29

some rename + better coverage

c77fd57

lint fix

950862e

comment

16a26bc

add tests for l2 admin factory

30f1e54

fix lint

2c6d116

exclude l2adminfactory as it is covered by l2 tests

5bd14e1

use legacy functions

61e8cda

ensure that the function never panics

a27fdea

fix things

8e4c1fc

vladbochok approved these changes Sep 25, 2024

View reviewed changes

vladbochok changed the base branch from sync-layer-stable to oz-audit-sep-head September 26, 2024 07:35

vladbochok requested a review from Deniallugo as a code owner September 26, 2024 07:35

Merge branch 'oz-audit-sep-head' into sb-better-governance-protection

b18032a

StanislavBreadless changed the base branch from oz-audit-sep-head to sync-layer-stable September 26, 2024 08:36

StanislavBreadless changed the base branch from sync-layer-stable to kl/sync-layer-reorg September 26, 2024 15:04

Merge branch 'kl/sync-layer-reorg' into sb-better-governance-protection

5c5c967

StanislavBreadless merged commit c99b936 into kl/sync-layer-reorg Sep 26, 2024
24 of 25 checks passed

StanislavBreadless deleted the sb-better-governance-protection branch September 26, 2024 15:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better permanent restriction protection #820

Better permanent restriction protection #820

StanislavBreadless commented Sep 25, 2024

github-actions bot commented Sep 25, 2024 •

edited

Loading

vladbochok Sep 25, 2024

StanislavBreadless Sep 25, 2024

StanislavBreadless Sep 25, 2024

github-actions bot commented Sep 26, 2024

Better permanent restriction protection #820

Better permanent restriction protection #820

Conversation

StanislavBreadless commented Sep 25, 2024

What ❔

Why ❔

Checklist

github-actions bot commented Sep 25, 2024 • edited Loading

Changes to gas cost

🧾 Summary (100% most significant diffs)

vladbochok Sep 25, 2024

Choose a reason for hiding this comment

StanislavBreadless Sep 25, 2024

Choose a reason for hiding this comment

StanislavBreadless Sep 25, 2024

Choose a reason for hiding this comment

github-actions bot commented Sep 26, 2024

github-actions bot commented Sep 25, 2024 •

edited

Loading