Contracts v9 (#2256)

* Protection against front-running and reentrancy Adding reentrancy guard on `activateExodusMode()`. Checking the length of block and proof.commitments in `proveBlocks(...)` * Protection against reentrancy attack Adding reentrancy guard on all external functions that write to storage. * Update ZkSync.sol Protection against duplicated proved blocks. * Update ZkSync.sol Adding a comment for front-running protection. * Update ZkSync.sol Gas Optimization * protection against griefing attack So, the server can check it's value before adding the `changePubKey` to the block (to protect against griefing attack). * Update AdditionalZkSyncCutNoticePeriodUnitTest.sol Initializing reentrancy guard. * immunefi bugs * removing complete withdrawals * removing complete withdrawals * removing complete withdrawals * Add server protection of block invalidation * Try to fix unit tests * Restore tests * Try to find workaround * Fix unit tests * gas optimization * gas optimization & change governor check * increasing the code readability * solving the mistake * fixing the logic break due to gas optimization * Update contracts/contracts/ZkSync.sol Co-authored-by: Igor Aleksanov <[email protected]> * cross-domain attck protection * domain-typehash changed * new change pubkey for cross-domain protection * removing extra spaces * adding test for changePubKey type EIP712 * name struct with a capital letter * prove blocks unit test + memory to calldata + adding length check to proveBlocks function * removing the redundant function * Fix ci Signed-off-by: deniallugo <[email protected]> * Update ZkSync.sol Changing the require message to be unique from "r" to "o3". * updating the front-running protection removing the proof.commitments[] length check. * Adding comment Adding comment to the require in `proveBlocks` function * Fix comment * Add primitives Signed-off-by: deniallugo <[email protected]> * Implement change pubkey eip712 Signed-off-by: deniallugo <[email protected]> * Use eip712 change pub key in zksync-rs Signed-off-by: deniallugo <[email protected]> * Add test Signed-off-by: deniallugo <[email protected]> * Deploy Signed-off-by: deniallugo <[email protected]> * Fix Signed-off-by: deniallugo <[email protected]> * Add eip712 to js sdk Signed-off-by: deniallugo <[email protected]> * Some refactoring Signed-off-by: deniallugo <[email protected]> * Add check eth witness generation Signed-off-by: deniallugo <[email protected]> * Fix sdk Signed-off-by: deniallugo <[email protected]> * Fix eth witness generation Signed-off-by: deniallugo <[email protected]> * Fix types Signed-off-by: deniallugo <[email protected]> * Remove strict node js Signed-off-by: deniallugo <[email protected]> * Fix ecdsa signatures Signed-off-by: deniallugo <[email protected]> * Add assertion for const generic size Signed-off-by: deniallugo <[email protected]> * Use u32 for chain id Signed-off-by: deniallugo <[email protected]> * Fix comment about costs for change pub key Signed-off-by: deniallugo <[email protected]> * Fix some nits Signed-off-by: deniallugo <[email protected]> * Change name of deprecated change pub key method Signed-off-by: deniallugo <[email protected]> * Smart contracts v9 (last changes before upgrade) (#2285) * Add event for different type of withdrawals * Fix bug from compiler side * Add owner field into withdraw event * Fix networks Signed-off-by: deniallugo <[email protected]> Signed-off-by: deniallugo <[email protected]> Co-authored-by: deniallugo <[email protected]> * Fix typos * Add a changelog * Small fix * Add missing point * Add info about changed event signature * Merge (#2295) * Remove sepolia (#2291) Signed-off-by: deniallugo <[email protected]> Signed-off-by: deniallugo <[email protected]> * Add correct pagination for transaction history query Signed-off-by: deniallugo <[email protected]> * Fix transactions_history_item_old Signed-off-by: deniallugo <[email protected]> * Fix tests but a lot of cludges Signed-off-by: deniallugo <[email protected]> * Fix test Signed-off-by: deniallugo <[email protected]> Signed-off-by: deniallugo <[email protected]> Co-authored-by: bors-matterlabs-dev[bot] <76108001+bors-matterlabs-dev[bot]@users.noreply.github.com> Signed-off-by: deniallugo <[email protected]> Co-authored-by: miladpiri <[email protected]> Co-authored-by: miladpiri <[email protected]> Co-authored-by: Vladyslav-Bochok <[email protected]> Co-authored-by: Vlad Bochok <[email protected]> Co-authored-by: Igor Aleksanov <[email protected]> Co-authored-by: bors-matterlabs-dev[bot] <76108001+bors-matterlabs-dev[bot]@users.noreply.github.com>
Dhenxscode · Sep 13, 2022 · 0bf5cd6 · 0bf5cd6
1 parent 6cf1c09
commit 0bf5cd6
Show file tree

Hide file tree

Showing 92 changed files with 1,870 additions and 390 deletions.
diff --git a/.github/workflows/deploy.rinkeby-beta.yml b/.github/workflows/deploy.rinkeby-beta.yml
@@ -4,6 +4,7 @@ on:
     branches:
       - dev
       - hotfix/*
+      - breaking
     tags:
       - hotfix-*
 

diff --git a/.github/workflows/zk-environment.publish.yml b/.github/workflows/zk-environment.publish.yml
@@ -20,7 +20,7 @@ jobs:
     steps:
       - uses: actions/checkout@v2
 
-      - name: Set up Docker Buildx
+      - name: Set up Docker Build
         uses: docker/setup-buildx-action@v1
 
       - name: Log in to Docker Hub

diff --git a/changelog/contracts.md b/changelog/contracts.md
@@ -2,6 +2,28 @@
 
 All notable changes to the contracts will be documented in this file.
 
+## 2022-08-19
+
+**Version 9** is scheduled for upgrade.
+
+### Added
+
+- `nonReentrant` modifier for all external functions as additional protection from reentrancy attack.
+- New method for EIP-712 `ChangePubKey` authorization.
+- A check for non-zero address when changing the governor.
+
+### Changed
+
+- `WithdrawalPending` event parameters changed.
+- `Withdrawal` event parameters changed.
+- Use `calldata` instead of `memory` for gas cost optimization.
+- The visibility of the function `authFactsResetTimer` changed from internal to public.
+- `proveBlocks` ignores already proved blocks.
+
+### Removed
+
+- The finalizing withdrawals on `executeBlocks` function are removed.
+
 ## 2022-02-27
 
 **Version 8** is scheduled for upgrade.

diff --git a/contracts/contracts/AdditionalZkSync.sol b/contracts/contracts/AdditionalZkSync.sol
@@ -38,7 +38,7 @@ contract AdditionalZkSync is Storage, Config, Events, ReentrancyGuard {
     /// @param _tokenId Verified token id
     /// @param _amount Amount for owner (must be total amount, not part of it)
     function performExodus(
-        StoredBlockInfo memory _storedBlockInfo,
+        StoredBlockInfo calldata _storedBlockInfo,
         address _owner,
         uint32 _accountId,
         uint32 _tokenId,
@@ -48,7 +48,7 @@ contract AdditionalZkSync is Storage, Config, Events, ReentrancyGuard {
         uint32 _nftSerialId,
         bytes32 _nftContentHash,
         uint256[] calldata _proof
-    ) external {
+    ) external nonReentrant {
         require(_accountId <= MAX_ACCOUNT_ID, "e");
         require(_accountId != SPECIAL_ACCOUNT_ID, "v");
         require(_tokenId < SPECIAL_NFT_TOKEN_ID, "T");
@@ -74,7 +74,7 @@ contract AdditionalZkSync is Storage, Config, Events, ReentrancyGuard {
         if (_tokenId <= MAX_FUNGIBLE_TOKEN_ID) {
             bytes22 packedBalanceKey = packAddressAndTokenId(_owner, uint16(_tokenId));
             increaseBalanceToWithdraw(packedBalanceKey, _amount);
-            emit WithdrawalPending(uint16(_tokenId), _owner, _amount);
+            emit WithdrawalPending(uint16(_tokenId), _owner, _amount, Operations.WithdrawalType.FullExit);
         } else {
             require(_amount != 0, "Z"); // Unsupported nft amount
             Operations.WithdrawNFT memory withdrawNftOp = Operations.WithdrawNFT(
@@ -91,7 +91,10 @@ contract AdditionalZkSync is Storage, Config, Events, ReentrancyGuard {
         performedExodus[_accountId][_tokenId] = true;
     }
 
-    function cancelOutstandingDepositsForExodusMode(uint64 _n, bytes[] calldata _depositsPubdata) external {
+    function cancelOutstandingDepositsForExodusMode(uint64 _n, bytes[] calldata _depositsPubdata)
+        external
+        nonReentrant
+    {
         require(exodusMode, "8"); // exodus mode not active
         uint64 toProcess = Utils.minU64(totalOpenPriorityRequests, _n);
         require(toProcess > 0, "9"); // no deposits to process
@@ -144,7 +147,7 @@ contract AdditionalZkSync is Storage, Config, Events, ReentrancyGuard {
 
     /// @notice approve to decrease upgrade notice period time to zero
     /// NOTE: сan only be called after the start of the upgrade
-    function cutUpgradeNoticePeriod(bytes32 targetsHash) external {
+    function cutUpgradeNoticePeriod(bytes32 targetsHash) external nonReentrant {
         require(upgradeStartTimestamp != 0, "p1");
         require(getUpgradeTargetsHash() == targetsHash, "p3"); // given targets are not in the active upgrade
 
@@ -154,7 +157,7 @@ contract AdditionalZkSync is Storage, Config, Events, ReentrancyGuard {
     /// @notice approve to decrease upgrade notice period time to zero by signatures
     /// NOTE: Can accept many signatures at a time, thus it is possible
     /// to completely cut the upgrade notice period in one transaction
-    function cutUpgradeNoticePeriodBySignature(bytes[] calldata signatures) external {
+    function cutUpgradeNoticePeriodBySignature(bytes[] calldata signatures) external nonReentrant {
         require(upgradeStartTimestamp != 0, "p2");
 
         bytes32 targetsHash = getUpgradeTargetsHash();
@@ -175,7 +178,7 @@ contract AdditionalZkSync is Storage, Config, Events, ReentrancyGuard {
 
     /// @return hash of the concatenation of targets for which there is an upgrade
     /// NOTE: revert if upgrade is not active at this moment
-    function getUpgradeTargetsHash() internal returns (bytes32) {
+    function getUpgradeTargetsHash() internal view returns (bytes32) {
         // Get the addresses of contracts that are being prepared for the upgrade.
         address gatekeeper = $(UPGRADE_GATEKEEPER_ADDRESS);
         (bool success0, bytes memory newTarget0) = gatekeeper.staticcall(
@@ -203,7 +206,7 @@ contract AdditionalZkSync is Storage, Config, Events, ReentrancyGuard {
     ///         2) After `AUTH_FACT_RESET_TIMELOCK` time is passed second `setAuthPubkeyHash` transaction will reset pubkey hash for `_nonce`.
     /// @param _pubkeyHash New pubkey hash
     /// @param _nonce Nonce of the change pubkey L2 transaction
-    function setAuthPubkeyHash(bytes calldata _pubkeyHash, uint32 _nonce) external {
+    function setAuthPubkeyHash(bytes calldata _pubkeyHash, uint32 _nonce) external nonReentrant {
         requireActive();
 
         require(_pubkeyHash.length == PUBKEY_HASH_BYTES, "y"); // PubKeyHash should be 20 bytes.
@@ -222,7 +225,7 @@ contract AdditionalZkSync is Storage, Config, Events, ReentrancyGuard {
     }
 
     /// @notice Reverts unverified blocks
-    function revertBlocks(StoredBlockInfo[] calldata _blocksToRevert) external {
+    function revertBlocks(StoredBlockInfo[] calldata _blocksToRevert) external nonReentrant {
         requireActive();
 
         governance.requireActiveValidator(msg.sender);

diff --git a/contracts/contracts/Config.sol b/contracts/contracts/Config.sol
@@ -109,4 +109,14 @@ contract Config {
     uint32 internal constant MAX_FUNGIBLE_TOKEN_ID = $$((2**16) - 1);
 
     uint256 internal constant SECURITY_COUNCIL_MEMBERS_NUMBER = $$(SECURITY_COUNCIL_MEMBERS_NUMBER);
+
+    string internal constant name = "ZkSync";
+
+    string internal constant version = "1.0";
+
+    bytes32 internal constant EIP712_DOMAIN_TYPEHASH =
+        keccak256("EIP712Domain(string name,string version,uint256 chainId)");
+
+    bytes32 internal constant EIP712_CHANGEPUBKEY_TYPEHASH =
+        keccak256("ChangePubKey(bytes20 pubKeyHash,uint32 nonce,uint32 accountId)");
 }
diff --git a/contracts/contracts/Events.sol b/contracts/contracts/Events.sol
@@ -15,10 +15,15 @@ interface Events {
     event BlockVerification(uint32 indexed blockNumber);
 
     /// @notice Event emitted when user funds are withdrawn from the zkSync state and contract
-    event Withdrawal(uint16 indexed tokenId, uint128 amount);
+    event Withdrawal(address indexed owner, uint16 indexed tokenId, uint128 amount);
 
     /// @notice Event emitted when user funds are withdrawn from the zkSync state but not from contract
-    event WithdrawalPending(uint16 indexed tokenId, address indexed recepient, uint128 amount);
+    event WithdrawalPending(
+        uint16 indexed tokenId,
+        address indexed recipient,
+        uint128 amount,
+        Operations.WithdrawalType withdrawalType
+    );
 
     /// @notice Event emitted when user NFT is withdrawn from the zkSync state and contract
     event WithdrawalNFT(uint32 indexed tokenId);

diff --git a/contracts/contracts/Governance.sol b/contracts/contracts/Governance.sol
@@ -78,6 +78,7 @@ contract Governance is Config {
     /// @notice Change current governor
     /// @param _newGovernor Address of the new governor
     function changeGovernor(address _newGovernor) external {
+        require(_newGovernor != address(0), "1n");
         requireGovernor(msg.sender);
         if (networkGovernor != _newGovernor) {
             networkGovernor = _newGovernor;

diff --git a/contracts/contracts/Operations.sol b/contracts/contracts/Operations.sol
@@ -27,6 +27,13 @@ library Operations {
         Swap
     }
 
+    /// @notice zkSync withdrawal types
+    enum WithdrawalType {
+        PartialExit,
+        ForcedExit,
+        FullExit
+    }
+
     // Byte lengths
 
     uint8 internal constant OP_TYPE_BYTES = 1;
@@ -191,7 +198,8 @@ library Operations {
         ECRECOVER,
         CREATE2,
         OldECRECOVER,
-        ECRECOVERV2
+        ECRECOVERV2,
+        EIP712
     }
 
     struct ChangePubKey {

diff --git a/contracts/contracts/Storage.sol b/contracts/contracts/Storage.sol
@@ -157,7 +157,7 @@ contract Storage {
 
     /// @dev Timer for authFacts entry reset (address, nonce -> timer).
     /// @dev Used when user wants to reset `authFacts` for some nonce.
-    mapping(address => mapping(uint32 => uint256)) internal authFactsResetTimer;
+    mapping(address => mapping(uint32 => uint256)) public authFactsResetTimer;
 
     mapping(uint32 => address) internal withdrawnNFTs;
 

diff --git a/contracts/contracts/Utils.sol b/contracts/contracts/Utils.sol
@@ -62,4 +62,12 @@ library Utils {
     function hashBytesToBytes20(bytes memory _bytes) internal pure returns (bytes20) {
         return bytes20(uint160(uint256(keccak256(_bytes))));
     }
+
+    function getChainId() internal pure returns (uint256) {
+        uint256 chainId;
+        assembly {
+            chainId := chainid()
+        }
+        return chainId;
+    }
 }