lukris02-Q.md

QA Report for zkSync v2 contest

Overview

During the audit, 1 low and 8 non-critical issues were found.

№	Title	Risk Rating	Instance Count
L-1	Missing check for zero address	Low	7
NC-1	Order of Functions	Non-Critical	10
NC-2	Order of Layout	Non-Critical	2
NC-3	Public functions can be external	Non-Critical	2
NC-4	Open TODOs	Non-Critical	5
NC-5	Typos in function name / argument name / struct name	Non-Critical	3
NC-6	Typos	Non-Critical	10
NC-7	No error message in require	Non-Critical	9
NC-8	Missing NatSpec	Non-Critical	13

Low Risk Findings(1)

L-1. Missing check for zero address

Description

If address(0x0) is set it may cause the contract to revert or work wrong.

Instances

• https://github.com/code-423n4/2022-10-zksync/blob/main/ethereum/contracts/zksync/DiamondInit.sol#L27
• https://github.com/code-423n4/2022-10-zksync/blob/main/ethereum/contracts/zksync/DiamondInit.sol#L28
• https://github.com/code-423n4/2022-10-zksync/blob/main/ethereum/contracts/zksync/facets/Governance.sol#L15
• https://github.com/code-423n4/2022-10-zksync/blob/main/ethereum/contracts/bridge/L1ERC20Bridge.sol#L73
• https://github.com/code-423n4/2022-10-zksync/blob/main/ethereum/contracts/bridge/L1ERC20Bridge.sol#L74
• https://github.com/code-423n4/2022-10-zksync/blob/main/zksync/contracts/bridge/L2ERC20Bridge.sol#L32
• https://github.com/code-423n4/2022-10-zksync/blob/main/zksync/contracts/bridge/L2ETHBridge.sol#L30

Recommendation

Add checks.

Non-Critical Risk Findings(8)

NC-1. Order of Functions

Description

According to Style Guide, ordering helps readers identify which functions they can call and to find the constructor and fallback definitions easier.
Functions should be grouped according to their visibility and ordered:

1. constructor
2. receive function (if exists)
3. fallback function (if exists)
4. external
5. public
6. internal
7. private

Instances

exteranl functions between internal:

• https://github.com/code-423n4/2022-10-zksync/blob/main/ethereum/contracts/zksync/facets/Executor.sol#L152
• https://github.com/code-423n4/2022-10-zksync/blob/main/ethereum/contracts/zksync/facets/Executor.sol#L208-L271
• https://github.com/code-423n4/2022-10-zksync/blob/main/ethereum/contracts/zksync/facets/Executor.sol#L336
• https://github.com/code-423n4/2022-10-zksync/blob/main/ethereum/contracts/bridge/L1ERC20Bridge.sol#L179-L257
• https://github.com/code-423n4/2022-10-zksync/blob/main/ethereum/contracts/bridge/L1EthBridge.sol#L140-L211
• https://github.com/code-423n4/2022-10-zksync/blob/main/ethereum/contracts/common/AllowList.sol#L94-L120
• https://github.com/code-423n4/2022-10-zksync/blob/main/zksync/contracts/bridge/L2ERC20Bridge.sol#L91

public function between/after internal:

• https://github.com/code-423n4/2022-10-zksync/blob/main/zksync/contracts/bridge/L2ERC20Bridge.sol#L113
• https://github.com/code-423n4/2022-10-zksync/blob/main/zksync/contracts/bridge/L2ERC20Bridge.sol#L113
• https://github.com/code-423n4/2022-10-zksync/blob/main/zksync/contracts/bridge/L2ETHBridge.sol#L79-L85

Recommendation

Reorder functions where possible.

NC-2. Order of Layout

Description

According to Order of Layout, inside each contract, library or interface, use the following order:

1. Type declarations
2. State variables
3. Events
4. Modifiers
5. Functions

Instances

structs should be placed before event:

• https://github.com/code-423n4/2022-10-zksync/blob/main/ethereum/contracts/zksync/libraries/Diamond.sol#L22-L77

modifier should be placed before constructor:

• https://github.com/code-423n4/2022-10-zksync/blob/main/ethereum/contracts/common/AllowList.sol#L37

NC-3. Public functions can be external

Description

If functions are not called by the contract where they are defined, they can be declared external.

Instances

• https://github.com/code-423n4/2022-10-zksync/blob/main/zksync/contracts/bridge/L2ETHBridge.sol#L79
• https://github.com/code-423n4/2022-10-zksync/blob/main/zksync/contracts/bridge/L2ETHBridge.sol#L84

Recommendation

Make public functions external, where possible.

NC-4. Open TODOs

Instances

• // TODO: change constant to the real root hash of empty Merkle tree (SMA-184)
• // TODO: estimate gas for L1 execute
• // TODO: Restore after stable priority op fee modeling. (SMA-1230)
• layer2Tip: uint192(0) // TODO: Restore after fee modeling will be stable. (SMA-1230)
• /// TODO: The verifier integration is not finished yet, change the structure for compatibility later

Recommendation

Resolve issues.

NC-5. Typos in function name / argument name / struct name

Instances

• function _verifyRecursivePartOfProof(uint256[] calldata _recurisiveAggregationInput) { => recursive (argument)
• function _blockAuxilaryOutput => Auxiliary
• uint256[] recurisiveAggregationInput; => recursive

NC-6. Typos

Instances

• /// @title Diamond Proxy Cotract (EIP-2535)=> Contract
• /// @dev The sender is an `address` type, although we are using `uint256` for addreses in `L2CanonicalTransaction`.=> addresses
• /// @param initAddress The address that's dellegate called after setting up new facet changes => delegate
• /// @param initCalldata Calldata for the delegete call to `initAddress => delegate
• /// NOTE: It is expected but NOT enforced that there are no selectors associated wih `_facet => with
• /// @dev It is standard implementation of ERC20 Bridge that can be used as a refference => reference
• // We are expecting to see the exect two bytecodes that are needed to initiailize the bridge => initialize
• // Save the deposit amount, to claim funds back if the L2 transaction will failed => transaction failed (without will)
• /// @notice Address of the L2 token by its L1 couterpart => counterpart
• /// @dev Stores the L1 address of the bridge and set `name`/`symbol`/`decimls` getters that L1 token has. => decimals

NC-7. No error message in require

Instances

• require(expectedNumberOfLayer1Txs == _newBlock.numberOfLayer1Txs);
• require(l2BlockTimestamp == _newBlock.timestamp);
• require(_recurisiveAggregationInput.length == 4);
• require(amount != 0);
• require(_message.length == 56);
• require(bytes4(functionSignature) == this.finalizeWithdrawal.selector);
• require(callSuccess);
• require(_l1Token == CONVENTIONAL_ETH_ADDRESS);
• require(msg.sender == l2Bridge);

Recommendation

Add error messages.

NC-8. Missing NatSpec

Description

NatSpec is missing for 13 functions in 5 contracts.

Instances

• https://github.com/code-423n4/2022-10-zksync/blob/main/ethereum/contracts/zksync/facets/Executor.sol#L80
• https://github.com/code-423n4/2022-10-zksync/blob/main/ethereum/contracts/zksync/facets/Executor.sol#L362
• https://github.com/code-423n4/2022-10-zksync/blob/main/ethereum/contracts/zksync/facets/Executor.sol#L372
• https://github.com/code-423n4/2022-10-zksync/blob/main/ethereum/contracts/zksync/facets/Executor.sol#L376
• https://github.com/code-423n4/2022-10-zksync/blob/main/ethereum/contracts/zksync/facets/Mailbox.sol#L116
• https://github.com/code-423n4/2022-10-zksync/blob/main/ethereum/contracts/zksync/interfaces/IExecutor.sol#L62
• https://github.com/code-423n4/2022-10-zksync/blob/main/ethereum/contracts/zksync/interfaces/IExecutor.sol#L65
• https://github.com/code-423n4/2022-10-zksync/blob/main/ethereum/contracts/zksync/interfaces/IExecutor.sol#L71
• https://github.com/code-423n4/2022-10-zksync/blob/main/ethereum/contracts/zksync/interfaces/IExecutor.sol#L73
• https://github.com/code-423n4/2022-10-zksync/blob/main/ethereum/contracts/common/ReentrancyGuard.sol#L43
• https://github.com/code-423n4/2022-10-zksync/blob/main/zksync/contracts/bridge/L2StandardERC20.sol#L114
• https://github.com/code-423n4/2022-10-zksync/blob/main/zksync/contracts/bridge/L2StandardERC20.sol#L120
• https://github.com/code-423n4/2022-10-zksync/blob/main/zksync/contracts/bridge/L2StandardERC20.sol#L126

Recommendation

Add NatSpec for all functions.