Skip to content

Latest commit

 

History

History
165 lines (126 loc) · 16.6 KB

tnevler-Q.md

File metadata and controls

165 lines (126 loc) · 16.6 KB

Report

Low Risk

[L-1]: Missing checks for address(0x0)

Context:

  1. s.governor = _governor; L39
  2. s.validators[_validator] = true; L40
  3. s.pendingGovernor = _newPendingGovernor; L21
  4. l2TokenFactory = _l2TokenFactory; L79
  5. l1Bridge = _l1Bridge; L36
  6. l1Bridge = _l1Bridge; L31

Recommendation:

Add non-zero address checks when set address state variables.

Non-Critical Issues

[N-1]: Function defines a named return variable but then it uses return statements

Context:

https://github.com/code-423n4/2022-10-zksync/blob/main/ethereum/contracts/zksync/facets/Executor.sol#L67-L77

Recommendation:

Choose named return variable or return statement. It is unnecessary to use both.

[N-2]: Wrong order of functions

Context:

  1. Executor.commitBlocks, Executor.executeBlocks, Executor.proveBlocks, Executor.revertBlocks (external functions must be before all public, internal, and private functions)
  2. Mailbox.l2TransactionBaseCost (public function can not go after internal function)
  3. L1ERC20Bridge.claimFailedDeposit (external function can not go after internal function)
  4. L1EthBridge.sol (all internal functions must be after all external functions)
  5. AllowList.sol (all internal functions must be after all external functions)
  6. L2ERC20Bridge.withdraw (external function can not go after internal function)
  7. L2ETHBridge.l2TokenAddress (public function can not go after internal function)

Description:

According to official solidity documentation functions should be grouped according to their visibility and ordered:

  • constructor

  • receive function (if exists)

  • fallback function (if exists)

  • external

  • public

  • internal

  • private

Within a grouping, place the view and pure functions last.

Recommendation:

Put the functions in the correct order according to the documentation.

[N-3]: Public functions can be external

Context:

  1. Mailbox.l2TransactionBaseCost
  2. L2ETHBridge.l2TokenAddress
  3. L2ETHBridge.l1TokenAddress

Description:

Public functions can be declared external if they are not called by the contract.

Recommendation:

Declare these functions as external instead of public.

[N-4]: Require() statements should have descriptive reason string

Context:

  1. require(expectedNumberOfLayer1Txs == _newBlock.numberOfLayer1Txs); L43
  2. require(l2BlockTimestamp == _newBlock.timestamp); L45
  3. require(_recurisiveAggregationInput.length == 4); L297
  4. require(amount != 0); L145
  5. require(_message.length == 56); L221
  6. require(bytes4(functionSignature) == this.finalizeWithdrawal.selector); L224
  7. require(callSuccess); L238
  8. require(_l1Token == CONVENTIONAL_ETH_ADDRESS); L50
  9. require(msg.sender == l2Bridge); L96

[N-5]: NatSpec is missing

Context:

  1. Executor._calculateBlockHash
  2. Executor._blockPassThroughData
  3. Executor._blockMetaParameters
  4. Executor._blockAuxilaryOutput
  5. Mailbox._requestL2Transaction
  6. IDiamondCut.sol
  7. IExecutor.commitBlocks
  8. IExecutor.proveBlocks
  9. IExecutor.executeBlocks
  10. IExecutor.revertBlocks
  11. IGetters.sol
  12. IGovernance.sol
  13. IMailbox.sol
  14. IZkSync.sol
  15. IL1Bridge.sol
  16. IL2Bridge.sol
  17. L2ContractHelper.sol
  18. ReentrancyGuard._initializeReentrancyGuard
  19. UncheckedMath.sol
  20. UnsafeBytes.sol
  21. IAllowList.sol
  22. L2StandardERC20.name
  23. L2StandardERC20.symbol
  24. L2StandardERC20.decimals
  25. IL1Bridge.sol
  26. IL2Bridge.sol
  27. IL2EthInitializable.sol
  28. L2ContractHelper.sol

[N-6]: Typos

Context:

  1. /// @title Diamond Proxy Cotract (EIP-2535) L7 (change Cotract to Contract)
  2. /// @dev The sender is an `address` type, although we are using `uint256` for addreses in `L2CanonicalTransaction`. L36 (change addreses to addresses)
  3. // Check that block contain all meta information for L2 logs. L30 (change contain to contains)
  4. /// @param initAddress The address that's dellegate called after setting up new facet changes L63 (change dellegate to delegate)
  5. /// @param initCalldata Calldata for the delegete call to 'initAddress' L64 (change delegete to delegate)
  6. /// NOTE: It is expected but NOT enforced that there are no selectors associated wih '_facet' L255 (change wih to with)
  7. /// @dev It is standard implementation of ERC20 Bridge that can be used as a refference L19 (change refference to reference)
  8. // We are expecting to see the exect two bytecodes that are needed to initiailize the bridge L76 (change exect to exact)
  9. // We are expecting to see the exect two bytecodes that are needed to initiailize the bridge L76 (change initiailize to initialize)
  10. // Save the deposit amount, to claim funds back if the L2 transaction will failed L106 (change will failed to will fail or will be failed)
  11. /// @notice Address of the L2 token by its L1 couterpart L78 (change couterpart to counterpart)
  12. /// @dev Stores the L1 address of the bridge and set `name`/`symbol`/`decimls` getters that L1 token has. L42 (change decimls to decimals)

[N-7]: TODO

Context:

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

[N-8]: Line is too long

Context:

  1. /// @dev Logically separated part of the storage structure, which is responsible for everything related to proxy upgrades and diamond cuts L9
  2. /// @param securityCouncilMemberLastApprovedProposalId The mapping of the security council addresses and the last diamond cut that they approved L15
  3. /// @notice Total number of executed blocks i.e. blocks[totalBlocksExecuted] points at the latest executed block (block 0 is genesis) L80
  4. /// @param _diamondCutHash The hash of the diamond cut that security council members want to approve. Needed to prevent unintentional approvals, including reorg attacks L104
  5. require(s.diamondCutStorage.securityCouncilMemberLastApprovedProposalId[msg.sender] < currentProposalId, "ao"); // already approved this proposal L108
  6. require(s.diamondCutStorage.proposedDiamondCutHash == _diamondCutHash, "f1"); // proposed diamond cut do not match to the approved L112
  7. /// @param _message Information about the sent message: sender address, the message itself, tx index in the L2 block where the message was sent L23
  8. bytes32 constant DIAMOND_STORAGE_POSITION = 0xc8fcad8db84d3cc18b4c41d551ea0ee66dd599cde068d998e57d5e09332c131b; // keccak256("diamond.standard.diamond.storage") - 1; L14
  9. // - l2ShardId = 0 (means that L1 -> L2 transaction was processed in a rollup shard, other shards are not available yet anyway) L196
  10. // It should be equal to the length of the function signature + address + address + uint256 = 4 + 20 + 20 + 32 = 76 (bytes). L270
  11. /// @param _enables The array of boolean flags, whether enable or disable the public access to the corresponding target address L64
  12. /// @param _enables The array of boolean flags, whether enable or disable the function access to the corresponding target address L88

Description:

Maximum suggested line length is 120 characters.