GalloDaSballo
commented
1 year ago L-1 | Missing check for zero address | Low | 7 L
NC-1 | Order of Functions | Non-Critical && NC-2 | Order of Layout | Non-Critical | 2 NC
NC-3 | Public functions can be external | Non-Critical | 2 R
NC-4 | Open TODOs | Non-Critical | 5 NC
NC-5 | Typos in function name / argument name / struct name | Non-Critical & NC-6 | Typos | Non-Critical | 10 NC
NC-7 | No error message in require | Non-Critical | 9 NC
NC-8 | Missing NatSpec | Non-Critical | 13 NC
1L 1R 5NC
c4-judge
QA Report for zkSync v2 contest
Overview
requireLow 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
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:
public function between/after internal:
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:
modifier should be placed before constructor:
#
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
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 laterRecommendation
Resolve issues. #
NC-5. Typos in function name / argument name / struct name
Instances
function _verifyRecursivePartOfProof(uint256[] calldata _recurisiveAggregationInput) {=>recursive(argument)function _blockAuxilaryOutput=>Auxiliaryuint256[] 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(withoutwill)/// @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
requireInstances
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
Recommendation
Add NatSpec for all functions.