l2TokenFactory = _l2TokenFactory; l1Bridge = _l1Bridge; l1Bridge = _l1Bridge;Outdated Open Zeppelin version are used. See ethereum/package.json and zksync/package.json
The versions used have known vulnerabilties, including three with high severity. See Security Advisories.
The contract name L2ContractHelper.sol is used twice in the contracts in scope, as shown below.
The existence of two contracts with the same name could cause confusion, as well as compilation problems under some circumstances.
zksync/contracts/L2ContractHelper.sol
ethereum/contracts/common/L2ContractHelper.sol
Similarly for IL1Bridge.sol and IL2Bridge.sol
The current solidity version in contracts is ^0.8.0, compared to the latest version of ^0.8.17. Only the latest version receives security fixes.
Also, five contracts use pragma solidity ^0.8;, which I assume are typos and have included under Typos
While a comment may contain abbreviations, shorthand for well-known or previously defined terms, inconsistent or nonstandard punctuation, and use of minor grammatical errors, it should be readable. In other words, it should communicate clearly, immediately and without ambiguity. The comments below should be improved:
/// `initCalldata` can be arbitrarily. // Check that called contract returns magic value to make sure that contract logic
// supposed to be used as diamond cut initializer. /// @dev Ignore function input and always return zero address as a only one token that the bridge processIn theory, comments over 80 characters should wrap using multi-line syntax.
Even if somewhat longer lines (up to 120 characters) are acceptable and a scroll bar is provided, there are cases where long comments interfere with readability. Below are instances of extra-long lines whose readability could be improved by wrapping.
I've divided them into three types. First are NatSpec statements of at least 120 characters.
Such a statement can be wrapped using a small indentation in the continuation line (to indicate that the line has wrapped), along with a period at its close (to indicate the end of the line), as shown below:
/// @param maxPriorityFeePerErg The additional fee that is paid directly to the validator to incentivize them to include the transaction in a block. Analog to the EIP-1559 `maxPriorityFeePerGas` on an L1 transactionsSuggestion:
/// @param maxPriorityFeePerErg The additional fee that is paid directly to the validator to incentivize them
/// to include the transaction in a block. Analog to the EIP-1559 `maxPriorityFeePerGas` on an L1 transactions.Similarly for the following 33 statements:
The second type are comment lines over 120 characters in length.
Like the first type, such statements can be wrapped using a small indentation in the second line (after the //) along with a closing period, as shown below:
// - l2ShardId = 0 (means that L1 -> L2 transaction was processed in a rollup shard, other shards are not available yet anyway)Suggestion:
// - l2ShardId = 0 (means that L1 -> L2 transaction was processed in a rollup shard,
// other shards are not available yet anyway).Similarly for the following eight cases:
The third type of extra-long line includes both code and comments. If a line containing a comment is longer than 120 characters, it makes sense to put the comment in the line above, as shown:
require(s.diamondCutStorage.securityCouncilMembers[msg.sender], "a9"); // not a security council memberSuggestion:
// not a security council member
require(s.diamondCutStorage.securityCouncilMembers[msg.sender], "a9"); Similarly for the following seven cases:
function _commitOneBlock(StoredBlockInfo memory _previousBlock, CommitBlockInfo calldata _newBlock)
internal
view
returns (StoredBlockInfo memory storedNewBlock)
{
require(_newBlock.blockNumber == _previousBlock.blockNumber + 1, "f"); // only commit next blockThe named return variable storedNewBlock is never used
Comments that refer to open items should be addressed and more fully explained, or else modified or removed. Below are several 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 // Reserved dynamic type for the future use-case. Using it should be avoided,
// But it is still here, just in case we want to enable some additional functionality. /// @dev Ergs limit for requesting L2 deposit finalization transaction
/// NOTE: constant is not calculated accurately, because ergs count in L2 is not stable yet
uint256 constant DEPOSIT_ERGS_LIMIT = 2097152;
/// @dev Ergs limit for requesting L1 -> L2 transaction of deploying L2 bridge instance
/// NOTE: constant is not calculated accurately, because ergs count in L2 is not stable yet
uint256 constant DEPLOY_L2_BRIDGE_COUNTERPART_ERGS_LIMIT = 2097152; * @dev WARNING!
* 1) Functions don't check the length of the bytes array, so it can go out of bounds.
* The user of the library must check for bytes length before using any functions from the library!
*
* 2) Read variables are not cleaned up - https://docs.soliditylang.org/en/v0.8.16/internals/variable_cleanup.html.
* Using data in inline assembly can lead to unexpected behavior!
*/NatSpec statements are partially or wholly missing for some functions, events, structs and constructors. Below are some representative examples.
Note that I am not including the many functions marked private or internal, for which NatSpec is not required.
/// @notice Executes a proposed governor upgrade. Only the current governor can execute the upgrade.
/// NOTE: Governor can execute diamond cut ONLY with proposed `facetCuts` and `initAddress`.
/// `initCalldata` can be arbitrarily.
function executeDiamondCutProposal(Diamond.DiamondCutData calldata _diamondCut) external onlyGovernor {Missing: @param _diamondCut
/// @notice Prove that a specific L2 log was sent in a specific L2 block
/// @param _blockNumber The executed L2 block number in which the log appeared
/// @param _index The position of the l2log in the L2 logs Merkle tree
/// @param _log Information about the sent log
/// @param _proof Merkle proof for inclusion of the L2 log
function proveL2LogInclusion(
uint256 _blockNumber,
uint256 _index,
L2Log memory _log,
bytes32[] calldata _proof
) external view returns (bool) {Missing: @return
NatSpec statements are also partially or wholly missing for the following:
AllowList.sol
DiamondProxy.sol
Executor.sol
L148-153, L205-208, L219-225, L332-336
ExternalDecoder.sol
Getters.sol
L21-22, L26-27, L31-32, L36-37, L41-42, L46-47
L51-52, L56-59, L63-64, L68-69, L73-74, L78-79
L83-86, L90-91, L95-96, L100-101, L105-106
L110-111, L115-116, L120-122, L126-127, L132-133
L145-146, L156-157, L171-172, L177-178, L183-184
L1ERC20Bridge.sol
L1EthBridge.sol
L2ContractHelper.sol
zksync/contracts/L2ContractHelper.sol
L2ContractHelper.sol
ethereum/contracts/common/L2ContractHelper.sol
L2ERC20Bridge.sol
L31-35, L112-113, L30-34, L36-47
L2StandardERC20.sol
L41-43, L100-102, L107-109, L114, L120, L126
Mailbox.sol
Each event should use three indexed fields if there are three or more fields
event DiamondCut(FacetCut[] facetCuts, address initAddress, bytes initCalldata); event EmergencyDiamondCutApproved(
address indexed _address,
uint256 currentProposalId,
uint256 securityCouncilEmergencyApprovals,
bytes32 indexed proposedDiamondCutHash
); event BlocksRevert(uint256 totalBlocksCommitted, uint256 totalBlocksVerified, uint256 totalBlocksExecuted); event NewPriorityRequest(
uint256 txId,
bytes32 txHash,
uint64 expirationBlock,
L2CanonicalTransaction transaction,
bytes[] factoryDeps
); event WithdrawalFinalized(address indexed to, address indexed l1Token, uint256 amount); event ClaimedFailedDeposit(address indexed to, address indexed l1Token, uint256 amount); event BridgeInitialization(address indexed l1Token, string name, string symbol, uint8 decimals);/// @title Diamond Proxy Cotract (EIP-2535)Change Cotract to Contract
/// @dev The sender is an `address` type, although we are using `uint256` for addreses in `L2CanonicalTransaction`.Change addreses to addresses
require(s.totalBlocksExecuted <= s.totalBlocksVerified, "n"); // Can't execute blocks more then committed and proven currently.Change then to than
require(s.totalBlocksCommitted > _newLastBlock, "v1"); // the last committed block is less new last blockChange less new to less than new
// Data that needed for operator to simulate priority queue offchainChange that needed to that is needed or needed
/// @param initAddress The address that's dellegate called after setting up new facet changesChange dellegate to delegate
/// @param initCalldata Calldata for the delegete call to `initAddress`Change delegete to delegate
/// NOTE: It is expected but NOT enforced that there are no selectors associated wih `_facet`Change wih to with
pragma solidity ^0.8;Change ^0.8; to ^0.8.0;`
The same typo appears in the following additional contracts:
/// @dev It is standard implementation of ERC20 Bridge that can be used as a refferenceChange refference to reference
// We are expecting to see the exect two bytecodes that are needed to initiailize the bridgeChange exect to exact
/// NOTE: In order to get funds on L1, receiver should finalise deposit on L1 counterpartChange finalise to finalize. Note that the remaining instances of this word in the contract are spelled using American English ("finalize").
/// @notice Address of the L2 token by its L1 couterpartChange couterpart to counterpart.
The same typo appears in the following line:
/// @dev Stores the L1 address of the bridge and set `name`/`symbol`/`decimls` getters that L1 token has.Change decimls to decimals
Terms incorporating "black," "white," "slave" or "master" are potentially problematic. Substituting more neutral terminology is becoming common practice. I see that the sensitive term 'whitelist' has been avoided by using 'allowlist'. Two of the contracts still use variations of master, which could be converted to 'primary', as shown below.
paymaster: uint256(0),Suggestion: Change paymaster to primarypayer
Similarly for the following instances of paymaster:
paymasterInput: new bytes(0),Suggestion: Change paymasterInput to primarypayerInput
Similarly for the following instances of 'paymasterInput`:
A require message should be included to enable users to understand the reason for failure
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(msg.sender == l2Bridge);