zkSync Era - lsaudit's results

[1] lastL2BlockTimestamp can be in future related to L1.

File: Executor.sol

File: code/contracts/ethereum/contracts/state-transition/chain-deps/facets/Executor.sol

114:         require(_previousBatchTimestamp < batchTimestamp, "h3");
115: 
116:         uint256 lastL2BlockTimestamp = _packedBatchAndL2BlockTimestamp & PACKED_L2_BLOCK_TIMESTAMP_MASK;
117: 
118:         // All L2 blocks have timestamps within the range of [batchTimestamp, lastL2BlockTimestamp].
119:         // So here we need to only double check that:
120:         // - The timestamp of the batch is not too small.
121:         // - The timestamp of the last L2 block is not too big.
122:         require(block.timestamp - COMMIT_TIMESTAMP_NOT_OLDER <= batchTimestamp, "h1"); // New batch timestamp is too small
123:         require(lastL2BlockTimestamp <= block.timestamp + COMMIT_TIMESTAMP_APPROXIMATION_DELTA, "h2"); // The last L2 block timestamp is too big

_verifyBatchTimestamp() verifies a few things. Firstly (line 114) - it checks if the new timestamp is bigger than the old one. Then (lines 122-123) - it verifies if the timestamp is not older than COMMIT_TIMESTAMP_NOT_OLDER and not bigger than COMMIT_TIMESTAMP_APPROXIMATION_DELTA. However, there's no check which verifies if the timestamp is not in the future. This basically means, that lastL2BlockTimestamp can be in future related to L1. The line responsible for that is: require(lastL2BlockTimestamp <= block.timestamp + COMMIT_TIMESTAMP_APPROXIMATION_DELTA, "h2"). Since this line checks if L2 timestamp is not too big, but it still can be big enough to be bigger than L1 timestamp.

Our recommendation is to implement additional check, which will verify that L2 timestamp is only in the past related to L1 timestamp.

[2] SystemContext.sol defines blockGasLimit as uint256, while EVM defines it as uint64

File: SystemContext.sol

Some inconsistent behavior of the zkSyncVM with EVM was noticed during the code-review process.

EVM defines gas limit as uint64:

https://github.com/ethereum/go-ethereum/blob/master/core/types/tx_legacy.go#L30

    Gas      uint64          // gas limit

while SystemContext.sol defines it as uint256:

File: code/system-contracts/contracts/SystemContext.sol

36:     /// @notice The current block's gasLimit.
37:     uint256 public blockGasLimit = type(uint32).max;

This issue violates the goal of zkSync EVM, which should be compatible with EVM spec.

Our recommendation is to change line 37 from uint256 public blockGasLimit = type(uint32).max; to uint64 public blockGasLimit = type(uint32).max;

[3] Implement EIP-712 phishing prevention for EIP712_DOMAIN_TYPEHASH

File: TransactionHelper.sol

File: code/system-contracts/contracts/libraries/TransactionHelper.sol

82:     bytes32 constant EIP712_DOMAIN_TYPEHASH = keccak256("EIP712Domain(string name,string version,uint256 chainId)");

EIP-712 defines address verifyingContract, which is: the address of the contract that will verify the signature. The user-agent may do contract specific phishing prevention. As described above , verifyingContract in EIP712_DOMAIN_TYPEHASH should be considered as a mechanism protecting from phishing attacks. The current implementation of EIP712_DOMAIN_TYPEHASH, however, does not implement it.

Our recommendation is to change above code to:

bytes32 constant EIP712_DOMAIN_TYPEHASH = keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)");

This change would also require to update the domainSeparator (line 138) to:

        bytes32 domainSeparator = keccak256(
            abi.encode(EIP712_DOMAIN_TYPEHASH, keccak256("zkSync"), keccak256("2"), block.chainid, address(this))
        );

[4] Diamond.sol violates EIP-2535

File: Diamond.sol

According to EIP-2535:

If the action is Replace, update the function selector mapping for each functionSelectors item to the facetAddress. If any of the functionSelectors had a value equal to facetAddress or the selector was unset, revert instead.

The current implementation does not revert when we try to replace a selector's facet with the same facet, but it emits DiamondCut() instead.

File: code/contracts/ethereum/contracts/state-transition/libraries/Diamond.sol

160:             SelectorToFacet memory oldFacet = ds.selectorToFacet[selector];
161:             require(oldFacet.facetAddress != address(0), "L"); // it is impossible to replace the facet with zero address
162: 
163:             _removeOneFunction(oldFacet.facetAddress, selector);
164:             // Add facet to the list of facets if the facet address is a new one
165:             _saveFacetIfNew(_facet);
166:             _addOneFunction(_facet, selector, _isFacetFreezable);

Our recommendation is to add additional require when action is Replace. It should verify that we are not replacing selector's facet with the same facet. E.g., below line of code should fix this issue: require(oldFacet.facetAddress != _facet).

[5] Some transactions might be lost forever when processing by the bootloader

File: bootloader.yul

The bootloader expects that the transactions are in a continuous array, however this exception is nowhere enforced. If - transactions weren't in the continuous array, the loop will break, ignoring the rest transactions. The ignored transaction will be lost forever, instead of being added to the L2 block.

Our recommendation is to revert the transaction (instead of break) when transactions are not in the continuous array. The revert will enforce the assumption that all transactions are in a continuous array - and if they weren't - operation will simply revert, without losing the rest of the transactions.

File: code/system-contracts/bootloader/bootloader.yul

3960:                 let execute := mload(txPtr)
3961: 
3962:                 debugLog("txPtr", txPtr)
3963:                 debugLog("execute", execute)
3964:                 
3965:                 if iszero(execute) {
3966:                     // We expect that all transactions that are executed
3967:                     // are continuous in the array.
3968:                     break
3969:                 }                

At line 3968 - revert the transaction, instead of break.

[6] Governance.sol does not support ERC-721 and ERC-1155

File: Governance.sol

File: code/contracts/ethereum/contracts/governance/Governance.sol

20: contract Governance is IGovernance, Ownable2Step {

The Governance does not support ERC-721 and ERC-1155, since it misses the implementation of onERC721Received(), onERC1155Received(), onERC1155BatchReceived() functions. This basically means, that proposals related to transfers of these tokens might not correctly work.

For the reference - please check Open Zeppelins TimelockController.sol - which implements additional onERC721Received(), onERC1155Received(), onERC1155BatchReceived() functions.

[7] executeInstant() might not execute instantly when predecessor is not yet executed

File: Governance.sol

Function executeInstant() is supposed to execute operations instantly. However, it might revert when its predecessor is not yet finalized. This violates the executeInstant() functionality, since it might not be able to instantly execute some operations.

File: code/contracts/ethereum/contracts/governance/Governance.sol

186:     function executeInstant(Operation calldata _operation) external payable onlySecurityCouncil {
187:         bytes32 id = hashOperation(_operation);
188:         // Check if the predecessor operation is completed.
189:         _checkPredecessorDone(_operation.predecessor);

File: code/contracts/ethereum/contracts/governance/Governance.sol

239:     function _checkPredecessorDone(bytes32 _predecessorId) internal view {
240:         require(_predecessorId == bytes32(0) || isOperationDone(_predecessorId), "Predecessor operation not completed");
241:     }

When isOperationDone(_predecessorId) returns false, _checkPredecessorDone() will revert, thus executeInstant() will also revert. Our recommendation is to remove the _checkPredecessorDone(_operation.predecessor) from the executeInstant().

[8] Setting new verifier does not check if all committed batches are verified

File: BaseZkSyncUpgrade.sol

File: code/contracts/ethereum/contracts/upgrades/BaseZkSyncUpgrade.sol

126:     function _setVerifier(IVerifier _newVerifier) private {
127:         // An upgrade to the verifier must be done carefully to ensure there aren't batches in the committed state
128:         // during the transition. If verifier is upgraded, it will immediately be used to prove all committed batches.
129:         // Batches committed expecting the old verifier will fail. Ensure all commited batches are finalized before the
130:         // verifier is upgraded.
131:         if (_newVerifier == IVerifier(address(0))) {
132:             return;
133:         }
134: 
135:         IVerifier oldVerifier = s.verifier;
136:         s.verifier = _newVerifier;
137:         emit NewVerifier(address(oldVerifier), address(_newVerifier));
138:     }

When new verifier is being set, there's no check if all committed batches are already verified. Since batches are proved in the same order as they were committed - the new validator may stuck.

1. _setVerifier() is being called when there are still some not verified batches
2. New verifier has different logic than the old one, thus it cannot verify the old batched
3. New verifier cannot proceed to verifying different batches, because they are still non-verified batches left
4. New verified has stuck - unless someone reverts the old, non-verified batches.

Our recommendation is to verify that s.totalBatchesVerified is the same as s.totalBatchesCommitted. Only then, it should be possible to set new verifier.

[9] storedBatchHash() might return reverted batches

File: Getters.sol

Function _revertBatches() from Executor.sol can be used to revert batches. However, when we call function storedBatchHash(), it does not take into consideration that some batches might be reverted:

File: code/contracts/ethereum/contracts/state-transition/chain-deps/facets/Getters.sol

122:     function storedBatchHash(uint256 _batchNumber) external view returns (bytes32) { 
123:         return s.storedBatchHashes[_batchNumber];
124:     }

The result returned by storedBatchHash() is incorrect when it's called on reverted batch. It will return that reverted batch is still valid.

Our recommendation is to implement additional check which will verify if _batchNumber is reverted, before returning s.storedBatchHashes[_batchNumber].

[10] increaseMinNonce() may overflow, due to addition in unchecked block

File: NonceHolder.sol

File: code/system-contracts/contracts/NonceHolder.sol

65:     function increaseMinNonce(uint256 _value) public onlySystemCall returns (uint256 oldMinNonce) {
66:         require(_value <= MAXIMAL_MIN_NONCE_INCREMENT, "The value for incrementing the nonce is too high");
67: 
68:         uint256 addressAsKey = uint256(uint160(msg.sender));
69:         uint256 oldRawNonce = rawNonces[addressAsKey];
70: 
71:         unchecked {
72:             rawNonces[addressAsKey] = (oldRawNonce + _value);
73:         }

Every time increaseMinNonce() is called, the unchecked addition (lines 71-73) is being made: rawNonces[addressAsKey] = (oldRawNonce + _value). Please notice, that the only protection from the overflow (line 66): require(_value <= MAXIMAL_MIN_NONCE_INCREMENT, "The value for incrementing the nonce is too high"); is not sufficient. The line 66 basically states, that we cannot increase rawNonces[addressAsKey] more than MAXIMAL_MIN_NONCE_INCREMENT. However, it's still possible to call increaseMinNonce(MAXIMAL_MIN_NONCE_INCREMENT) multiple of times.

It's possible to call increaseMinNonce(MAXIMAL_MIN_NONCE_INCREMENT) multiple of times (each call will fulfill the condition at line 66, thus function won't revert) - but on every call rawNonces[addressAsKey] will be bigger and bigger. E.g. after the first call, rawNonces[addressAsKey] = MAXIMAL_MIN_NONCE_INCREMENT, after the 2nd call: rawNonces[addressAsKey] = 2 * MAXIMAL_MIN_NONCE_INCREMENT, etc. until it finally overflows. Since the addition is being made in the unchecked block, function will silently overflow, instead of reverting - thus at some point it might return incorrect results.

[11] s.l2SystemContractsUpgradeTxHash is not deleted in _revertBatches().

File: Executor.sol

File: code/contracts/ethereum/contracts/state-transition/chain-deps/facets/Executor.sol

492:         if (s.l2SystemContractsUpgradeBatchNumber > _newLastBatch) {
493:             delete s.l2SystemContractsUpgradeBatchNumber;
494:         }

When s.l2SystemContractsUpgradeBatchNumber > _newLastBatch, function should clear both s.l2SystemContractsUpgradeBatchNumber and s.l2SystemContractsUpgradeTxHash. However, as noticed at line 493 - it deletes only s.l2SystemContractsUpgradeBatchNumber. For comparison, please take a look at _executeBatch(), where both values are being reseted:

File: code/contracts/ethereum/contracts/state-transition/chain-deps/facets/Executor.sol

361:         if (batchWhenUpgradeHappened != 0 && batchWhenUpgradeHappened <= newTotalBatchesExecuted) {
362:             delete s.l2SystemContractsUpgradeTxHash;
363:             delete s.l2SystemContractsUpgradeBatchNumber;
364:         }

When function _revertBatches() is being called, both s.l2SystemContractsUpgradeBatchNumber and s.l2SystemContractsUpgradeTxHash should be cleared.

[12] Check return value of staticcall in _getERC20Getters()

File: L1SharedBridge.sol

File: code/contracts/ethereum/contracts/bridge/L1SharedBridge.sol

262:         (, bytes memory data1) = _token.staticcall(abi.encodeCall(IERC20Metadata.name, ()));  
263:         (, bytes memory data2) = _token.staticcall(abi.encodeCall(IERC20Metadata.symbol, ()));
264:         (, bytes memory data3) = _token.staticcall(abi.encodeCall(IERC20Metadata.decimals, ())); 
265:         return abi.encode(data1, data2, data3);

There's no check if staticcall failed or succeeded. This basically means, that even when the staticcall reverts, _getERC20Getters() will treat data as successful response. Our recommendation is to verify the return value of staticcall.

[13] decodeString() in L2StandardERC20.sol won't work with ERC-20 tokens with non-string metadata

File: L2StandardERC20.sol

File: code/contracts/zksync/contracts/bridge/L2StandardERC20.sol

75:         try this.decodeString(nameBytes) returns (string memory nameString) {  
76:             decodedName = nameString;
77:         } catch {
78:             getters.ignoreName = true;
79:         }
80: 
81:         try this.decodeString(symbolBytes) returns (string memory symbolString) {
82:             decodedSymbol = symbolString;
83:         } catch {
84:             getters.ignoreSymbol = true;
85:         }

Some tokens (e.g. MKR) have metadata fields (name / symbol) encoded as bytes32 instead of the string prescribed by the ERC20 specification. Those tokens won't correctly work with decodeString() as it accepts string instead of bytes.

File: code/contracts/zksync/contracts/bridge/L2StandardERC20.sol

178:     function decodeString(bytes memory _input) external pure returns (string memory result) { 
179:         (result) = abi.decode(_input, (string));
180:     }

Consider reimplementing above code, so that it would accept either string or bytes. If you do not plan to support such tokens - explicitly list them as not-supported.

[14] isFacetFreezable() should revert when non-existing faces was provided

File: Getters.sol

File: code/contracts/ethereum/contracts/state-transition/chain-deps/facets/Getters.sol

163:     function isFacetFreezable(address _facet) external view returns (bool isFreezable) {  
164:         Diamond.DiamondStorage storage ds = Diamond.getDiamondStorage();
165: 
166:         // There is no direct way to get whether the facet address is freezable,
167:         // so we get it from one of the selectors that are associated with the facet.
168:         uint256 selectorsArrayLen = ds.facetToSelectors[_facet].selectors.length;
169:         if (selectorsArrayLen != 0) {
170:             bytes4 selector0 = ds.facetToSelectors[_facet].selectors[0];
171:             isFreezable = ds.selectorToFacet[selector0].isFreezable;
172:         }
173:     }

According to the comment section and function's name - isFacetFreezable() should return true - when facet is freezable or false when it is not freezable. Above code-base will, however, return false also for facet which does not exist. This might lead to confusion and potentially breaks future integrations, because non-existing facets are neither freezable, nor non-freezable. When facet is non-existing, it's not possible to establish if it's freezable or not, thus function should revert for that case, instead of returning false.

[15] L1_GAS_PER_PUBDATA_BYTE should not be constant, since it might be changed in the future

File: bootloader.yul

File: code/system-contracts/bootloader/bootloader.yul

101:             /// @dev The number of L1 gas needed to be spent for
102:             /// L1 byte. While a single pubdata byte costs `16` gas, 
103:             /// we demand at least 17 to cover up for the costs of additional
104:             /// hashing of it, etc.
105:             function L1_GAS_PER_PUBDATA_BYTE() -> ret {
106:                 ret := 17
107:             }

The current implementation assumes that the gas cost is constant and will never be changed in the future. This assumption is, however incorrect. There are some proposals (e.g. EIP-4488) which suggests the gas cost reduction. By having L1_GAS_PER_PUBDATA_BYTE() as a constant (returning constant value 17) - it won't be possible to alter this value in the future. Our recommendation is to improve the code flexibility and re-implement this functions so that it could be changed in the future.

[16] DiamondProxy doesn't check for the contract existence before the delegatecall

File: DiamondProxy.sol

File: code/contracts/ethereum/contracts/state-transition/chain-deps/DiamondProxy.sol

27:         Diamond.SelectorToFacet memory facet = diamondStorage.selectorToFacet[msg.sig];
28:         address facetAddress = facet.facetAddress;
29: 
30:         require(facetAddress != address(0), "F"); // Proxy has no facet for this selector
31:         require(!diamondStorage.isFrozen || !facet.isFreezable, "q1"); // Facet is frozen
32: 
33:         assembly {
34:             // The pointer to the free memory slot
35:             let ptr := mload(0x40)
36:             // Copy function signature and arguments from calldata at zero position into memory at pointer position
37:             calldatacopy(ptr, 0, calldatasize())
38:             // Delegatecall method of the implementation contract returns 0 on error
39:             let result := delegatecall(gas(), facetAddress, ptr, calldatasize(), 0, 0)

The current implementation verifies only if facetAddress is not address(0) (line 30). However, there's no contract existence check. When proxy tries to delegate to an address (line 39) which is not a contract - the fallback function will return success, thus the silent failure will occur. It's recommended to add additional check which will be responsible for verification the contract's code before degate-calling it.

[17] Unsafe memory reading in _processL2Logs()

Files: Executor.sol, UnsafeBytes.sol

File: code/contracts/ethereum/contracts/state-transition/chain-deps/facets/Executor.sol

142:         for (uint256 i = 0; i < emittedL2Logs.length; i = i.uncheckedAdd(L2_TO_L1_LOG_SERIALIZE_SIZE)) {
143:             // Extract the values to be compared to/used such as the log sender, key, and value
144:             (address logSender, ) = UnsafeBytes.readAddress(emittedL2Logs, i + L2_LOG_ADDRESS_OFFSET);
145:             (uint256 logKey, ) = UnsafeBytes.readUint256(emittedL2Logs, i + L2_LOG_KEY_OFFSET);
146:             (bytes32 logValue, ) = UnsafeBytes.readBytes32(emittedL2Logs, i + L2_LOG_VALUE_OFFSET);

As demonstrated above, function _processL2Logs() utilizes UnsafeBytes library. The library straightforwardly warns that it might go out of bounds:

File: code/contracts/ethereum/contracts/common/libraries/UnsafeBytes.sol

11:  * @dev WARNING!
12:  * 1) Functions don't check the length of the bytes array, so it can go out of bounds.
13:  * The user of the library must check for bytes length before using any functions from the library!

Function _processL2Logs() does not check for bytes length before using UnsafeBytes functions - even it's explicitly recommended to do that (see line 12).

The loop in _processL2Logs() increments by L2_TO_L1_LOG_SERIALIZE_SIZE, however function does not verify if emittedL2Logs are divisible by L2_TO_L1_LOG_SERIALIZE_SIZE. This basically means, that when emittedL2Logs are not divisible by L2_TO_L1_LOG_SERIALIZE_SIZE, function may try to read any random values from out of bounds array. Our recommendation is to add additional require check, which will verify if emittedL2Logs length is divisible by L2_TO_L1_LOG_SERIALIZE_SIZE. If it's not, function should revert.

Since the attack scenario is rather theoretical - the severity of this issue was evaluated as Low and is included in the QA Report.

[18] Tokens might be minted to address(0), because there's no address(0)-check

File: L2SharedBridge.sol

File: code/contracts/zksync/contracts/bridge/L2SharedBridge.sol

079:     function finalizeDeposit(
080:         address _l1Sender,
081:         address _l2Receiver,
082:         address _l1Token,
083:         uint256 _amount,
084:         bytes calldata _data
085:     ) external payable override {
[...]
104:         IL2StandardToken(expectedL2Token).bridgeMint(_l2Receiver, _amount);
105:         emit FinalizeDeposit(_l1Sender, _l2Receiver, expectedL2Token, _amount);
106:     }

Function finalizeDeposit() does not verify if provided _l2Receiver is not address(0). This basically means that it's possible to call bridgeMint() on address(0), thus mint tokens to address(0).

[19] Proof verification in Executor.sol is performed twice

File: Executor.sol

File: code/contracts/ethereum/contracts/state-transition/chain-deps/facets/Executor.sol

429:         if (_proof.serializedProof.length > 0) {
430:             _verifyProof(proofPublicInput, _proof);
431:         }
432:         // #else
433:         _verifyProof(proofPublicInput, _proof);
434:         // #endif

When _proof.serializedProof.length > 0 function will call _verifyProof(proofPublicInput, _proof) twice - at 430 and 433. Performing the same execution of code twice is redundant and should be removed.

[20] Unsafe downcast in _processL2Logs()

File: Executor.sol

File: code/contracts/ethereum/contracts/state-transition/chain-deps/facets/Executor.sol

145:             (uint256 logKey, ) = UnsafeBytes.readUint256(emittedL2Logs, i + L2_LOG_KEY_OFFSET);
146:             (bytes32 logValue, ) = UnsafeBytes.readBytes32(emittedL2Logs, i + L2_LOG_VALUE_OFFSET);
147: 
148:             // Ensure that the log hasn't been processed already
149:             require(!_checkBit(processedLogs, uint8(logKey)), "kp");

Variable logKey is declared as uint256 and is retrieved from emittedL2Logs at line 145. Then, at line 149 - logKey is downcasted to uint8. Since logKey is received via UnsafeBytes.readUint256() (line 145) - nothing guarantees that its value is lower than type(uint8).max. This means, that downcasting it at line 149 might silently underflow leading to incorrect results.

[21] lengthRoundedByWords() may silently overflow

File: bootloader.yul

File: code/system-contracts/bootloader/bootloader.yul

561:             function lengthRoundedByWords(len) -> ret {
562:                 let neededWords := div(add(len, 31), 32)
563:                 ret := safeMul(neededWords, 32, "xv")
564:             }

When len is greater than type(uint256).max - 30 - line 562 might silently overflow and might start returning 0. This will lead to incorrect results returned by lengthRoundedByWords(). Since function already uses safe math (safeMul() ad line 563) - it's recommended to use safeAdd() at line 562 too.

[22] increaseMinNonce() won't revert when value is 0

File: NonceHolder.sol

File: code/system-contracts/contracts/NonceHolder.sol

65:     function increaseMinNonce(uint256 _value) public onlySystemCall returns (uint256 oldMinNonce) {
66:         require(_value <= MAXIMAL_MIN_NONCE_INCREMENT, "The value for incrementing the nonce is too high");
67: 
68:         uint256 addressAsKey = uint256(uint160(msg.sender));
69:         uint256 oldRawNonce = rawNonces[addressAsKey];
70: 
71:         unchecked {
72:             rawNonces[addressAsKey] = (oldRawNonce + _value);
73:         }
74: 
75:         (, oldMinNonce) = _splitRawNonce(oldRawNonce);
76:     }

Providing 0 as value parameter won't increase the nonce at all. It's recommend to revert when value is 0. Otherwise, it would be possible to call increaseMinNonce(0) every time without any effect

[23] getCodeSize() and getCodeHash() may overflow due to lack of input validation

File: AccountCodeStorage.sol

File: code/system-contracts/contracts/AccountCodeStorage.sol

117:     function getCodeSize(uint256 _input) external view override returns (uint256 codeSize) {
118:         // We consider the account bytecode size of the last 20 bytes of the input, because
119:         // according to the spec "If EXTCODESIZE of A is X, then EXTCODESIZE of A + 2**160 is X".
120:         address account = address(uint160(_input));

File: code/system-contracts/contracts/AccountCodeStorage.sol

89:     function getCodeHash(uint256 _input) external view override returns (bytes32) {  
90:         // We consider the account bytecode hash of the last 20 bytes of the input, because
91:         // according to the spec "If EXTCODEHASH of A is X, then EXTCODEHASH of A + 2**160 is X".
92:         address account = address(uint160(_input));

None of above function verifies if _input is not greater than type(uint160).max. Since both functions perform downcasting uint256 to uint160 - whenever user provided _input is greater than type(uint160).max - the value will overflow. Our recommendation is to add additional require, which would verify that _input never exceeds type(uint160).max.

[24] updateDelay() can be set to 0 - which disables the delay

File: Governance.sol

File: code/contracts/ethereum/contracts/governance/Governance.sol

249:     function updateDelay(uint256 _newDelay) external onlySelf {
250:         emit ChangeMinDelay(minDelay, _newDelay);
251:         minDelay = _newDelay;
252:     }

Ability to set minDelay to 0, basically removes the delay protection. It allows the administrator (onlySelf) to update the delay to 0 - and the - immediately execute transactions.

[25] Incorrect comparison operator in _proveL2LogInclusion()

File: Mailbox.sol

File: code/contracts/ethereum/contracts/state-transition/chain-deps/facets/Mailbox.sol

110:         require(_batchNumber <= s.totalBatchesExecuted, "xx");

Because the maximum _batchNumber which can be verified is s.totalBatchesExecuted - 1, line: require(_batchNumber <= s.totalBatchesExecuted, "xx"); should be changed to: require(_batchNumber < s.totalBatchesExecuted, "xx");

[26] Missing address(0) checks in the constructor

Files: L1ERC20Bridge.sol, Governance.sol

Make sure that user-supplied parameters are not address(0).

File: code/contracts/ethereum/contracts/bridge/L1ERC20Bridge.sol

56:         sharedBridge = _sharedBridge;

File: code/contracts/ethereum/contracts/governance/Governance.sol

46:         securityCouncil = _securityCouncil;

[27] Missing address(0) checks in functions

Files: Governance.sol, SystemContext.sol, ValidatorTimelock.sol

Make sure that user-supplied parameters are not address(0).

It's possible to set new validator to address(0):

File: code/contracts/ethereum/contracts/state-transition/ValidatorTimelock.sol

78:     function addValidator(uint256 _chainId, address _newValidator) external onlyChainAdmin(_chainId) {
79:         if (validators[_chainId][_newValidator]) {
80:             revert AddressAlreadyValidator(_chainId);
81:         }
82:         validators[_chainId][_newValidator] = true;
83:         emit ValidatorAdded(_chainId, _newValidator);
84:     }

It's possible to set origin to address(0).

File: code/system-contracts/contracts/SystemContext.sol

099:     function setTxOrigin(address _newOrigin) external onlyCallFromBootloader {
100:         origin = _newOrigin;
101:     }
102: 

It's possible to set security council to address(0)

File: code/contracts/ethereum/contracts/governance/Governance.sol

256:     function updateSecurityCouncil(address _newSecurityCouncil) external onlySelf {
257:         emit ChangeSecurityCouncil(securityCouncil, _newSecurityCouncil);
258:         securityCouncil = _newSecurityCouncil;
259:     }

[28] Conflict require statements in DiamondProxy.sol may lead to revert

File: DiamondProxy.sol

File: code/contracts/ethereum/contracts/state-transition/chain-deps/DiamondProxy.sol

25:         require(msg.data.length >= 4 || msg.data.length == 0, "Ut");
26:         // Get facet from function selector
27:         Diamond.SelectorToFacet memory facet = diamondStorage.selectorToFacet[msg.sig];
28:         address facetAddress = facet.facetAddress;
29: 
30:         require(facetAddress != address(0), "F"); // Proxy has no facet for this selector

DiamondProxy utilizes fallback() to delegate call to its facets. The first require (line 25) allows to delegate either with msg.data or with empty msg.data (msg.data.length == 0). However, please notice, that when msg.data.length is empty, then diamondStorage.selectorToFacet[msg.sig].facetAddress will be address(0). This constitutes some issue, because the next require - at line 30 - checks facetAddress against address(0). This behavior suggests that two require statements conflict with each other. Whenever msg.data.length == 0, then facet.facetAddress == address(0), thus line 30 will revert.

[29] Incorrect MINIBLOCK_HASHES_TO_STORE constant value in SystemContext.sol

File: SystemContext.sol

File: code/system-contracts/contracts/SystemContext.sol

20:     /// We could either:
21:     /// - Store the latest 256 hashes (and strictly rely that we do not accidentally override the hash of the block 256 blocks ago)
22:     /// - Store the latest 257 blocks' hashes.
23:     uint256 internal constant MINIBLOCK_HASHES_TO_STORE = 257;

Protocol stores the latest 257 blocks' hashes. This is even confirmed by the function names: _getLatest257L2blockHash(). However, please notice, that MINIBLOCK_HASHES_TO_STORE is used as an array's max index:

File: code/system-contracts/contracts/SystemContext.sol

212:     function _setL2BlockHash(uint256 _block, bytes32 _hash) internal {
213:         l2BlockHash[_block % MINIBLOCK_HASHES_TO_STORE] = _hash;
214:     }

Since every array starts from 0, if we want to store 257 blocks' hashes, the MINIBLOCK_HASHES_TO_STORE should be set to 256 (instead of 257). l2BlockHash from 0 to 256 is 257 blocks' hashes.

[30] setGasPrice() can set gas price to 0

File: SystemContext.sol

File: code/system-contracts/contracts/SystemContext.sol

105:     function setGasPrice(uint256 _gasPrice) external onlyCallFromBootloader {
106:         gasPrice = _gasPrice;
107:     }

Function setGasPrice() does not perform any validation of the _gasPrice value. It's possible to set the gas price to 0. Our recommendation is add additional line which won't let setting the gas price to 0: require(_gasPrice > 0, "gas price cannot be 0");.

[31] Function validateBytes() reverts instead of returning 0

File: bootloader.yul

File: code/system-contracts/bootloader/bootloader.yul

3424:                     // If last word contains some unintended bits
3425:                     // return 0
3426:                     if and(lastWord, mask) {
3427:                         assertionError("bad bytes encoding")
3428:                     }

According to the comment at line 3424 - function should return with 0 - instead, because of the assertionError() at line 3427 - it reverts.

[32] Implement some boundary checks for setExecutionDelay()

File: ValidatorTimelock.sol

File: code/contracts/ethereum/contracts/state-transition/ValidatorTimelock.sol

96:     function setExecutionDelay(uint32 _executionDelay) external onlyOwner {
97:         executionDelay = _executionDelay;
98:         emit NewExecutionDelay(_executionDelay);
99:     }

It's possible to set execution delay to extremely large value or very small value (e.g. 0). It's recommended to set some min and max value of executionDelay and perform verification of it. It should not be possible to set executionDelay to too huge (or too small) value.

[33] Clean local variable before using them in inline assembly

File: EfficientCall.sol

Function rawMimicCall() clears variables before calling call in inline assembler:

File: code/system-contracts/contracts/libraries/EfficientCall.sol

204:             // Clearing values before usage in assembly, since Solidity
205:             // doesn't do it by default
206:             _whoToMimic := and(_whoToMimic, cleanupMask)

However, there are some places, where this cleaning is missing. Every local variable should be cleaned before usage in the inline assembly.

There's no cleaning of _address in rawCall(): File: code/system-contracts/contracts/libraries/EfficientCall.sol

134:             address callAddr = RAW_FAR_CALL_BY_REF_CALL_ADDRESS;
135:             assembly {
136:                 success := call(_address, callAddr, 0, 0, 0xFFFF, 0, 0)

File: code/system-contracts/contracts/libraries/EfficientCall.sol

148:             assembly {
149:                 success := call(msgValueSimulator, callAddr, _value, _address, 0xFFFF, forwardMask, 0)
150:             }

In rawMimicCall() only _whoToMimic is cleaned, but _address is not cleaned:

File: code/system-contracts/contracts/libraries/EfficientCall.sol

208:             success := call(_address, callAddr, 0, 0, _whoToMimic, 0, 0)
209:         }

In rawStaticCall() _address is not cleaned:

File: code/system-contracts/contracts/libraries/EfficientCall.sol

163:         assembly {
164:             success := staticcall(_address, callAddr, 0, 0xFFFF, 0, 0)
165:         }

In rawDelegateCall() _address is not cleaned:

File: code/system-contracts/contracts/libraries/EfficientCall.sol

177:         assembly {
178:             success := delegatecall(_address, callAddr, 0, 0xFFFF, 0, 0)
179:         }

[34] Implement time locks for critical operations in Admin.sol

File: Admin.sol

File: code/contracts/ethereum/contracts/state-transition/chain-deps/facets/Admin.sol

133:     function freezeDiamond() external onlyAdminOrStateTransitionManager {
134:         Diamond.DiamondStorage storage diamondStorage = Diamond.getDiamondStorage();
[...]
143:     function unfreezeDiamond() external onlyAdminOrStateTransitionManager {
144:         Diamond.DiamondStorage storage diamondStorage = Diamond.getDiamondStorage();

Functions freezeDiamond() and unfreezeDiamond() misses time lock controls. It's recommended that it should not be possible to execute critical operations instantly, without giving users any time to react for the changes. Freezing and unfreezing Diamond should be considered as critical operations which should be protected by the timelock.

[35] Consider using 2-step procedure to update security council

File: Governance.sol

File: code/contracts/ethereum/contracts/governance/Governance.sol

256:     function updateSecurityCouncil(address _newSecurityCouncil) external onlySelf {
257:         emit ChangeSecurityCouncil(securityCouncil, _newSecurityCouncil);
258:         securityCouncil = _newSecurityCouncil;
259:     }

Use 2-step procedure to update the security council. For the reference, please check that the 2-step procedure is implemented, i.e., in Admin.sol file (see functions setPendingAdmin() and acceptAdmin()).

[36] Remove debugLog() from production code in bootloader.yul

There's no need for bootloader.yul to contain debug logs in the production code.

For the report readability, we provide the grep command which finds every occurrence of debugLog in bootloader.yul - which should be removed.

% grep debugLog -ri . | wc -l
      87

[37] Storage values shouldn't be updated when value is not being changed

Files: ValidatorTimelock.sol, Bridgehub.sol, Admin.sol, Governance.sol, ContractDeployer.sol, StateTransitionManager.sol, SystemContext.sol

It's recommended to add additional require statement, which will verify if the value is really changed, before we will store it in the storage variables E.g., let's consider a function setChainId() from SystemContext.sol. When chainId is 1 and we will call setChainId(1), function will re-store the same value. It's recommended to add additional require which will detect that there's nothing to update:

require(_newChainId != chainId, "Nothing to update");

File: code/contracts/ethereum/contracts/bridgehub/Bridgehub.sol

51:     function setPendingAdmin(address _newPendingAdmin) external onlyOwnerOrAdmin {

File: code/contracts/ethereum/contracts/bridgehub/Bridgehub.sol

108:     function setSharedBridge(address _sharedBridge) external onlyOwner {

File: code/contracts/ethereum/contracts/state-transition/StateTransitionManager.sol

132:     function setValidatorTimelock(address _validatorTimelock) external onlyOwnerOrAdmin {
[...]
137:     function setInitialCutHash(Diamond.DiamondCutData calldata _diamondCut) external onlyOwner {
[...]
142:     function setNewVersionUpgrade(
[...]
152:     function setUpgradeDiamondCut(

File: code/contracts/ethereum/contracts/state-transition/StateTransitionManager.sol

110:     function setPendingAdmin(address _newPendingAdmin) external onlyOwnerOrAdmin {

File: code/contracts/ethereum/contracts/state-transition/ValidatorTimelock.sol

73:     function setStateTransitionManager(IStateTransitionManager _stateTransitionManager) external onlyOwner {

File: code/contracts/ethereum/contracts/state-transition/ValidatorTimelock.sol

96:     function setExecutionDelay(uint32 _executionDelay) external onlyOwner { 

File: code/contracts/ethereum/contracts/state-transition/chain-deps/facets/Admin.sol

79:     function setTokenMultiplier(uint128 _nominator, uint128 _denominator) external onlyAdminOrStateTransitionManager {
[...]
89:     function setValidiumMode(PubdataPricingMode _validiumMode) external onlyAdmin {

File: code/contracts/ethereum/contracts/state-transition/chain-deps/facets/Admin.sol

45:     function setValidator(address _validator, bool _active) external onlyStateTransitionManager {
[...]
51:     function setPorterAvailability(bool _zkPorterIsAvailable) external onlyStateTransitionManager {
[...]
58:     function setPriorityTxMaxGasLimit(uint256 _newPriorityTxMaxGasLimit) external onlyStateTransitionManager { 

File: code/system-contracts/contracts/SystemContext.sol

84:     function setChainId(uint256 _newChainId) external onlyCallFromForceDeployer {

File: code/system-contracts/contracts/SystemContext.sol

099:     function setTxOrigin(address _newOrigin) external onlyCallFromBootloader {
[...]
105:     function setGasPrice(uint256 _gasPrice) external onlyCallFromBootloader {
[...]
112:     function setPubdataInfo(uint256 _gasPerPubdataByte, uint256 _basePubdataSpent) external onlyCallFromBootloader {

File: code/contracts/ethereum/contracts/governance/Governance.sol

249:     function updateDelay(uint256 _newDelay) external onlySelf {
[...]
256:     function updateSecurityCouncil(address _newSecurityCouncil) external onlySelf {

File: code/system-contracts/contracts/ContractDeployer.sol

65:     function updateAccountVersion(AccountAbstractionVersion _version) external onlySystemCall {

File: code/contracts/ethereum/contracts/state-transition/chain-deps/facets/Admin.sol

67:     function changeFeeParams(FeeParams calldata _newFeeParams) external onlyAdminOrStateTransitionManager {

[38] Functions which updates important storage variables should emit events

Files: ValidatorTimelock.sol, StateTransitionManager.sol, Bridgehub.sol

During the code-review process, it was detected that some functions do not emit events when some state variables are being updated. The list of functions which should emit event is listed below:

File: code/contracts/ethereum/contracts/bridgehub/Bridgehub.sol

108:     function setSharedBridge(address _sharedBridge) external onlyOwner {
109:         sharedBridge = IL1SharedBridge(_sharedBridge);
110:     }

File: code/contracts/ethereum/contracts/state-transition/StateTransitionManager.sol

132:     function setValidatorTimelock(address _validatorTimelock) external onlyOwnerOrAdmin {
[...]
137:     function setInitialCutHash(Diamond.DiamondCutData calldata _diamondCut) external onlyOwner {
[...]
142:     function setNewVersionUpgrade(
[...]
152:     function setUpgradeDiamondCut(
[...]

File: code/contracts/ethereum/contracts/state-transition/ValidatorTimelock.sol

73:     function setStateTransitionManager(IStateTransitionManager _stateTransitionManager) external onlyOwner {
74:         stateTransitionManager = _stateTransitionManager;
75:     }

[39] Incorrect revert string in BaseZkSyncUpgradeGenesis.sol

File: BaseZkSyncUpgradeGenesis.sol

File: code/contracts/ethereum/contracts/upgrades/BaseZkSyncUpgradeGenesis.sol

22:         require(
23:             // Genesis Upgrade difference: Note this is the only thing change > to >=
24:             _newProtocolVersion >= previousProtocolVersion,
25:             "New protocol version is not greater than the current one"
26:         );

As the comment section explains, the only difference from code/contracts/ethereum/contracts/upgrades/BaseZkSyncUpgrade.sol is the >= operator. BaseZkSyncUpgrade.sol uses: _newProtocolVersion > previousProtocolVersion. BaseZkSyncUpgradeGenesis uses: _newProtocolVersion >= previousProtocolVersion, thus, as mentioned in the comment section, > has been changed to >=. However, the revert string was not changed and is the same as in BaseZkSyncUpgrade.sol. Since now _newProtocolVersion >= previousProtocolVersion, the revert string should be changed to: "New protocol version is not greater or equal than the current one".

[40] Incorrect description of VM_HOOK_TX_HAS_ENDED() in bootloader.yul

File: bootloader.yul

File: code/system-contracts/bootloader/bootloader.yul

3767:             /// @notice The id of the VM hook that notifies the operator that the transaction's execution has started.
3768:             function VM_HOOK_TX_HAS_ENDED() -> ret { 
3769:                 ret := 4
3770:             }

Function VM_HOOK_TX_HAS_ENDED() - as its name suggests notifies the operator that the transaction's execution has ended (change has started to has ended).

[41] Incorrect comment for create()

File: ContractDeployer.sol

File: code/system-contracts/contracts/ContractDeployer.sol

143:     /// @dev This method also accepts nonce as one of its parameters.
144:     /// It is not used anywhere and it needed simply for the consistency for the compiler
145:     /// Note: this method may be callable only in system mode,
146:     /// that is checked in the `createAccount` by `onlySystemCall` modifier.
147:     function create(
148:         bytes32 _salt,
149:         bytes32 _bytecodeHash,
150:         bytes calldata _input
151:     ) external payable override returns (address) {

Since the parameter accepts salt, it would be better to change @dev NatSpec from also accepts nonce to also accepts salt.

[42] Incorrect NatSpec in _executedBatchIdx()

File: Executor.sol

File: code/contracts/ethereum/contracts/state-transition/chain-deps/facets/Executor.sol

320:     /// @dev _executedBatchIdx is an index in the array of the batches that we want to execute together
321:     function _executeOneBatch(StoredBatchInfo memory _storedBatch, uint256 _executedBatchIdx) internal {

_executedBatchIdx is a parameter passed to the function, thus @dev _executedBatchIdx should be changed to @param _executedBatchIdx

[43] Uncompleted comment in ContractDeployer.sol

File: ContractDeployer.sol

File: code/system-contracts/contracts/ContractDeployer.sol

306:     /// @notice Ensures that the _newAddress and assigns a new contract hash to it

The NatSpec comment seems to miss the word describing _newAddress. Ensures that the _newAddress and assigns should be changed to: Ensures that the _newAddress exists and assigns

[44] Incorrect comment in TransactionHelper.sol

File: TransactionHelper.sol

File: code/system-contracts/contracts/libraries/TransactionHelper.sol

17: /// @dev The type id of legacy transactions.
18: uint8 constant LEGACY_TX_TYPE = 0x0;
19: /// @dev The type id of legacy transactions.
20: uint8 constant EIP_2930_TX_TYPE = 0x01; 

Comment at line 19 seems to be copy-pasted from line 17. Actually, it should be: /// @dev The type id of EIP-2930 transactions., since EIP_2930_TX_TYPE is related to EIP-2930.

[45] Unused constant MAX_MSG_VALUE in Constants.sol

File: Constants.sol

Constant MAX_MSG_VALUE defined in Constants.sol is not used anywhere, thus should be removed:

% grep MAX_MSG_VALUE -ri .
./system-contracts/contracts/Constants.sol:uint256 constant MAX_MSG_VALUE = 2 ** 128 - 1; 

File: code/system-contracts/contracts/Constants.sol

93: /// @dev The maximal msg.value that context can have
94: uint256 constant MAX_MSG_VALUE = 2 ** 128 - 1;

[46] Redundant variables in bootloader.yul should be removed

File: bootloader.yul

File: code/system-contracts/bootloader/bootloader.yul

1419:                 let innerTxDataOffset := add(txDataOffset, 32)  
1420:                 debugLog("Starting validation", 0)
1421: 
1422:                 accountValidateTx(txDataOffset)
1423:                 debugLog("Tx validation complete", 1)
1424:                 
1425:                 ensurePayment(txDataOffset, gasPrice)
1426:                 
1427:                 ret := 1
1428:             }

In function ZKSYNC_NEAR_CALL_validateTx() variable innerTxDataOffset is nowhere used. We can directly call add(txDataOffset, 32), instead of assigning it to innerTxDataOffset.

[47] Typo in variable name in bootloader.yul

File: bootloader.yul

File: code/system-contracts/bootloader/bootloader.yul

2350:                 let innerTxDataOffst := add(txDataOffset, 32)
2351:                 let from := getFrom(innerTxDataOffst)
2352:                 ensureAccount(from)
2353: 
2354:                 // The nonce should be unique for each transaction.
2355:                 let nonce := getNonce(innerTxDataOffst)

innerTxDataOffst should actually be innerTxDataOffset (missing letter e in the variable name).

[48] Remove code related to local testing

Files: DiamondInit.sol, StateTransitionManager.sol

File: code/contracts/ethereum/contracts/state-transition/chain-deps/DiamondInit.sol

49:         // While this does not provide a protection in the production, it is needed for local testing
50:         // Length of the L2Log encoding should not be equal to the length of other L2Logs' tree nodes preimages
51:         assert(L2_TO_L1_LOG_SERIALIZE_SIZE != 2 * 32); 

File: code/contracts/ethereum/contracts/state-transition/StateTransitionManager.sol

104:         // While this does not provide a protection in the production, it is needed for local testing
105:         // Length of the L2Log encoding should not be equal to the length of other L2Logs' tree nodes preimages
106:         assert(L2_TO_L1_LOG_SERIALIZE_SIZE != 2 * 32); 

Remove code related to local testing, which won't work in the production.

[49] Constants should be uppercased

File: ValidatorTimelock.sol

Please notice that this is additional instance which was not detected by the bot. The bot detected AddressAliasHelper.sol only, while this issue is about ValidatorTimelock.sol.

File: code/contracts/ethereum/contracts/state-transition/ValidatorTimelock.sol

26:     string public constant override getName = "ValidatorTimelock";

According to the Solidity style guide, getName should be changed to GET_NAME - because it's a constant.

[50] Incorrect variable name in SystemContext.sol

File: SystemContext.sol

File: code/system-contracts/contracts/SystemContext.sol

450:         (uint128 previousBatchNumber, uint128 previousBatchTimestamp) = getBatchNumberAndTimestamp();

Since getBatchNumberAndTimestamp() returns the current batch number and the current batch timestamp, variable names previousBatchNumber and previousBatchTimestamp may be misleading. Those variables are being updated later - but at the time of getBatchNumberAndTimestamp() execution they still contain the current values.

[51] Function _commintOneBatch should be named differently

File: Executor.sol

File: code/contracts/ethereum/contracts/state-transition/chain-deps/facets/Executor.sol

32:     function _commitOneBatch( 
33:         StoredBatchInfo memory _previousBatch,
34:         CommitBatchInfo calldata _newBatch,
35:         bytes32 _expectedSystemContractUpgradeTxHash
36:     ) internal view returns (StoredBatchInfo memory) {

The name of the function suggests, that it commits one batch - however, this function is a view function, thus it doesn't change the state on the blockchain. This is even confirmed in the function's NatSpec: @notice Does not change storage.

[52] Incorrect NatSpec for _isValidSignature()

File: DefaultAccount.sol

File: code/system-contracts/contracts/DefaultAccount.sol

158:     /// @return EIP1271_SUCCESS_RETURN_VALUE if the signaure is correct. It reverts otherwise.
159:     function _isValidSignature(bytes32 _hash, bytes memory _signature) internal view returns (bool) {

When signature is correct, function returns EIP1271_SUCCESS_RETURN_VALUE. In any other cases, function either return false or reverts. The current NatSpec's comments does not mention that _isValidSIgnature() might return false. The NatSpec should be changed to: @return EIP1271_SUCCESS_RETURN_VALUE if the signaure is correct. It returns false or reverts otherwise

[53] IMailbox.sol does not implement functions from Mailbox.sol

File: Mailbox.sol

File: code/contracts/ethereum/contracts/state-transition/chain-deps/facets/Mailbox.sol

196:     ///  @inheritdoc IMailbox
197:     function requestL2Transaction(

According to the NatSpec, function requestL2Transaction() should be implemented in IMailbox.sol. However, IMailbox.sol does not implement this function, the only one IMailbox.sol implements is finalizeEthWithdrawal(). Our recommendation is to add missing requestL2Transaction() to IMailbox.sol.

[54] Getters.sol missing implementing some getters

File: Getters.sol

File: code/contracts/ethereum/contracts/state-transition/chain-deps/facets/Getters.sol

20: contract GettersFacet is ZkSyncStateTransitionBase, IGetters, ILegacyGetters {

File Getters.sol does not implement function getZkPorterIsAvailable() responsible for getting the zkPorterIsAvailable value. This variable can be set in Admin.sol by calling setPorterAvailability(), thus Getters.sol should implement a getter to expose zkPorterIsAvailable value.

[55] Change the name of events related to Blocks

File: IExecutor.sol

File: code/contracts/ethereum/contracts/state-transition/chain-interfaces/IExecutor.sol

186:     /// @notice Event emitted when a batch is committed
[...]
191:     event BlockCommit(uint256 indexed batchNumber, bytes32 indexed batchHash, bytes32 indexed commitment);
192: 
193:     /// @notice Event emitted when batches are verified
[...]
197:     event BlocksVerification(uint256 indexed previousLastVerifiedBatch, uint256 indexed currentLastVerifiedBatch);
198: 
199:     /// @notice Event emitted when a batch is executed
[...]
204:     event BlockExecution(uint256 indexed batchNumber, bytes32 indexed batchHash, bytes32 indexed commitment);
205: 
206:     /// @notice Event emitted when batches are reverted
[...]
211:     event BlocksRevert(uint256 totalBatchesCommitted, uint256 totalBatchesVerified, uint256 totalBatchesExecuted);

The current implementation uses batches instead of blocks - thus it would increase the code readability to change the names from Block* to Batch*. This is even confirmed in the NatSpec of the events: line 186 - Event emitted when a batch is committed - change event BlockCommit() to even BatchCommit(). line 193 - Event emitted when batches are verified - change event BlocksVerification() to event BatchVerification(). line 199 - Event emitted when a batch is executed - change event BlockExecution() to event BatchExecution(). line 206 - Event emitted when batches are reverted - change event BlocksRevert() to event BatchRevert().

[56] else-block is not required in getOperationState()

File: Governance.sol

File: code/contracts/ethereum/contracts/governance/Governance.sol

105:     function getOperationState(bytes32 _id) public view returns (OperationState) {
106:         uint256 timestamp = timestamps[_id];
107:         if (timestamp == 0) {
108:             return OperationState.Unset;
109:         } else if (timestamp == EXECUTED_PROPOSAL_TIMESTAMP) {
110:             return OperationState.Done;
111:         } else if (timestamp > block.timestamp) {
112:             return OperationState.Waiting;
113:         } else {
114:             return OperationState.Ready;
115:         }
116:     }

The else instruction at line 113 can be removed. When previous conditions won't be fulfilled, function will still return OperationState.Ready. Getting rid of redundant else instruction simplifies the code, thus it's highly recommended.

[57] Do not add a return statement when the function defines a named return variable

File: NonceHolder.sol

Once the return variable has been assigned (or has its default value), there is no need to explicitly return it at the end of the function, since it's returned automatically.

File: code/system-contracts/contracts/NonceHolder.sol

125:     function getDeploymentNonce(address _address) external view returns (uint256 deploymentNonce) {
126:         uint256 addressAsKey = uint256(uint160(_address));
127:         (deploymentNonce, ) = _splitRawNonce(rawNonces[addressAsKey]);
128: 
129:         return deploymentNonce;
130:     }

[58] Use gas instead of ergs

Files: Constants.sol, GasBoundCaller.sol

The current implementation uses gas term, instead of ergs. However, there are some places, where ergs were not updated to gas:

File: code/system-contracts/contracts/GasBoundCaller.sol

16:     /// @notice We assume that no more than `CALL_ENTRY_OVERHEAD` ergs are used for the O(1) operations at the start
17:     /// of execution of the contract, such as abi decoding the parameters, jumping to the correct function, etc.
18:     uint256 constant CALL_ENTRY_OVERHEAD = 800;
19:     /// @notice We assume that no more than `CALL_RETURN_OVERHEAD` ergs are used for the O(1) operations at the end of the execution,

File: code/system-contracts/contracts/Constants.sol

37: /// @dev The number of ergs that need to be spent for a single byte of pubdata regardless of the pubdata price.

[59] Use delete instead of assigning variable to their default values

File: Admin.sol

File: code/contracts/ethereum/contracts/state-transition/chain-deps/facets/Admin.sol

147:         diamondStorage.isFrozen = false;

It's recommended to use delete keyword, when some variable/mapping/struct fields needs to be cleared and assigned to its default value.

[60] ISystemContract.sol should be interface, instead of abstract

File: ISystemContract.sol

File: code/system-contracts/contracts/interfaces/ISystemContract.sol

17: abstract contract ISystemContract {

File ISystemContract.sol is inside interfaces directory - which suggests it should be an interface. Moreover its name ISystemContract.sol also suggests it should be an interface. However, file ISystemContract.sol contains an implementation of abstract contract. Our recommendation is to stick to the Solidity style guide and do not put abstract contracts into intterfaces directory - among other files which implement interfaces only.

[61] Links in the NatSpec point to non-existing websites

File: ReentrancyGuard.sol

File: code/contracts/ethereum/contracts/common/ReentrancyGuard.sol

19:  * to protect against it, check out our blog post
20:  * https://blog.openzeppelin.com/reentrancy-after-istanbul/[Reentrancy After Istanbul].

The link from line 20 points to the non-existing webpage (404 error code). Whenever inserting links to the external websites, make sure that they always point to the current, fresh version of the webpage. The correct link should be: https://blog.openzeppelin.com/reentrancy-after-istanbul.

[62] Implement modifiers instead of repeating the same code twice

File: Address.sol

File: code/system-contracts/contracts/openzeppelin/utils/Address.sol

61:         require(
62:             address(this).balance >= amount,
63:             "Address: insufficient balance"
64:         );

File: code/system-contracts/contracts/openzeppelin/utils/Address.sol

155:         require(
156:             address(this).balance >= value,
157:             "Address: insufficient balance for call"
158:         );

Repeated checks can be easily re-implemented as modifiers.

[63] Simplify expression by using de Morgan's laws

File: DiamondProxy.sol

File: code/contracts/ethereum/contracts/state-transition/chain-deps/DiamondProxy.sol

31:         require(!diamondStorage.isFrozen || !facet.isFreezable, "q1"); // Facet is frozen

According to de Morgan's laws: !A || !B <=> !(A & B), which means it's possible to remove one negation from above expression. This allows to simplify some expressions, e.g., above expression from DiamondProxy.sol at line 31 can be rewritten to:

require(!(diamondStorage.isFrozen && facet.isFreezable), "q1"); // Facet is frozen

[64] Rename variable in setNewBatch()

File: SystemContext.sol

File: code/system-contracts/contracts/SystemContext.sol

458:         // Setting new block number and timestamp
459:         BlockInfo memory newBlockInfo = BlockInfo({number: previousBatchNumber + 1, timestamp: _newTimestamp});

The whole function setNewBatch() names blocks as batches (actually, the whole SystemContext.sol) uses term batch instead of block. Thus it would improve the code readability when we will change the name of the newBlockInfo variable to newBatchInfo.

[65] Use require instead of assert

Files: RLPEncoder.sol, DiamondInit.sol, Executor.sol, DefaultAccount.sol

File: code/system-contracts/contracts/DefaultAccount.sol

222:         assert(msg.sender != BOOTLOADER_FORMAL_ADDRESS);

File: code/system-contracts/contracts/libraries/RLPEncoder.sol

50:         assert(_len != 1);

File: code/contracts/ethereum/contracts/state-transition/chain-deps/DiamondInit.sol

51:         assert(L2_TO_L1_LOG_SERIALIZE_SIZE != 2 * 32); 

File: code/contracts/ethereum/contracts/state-transition/chain-deps/facets/Executor.sol

426:         assert(block.chainid != 1); 

File: code/contracts/ethereum/contracts/state-transition/chain-deps/facets/Executor.sol

426:         assert(block.chainid != 1); 

[66] Use descriptive require strings

Whenever code base uses require(), it should provide a descriptive string - why the revert happens. However, across the whole code-base, there are multiple of require statements with non-descriptive strings, e.g.:

./contracts/ethereum/contracts/state-transition/chain-deps/DiamondProxy.sol: require(msg.data.length >= 4 || msg.data.length == 0, "Ut");
./contracts/ethereum/contracts/state-transition/chain-deps/facets/Executor.sol: require(blobVersionedHash != bytes32(0), "vh");

For the report readability, we provide a simple grep which allows to detect those instances:

% grep "require(" -ri . | grep '"' | tr -s ' ' | grep .sol | grep -E "\".{1,4}\"" | wc -l
     138

[67] Use named mappings

Files: StateTransitionManager.sol, ContractDeployer.sol, ValidatorTimelock.sol

Using named mapping increases the code readability.

File: code/contracts/ethereum/contracts/state-transition/StateTransitionManager.sol

30:     mapping(uint256 => address) public stateTransition;
[...]
48:     mapping(uint256 => bytes32) public upgradeCutHash;

File: code/contracts/ethereum/contracts/state-transition/ValidatorTimelock.sol

47:     mapping(uint256 => LibMap.Uint32Map) internal committedBatchTimestamp;

File: code/system-contracts/contracts/ContractDeployer.sol

26:     mapping(address => AccountInfo) internal accountInfo;

[68] Import specified identifier, instead of the whole file

It's recommended to specify what we're importing: import {Test} from "File.sol" instead of importing the whole file: import "File.sol".

For the report readability, we provide a simple grep which allows to detect affected instances:

% grep "import \"" -ri . | grep ".sol" | grep -v "/test" | wc -l
      61

[69] Missing @NatSpec declarations

Across the whole code-base, it was noticed that multiple of functions missed proper @NatSpec. A lot of functions miss @author, @dev, @notice tags. Make sure that every function has proper @NatSpec.

[70] Remove testing code from the production

File: SystemContext.sol

File: code/system-contracts/contracts/SystemContext.sol

469:     /// @notice A testing method that manually sets the current blocks' number and timestamp.
470:     /// @dev Should be used only for testing / ethCalls and should never be used in production.
471:     function unsafeOverrideBatch(
472:         uint256 _newTimestamp,
473:         uint256 _number,
474:         uint256 _baseFee
475:     ) external onlyCallFromBootloader {
476:         BlockInfo memory newBlockInfo = BlockInfo({number: uint128(_number), timestamp: uint128(_newTimestamp)});
477:         currentBatchInfo = newBlockInfo;
478: 
479:         baseFee = _baseFee;
480:     }

As both NatSpec comment and the code-base suggests, above code is related to the testing environment. This code allows to manually set the blocks's number and timestamp. It's recommended to remove any testing-related code from the production. The severity of this issue was estimated as Informative only, since this method implements onlyCallFromBootloader - thus it's not possible to call it directly. Nonetheless, having testing code on the production is considered as bad practice.

[71] Use descriptive comments when packing/unpacking integers

File: SystemContractsCaller.sol

Packing/unpacking integers may be very vague to the reader. It's recommended to provide a detailed description of each of the unpacked values. Our recommendation is to add detailed comment, which will describe how exactly the farCallAbi and farCallAbiWithEmptyFarPtr is being packed.

File: code/contracts/zksync/contracts/SystemContractsCaller.sol

115:         farCallAbi |= dataOffset;
116:         farCallAbi |= (uint256(memoryPage) << 32);
117:         farCallAbi |= (uint256(dataStart) << 64);
118:         farCallAbi |= (uint256(dataLength) << 96);

File: code/contracts/zksync/contracts/SystemContractsCaller.sol

128:         farCallAbiWithEmptyFatPtr |= (uint256(gasPassed) << 192);
129:         farCallAbiWithEmptyFatPtr |= (uint256(forwardingMode) << 224);
130:         farCallAbiWithEmptyFatPtr |= (uint256(shardId) << 232);
131:         if (isConstructorCall) {
132:             farCallAbiWithEmptyFatPtr |= (1 << 240);
133:         }
134:         if (isSystemCall) {
135:             farCallAbiWithEmptyFatPtr |= (1 << 248);

[72] Sort contract addresses based on the offset

File: L2ContractHelper.sol

File: code/contracts/zksync/contracts/L2ContractHelper.sol

70: address constant BOOTLOADER_ADDRESS = address(SYSTEM_CONTRACTS_OFFSET + 0x01);
71: address constant MSG_VALUE_SYSTEM_CONTRACT = address(SYSTEM_CONTRACTS_OFFSET + 0x09);
72: address constant DEPLOYER_SYSTEM_CONTRACT = address(SYSTEM_CONTRACTS_OFFSET + 0x06);
73: 
74: IL2Messenger constant L2_MESSENGER = IL2Messenger(address(SYSTEM_CONTRACTS_OFFSET + 0x08));
75: 
76: IBaseToken constant L2_BASE_TOKEN_ADDRESS = IBaseToken(address(SYSTEM_CONTRACTS_OFFSET + 0x0a));

To increase the readability of the code, it will be much better to sort the addresses based on the offset:

address constant BOOTLOADER_ADDRESS = address(SYSTEM_CONTRACTS_OFFSET + 0x01);
address constant DEPLOYER_SYSTEM_CONTRACT = address(SYSTEM_CONTRACTS_OFFSET + 0x06);
IL2Messenger constant L2_MESSENGER = IL2Messenger(address(SYSTEM_CONTRACTS_OFFSET + 0x08));
address constant MSG_VALUE_SYSTEM_CONTRACT = address(SYSTEM_CONTRACTS_OFFSET + 0x09);
IBaseToken constant L2_BASE_TOKEN_ADDRESS = IBaseToken(address(SYSTEM_CONTRACTS_OFFSET + 0x0a));

[73] Keep proper structure in IBridgehub.sol

File: IBridgehub.sol

File: code/contracts/ethereum/contracts/bridgehub/IBridgehub.sol

45:     event NewPendingAdmin(address indexed oldPendingAdmin, address indexed newPendingAdmin);
46: 
47:     /// @notice Admin changed
48:     event NewAdmin(address indexed oldAdmin, address indexed newAdmin);

File: code/contracts/ethereum/contracts/bridgehub/IBridgehub.sol

131:     function setSharedBridge(address _sharedBridge) external;
132: 
133:     event NewChain(uint256 indexed chainId, address stateTransitionManager, address indexed chainGovernance);

As demonstrated above, interface firstly lists all events, then all functions. The only exception is the NewChain event which is listed as the last one in the interface. Our recommendation is to always group functions and events. Please consider moving NewChain event from line 133 above (to lines 45-48) - where other events are.

[74] Remove todo code

File: IZkSyncStateTransition.sol

File: code/contracts/ethereum/contracts/state-transition/chain-interfaces/IZkSyncStateTransition.sol

12: // kl to do remove this, needed for the server for now
13: import "../libraries/Diamond.sol";
14: 
15: interface IZkSyncStateTransition is IAdmin, IExecutor, IGetters, IMailbox {
16:     // KL todo: need this in the server for now

[75] Typos

Files: SystemContractHelper.sol, DefaultAccount.sol, IPaymasterFlow.sol

File: code/system-contracts/contracts/libraries/SystemContractHelper.sol

242:     /// of the auxilary heap in bytes.
243:     /// @param meta Packed representation of the ZkSyncMeta.
244:     /// @return auxHeapSize The size of the auxilary memory in bytes byte.
245:     /// @dev You can read more on auxilary memory in the VM1.2 documentation.

auxilary should be changed to auxiliary.

File: code/contracts/zksync/contracts/interfaces/IPaymasterFlow.sol

10:  * @notice This is NOT an interface to be implementated

implementated should be change to implemented.

File: code/system-contracts/contracts/DefaultAccount.sol

158:     /// @return EIP1271_SUCCESS_RETURN_VALUE if the signaure is correct. It reverts otherwise.

signaure should be changed to signature.

[76] Incorrect grammar

Files: L2BaseToken.sol, DefaultAccount.sol, ISystemContext.sol

File: code/system-contracts/contracts/interfaces/ISystemContext.sol

18:     /// @dev It will used for the L1 batch -> L2 block migration in Q3 2023 only.

It will used should be changed to It will be used.

File: code/system-contracts/contracts/DefaultAccount.sol

96:         // The fact there is are enough balance for the account

there is are enough should be changed to there is enough

File: code/system-contracts/contracts/L2BaseToken.sol

104:             // This is safe, since this contract holds the ether balances, and if user
105:             // send a `msg.value` it will be added to the contract (`this`) balance.

if user send should be changed to if user sends.

To improve the report readability and its transparency - the report is divided into two sections. The first section contains issues evaluated during the manual process of code-review. It focuses on two types of findings:

• Full optimizations - e.g., "Function X() can be optimized" - which describes multiple of optimizations related to a single feature. Each issue requires some code refactoring.
• Issue related optimization - e.g., repacking of structs, changing orders of require statements, etc. Each of these issue required a manual review, which allowed us to confirm that the optimization would indeed save some gas. The manual process allowed us to provide a short explanation, why the proposed solution will save gas.

The second section - is related to automated findings. These findings - will - most likely - be ignored and their proposed fixes not implemented, because they significantly affect the code readability and provide just a minimal gas savings. These findings describe issues like: using assembly for simple operations, marking constructors as payable, etc. Nonetheless, we've decide to contain them in the report for the completeness. Since during the automated process - we have been using own tooling, we've decide to simplify some of its detectors and re-write them as a simple grep commands. To improve the report quality and readability, we've decide to not list the exact instances found during the automated process - but instead, we've provided a simple grep instruction, which allows to identified those issue by the reader. Finding marked as [AUTOMATED-FINDING-N] describes how to use grep to identify most of the instances related to each finding. Please notice, that those grep instructions won't identify every affected instance - but in most cases - they will report enough results to understand the crux of the issue. In the automated findings, we've piped our grep detectors by | wc -l, just to demonstrate how many instances are affected. To list exact lines affected, please remove it from the command.

[1] Function _revertBatches() can be optimized

File: Executor.sol

This issue contains multiple of optimizations which requires code refactoring. Thus it was reported as separated finding.

File: code/contracts/ethereum/contracts/state-transition/chain-deps/facets/Executor.sol

481:     function _revertBatches(uint256 _newLastBatch) internal {
482:         require(s.totalBatchesCommitted > _newLastBatch, "v1"); // The last committed batch is less than new last batch
483:         require(_newLastBatch >= s.totalBatchesExecuted, "v2"); // Already executed batches cannot be reverted
484: 
485:         if (_newLastBatch < s.totalBatchesVerified) {
486:             s.totalBatchesVerified = _newLastBatch;
487:         }
488:         s.totalBatchesCommitted = _newLastBatch;
489: 
490:         // Reset the batch number of the executed system contracts upgrade transaction if the batch
491:         // where the system contracts upgrade was committed is among the reverted batches.
492:         if (s.l2SystemContractsUpgradeBatchNumber > _newLastBatch) {
493:             delete s.l2SystemContractsUpgradeBatchNumber;
494:         }
495: 
496:         emit BlocksRevert(s.totalBatchesCommitted, s.totalBatchesVerified, s.totalBatchesExecuted);

At line 488, we assign _newLastBatch to state variable s.totalBatchesCommitted. Since this value is not being changed, we can re-use _newLastBatch at line 496, to avoid additional reading of a state variable: emit BlocksRevert(_newLastBatch, s.totalBatchesVerified, s.totalBatchesExecuted);.

Moreover, s.totalBatchesExecuted is used twice and can be cached.

At line 485, when the condition if fulfilled, we will be reading s.totalBatchesVerified three times (line 485, 486, 487). We can re-factor the code to avoid additional reading of s.totalBatchesVerified. This is what needs to be done:

1. move s.totalBatchesCommitted = _newLastBatch higher
2. move if (s.l2SystemContractsUpgradeBatchNumber > _newLastBatch) higher
3. rewrite if (_newLastBatch < s.totalBatchesVerified) to if-else condition, when if block will be executed, we will avoid additional reading. Please notice that in that case, we will read s.totalBatchesVerified only twice (explained in the comment section)

   function _revertBatches(uint256 _newLastBatch) internal {
        require(s.totalBatchesCommitted > _newLastBatch, "v1"); 
        uint256 cachedtotalBatchesExecuted = s.totalBatchesExecuted;
        require(_newLastBatch >= cachedtotalBatchesExecuted, "v2"); 

        s.totalBatchesCommitted = _newLastBatch;  // line 488 can be moved higher

        if (s.l2SystemContractsUpgradeBatchNumber > _newLastBatch) { // line 497 can be moved higher
            delete s.l2SystemContractsUpgradeBatchNumber;
        }

        if (_newLastBatch < s.totalBatchesVerified) { // first read of `s.totalBatchesVerified`
            s.totalBatchesVerified = _newLastBatch;  // when if returns true, 2nd read of `s.totalBatchesVerified`
            emit BlocksRevert(_newLastBatch, _newLastBatch, cachedtotalBatchesExecuted);  // we can use _newLastBatch now
        } else  {
            emit BlocksRevert(_newLastBatch, s.totalBatchesVerified, cachedtotalBatchesExecuted); // when if returns false, 2nd read of `s.totalBatchesVerified`
        }
 
    }

[2] Function setNewBatch() from SystemContext.sol can be optimized

File: SystemContext.sol This issue requires code-refactoring of some code, thus it was reported separately.

File: code/system-contracts/contracts/SystemContext.sol

452:         require(previousBatchNumber + 1 == _expectedNewNumber, "The provided block number is not correct");
453: 
454:         _ensureBatchConsistentWithL2Block(_newTimestamp);
455: 
456:         batchHashes[previousBatchNumber] = _prevBatchHash;
457: 
458:         // Setting new block number and timestamp
459:         BlockInfo memory newBlockInfo = BlockInfo({number: previousBatchNumber + 1, timestamp: _newTimestamp});

There's unnecessary addition, which can be removed from above function. Let's take a look at lines 452 and 459: previousBatchNumber + 1. Since line 452 requires, that previousBatchNumber + 1 == _expectedNewNumber (otherwise, function will revert), we can be sure, that _expectedNewNumber == previousBatchNumber + 1. This implies, that at line 459, instead of using addition again, we can simply use _expectedNewNumber instead:

459:         BlockInfo memory newBlockInfo = BlockInfo({number: _expectedNewNumber, timestamp: _newTimestamp});

This will save us from spending additional gas on unnecessary operation.

[3] Function initialize() in L2SharedBridge.sol can be optimized

File: L2SharedBridge.sol

This issue requires code-refactoring of some code, thus it was reported separately.

File: code/contracts/zksync/contracts/bridge/L2SharedBridge.sol

60:         l1Bridge = _l1Bridge;
61:         l2TokenProxyBytecodeHash = _l2TokenProxyBytecodeHash;
62: 
63:         if (block.chainid != ERA_CHAIN_ID) {
64:             address l2StandardToken = address(new L2StandardERC20{salt: bytes32(0)}());
65:             l2TokenBeacon = new UpgradeableBeacon{salt: bytes32(0)}(l2StandardToken);
66:             l2TokenBeacon.transferOwnership(_aliasedOwner);
67:         } else {
68:             require(_l1LegecyBridge != address(0), "bf2");
69:             // l2StandardToken and l2TokenBeacon are already deployed on ERA, and stored in the proxy
70:         }

When block.chainid is ERA_CHAIN_ID and _l1LegecyBridge is address(0) - we can revert earlier, without wasting gas on reading state variables: l1Bridge and l2TokenProxyBytecodeHash. Moreover, we can use == operator, instead of !=.

        if (block.chainid == ERA_CHAIN_ID) {
        require(_l1LegecyBridge != address(0), "bf2");
        } else {
            address l2StandardToken = address(new L2StandardERC20{salt: bytes32(0)}());
            l2TokenBeacon = new UpgradeableBeacon{salt: bytes32(0)}(l2StandardToken);
            l2TokenBeacon.transferOwnership(_aliasedOwner);
        }
        l1Bridge = _l1Bridge;
        l2TokenProxyBytecodeHash = _l2TokenProxyBytecodeHash;

[4] Refactor Diamond.sol by inlining _saveFacetIfNew() function

File: Diamond.sol

This issue is reported as separate finding, since it requires multiple of changes in the code base. Our recommendation is to inline _saveFacetIfNew() to avoid additional SSTORE performed by DiamondStorage storage ds = getDiamondStorage().

File: code/contracts/ethereum/contracts/state-transition/libraries/Diamond.sol

189:     function _saveFacetIfNew(address _facet) private {
190:         DiamondStorage storage ds = getDiamondStorage();
191: 
192:         uint256 selectorsLength = ds.facetToSelectors[_facet].selectors.length;
193:         // If there are no selectors associated with facet then save facet as new one
194:         if (selectorsLength == 0) {
195:             ds.facetToSelectors[_facet].facetPosition = ds.facets.length.toUint16();
196:             ds.facets.push(_facet);
197:         }
198:     }

Function _saveFacetIfNew() executes DiamondStorage storage ds = getDiamondStorage(); (line 189). We can spot, that function _saveFacetIfNew() is called inside _addFunctions() and _replaceFunctions(). Those functions - at the beginning of their implementation perform the same operation: DiamondStorage storage ds = getDiamondStorage();. This basically means, that DiamondStorage storage ds = getDiamondStorage(); is redundant in the _saveFacetIfNew(). We can inline _saveFacetIfNew() in _addFunctions() and _replaceFunctions() to avoid repeating DiamondStorage storage ds = getDiamondStorage();.

[5] Alter some struct's fields datatype to improve packing

Files: Messaging.sol, BaseZkSyncUpgrade.sol, IExecutor.sol

File: code/contracts/ethereum/contracts/upgrades/BaseZkSyncUpgrade.sol

28: struct ProposedUpgrade {
29:     L2CanonicalTransaction l2ProtocolUpgradeTx;
30:     bytes[] factoryDeps;
31:     bytes32 bootloaderHash;
32:     bytes32 defaultAccountHash;
33:     address verifier;
34:     VerifierParams verifierParams;
35:     bytes l1ContractsUpgradeCalldata;
36:     bytes postUpgradeCalldata;
37:     uint256 upgradeTimestamp;
38:     uint256 newProtocolVersion;
39: }

newProtocolVersion is being increases every time upgrade is made. upgradeTimestamp contains the timestamp of the upgrade. Both these fields can be uint128 (instead of uint256) to fit into one slot.

File: code/contracts/ethereum/contracts/common/Messaging.sol

098: struct L2CanonicalTransaction {
099:     uint256 txType;
100:     uint256 from;
101:     uint256 to;
102:     uint256 gasLimit;
103:     uint256 gasPerPubdataByteLimit;
104:     uint256 maxFeePerGas;
105:     uint256 maxPriorityFeePerGas;
106:     uint256 paymaster;
107:     uint256 nonce;
108:     uint256 value;

Both nonce and txType fields can be uint128 to fit into single slot. Both gasLimit and gasPerPubdataByteLimit can be uint128 to fit into single slot.

File: code/contracts/ethereum/contracts/state-transition/chain-interfaces/IExecutor.sol

83:     struct StoredBatchInfo {
84:         uint64 batchNumber;
85:         bytes32 batchHash;
86:         uint64 indexRepeatedStorageChanges; 
87:         uint256 numberOfLayer1Txs;
88:         bytes32 priorityOperationsHash;
89:         bytes32 l2LogsTreeRoot;
90:         uint256 timestamp;
91:         bytes32 commitment;
92:     }

timestamp field can be uint64. Then, it can be moved after batchNumber to fit a single slot (uint64 + uint64).

[6] State variables may be packed into fewer storage slots

File: Governance.sol

File: code/contracts/ethereum/contracts/governance/Governance.sol

26:     address public securityCouncil;
27: 
28:     /// @notice A mapping to store timestamps when each operation will be ready for execution.
29:     /// @dev - 0 means the operation is not created.
30:     /// @dev - 1 (EXECUTED_PROPOSAL_TIMESTAMP) means the operation is already executed.
31:     /// @dev - any other value means timestamp in seconds when the operation will be ready for execution.
32:     mapping(bytes32 operationId => uint256 executionTimestamp) public timestamps;
33: 
34:     /// @notice The minimum delay in seconds for operations to be ready for execution.
35:     uint256 public minDelay;

Consider changing minDelay type from uint256 to uint64. Then, it could be packed in a single storage slot with address public securityCouncil.

[7] In NonceHolder.sol, ID mappings can be combined into a single mapping of an ID to a struct

File: NonceHolder.sol

File: code/system-contracts/contracts/NonceHolder.sol

36:     mapping(uint256 account => uint256 packedMinAndDeploymentNonce) internal rawNonces;
37: 
38:     /// Mapping of values under nonces for accounts.
39:     /// The main key of the mapping is the 256-bit address of the account, while the
40:     /// inner mapping is a mapping from a nonce to the value stored there.
41:     mapping(uint256 account => mapping(uint256 nonceKey => uint256 value)) internal nonceValues;

[8] Repack struct by putting data types that fit together into one slot next to each other

This issue, in comparison to # [5] Alter some struct's fields datatype to improve packing is related to changing the order of fields in the struct (while [5] suggests changing the datatype of fields) - thus it's being reported separately.

File: IExecutor.sol

File: code/contracts/ethereum/contracts/state-transition/chain-interfaces/IExecutor.sol

83:     struct StoredBatchInfo {
84:         uint64 batchNumber;
85:         bytes32 batchHash;
86:         uint64 indexRepeatedStorageChanges; 
87:         uint256 numberOfLayer1Txs;
88:         bytes32 priorityOperationsHash;
89:         bytes32 l2LogsTreeRoot;
90:         uint256 timestamp;
91:         bytes32 commitment;
92:     }

batchNumber and indexRepeatedStorageChanges will fit into one slot.

[9] Avoid reading state variable twice

Files: SystemContext.sol, L2StandardERC20.sol

Reading state variables costs a lot of gas. It's better to always cache the state variable into local variable and read the local variable instead.

File: code/contracts/zksync/contracts/bridge/L2StandardERC20.sol

093:         try this.decodeUint8(decimalsBytes) returns (uint8 decimalsUint8) {
094:             // Set decoded value for decimals.
095:             decimals_ = decimalsUint8;
096:         } catch {
097:             getters.ignoreDecimals = true;
098:         }
099: 
100:         availableGetters = getters;
101:         emit BridgeInitialize(_l1Address, decodedName, decodedSymbol, decimals_);

Variable decimals_ is a state variable, thus reading it costs more gas than local variable. Please notice, that we're reading this variable twice - firstly, at line 95, then at line 101.

Reading state variables multiple of times costs a lot of gas. Much more effective solution would be to just read it once and cache the result into local variable.

File: code/system-contracts/contracts/SystemContext.sol

117:     function getCurrentPubdataSpent() public view returns (uint256) {
118:         uint256 pubdataPublished = SystemContractHelper.getZkSyncMeta().pubdataPublished;
119:         return pubdataPublished > basePubdataSpent ? pubdataPublished - basePubdataSpent : 0;
120:     }

To avoid reading basePubdataSpent twice, above code can be rewritten to:

    function getCurrentPubdataSpent() public view returns (uint256) {
        uint256 cachedBasePubdataSpent = basePubdataSpent;
        uint256 pubdataPublished = SystemContractHelper.getZkSyncMeta().pubdataPublished;
        return pubdataPublished > cachedBasePubdataSpent ? pubdataPublished - cachedBasePubdataSpent : 0;
    }

[10] Refactor functions with events which emit both new and old values

Files: StateTransitionManager.sol, Admin.sol

File: code/contracts/ethereum/contracts/state-transition/chain-deps/facets/Admin.sol

23:     function setPendingAdmin(address _newPendingAdmin) external onlyAdmin {
24:         // Save previous value into the stack to put it into the event later
25:         address oldPendingAdmin = s.pendingAdmin;
26:         // Change pending admin
27:         s.pendingAdmin = _newPendingAdmin;
28:         emit NewPendingAdmin(oldPendingAdmin, _newPendingAdmin);
29:     }

To avoid creating additional variable oldPendingAdmin which stores s.pendingAdmin before the update - we can move emitting event one line above:

   function setPendingAdmin(address _newPendingAdmin) external onlyAdmin {
        emit NewPendingAdmin(s.pendingAdmin, _newPendingAdmin);
        s.pendingAdmin = _newPendingAdmin;  
    }

The same issue was observed in other instances:

File: code/contracts/ethereum/contracts/state-transition/StateTransitionManager.sol

110:     function setPendingAdmin(address _newPendingAdmin) external onlyOwnerOrAdmin {
111:         // Save previous value into the stack to put it into the event later
112:         address oldPendingAdmin = pendingAdmin;
113:         // Change pending admin
114:         pendingAdmin = _newPendingAdmin;
115:         emit NewPendingAdmin(oldPendingAdmin, _newPendingAdmin);
116:     }

Should be changed as above.

File: code/contracts/ethereum/contracts/state-transition/chain-deps/facets/Admin.sol

58:     function setPriorityTxMaxGasLimit(uint256 _newPriorityTxMaxGasLimit) external onlyStateTransitionManager { 
59:         require(_newPriorityTxMaxGasLimit <= MAX_GAS_PER_TRANSACTION, "n5");
60: 
61:         uint256 oldPriorityTxMaxGasLimit = s.priorityTxMaxGasLimit;
62:         s.priorityTxMaxGasLimit = _newPriorityTxMaxGasLimit;
63:         emit NewPriorityTxMaxGasLimit(oldPriorityTxMaxGasLimit, _newPriorityTxMaxGasLimit);
64:     }

can be changed to:

    function setPriorityTxMaxGasLimit(uint256 _newPriorityTxMaxGasLimit) external onlyStateTransitionManager { 
        require(_newPriorityTxMaxGasLimit <= MAX_GAS_PER_TRANSACTION, "n5");
        emit NewPriorityTxMaxGasLimit(s.priorityTxMaxGasLimit, _newPriorityTxMaxGasLimit);
        s.priorityTxMaxGasLimit = _newPriorityTxMaxGasLimit;
        
    }

[11] Do not calculate the length of array multiple of times

Files: L2ContractHelper.sol, Compressor.sol, BaseZkSyncUpgrade.sol, PubdataChunkPublisher.sol, Executor.sol, DiamondProxy.sol

While calculating the length of an array costs a lot of gas - it's recommended to calculate it once and then, cache the result inside the local variable. Please notice that this is NOT the same issue as bot-report mentions. The bot-report contains instances of calculating array's length in the loop iterator, e.g.: for (uint i=0; i < array.length; i++), while this issue reports that the length of the array is calculated within the loop (line 42).

File: code/system-contracts/contracts/PubdataChunkPublisher.sol

36:         for (uint256 i = 0; i < MAX_NUMBER_OF_BLOBS; i++) {
37:             uint256 start = BLOB_SIZE_BYTES * i;
38: 
39:             // We break if the pubdata isn't enough to cover 2 blobs. On L1 it is expected that the hash
40:             // will be bytes32(0) if a blob isn't going to be used.
41:             if (start >= _pubdata.length) {
42:                 break;
43:             }

In function chunkAndPublishPubdata(), the length of array is calculated on every loop iteration.

File: code/system-contracts/contracts/Compressor.sol

52:                 encodedData.length * 4 == _bytecode.length,
53:                 "Encoded data length should be 4 times shorter than the original bytecode"
54:             );
55: 
56:             require(
57:                 dictionary.length / 8 <= encodedData.length / 2,
58:                 "Dictionary should have at most the same number of entries as the encoded data"
59:             );
60: 
61:             for (uint256 encodedDataPointer = 0; encodedDataPointer < encodedData.length; encodedDataPointer += 2) {
62:                 uint256 indexOfEncodedChunk = uint256(encodedData.readUint16(encodedDataPointer)) * 8;
63:                 require(indexOfEncodedChunk < dictionary.length, "Encoded chunk index is out of bounds");

encodedData.length is calculated 3 times (lines 52, 57, 61). Calculate it once and cache its value to local variable. dictionary.length is calculated 2 times (line 57, 63). Calculate it once and cache its value to local variable.

File: code/contracts/ethereum/contracts/common/libraries/L2ContractHelper.sol

23:         require(_bytecode.length % 32 == 0, "pq");
24: 
25:         uint256 bytecodeLenInWords = _bytecode.length / 32;

_bytecode.length is calculated twice, it would be more efficient to calculate it once and cache the result before line 23.

File: code/contracts/ethereum/contracts/upgrades/BaseZkSyncUpgrade.sol

223:         require(_factoryDeps.length == _expectedHashes.length, "Wrong number of factory deps");
224:         require(_factoryDeps.length <= MAX_NEW_FACTORY_DEPS, "Factory deps can be at most 32");
225: 
226:         for (uint256 i = 0; i < _factoryDeps.length; ++i) {

_factoryDeps.length is calculated 3 times (lines 223, 224, 226). Calculate it once and cache the result into local variable before line 223.

File: code/contracts/ethereum/contracts/state-transition/chain-deps/DiamondProxy.sol

25:         require(msg.data.length >= 4 || msg.data.length == 0, "Ut");

msg.data.length is calculated twice. Calculate it once and cache the result into local variable.

File: code/contracts/ethereum/contracts/state-transition/chain-deps/facets/Executor.sol

631:         require(_pubdataCommitments.length > 0, "pl");
632:         require(_pubdataCommitments.length <= PUBDATA_COMMITMENT_SIZE * MAX_NUMBER_OF_BLOBS, "bd");
633:         require(_pubdataCommitments.length % PUBDATA_COMMITMENT_SIZE == 0, "bs");
634:         blobCommitments = new bytes32[](MAX_NUMBER_OF_BLOBS);
635: 
636:         for (uint256 i = 0; i < _pubdataCommitments.length; i += PUBDATA_COMMITMENT_SIZE) {

_pubdataCommitments.length is calculated 4 times (lines 631, 632, 633m 636). Calculate it once and cache the result into local variable.

[12] Proof verification in Executor.sol is performed twice

File: Executor.sol

File: code/contracts/ethereum/contracts/state-transition/chain-deps/facets/Executor.sol

429:         if (_proof.serializedProof.length > 0) {
430:             _verifyProof(proofPublicInput, _proof);
431:         }
432:         // #else
433:         _verifyProof(proofPublicInput, _proof);
434:         // #endif

When _proof.serializedProof.length > 0 function will call _verifyProof(proofPublicInput, _proof) twice - at line 430 and at line 433.

[13] Divide loop in _commitBatchesWithSystemContractsUpgrade() to avoid redundant if

File: Executor.sol

File: code/contracts/ethereum/contracts/state-transition/chain-deps/facets/Executor.sol

289:         for (uint256 i = 0; i < _newBatchesData.length; i = i.uncheckedInc()) {
290:             // The upgrade transaction must only be included in the first batch.
291:             bytes32 expectedUpgradeTxHash = i == 0 ? _systemContractUpgradeTxHash : bytes32(0);
292:             _lastCommittedBatchData = _commitOneBatch(
293:                 _lastCommittedBatchData,
294:                 _newBatchesData[i],
295:                 expectedUpgradeTxHash
296:             );

In function _commitBatchesWithSystemContractsUpgrade() - on every loop iteration - we check if i == 0 (line 291). This condition is obviously true only during the first loop iteration. It's recommended to divide the loop into two parts:

for (uint256 i = 0; i < 1; i = i.uncheckedInc())

for (uint256 i = 1; i < _newBatchesData.length; i = i.uncheckedInc())

Moreover, this would allow us to declare expectedUpgradeTxHash outside the loop - thus we won't spend additional gas on declaring this variable during every loop iteration.

That way, in the second loop we will be able to remove i == 0 condition.

[14] Pre-compute and cache domainSeparator in TransactionHelper.sol

File: TransactionHelper.sol

File: code/system-contracts/contracts/libraries/TransactionHelper.sol

138:         bytes32 domainSeparator = keccak256(
139:             abi.encode(EIP712_DOMAIN_TYPEHASH, keccak256("zkSync"), keccak256("2"), block.chainid)
140:         );

It's a common pattern to pre-compute and cache domain separator and then, compare it with pre-computed, cached value, instead of calculating it every time.

[15] Create one, specific event, instead of emitting two events in a row

Files: StateTransitionManager.sol, Bridgehub.sol, Admin.sol

Instead of spending gas on emitting two events at once, it's better to create one, single event and emit it once.

File: code/contracts/ethereum/contracts/state-transition/chain-deps/facets/Admin.sol

40:         emit NewPendingAdmin(pendingAdmin, address(0));
41:         emit NewAdmin(previousAdmin, pendingAdmin);

File: code/contracts/ethereum/contracts/state-transition/StateTransitionManager.sol

127:         emit NewPendingAdmin(currentPendingAdmin, address(0));
128:         emit NewAdmin(previousAdmin, pendingAdmin);

File: code/contracts/ethereum/contracts/bridgehub/Bridgehub.sol

68:         emit NewPendingAdmin(currentPendingAdmin, address(0));
69:         emit NewAdmin(previousAdmin, pendingAdmin);

[16] Redundant casting in getSize()

File: PriorityQueue.sol

File: code/contracts/ethereum/contracts/state-transition/libraries/PriorityQueue.sol

45:     function getSize(Queue storage _queue) internal view returns (uint256) {
46:         return uint256(_queue.tail - _queue.head);
47:     }

Since both _queue.tail and _queue.head are uint256, there's no need to cast _queue.tail - _queue.head to uint256.

[17] Utilize post-incrementing to optimize pushBack() function

File: PriorityQueue.sol

This issue is reported as separate finding, because proposed optimizations require code refactoring.

File: code/contracts/ethereum/contracts/state-transition/libraries/PriorityQueue.sol

55:     function pushBack(Queue storage _queue, PriorityOperation memory _operation) internal {
56:         // Save value into the stack to avoid double reading from the storage
57:         uint256 tail = _queue.tail;
58: 
59:         _queue.data[tail] = _operation;
60:         _queue.tail = tail + 1;
61:     }

We can get rid of variable tail by using _queue.tail++ directly in _queue.data:

function pushBack(Queue storage _queue, PriorityOperation memory _operation) internal {
    _queue.data[_queue.tail++] = _operation;
}

[18] Redundant casting of addresses in isSystemContract()

File: SystemContractHelper.sol

File: code/system-contracts/contracts/libraries/SystemContractHelper.sol

338:     function isSystemContract(address _address) internal pure returns (bool) {
339:         return uint160(_address) <= uint160(MAX_SYSTEM_CONTRACT_ADDRESS);
340:     }

Solidity allows to compare two addresses without casting them to integer first. This means, that above code can be changed to:

return _address <= MAX_SYSTEM_CONTRACT_ADDRESS;

[19] Checking balance after the transfer cannot underflow, thus can be unchecked

File: L1ERC20Bridge.sol

This issue has been reported as separate finding, as it uses different invariant for underflow detection. While another finding uses the invariant that subtraction won't underflow, because require guarantees that subtrahend is lower than minuend, this one utilizes the fact that after the transfer, the balance cannot be lower then the balance before the transfer, thus line 165 won't underflow and can be unchecked.

File: code/contracts/ethereum/contracts/bridge/L1ERC20Bridge.sol

161:         uint256 balanceBefore = _token.balanceOf(address(sharedBridge));
162:         _token.safeTransferFrom(_from, address(sharedBridge), _amount);
163:         uint256 balanceAfter = _token.balanceOf(address(sharedBridge));
164: 
165:         return balanceAfter - balanceBefore;

Balance after the transfer cannot be lower then before the transfer, thus balanceAfter - balanceBefore won't underflow and can be unchecked.

[20] Removing code related to testing - to decrease the contract size and the deployment costs

File: SystemContext.sol

File: code/system-contracts/contracts/SystemContext.sol

469:     /// @notice A testing method that manually sets the current blocks' number and timestamp.
470:     /// @dev Should be used only for testing / ethCalls and should never be used in production.
471:     function unsafeOverrideBatch(
472:         uint256 _newTimestamp,
473:         uint256 _number,
474:         uint256 _baseFee
475:     ) external onlyCallFromBootloader {
476:         BlockInfo memory newBlockInfo = BlockInfo({number: uint128(_number), timestamp: uint128(_newTimestamp)});
477:         currentBatchInfo = newBlockInfo;
478: 
479:         baseFee = _baseFee;
480:     }

Both NatSpec and the code-base suggests that above code is just for testing and should not be on the production. While it's not possible to use this code in a non-testing environment (onlyCallFromBootloader modifier won't let to call this code directly), it's recommend to remove this function. Getting rid of redundant functions will decrease the contract size, thus less gas will be spent during the deployment process.

[21] Do not assign offset in the last call of UnsafeBytes.readUint256() in _parseL2WithdrawalMessage()

File: L1SharedBridge.sol

A simple test in Remix IDE has been created, to compare gas usage of assigning more than one value from function call:

  function getUints() public pure returns (uint, uint) {return (1, 2);}
   function funcA() public view {
    uint a; uint b; 
    (a, b) = getUints();
    uint g = gasleft();
    (a, b) = getUints();
    console.log(g - gasleft());
   }
    function funcB() public view {
    uint a; uint b; 
    (a, b) = getUints();
    uint g = gasleft();
    (a, ) = getUints();
    console.log(g - gasleft());
   }

funcB() costs 74 gas, while funcA() costs 82 gas. This implies, that we shouldn't assign every parameter from function call if it's not needed.

File: code/contracts/ethereum/contracts/bridge/L1SharedBridge.sol

520:             (amount, offset) = UnsafeBytes.readUint256(_l2ToL1message, offset);

In the last call of UnsafeBytes.readUint256(_l2ToL1message, offset) (line 520), we won't need offset anymore, thus we can remove offset and change this line to: (amount, ) = UnsafeBytes.readUint256(_l2ToL1message, offset).

[22] Operations capped by totalSupply in L2BaseToken can be unchecked

File: L2BaseToken.sol

File: code/system-contracts/contracts/L2BaseToken.sol

64:     function mint(address _account, uint256 _amount) external override onlyCallFromBootloader {
65:         totalSupply += _amount;
66:         balance[_account] += _amount;
67:         emit Mint(_account, _amount);
68:     }

User's balance is capped by totalSupply. When totalSupply += _amount won't revert (due to overflow) - we can be sure, that balance[_account] += _amount won't overflow either, thus line 66 can be unchecked. Important - please notice, that only line 66: balance[_account] += _amount can be unchecked.

[23] Use delete to clear data, instead of assigning data to 0

File: L1Messenger.sol

File: code/system-contracts/contracts/L1Messenger.sol

328:         /// Clear logs state
329:         chainedLogsHash = bytes32(0);
330:         numberOfLogsToProcess = 0;
331:         chainedMessagesHash = bytes32(0);
332:         chainedL1BytecodesRevealDataHash = bytes32(0);

[24] Redundant require in BaseZkSyncUpgradeGenesis

File: BaseZkSyncUpgradeGenesis.sol

File: code/contracts/ethereum/contracts/upgrades/BaseZkSyncUpgradeGenesis.sol

22:         require(
23:             // Genesis Upgrade difference: Note this is the only thing change > to >=
24:             _newProtocolVersion >= previousProtocolVersion,
25:             "New protocol version is not greater than the current one"
26:         );
27:         require(
28:             _newProtocolVersion - previousProtocolVersion <= MAX_ALLOWED_PROTOCOL_VERSION_DELTA,
29:             "Too big protocol version difference"
30:         );

To not revert, two conditions must occur: _newProtocolVersion >= previousProtocolVersion and _newProtocolVersion - previousProtocolVersion <= MAX_ALLOWED_PROTOCOL_VERSION_DELTA. However, please notice, that the first require (lines 23-26) is redundant and can be removed. When _newProtocolVersion < previousProtocolVersion, function will already revert due to underflow in the second require at line 28. This basically means, that it's enough to leave just only the second require: (when the first condition won't be fulfilled, function will revert due to underflow in the 2nd require).

[25] Use de Morgan's laws to reduce number of negations

File: DiamondProxy.sol

File: code/contracts/ethereum/contracts/state-transition/chain-deps/DiamondProxy.sol

31:         require(!diamondStorage.isFrozen || !facet.isFreezable, "q1"); // Facet is frozen

According to de Morgan's laws: !A || !B <=> !(A & B), which means we can reduce one negation and rewrite above line to:

require(!(diamondStorage.isFrozen && facet.isFreezable), "q1"); // Facet is frozen

[26] Do not repeat if checks in _constructContract()

File: ContractDeployer.sol

Function _constructContract() performs the same conditional if check twice. This is a waste of gas:

File: code/system-contracts/contracts/ContractDeployer.sol

332:             if (value > 0) {
333:                 BASE_TOKEN_SYSTEM_CONTRACT.transferFromTo(address(this), _newAddress, value);
334:             }
335:             // 2. Set the constructed code hash on the account
336:             _storeConstructingByteCodeHashOnAddress(_newAddress, _bytecodeHash);
337: 
338:             // 3. Call the constructor on behalf of the account
339:             if (value > 0) {
340:                 // Safe to cast value, because `msg.value` <= `uint128.max` due to `MessageValueSimulator` invariant
341:                 SystemContractHelper.setValueForNextFarCall(uint128(value));
342:             }

Firstly, there's a check (line 332) if value is bigger then 0. Then, at line 339, the same check is being made again.

[27] Do not import the whole library, when only a few functions from that library are being used

File: Mailbox.sol

File: code/contracts/ethereum/contracts/state-transition/chain-deps/facets/Mailbox.sol

5: import {Math} from "@openzeppelin/contracts/utils/math/Math.sol";

Mailbox.sol imports the whole Math.sol library, while it only uses Math.max() function.

[28] Calculate simple operations at once

File: TransactionValidator.sol

File: code/contracts/ethereum/contracts/state-transition/libraries/TransactionValidator.sol

77:             costForComputation = L1_TX_INTRINSIC_L2_GAS;
78: 
79:             // Taking into account the hashing costs that depend on the length of the transaction
80:             // Note that L1_TX_DELTA_544_ENCODING_BYTES is the delta in the price for every 544 bytes of
81:             // the transaction's encoding. It is taken as LCM between 136 and 32 (the length for each keccak256 round
82:             // and the size of each new encoding word).
83:             costForComputation += Math.ceilDiv(_encodingLength * L1_TX_DELTA_544_ENCODING_BYTES, 544);
84: 
85:             // Taking into the account the additional costs of providing new factory dependencies
86:             costForComputation += _numberOfFactoryDependencies * L1_TX_DELTA_FACTORY_DEPS_L2_GAS;
87: 
88:             // There is a minimal amount of computational L2 gas that the transaction should cover
89:             costForComputation = Math.max(costForComputation, L1_TX_MIN_L2_GAS_BASE);

Above code performs redundant addition at lines 83, 86. Every time it adds the value to costForComputation, instead of doing it only once:

            costForComputation = Math.max(L1_TX_INTRINSIC_L2_GAS + Math.ceilDiv(_encodingLength * L1_TX_DELTA_544_ENCODING_BYTES, 544) + _numberOfFactoryDependencies * L1_TX_DELTA_FACTORY_DEPS_L2_GAS, L1_TX_MIN_L2_GAS_BASE);

[29] Use calldata instead of memory, when struct is not being changed

File: TransactionValidator.sol

File: code/contracts/ethereum/contracts/state-transition/libraries/TransactionValidator.sol

46:     function validateUpgradeTransaction(L2CanonicalTransaction memory _transaction) internal pure {

Function validateUpgradeTransaction() verifies fields of _transaction, but does not alter it. This implies, that instead of memory, calldata should be used.

[30] Some operations which won't underflow may be unchecked

Files: GasBoundCaller.sol, SystemContext.sol, MsgValueSimulator.sol

File: code/system-contracts/contracts/GasBoundCaller.sol

49:         uint256 pubdataAllowance = _maxTotalGas > expectedForCompute ? _maxTotalGas - expectedForCompute : 0;

Since _maxTotalGas > expectedForCompute, we know that _maxTotalGas - expectedForCompute won't underflow, thus it can be unchecked.

File: code/system-contracts/contracts/GasBoundCaller.sol

63:         uint256 pubdataSpent = pubdataPublishedAfter > pubdataPublishedBefore
64:             ? pubdataPublishedAfter - pubdataPublishedBefore
65:             : 0;

Since pubdataPublishedAfter > pubdataPublishedBefore, we know that pubdataPublishedAfter - pubdataPublishedBefore won't underflow, thus it can be unchecked.

File: code/system-contracts/contracts/SystemContext.sol

119:         return pubdataPublished > basePubdataSpent ? pubdataPublished - basePubdataSpent : 0;

Since pubdataPublished > basePubdataSpent, we know that pubdataPublished - basePubdataSpent won't underflow, thus it can be unchecked.

File: code/system-contracts/contracts/MsgValueSimulator.sol

53:         uint256 userGas = gasInContext > MSG_VALUE_SIMULATOR_STIPEND_GAS
54:             ? gasInContext - MSG_VALUE_SIMULATOR_STIPEND_GAS
55:             : 0;

Since gasInContext > MSG_VALUE_SIMULATOR_STIPEND_GAS, we know that gasInContext - MSG_VALUE_SIMULATOR_STIPEND_GAS won't underflow, thus it can be unchecked.

[31] Use require which costs less gas first

Files: Executor.sol, BaseZkSyncUpgrade.sol, L2SharedBridge.sol, Admin.sol

Because failed require revert the whole operation - it's more profitable to use require which spends less gas first.

File: code/contracts/zksync/contracts/bridge/L2SharedBridge.sol

87:         require(
88:             AddressAliasHelper.undoL1ToL2Alias(msg.sender) == l1Bridge ||
89:                 AddressAliasHelper.undoL1ToL2Alias(msg.sender) == l1LegacyBridge,
90:             "mq"
91:         );
92:         require(msg.value == 0, "Value should be 0 for ERC20 bridge");

Reading msg.value costs less gas than reading l1Bridge and l1LegacyBridge, thus order of require can be changed.

File: code/contracts/ethereum/contracts/upgrades/BaseZkSyncUpgrade.sol

223:         require(_factoryDeps.length == _expectedHashes.length, "Wrong number of factory deps");
224:         require(_factoryDeps.length <= MAX_NEW_FACTORY_DEPS, "Factory deps can be at most 32");

Comparing _factoryDeps.length to constant MAX_NEW_FACTORY_DEPS costs less gas than comparing it to length of another array: _expectedHashes.length. Above code should be changed to:

require(_factoryDeps.length <= MAX_NEW_FACTORY_DEPS, "Factory deps can be at most 32");
require(_factoryDeps.length == _expectedHashes.length, "Wrong number of factory deps");

File: code/contracts/ethereum/contracts/state-transition/chain-deps/facets/Admin.sol

105:         require(
106:             cutHashInput == IStateTransitionManager(s.stateTransitionManager).upgradeCutHash(_oldProtocolVersion),
107:             "StateTransition: cutHash mismatch"
108:         );
109: 
110:         require(
111:             s.protocolVersion == _oldProtocolVersion,
112:             "StateTransition: protocolVersion mismatch in STC when upgrading"
113:         );

s.protocolVersion == _oldProtocolVersion uses less gas than cutHashInput == IStateTransitionManager(s.stateTransitionManager).upgradeCutHash(_oldProtocolVersion). The second require is just reading, while the first one - calling external function upgradeCutHash(). Thus the order of require should be changed.

File: code/contracts/ethereum/contracts/state-transition/chain-deps/facets/Executor.sol

226:         require(
227:             IStateTransitionManager(s.stateTransitionManager).protocolVersion() == s.protocolVersion,
228:             "Executor facet: wrong protocol version"
229:         );
230:         // With the new changes for EIP-4844, namely the restriction on number of blobs per block, we only allow for a single batch to be committed at a time.
231:         require(_newBatchesData.length == 1, "e4");

_newBatchesData.length == 1 uses less gas than calling protocolVersion() in the first require: IStateTransitionManager(s.stateTransitionManager).protocolVersion() == s.protocolVersion (especially, that we expect _newBatchesData.length to be equal to 1). This implies, that the order of require statements should be changed.

[32] Execute require before other operations

Files: GasBoundCaller.sol, Executor.sol

Because require reverts the whole operation, it's better to perform require statements before any other operation. When require reverts, function won't use additional gas for other operations.

File: code/system-contracts/contracts/GasBoundCaller.sol

40:         uint256 expectedForCompute = gasleft() + CALL_ENTRY_OVERHEAD;
[...]
46:         require(_maxTotalGas >= gasleft(), "Gas limit is too low");

We can move require from line 46 above line 40. When _maxTotalGas won't be greater or equal gasleft() - function will revert immediately, thus we won't waste additional gas on expectedForCompute assignment.

File: code/contracts/ethereum/contracts/state-transition/chain-deps/facets/Executor.sol

356:         uint256 newTotalBatchesExecuted = s.totalBatchesExecuted + nBatches;
357:         s.totalBatchesExecuted = newTotalBatchesExecuted;
358:         require(newTotalBatchesExecuted <= s.totalBatchesVerified, "n"); // Can't execute batches more than committed and proven currently.

We can move require from line 358, before line 357. When condition newTotalBatchesExecuted <= s.totalBatchesVerified won't be fulfilled, function will revert without wasting gas on executing line 357. The fixed code should look like this:

        uint256 newTotalBatchesExecuted = s.totalBatchesExecuted + nBatches;
        require(newTotalBatchesExecuted <= s.totalBatchesVerified, "n"); // Can't execute batches more than committed and proven currently.
        s.totalBatchesExecuted = newTotalBatchesExecuted;

File: code/contracts/ethereum/contracts/state-transition/chain-deps/facets/Executor.sol

629:         uint256 versionedHashIndex = 0;
630: 
631:         require(_pubdataCommitments.length > 0, "pl");
632:         require(_pubdataCommitments.length <= PUBDATA_COMMITMENT_SIZE * MAX_NUMBER_OF_BLOBS, "bd");
633:         require(_pubdataCommitments.length % PUBDATA_COMMITMENT_SIZE == 0, "bs");

Move require statements from lines 631, 632, 633 above line 629. If any of these require revert, function won't waste additional gas on versionedHashIndex initialization.

[33] Use short-circuiting in OR operations

File: NonceHolder.sol

Solidity uses short-circuiting while performing OR operations. This means, that in the expression A() OR B() - when A() evaluates to true - expression B() won't be executed (because true OR whatever evaluates to true). This behavior suggests, that it's better to use less-gas-costly operation first (if it will be evaluated to true, Solidity won't waste gas on executing the 2nd operation).

File: code/system-contracts/contracts/NonceHolder.sol

156:         return (_nonce < getMinNonce(_address) || nonceValues[addressAsKey][_nonce] > 0);

Function getMinNonce() reads state variable and performs multiple of other operations. This means, that it uses more gas than just reading nonceValues[addressAsKey][_nonce] state variable and comparing it to 0. Above code should be rewritten to:

 return (nonceValues[addressAsKey][_nonce] > 0 || _nonce < getMinNonce(_address));

[34] Do not declare variables inside loop

Files: Compressor.sol, Mailbox.sol, ImmutableSimulator.sol, ValidatorTimelock.sol, Executor.sol

Move variable declaration outside the loop. While the variable is declared inside the loop - every loop iteration will spend gas on that variable declaration.

File: code/system-contracts/contracts/ImmutableSimulator.sol

38:             for (uint256 i = 0; i < immutablesLength; ++i) {
39:                 uint256 index = _immutables[i].index;
40:                 bytes32 value = _immutables[i].value;

should be changed to:

    uint256 index;
    bytes32 value;
    for (uint256 i = 0; i < immutablesLength; ++i) {
        index = _immutables[i].index;
        value = _immutables[i].value;

File: code/system-contracts/contracts/Compressor.sol

61:             for (uint256 encodedDataPointer = 0; encodedDataPointer < encodedData.length; encodedDataPointer += 2) {
62:                 uint256 indexOfEncodedChunk = uint256(encodedData.readUint16(encodedDataPointer)) * 8;

should be changed to:

            uint256 indexOfEncodedChunk;
            for (uint256 encodedDataPointer = 0; encodedDataPointer < encodedData.length; encodedDataPointer += 2) {
                indexOfEncodedChunk = uint256(encodedData.readUint16(encodedDataPointer)) * 8;

File: code/contracts/ethereum/contracts/state-transition/chain-deps/facets/Executor.sol

289:         for (uint256 i = 0; i < _newBatchesData.length; i = i.uncheckedInc()) {
290:             // The upgrade transaction must only be included in the first batch.
291:             bytes32 expectedUpgradeTxHash = i == 0 ? _systemContractUpgradeTxHash : bytes32(0);

Should be changed to:

        bytes32 expectedUpgradeTxHash;
        for (uint256 i = 0; i < _newBatchesData.length; i = i.uncheckedInc()) {
            // The upgrade transaction must only be included in the first batch.
            expectedUpgradeTxHash = i == 0 ? _systemContractUpgradeTxHash : bytes32(0);

File: code/contracts/ethereum/contracts/state-transition/chain-deps/facets/Mailbox.sol

356:         for (uint256 i = 0; i < factoryDepsLen; i = i.uncheckedInc()) {
357:             bytes32 hashedBytecode = L2ContractHelper.hashL2Bytecode(_factoryDeps[i]);

Should be changed to:

        bytes32 hashedBytecode;
        for (uint256 i = 0; i < factoryDepsLen; i = i.uncheckedInc()) {
            hashedBytecode = L2ContractHelper.hashL2Bytecode(_factoryDeps[i]);

File: code/contracts/ethereum/contracts/state-transition/ValidatorTimelock.sol

185:             for (uint256 i = 0; i < _newBatchesData.length; ++i) {
186:                 uint256 commitBatchTimestamp = committedBatchTimestamp[ERA_CHAIN_ID].get(

Should be changed to :

            uint256 commitBatchTimestamp;
            for (uint256 i = 0; i < _newBatchesData.length; ++i) {
                commitBatchTimestamp = committedBatchTimestamp[ERA_CHAIN_ID].get(

File: code/contracts/ethereum/contracts/state-transition/ValidatorTimelock.sol

209:             for (uint256 i = 0; i < _newBatchesData.length; ++i) {
210:                 uint256 commitBatchTimestamp = committedBatchTimestamp[_chainId].get(_newBatchesData[i].batchNumber);

Should be changed to:

            uint256 commitBatchTimestamp;
            for (uint256 i = 0; i < _newBatchesData.length; ++i) {
                commitBatchTimestamp = committedBatchTimestamp[_chainId].get(_newBatchesData[i].batchNumber);

[35] Avoid declaring unused variables

File: Constants.sol

File: code/system-contracts/contracts/Constants.sol

93: /// @dev The maximal msg.value that context can have
94: uint256 constant MAX_MSG_VALUE = 2 ** 128 - 1;

Constant MAX_MSG_VALUE is not used anymore, thus it should be removed from Constnats.sol

[36] Use bitwise operators instead of multiplication/division by value of 2 to the power of N

Files: L2ContractHelper.sol, Compressor.sol, Executor.sol, Merkle.sol

File: code/system-contracts/contracts/Compressor.sol

52:                 encodedData.length * 4 == _bytecode.length,
[...]
57:                 dictionary.length / 8 <= encodedData.length / 2,
[...]
62:                 uint256 indexOfEncodedChunk = uint256(encodedData.readUint16(encodedDataPointer)) * 8;
[...]
66:                 uint64 realChunk = _bytecode.readUint64(encodedDataPointer * 4);

File: code/contracts/ethereum/contracts/state-transition/libraries/Merkle.sol

33:             _index /= 2;

File: code/contracts/ethereum/contracts/common/libraries/L2ContractHelper.sol

25:         uint256 bytecodeLenInWords = _bytecode.length / 32;
26:         require(bytecodeLenInWords < 2 ** 16, "pp"); // bytecode length must be less than 2^16 words

File: code/contracts/ethereum/contracts/state-transition/chain-deps/facets/Executor.sol

581:             blobAuxOutputWords[i * 2] = _blobHashes[i];
582:             blobAuxOutputWords[i * 2 + 1] = _blobCommitments[i];

[37] Code related to local testing should be removed

Files: StateTransitionManager.sol, DiamondInit.sol

File: code/contracts/ethereum/contracts/state-transition/chain-deps/DiamondInit.sol

49:         // While this does not provide a protection in the production, it is needed for local testing
50:         // Length of the L2Log encoding should not be equal to the length of other L2Logs' tree nodes preimages
51:         assert(L2_TO_L1_LOG_SERIALIZE_SIZE != 2 * 32); 

File: code/contracts/ethereum/contracts/state-transition/StateTransitionManager.sol

104:         // While this does not provide a protection in the production, it is needed for local testing
105:         // Length of the L2Log encoding should not be equal to the length of other L2Logs' tree nodes preimages
106:         assert(L2_TO_L1_LOG_SERIALIZE_SIZE != 2 * 32); 

Above assert() is just a waste of gas in the production environment. Moreover it increases the size of the contract, thus increases the deployment cost. Our recommendation is to get rid of code related to local testing only.

[38] Dot notation for struct assignment costs less gas

File: Diamond.sol

Using named arguments for struct means that the compiler needs to organize the fields in memory before doing the assignment, which wastes gas. Set each field directly in storage (use dot-notation), or use the unnamed version of the constructor.

File: code/contracts/ethereum/contracts/state-transition/libraries/Diamond.sol

218:         ds.selectorToFacet[_selector] = SelectorToFacet({
219:             facetAddress: _facet,
220:             selectorPosition: selectorPosition,
221:             isFreezable: _isSelectorFreezable
222:         });

should be optimized to:

ds.selectorToFacet[_selector].facetAddress = _facet;
ds.selectorToFacet[_selector].selectorPosition = selectorPosition;
ds.selectorToFacet[_selector].isFreezable = _isSelectorFreezable;

[39] Use AND operators instead of modulo (%) to check if number is odd/even

Files: L2ContractHelper.sol, Utils.sol, Merkle.sol, KnownCodesStorage.sol

File: code/contracts/ethereum/contracts/common/libraries/L2ContractHelper.sol

43:         require(bytecodeLen(_bytecodeHash) % 2 == 1, "uy"); // Code length in words must be odd

File: code/contracts/ethereum/contracts/common/libraries/L2ContractHelper.sol

27:         require(bytecodeLenInWords % 2 == 1, "ps"); // bytecode length in words must be odd

File: code/contracts/ethereum/contracts/state-transition/libraries/Merkle.sol

30:             currentHash = (_index % 2 == 0)

File: code/system-contracts/contracts/KnownCodesStorage.sol

78:         require(Utils.bytecodeLenInWords(_bytecodeHash) % 2 == 1, "Code length in words must be odd");

File: code/system-contracts/contracts/libraries/Utils.sol

88:         require(lengthInWords % 2 == 1, "pr"); // bytecode length in words must be odd

[40] else-block is not required in getOperationState()

File: Governance.sol

File: code/contracts/ethereum/contracts/governance/Governance.sol

105:     function getOperationState(bytes32 _id) public view returns (OperationState) {
106:         uint256 timestamp = timestamps[_id];
107:         if (timestamp == 0) {
108:             return OperationState.Unset;
109:         } else if (timestamp == EXECUTED_PROPOSAL_TIMESTAMP) {
110:             return OperationState.Done;
111:         } else if (timestamp > block.timestamp) {
112:             return OperationState.Waiting;
113:         } else {
114:             return OperationState.Ready;
115:         }
116:     }

The else instruction at line 113 can be removed. When previous conditions won't be fulfilled, function will still return OperationState.Ready. Getting rid of redundant else instruction saves additional gas.

[41] Inline modifiers that are only used once, to save gas

Files: ContractDeployer.sol, KnownCodesStorage.sol

File: code/system-contracts/contracts/ContractDeployer.sol

213:     function forceDeployOnAddress(ForceDeployment calldata _deployment, address _sender) external payable onlySelf {

File: code/system-contracts/contracts/KnownCodesStorage.sol

39:     function markBytecodeAsPublished(bytes32 _bytecodeHash) external onlyCompressor {

Modifiers onlySelf and onlyCompressor where used only once, thus they can be inlined.

[42] Use "" instead of new bytes(0)

Files: StateTransitionManager.sol, Mailbox.sol

File: code/contracts/ethereum/contracts/state-transition/chain-deps/facets/Mailbox.sol

308:             signature: new bytes(0),
309:             factoryDeps: _hashFactoryDeps(_factoryDeps),
310:             paymasterInput: new bytes(0),
311:             reservedDynamic: new bytes(0)

File: code/contracts/ethereum/contracts/state-transition/StateTransitionManager.sol

196:             signature: new bytes(0),
197:             factoryDeps: uintEmptyArray,
198:             paymasterInput: new bytes(0),
199:             reservedDynamic: new bytes(0)

[43] public functions which are not called by the contract should be declared as external

File: Mailbox.sol

When public function is never called internally and is only expected to be invoked externally, it is more gas-efficient to explicitly declare it as external.

File: code/contracts/ethereum/contracts/state-transition/chain-deps/facets/Mailbox.sol

74:     function proveL1ToL2TransactionStatus(
75:         bytes32 _l2TxHash,
76:         uint256 _l2BatchNumber,
77:         uint256 _l2MessageIndex,
78:         uint16 _l2TxNumberInBatch,
79:         bytes32[] calldata _merkleProof,
80:         TxStatus _status
81:     ) public view returns (bool) {

File: code/contracts/ethereum/contracts/state-transition/chain-deps/facets/Mailbox.sol

143:     function l2TransactionBaseCost(
144:         uint256 _gasPrice,
145:         uint256 _l2GasLimit,
146:         uint256 _l2GasPerPubdataByteLimit
147:     ) public view returns (uint256) {

[44] Do not emit events with empty data

File: Admin.sol

File: code/contracts/ethereum/contracts/state-transition/chain-deps/facets/Admin.sol

139:         emit Freeze();

File: code/contracts/ethereum/contracts/state-transition/chain-deps/facets/Admin.sol

149:         emit Unfreeze();

[45] Do not declare local variables used only once

Files: L2ContractHelper.sol, Compressor.sol, L2SharedBridge.sol, Mailbox.sol, L1ERC20Bridge.sol, NonceHolder.sol, DefaultAccount.sol, EfficientCall.sol, AccountCodeStorage.sol, L1SharedBridge.sol, Diamond.sol, L2StandardERC20.sol, SystemContractHelper.sol, KnownCodesStorage.sol, Admin.sol, ContractDeployer.sol

File: code/contracts/zksync/contracts/bridge/L2StandardERC20.sol

119:         address beaconAddress = _getBeacon();
120:         require(msg.sender == UpgradeableBeacon(beaconAddress).owner(), "tt");

Variable beaconAddress is used only once, thus this variable is redundant. Above code can be rewritten to:

require(msg.sender == UpgradeableBeacon(_getBeacon()).owner(), "tt");

File: code/contracts/zksync/contracts/bridge/L2SharedBridge.sol

64:             address l2StandardToken = address(new L2StandardERC20{salt: bytes32(0)}());
65:             l2TokenBeacon = new UpgradeableBeacon{salt: bytes32(0)}(l2StandardToken);

Variable l2StandardToken is used only once, thus it is redundant. Above code can be changed to:

l2TokenBeacon = new UpgradeableBeacon{salt: bytes32(0)}(address(new L2StandardERC20{salt: bytes32(0)}()));

File: code/contracts/zksync/contracts/bridge/L2SharedBridge.sol

110:         bytes32 salt = _getCreate2Salt(_l1Token);
111: 
112:         BeaconProxy l2Token = _deployBeaconProxy(salt);

Variable salt is used only once, thus it is redundant. Above code can be changed to:

BeaconProxy l2Token = _deployBeaconProxy(_getCreate2Salt(_l1Token));

File: code/contracts/zksync/contracts/bridge/L2SharedBridge.sol

150:         bytes32 constructorInputHash = keccak256(abi.encode(address(l2TokenBeacon), ""));
151:         bytes32 salt = _getCreate2Salt(_l1Token);
152:         return
153:             L2ContractHelper.computeCreate2Address(address(this), salt, l2TokenProxyBytecodeHash, constructorInputHash);

Variables constructorInputHash and salt are used only once, thus they are redundant. Above code can be changed to:

return
    L2ContractHelper.computeCreate2Address(address(this), _getCreate2Salt(_l1Token), l2TokenProxyBytecodeHash, keccak256(abi.encode(address(l2TokenBeacon), "")));

File: code/system-contracts/contracts/libraries/EfficientCall.sol

261:         uint32 shrinkTo = uint32(msg.data.length - (_data.length + dataOffset));
262:         SystemContractHelper.ptrShrinkIntoActive(shrinkTo);
263: 
264:         uint32 gas = Utils.safeCastToU32(_gas);
265:         uint256 farCallAbi = SystemContractsCaller.getFarCallABIWithEmptyFatPointer(
266:             gas,

shrinkTo and gas are used only once, thus there's no need to declare them at all:

        SystemContractHelper.ptrShrinkIntoActive(uint32(msg.data.length - (_data.length + dataOffset)));

        uint256 farCallAbi = SystemContractsCaller.getFarCallABIWithEmptyFatPointer(
            Utils.safeCastToU32(_gas),

File: code/system-contracts/contracts/libraries/SystemContractHelper.sol

217:         uint256 shifted = (meta << (256 - size - offset));
218:         // Then we shift everything back
219:         result = (shifted >> (256 - size));

shifted is used only once, thus there's no need to declare this variable at all:

result = ((meta << (256 - size - offset)) >> (256 - size));

File: code/system-contracts/contracts/NonceHolder.sol

92:         uint256 addressAsKey = uint256(uint160(msg.sender));
93: 
94:         nonceValues[addressAsKey][_key] = _value;

addressAsKey is used only once, thus there's no need to declare this variable at all:

 nonceValues[uint256(uint160(msg.sender))][_key] = _value;

File: code/system-contracts/contracts/NonceHolder.sol

103:         uint256 addressAsKey = uint256(uint160(msg.sender));
104:         return nonceValues[addressAsKey][_key];

addressAsKey is used only once, thus there's no need to declare this variable at all:

 return nonceValues[uint256(uint160(msg.sender))][_key];

File: code/system-contracts/contracts/KnownCodesStorage.sol

75:         uint8 version = uint8(_bytecodeHash[0]);
76:         require(version == 1 && _bytecodeHash[1] == bytes1(0), "Incorrectly formatted bytecodeHash");

version is used only once, thus there's no need to declare this variable at all:

require(uint8(_bytecodeHash[0]) == 1 && _bytecodeHash[1] == bytes1(0), "Incorrectly formatted bytecodeHash");

File: code/system-contracts/contracts/DefaultAccount.sol

099:         uint256 totalRequiredBalance = _transaction.totalRequiredBalance();
100:         require(totalRequiredBalance <= address(this).balance, "Not enough balance for fee + value");

totalRequiredBalance is used only once, thus there's no need to declare this variable at all:

require(_transaction.totalRequiredBalance() <= address(this).balance, "Not enough balance for fee + value");

File: code/system-contracts/contracts/ContractDeployer.sol

121:         bytes32 hash = keccak256(
122:             bytes.concat(CREATE_PREFIX, bytes32(uint256(uint160(_sender))), bytes32(_senderNonce))
123:         );
124: 
125:         newAddress = address(uint160(uint256(hash)));

hash is used only once, thus there's no need to declare this variable at all:

newAddress = address(uint160(uint256(keccak256(bytes.concat(CREATE_PREFIX, bytes32(uint256(uint160(_sender))), bytes32(_senderNonce))))));

File: code/system-contracts/contracts/ContractDeployer.sol

302:         uint256 knownCodeMarker = KNOWN_CODE_STORAGE_CONTRACT.getMarker(_bytecodeHash);
303:         require(knownCodeMarker > 0, "The code hash is not known");

knownCodeMarker is used only once, thus there's no need to declare this variable at all:

require(KNOWN_CODE_STORAGE_CONTRACT.getMarker(_bytecodeHash) > 0, "The code hash is not known");

File: code/system-contracts/contracts/Compressor.sol

65:                 uint64 encodedChunk = dictionary.readUint64(indexOfEncodedChunk);
66:                 uint64 realChunk = _bytecode.readUint64(encodedDataPointer * 4);
67: 
68:                 require(encodedChunk == realChunk, "Encoded chunk does not match the original bytecode");

encodedChunk and realChunk are used only once, thus there's no need to declare those variables at all:

require(dictionary.readUint64(indexOfEncodedChunk) == _bytecode.readUint64(encodedDataPointer * 4), "Encoded chunk does not match the original bytecode");

File: code/system-contracts/contracts/AccountCodeStorage.sol

60:         bytes32 constructedBytecodeHash = Utils.constructedBytecodeHash(codeHash);
61: 
62:         _storeCodeHash(_address, constructedBytecodeHash);

constructedBytecodeHash is used only once, thus there's no need to declare this variable at all:

_storeCodeHash(_address, Utils.constructedBytecodeHash(codeHash));

File: code/contracts/ethereum/contracts/state-transition/libraries/Diamond.sol

192:         uint256 selectorsLength = ds.facetToSelectors[_facet].selectors.length;
193:         // If there are no selectors associated with facet then save facet as new one
194:         if (selectorsLength == 0) {

selectorsLength is used only once, thus there's no need to declare this variable at all:

if (ds.facetToSelectors[_facet].selectors.length == 0) {

File: code/contracts/ethereum/contracts/common/libraries/L2ContractHelper.sol

40:         uint8 version = uint8(_bytecodeHash[0]);
41:         require(version == 1 && _bytecodeHash[1] == bytes1(0), "zf"); // Incorrectly formatted bytecodeHash

version is used once, thus there's no need to declare this variable at all:

require(uint8(_bytecodeHash[0]) == 1 && _bytecodeHash[1] == bytes1(0), "zf"); // Incorrectly formatted bytecodeHash

File: code/contracts/ethereum/contracts/common/libraries/L2ContractHelper.sol

66:         bytes32 senderBytes = bytes32(uint256(uint160(_sender)));
67:         bytes32 data = keccak256(
68:             bytes.concat(CREATE2_PREFIX, senderBytes, _salt, _bytecodeHash, _constructorInputHash)
69:         );
70: 
71:         return address(uint160(uint256(data)));

senderBytes and data are used once, thus there's no need to declare those variables:

return address(uint160(uint256( keccak256(
            bytes.concat(CREATE2_PREFIX, bytes32(uint256(uint160(_sender))), _salt, _bytecodeHash, _constructorInputHash)
        ))));

File: code/contracts/ethereum/contracts/state-transition/chain-deps/facets/Admin.sol

104:         bytes32 cutHashInput = keccak256(abi.encode(_diamondCut));
105:         require(
106:             cutHashInput == IStateTransitionManager(s.stateTransitionManager).upgradeCutHash(_oldProtocolVersion),
107:             "StateTransition: cutHash mismatch"
108:         );
109: 

cutHashInput is used only once, thus there's no need to declare this variable at all:

        require(
            keccak256(abi.encode(_diamondCut)) == IStateTransitionManager(s.stateTransitionManager).upgradeCutHash(_oldProtocolVersion),
            "StateTransition: cutHash mismatch"
        );

File: code/contracts/ethereum/contracts/state-transition/chain-deps/facets/Mailbox.sol

41:         uint256 amount = address(this).balance;
42:         address sharedBridgeAddress = s.baseTokenBridge;
43:         IL1SharedBridge(sharedBridgeAddress).receiveEth{value: amount}(ERA_CHAIN_ID);

amount and sharedBridgeAddress are used only once, thus there's no need to declare those variables at all:

IL1SharedBridge(s.baseTokenBridge).receiveEth{value: address(this).balance}(ERA_CHAIN_ID);

File: code/contracts/ethereum/contracts/state-transition/chain-deps/facets/Mailbox.sol

123:         bytes32 calculatedRootHash = Merkle.calculateRoot(_proof, _index, hashedLog);
124:         bytes32 actualRootHash = s.l2LogsRootHashes[_batchNumber];
125: 
126:         return actualRootHash == calculatedRootHash;

calculatedRootHash and actualRootHash are used only once, thus there's no need to declare those variables at all:

return Merkle.calculateRoot(_proof, _index, hashedLog) == s.l2LogsRootHashes[_batchNumber];

File: code/contracts/ethereum/contracts/state-transition/chain-deps/facets/Mailbox.sol

148:         uint256 l2GasPrice = _deriveL2GasPrice(_gasPrice, _l2GasPerPubdataByteLimit);
149:         return l2GasPrice * _l2GasLimit;

l2GasPrice is used only once, thus there's no need to declare this variable at all:

return _deriveL2GasPrice(_gasPrice, _l2GasPerPubdataByteLimit) * _l2GasLimit;;

File: code/contracts/ethereum/contracts/state-transition/chain-deps/facets/Mailbox.sol

271:         uint256 baseCost = _params.l2GasPrice * _params.l2GasLimit;
272:         require(_mintValue >= baseCost + _params.l2Value, "mv"); // The `msg.value` doesn't cover the transaction cost

baseCost is used only once, thus there's no need to declare this variable at all:

require(_mintValue >= _params.l2GasPrice * _params.l2GasLimit + _params.l2Value, "mv"); // The `msg.value` doesn't cover the transaction cost

File: code/contracts/ethereum/contracts/bridge/L1SharedBridge.sol

160:             uint256 amount = _depositFunds(_prevMsgSender, IERC20(_l1Token), _amount); // note if _prevMsgSender is this contract, this will return 0. This does not happen.
161:             require(amount == _amount, "3T"); // The token has non-standard transfer logic

amount is used only once, thus there's no need to declare this variable at all:

require(_depositFunds(_prevMsgSender, IERC20(_l1Token), _amount) == _amount, "3T"); // The token has non-standard transfer logic

File: code/contracts/ethereum/contracts/bridge/L1SharedBridge.sol

205:             uint256 withdrawAmount = _depositFunds(_prevMsgSender, IERC20(_l1Token), _depositAmount);
206:             require(withdrawAmount == _depositAmount, "5T"); // The token has non-standard transfer logic

withdrawAmount is used only once, thus there's no need to declare this variable at all:

 require(_depositFunds(_prevMsgSender, IERC20(_l1Token), _depositAmount) == _depositAmount, "5T"); // The token has non-standard transfer logic

File: code/contracts/ethereum/contracts/bridge/L1SharedBridge.sol

340:                 bytes32 dataHash = depositHappened[_chainId][_l2TxHash];
341:                 bytes32 txDataHash = keccak256(abi.encode(_depositSender, _l1Token, _amount));
342:                 require(dataHash == txDataHash, "ShB: d.it not hap");

dataHash and txDataHash are used only once, thus there's no need to declare those variables at all:

require(depositHappened[_chainId][_l2TxHash] == keccak256(abi.encode(_depositSender, _l1Token, _amount)), "ShB: d.it not hap");

File: code/contracts/ethereum/contracts/bridge/L1ERC20Bridge.sol

76:         bytes32 constructorInputHash = keccak256(abi.encode(l2TokenBeacon, ""));
77:         bytes32 salt = bytes32(uint256(uint160(_l1Token)));
78: 
79:         return L2ContractHelper.computeCreate2Address(l2Bridge, salt, l2TokenProxyBytecodeHash, constructorInputHash);

constructorInputHash and salt are used only once, thus there's no need to declare those variables at all:

return L2ContractHelper.computeCreate2Address(l2Bridge,  bytes32(uint256(uint160(_l1Token))), l2TokenProxyBytecodeHash, keccak256(abi.encode(l2TokenBeacon, "")));

[46] Use msg.value directly, instead of assigning it to variable

File: ContractDeployer.sol

File: code/system-contracts/contracts/ContractDeployer.sol

329:         uint256 value = msg.value;

[47] Storage values shouldn't be updated when value is not being changed

Files: SystemContext.sol, Bridgehub.sol, ValidatorTimelock.sol, Governance.sol, StateTransitionManager.sol, Admin.sol, ContractDeployer.sol

It's recommended to add additional require statement, which will verify if the value is really changed, before we will store it in the storage variables E.g., let's consider a function setChainId() from SystemContext.sol. When chainId is 1 and we will call setChainId(1), function will re-store the same value (Gsreset (900 gas)).

File: code/contracts/ethereum/contracts/bridgehub/Bridgehub.sol

51:     function setPendingAdmin(address _newPendingAdmin) external onlyOwnerOrAdmin {

File: code/contracts/ethereum/contracts/bridgehub/Bridgehub.sol

108:     function setSharedBridge(address _sharedBridge) external onlyOwner {

File: code/contracts/ethereum/contracts/state-transition/StateTransitionManager.sol

132:     function setValidatorTimelock(address _validatorTimelock) external onlyOwnerOrAdmin {
[...]
137:     function setInitialCutHash(Diamond.DiamondCutData calldata _diamondCut) external onlyOwner {
[...]
142:     function setNewVersionUpgrade(
[...]
152:     function setUpgradeDiamondCut(

File: code/contracts/ethereum/contracts/state-transition/StateTransitionManager.sol

110:     function setPendingAdmin(address _newPendingAdmin) external onlyOwnerOrAdmin {

File: code/contracts/ethereum/contracts/state-transition/ValidatorTimelock.sol

73:     function setStateTransitionManager(IStateTransitionManager _stateTransitionManager) external onlyOwner {

File: code/contracts/ethereum/contracts/state-transition/ValidatorTimelock.sol

96:     function setExecutionDelay(uint32 _executionDelay) external onlyOwner { 

File: code/contracts/ethereum/contracts/state-transition/chain-deps/facets/Admin.sol

79:     function setTokenMultiplier(uint128 _nominator, uint128 _denominator) external onlyAdminOrStateTransitionManager {
[...]
89:     function setValidiumMode(PubdataPricingMode _validiumMode) external onlyAdmin {

File: code/contracts/ethereum/contracts/state-transition/chain-deps/facets/Admin.sol

45:     function setValidator(address _validator, bool _active) external onlyStateTransitionManager {
[...]
51:     function setPorterAvailability(bool _zkPorterIsAvailable) external onlyStateTransitionManager {
[...]
58:     function setPriorityTxMaxGasLimit(uint256 _newPriorityTxMaxGasLimit) external onlyStateTransitionManager { 

File: code/system-contracts/contracts/SystemContext.sol

84:     function setChainId(uint256 _newChainId) external onlyCallFromForceDeployer {

File: code/system-contracts/contracts/SystemContext.sol

099:     function setTxOrigin(address _newOrigin) external onlyCallFromBootloader {
[...]
105:     function setGasPrice(uint256 _gasPrice) external onlyCallFromBootloader {
[...]
112:     function setPubdataInfo(uint256 _gasPerPubdataByte, uint256 _basePubdataSpent) external onlyCallFromBootloader {

File: code/contracts/ethereum/contracts/governance/Governance.sol

249:     function updateDelay(uint256 _newDelay) external onlySelf {
[...]
256:     function updateSecurityCouncil(address _newSecurityCouncil) external onlySelf {

File: code/system-contracts/contracts/ContractDeployer.sol

65:     function updateAccountVersion(AccountAbstractionVersion _version) external onlySystemCall {

File: code/contracts/ethereum/contracts/state-transition/chain-deps/facets/Admin.sol

67:     function changeFeeParams(FeeParams calldata _newFeeParams) external onlyAdminOrStateTransitionManager {

[AUTOMATED-FINDING-1] Using bools for storage incurs overhead

Booleans are more expensive than uint256 or any type that takes up a full word because each write operation emits an extra SLOAD to first read the slot's contents, replace the bits taken up by the boolean, and then write back. This is the compiler's defense against contract upgrades and pointer aliasing, and it cannot be disabled.

Above code detects every instance of mappings which utilizes boolean instead of uint

% grep "=>" -ri . | grep mapping | grep bool | wc -l                    
       9

[AUTOMATED-FINDING-2] Mark constructor as payable

Payable functions cost less gas to execute, since the compiler does not have to add extra checks to ensure that a payment wasn't provided. A constructor can safely be marked as payable, since only the deployer would be able to pass funds, and the project itself would not pass any funds.

% grep "constructor(" -ri . |  grep -v "//" | grep "{" | grep -v "payable" | grep -v "/test"  | wc -l
      16

[AUTOMATED-FINDING-3] Functions guaranteed to revert when called by normal users can be marked payable

If a function modifier such as onlyOwner is used, the function will revert if a normal user tries to pay the function. Marking the function as payable will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided.

% grep "function" -ri . |  grep -v "//" | grep "{" | grep "only" | grep -v "payable" | grep -v "/test" | grep only | wc -l
      64

[AUTOMATED-FINDING-4] Index all 3 parameters in events

% grep "event " -ri . |  grep -v "//" | grep -v "/test" | grep ";" | grep -v indexed | grep -v "()" | wc -l
      19

Above detector identifies events which are not properly indexed.

[AUTOMATED-FINDING-5] Nesting if-statements is cheaper than using &&

Nesting if-statements avoids the stack operations of setting up and using an extra jumpdest, and saves 6 gas. This basically means that: if (A && B) can be represented as if (A) { if (B) }.

% grep "if (" -r . | grep "&&" | grep -v "/test" | grep ".sol" | wc -l
       9

[AUTOMATED-FINDING-6] Assembly: Use scratch space for calculating small keccak256 hashes

If the arguments to the encode call can fit into the scratch space (two words or fewer), then it's more efficient to use assembly to generate the hash (80 gas)

Below code detects only the instances where there's a single argument in keccak256.

% grep "keccak256(" -r . | grep ")" | grep -v "/test" | grep ".sol" | grep -v ":=" | grep -v "," | grep -v "//" | grep -v "constant" | wc -l
      17

[AUTOMATED-FINDING-7] Assembly: Use scratch space when building emitted events with two data arguments

To efficiently emit events, it's possible to utilize assembly by making use of scratch space and the free memory pointer. This approach has the advantage of potentially avoiding the costs associated with memory expansion.

Below code detects only the instances where there's a single (or no) argument in the event.

% grep "emit " -r . | grep ")" | grep -v "/test" | grep ".sol" | grep -v "," | grep -v "//" | wc -l
      12

[AUTOMATED-FINDING-8] abi.encode() is less efficient than abi.encodePacked()

% grep "abi.encode" -r . | grep -v "//" | grep -v "/test" | wc -l
      63

[AUTOMATED-FINDING-9] "" has the same value as new bytes(0) but costs less gas

% grep "new bytes(0)" -ri . | wc -l
      17

[AUTOMATED-FINDING-10] Function names can be optimized to save gas

Function that are public/external and public state variable names can be optimized to save gas.

Method IDs that have two leading zero bytes can save 128 gas each during deployment, and renaming functions to have lower method IDs will save 22 gas per call, per sorted position shifted.

You can use the Function Name Optimizer Tool to find new function names.

[AUTOMATED-FINDING-11] Integer increments by one can be unchecked

% grep  "+= 1;" -r . | wc -l
       9

[AUTOMATED-FINDING-12] Low-level call can be optimized with assembly

returnData is copied to memory even if the variable is not utilized: the proper way to handle this is through a low level assembly call and save 159 gas.

% grep "call{" -ri . | wc -l
      11

[AUTOMATED-FINDING-13] Operator >=/<= costs less gas than operator >/<

The compiler uses opcodes GT and ISZERO for solidity code that uses >, but only requires LT for >=, which saves 3 gas. If < is being used, the condition can be inverted.

% grep -E "if \(|require\(" -r . | grep ">" | grep -v ">=" | grep ".sol" | wc -l
      42

% grep -E "if \(|require\(" -r . | grep "<" | grep -v "<=" | grep ".sol" | wc -l
      15

[AUTOMATED-FINDING-14] Reduce deployment gas costs by fine-tuning IPFS file hashes

Solc currently by default appends the IPFS hash (in CID v0) of the canonical metadata file and the compiler version to the end of the bytecode. This value is variable-length CBOR-encoded i.e. it can be optimized in order to reduce deployment gas costs.

[AUTOMATED-FINDING-15] Simple checks for zero can be done using assembly to save gas

Using assembly for simple zero checks on unsigned integers can save gas due to lower-level, optimized operations.

Below code returns only the simple if conditions without additional && and || operators, such as: if (A == 0)

% grep "if (" -r . | grep -v "//" | grep -v "&&" | grep -v "||"  | grep " == 0)" | grep .sol | wc -l
      14

[AUTOMATED-FINDING-16] Use assembly to validate msg.sender

We can use assembly to efficiently validate msg.sender with the least amount of opcodes necessary.

Below code returns only the simple if conditions without additional && and || operators, such as: if (msg.sender == 0)

% grep -E "require|if \(" -r . | grep -v "//" | grep -v "&&" | grep -v "||"  | grep ")" | grep .sol | grep "msg.sender" | wc -l
      29

[AUTOMATED-FINDING-17] Use assembly in place of abi.decode to save gas

Instead of using abi.decode, we can use assembly to decode our desired calldata values directly. This will allow us to avoid decoding calldata values that we will not use.

% grep "abi.decode" -r . | grep -v "//" | wc -l
      13

[AUTOMATED-FINDING-18] Counting down in for-loop is more gas efficient

Counting down is more gas efficient than counting up because neither we are making zero variable to non-zero variable and also we will get gas refund in the last transaction when making non-zero to zero variable

% grep "for (" -r . | grep " = 0" | grep "++" | grep -v "/test" | grep ".sol" | wc -l
      18

[AUTOMATED-FINDING-19] do-while is cheaper than for-loops when the initial check can be skipped

Using do-while loops instead of for loops can be more gas-efficient. Even if you add an if condition to account for the case where the loop doesn't execute at all, a do-while loop can still be cheaper in terms of gas.

% grep "for (" -r . | grep " = 0" | grep "++" | grep -v "/test" | grep ".sol" | wc -l
      18

[AUTOMATED-FINDING-20] Use direct unchecked block, instead of importing it from the library

Library UncheckedMath.sol implements a simple uncheckedInc() function which is responsible for performing unchecked incrementation. This function is used in multiple of loops in the code-base. However, it's cheaper to use unchecked block directly, like that:

for (uint256 i = 0; i < length;) {
      ...
      unchecked {
            ++i;
      }
}

instead using imported function:

for (uint256 i = 0; i < length; i = i.uncheckedInc()) {
      ...
}

Below code detects the instances where i.uncheckedInc() is used inside the loops:

% grep "uncheckedInc()" -r . | grep "for (" | wc
      12     154    1772