QA

Low

Emitted amount can be bigger than expected

Impact

There are ERC20 tokens with transfer at fees. For checking if the transferred amount is the same as expected, code already compares balanceOf before and balanceOf after transfer. People can get confused in cases where real value doesn't match

Github Permalinks

https://github.com/code-423n4/2022-10-zksync/blob/5a31c9db8ab32175dbd7264b05ce84931b6c0428/ethereum/contracts/bridge/L1ERC20Bridge.sol#L214-L216 https://github.com/code-423n4/2022-10-zksync/blob/5a31c9db8ab32175dbd7264b05ce84931b6c0428/ethereum/contracts/bridge/L1ERC20Bridge.sol#L254-L256

Mitigation

Consider implementing same system as implemented: https://github.com/code-423n4/2022-10-zksync/blob/5a31c9db8ab32175dbd7264b05ce84931b6c0428/ethereum/contracts/bridge/L1ERC20Bridge.sol#L141-L143

Variable shadows another variable

Summary

Name shadowing where two or more variables/functions share the same name could be confusing to developers and/or reviewers

Details

Use of facets as input variable shadows facets()

Github Permalinks

function
https://github.com/code-423n4/2022-10-zksync/blob/5a31c9db8ab32175dbd7264b05ce84931b6c0428/ethereum/contracts/zksync/interfaces/IGetters.sol#L66
variable https://github.com/code-423n4/2022-10-zksync/blob/5a31c9db8ab32175dbd7264b05ce84931b6c0428/ethereum/contracts/zksync/interfaces/IGetters.sol#L70

Mitigation

Replace facets variable in the function parameter to _facets, facets_ or a similar substitution

Use of hardcoded amount of days

Summary

Formula uses a hardcoded value of 365 (days) which would be wrong applied in a leap year (366 days)

Github Permalinks

https://github.com/code-423n4/2022-10-zksync/blob/5a31c9db8ab32175dbd7264b05ce84931b6c0428/ethereum/contracts/zksync/Config.sol#L64 defined(COMMIT_TIMESTAMP_NOT_OLDER) ? COMMIT_TIMESTAMP_NOT_OLDER : "365 days"

https://github.com/code-423n4/2022-10-zksync/blob/5a31c9db8ab32175dbd7264b05ce84931b6c0428/ethereum/contracts/zksync/Config.sol#L70 defined(COMMIT_TIMESTAMP_APPROXIMATION_DELTA) ? COMMIT_TIMESTAMP_APPROXIMATION_DELTA : "365 days"

Mitigation

Consider using an oracle for this Consider using a method that change the value between 365 and 366 for the operations in leap years and regular years

Missing checks for address(0x0) when assigning values to `address` state or `immutable` variables

Summary

Zero address should be checked for state variables, immutable variables. A zero address can lead into unexpected behaviors.

Github Permalinks

https://github.com/code-423n4/2022-10-zksync/blob/5a31c9db8ab32175dbd7264b05ce84931b6c0428/ethereum/contracts/bridge/L1ERC20Bridge.sol#L57-L60 https://github.com/code-423n4/2022-10-zksync/blob/5a31c9db8ab32175dbd7264b05ce84931b6c0428/ethereum/contracts/bridge/L1EthBridge.sol#L48-L51 https://github.com/code-423n4/2022-10-zksync/blob/5a31c9db8ab32175dbd7264b05ce84931b6c0428/zksync/contracts/bridge/L2ERC20Bridge.sol#L36 https://github.com/code-423n4/2022-10-zksync/blob/5a31c9db8ab32175dbd7264b05ce84931b6c0428/zksync/contracts/bridge/L2ERC20Bridge.sol#L41 https://github.com/code-423n4/2022-10-zksync/blob/5a31c9db8ab32175dbd7264b05ce84931b6c0428/zksync/contracts/bridge/L2ETHBridge.sol#L31 https://github.com/code-423n4/2022-10-zksync/blob/5a31c9db8ab32175dbd7264b05ce84931b6c0428/ethereum/contracts/bridge/L1ERC20Bridge.sol#L79 https://github.com/code-423n4/2022-10-zksync/blob/5a31c9db8ab32175dbd7264b05ce84931b6c0428/ethereum/contracts/zksync/DiamondInit.sol#L38-L39

Mitigation

Check zero address before assigning or using it

block.timestamp used as time proxy

Summary

Risk of using block.timestamp for time should be considered.

Details

block.timestamp is not an ideal proxy for time because of issues with synchronization, miner manipulation and changing block times.

This kind of issue may affect the code allowing or reverting the code before the expected deadline, modifying the normal functioning or reverting sometimes.

References

SWC ID: 116

Github Permalinks

Dangerous comparisons with block.timestamp https://github.com/code-423n4/2022-10-zksync/blob/5a31c9db8ab32175dbd7264b05ce84931b6c0428/ethereum/contracts/zksync/facets/DiamondCut.sol#L52 bool upgradeNoticePeriodPassed = block.timestamp >=

https://github.com/code-423n4/2022-10-zksync/blob/5a31c9db8ab32175dbd7264b05ce84931b6c0428/ethereum/contracts/zksync/facets/Executor.sol#L45 require(l2BlockTimestamp == _newBlock.timestamp);

https://github.com/code-423n4/2022-10-zksync/blob/5a31c9db8ab32175dbd7264b05ce84931b6c0428/ethereum/contracts/zksync/facets/Executor.sol#L50 bool timestampNotTooSmall = block.timestamp - COMMIT_TIMESTAMP_NOT_OLDER <= l2BlockTimestamp;

https://github.com/code-423n4/2022-10-zksync/blob/5a31c9db8ab32175dbd7264b05ce84931b6c0428/ethereum/contracts/zksync/facets/Executor.sol#L51 bool timestampNotTooBig = l2BlockTimestamp <= block.timestamp + COMMIT_TIMESTAMP_APPROXIMATION_DELTA;

https://github.com/code-423n4/2022-10-zksync/blob/5a31c9db8ab32175dbd7264b05ce84931b6c0428/ethereum/contracts/zksync/facets/Executor.sol#L52 require(timestampNotTooSmall, "h"); // New block timestamp is too small

https://github.com/code-423n4/2022-10-zksync/blob/5a31c9db8ab32175dbd7264b05ce84931b6c0428/ethereum/contracts/zksync/facets/Executor.sol#L53 require(timestampNotTooBig, "h1"); // New block timestamp is too big

Time proxy storage https://github.com/code-423n4/2022-10-zksync/blob/5a31c9db8ab32175dbd7264b05ce84931b6c0428/ethereum/contracts/zksync/facets/DiamondCut.sol#L28 s.diamondCutStorage.proposedDiamondCutTimestamp = block.timestamp;

https://github.com/code-423n4/2022-10-zksync/blob/5a31c9db8ab32175dbd7264b05ce84931b6c0428/ethereum/contracts/zksync/facets/DiamondCut.sol#L85 s.diamondCutStorage.lastDiamondFreezeTimestamp = block.timestamp; https://github.com/code-423n4/2022-10-zksync/blob/5a31c9db8ab32175dbd7264b05ce84931b6c0428/ethereum/contracts/zksync/facets/Executor.sol#L75 _newBlock.timestamp,

Mitigation

Consider the risk of using block.timestamp as time proxy and evaluate if block numbers can be used as an approximation for the application logic. Both have risks that need to be factored in.
Consider using an oracle for precision

Front run initializer

Summary

The initialize function that initializes important contract state can be called by anyone.

Details

The attacker can initialize the contract before the legitimate deployer, hoping that the victim continues to use the same contract.

In the best case for the victim, they notice it and have to redeploy their contract costing gas.

Github Permalinks

https://github.com/code-423n4/2022-10-zksync/blob/5a31c9db8ab32175dbd7264b05ce84931b6c0428/ethereum/contracts/zksync/DiamondInit.sol#L25 function initialize(

https://github.com/code-423n4/2022-10-zksync/blob/5a31c9db8ab32175dbd7264b05ce84931b6c0428/ethereum/contracts/bridge/L1ERC20Bridge.sol#L71 function initialize(

https://github.com/code-423n4/2022-10-zksync/blob/5a31c9db8ab32175dbd7264b05ce84931b6c0428/ethereum/contracts/bridge/L1EthBridge.sol#L56 function initialize(bytes calldata _l2BridgeBytecode) external reentrancyGuardInitializer {

https://github.com/code-423n4/2022-10-zksync/blob/5a31c9db8ab32175dbd7264b05ce84931b6c0428/zksync/contracts/bridge/L2StandardERC20.sol#L43 function bridgeInitialize(address _l1Address, bytes memory _data) external initializer {

Mitigation

Use the constructor to initialize non-proxied contracts.

For initializing proxy contracts deploy contracts using a factory contract that immediately calls initialize after deployment or make sure to call it immediately after deployment and verify the transaction succeeded.

Return value not being checked

Details

Return values not being checked may lead into unexpected behaviors with functions. Not events/Error are being emitted if that fails, so functions would be called even of not being working as expect.

Github Permalinks

ignores return value by zkSyncMailbox.requestL2Transaction https://github.com/code-423n4/2022-10-zksync/blob/5a31c9db8ab32175dbd7264b05ce84931b6c0428/ethereum/contracts/bridge/L1EthBridge.sol#L73-L79 https://github.com/code-423n4/2022-10-zksync/blob/5a31c9db8ab32175dbd7264b05ce84931b6c0428/ethereum/contracts/bridge/L1ERC20Bridge.sol#L96-L102

Mitigation

Check values and revert/emit events if needed

`abi.encodePacked()` should not be used with dynamic types when passing the result to a hash function such as `keccack256()`

Summary

If you are dealing with more than one dynamic data type, abi.encodePacked() can lead to collisions when used with a hash function.

Use abi.encode() instead which will pad items to 32 bytes, which will prevent hash collisions (e.g. abi.encodePacked(0x123,0x456) => 0x123456 => abi.encodePacked(0x1,0x23456), but abi.encode(0x123,0x456) => 0x0...1230...456). “Unless there is a compelling reason, abi.encode should be preferred”. If there is only one argument to abi.encodePacked() it can often be cast to bytes() or bytes32() instead.

Details

abi.encodePacked will only use the only use the minimal required memory to encode the data. E.g. an address will only use 20 bytes and for dynamic arrays only the elements will be stored without length. For more info see the Solidity docs for packed mode.

For the input of the keccak method it is important that you can ensure that the resulting bytes of the encoding are unique. So if you always encode the same types and arrays always have the same length then there is no problem. But if you switch the parameters that you encode or encode multiple dynamic arrays you might have conflicts.

https://ethereum.stackexchange.com/questions/119583/when-to-use-abi-encode-abi-encodepacked-or-abi-encodewithsignature-in-solidity

Github Permalinks

https://github.com/code-423n4/2022-10-zksync/blob/5a31c9db8ab32175dbd7264b05ce84931b6c0428/ethereum/contracts/zksync/facets/Executor.sol#L373 https://github.com/code-423n4/2022-10-zksync/blob/5a31c9db8ab32175dbd7264b05ce84931b6c0428/ethereum/contracts/zksync/facets/Executor.sol#L282-L291

Mitigation

Change abi.encodePacked to abi.encode when data collision may happen

Use of `asserts()`

Impact

From solidity docs: Properly functioning code should never reach a failing assert statement; if this happens there is a bug in your contract which you should fix. With assert the user pays the gas and with require it doesn't. The ETH network gas isn't cheap and users can see it as a scam. You have reachable asserts in the following locations (which should be replaced by require / are mistakenly left from development phase):

Details

The Solidity assert() function is meant to assert invariants. Properly functioning code should never reach a failing assert statement. A reachable assertion can mean one of two things:

A bug exists in the contract that allows it to enter an invalid state; The assert statement is used incorrectly, e.g. to validate inputs.

References

SWC ID: 110

Github Permalinks

https://github.com/code-423n4/2022-10-zksync/blob/5a31c9db8ab32175dbd7264b05ce84931b6c0428/ethereum/contracts/zksync/facets/DiamondCut.sol#L16 assert(SECURITY_COUNCIL_APPROVALS_FOR_EMERGENCY_UPGRADE > 0);

Recommended Mitigation Steps

Substitute asserts with require/revert.

Informational

Inconsistent version of english being used

Some functions use American English, whereas others use British English. A single project should use only one of the two

Github Permalinks

Finalise & finalize https://github.com/code-423n4/2022-10-zksync/blob/5a31c9db8ab32175dbd7264b05ce84931b6c0428/zksync/contracts/bridge/L2ETHBridge.sol#L58 /// NOTE: In order to get funds on L1, receiver should finalise deposit on L1 counterpart https://github.com/code-423n4/2022-10-zksync/blob/5a31c9db8ab32175dbd7264b05ce84931b6c0428/zksync/contracts/bridge/L2ETHBridge.sol#L35-L41


    /// @dev Finalize the deposit and mint ether to the deposited address
    /// @param _l1Sender The account address that initiate the deposit on L1
    /// @param _l2Receiver The account address that would receive minted ether
    /// @param _l1Token The address of the token that was locked on the L1. Always should be equal to zero (conventional value)
    /// @param _amount Total amount of ether deposited from L1
    function finalizeDeposit(

Missing indexed event parameters

Summary

Events without indexed event parameters make it harder and inefficient for off-chain tools to analyze them.

Details

Indexed parameters (“topics”) are searchable event parameters. They are stored separately from unindexed event parameters in an efficient manner to allow for faster access. This is useful for efficient off-chain-analysis, but it is also more costly gas-wise.

Github Permalinks

https://github.com/code-423n4/2022-10-zksync/blob/5a31c9db8ab32175dbd7264b05ce84931b6c0428/ethereum/contracts/zksync/libraries/Diamond.sol#L16 event DiamondCut(FacetCut[] facetCuts, address initAddress, bytes initCalldata);

https://github.com/code-423n4/2022-10-zksync/blob/5a31c9db8ab32175dbd7264b05ce84931b6c0428/ethereum/contracts/zksync/interfaces/IDiamondCut.sol#L20 event DiamondCutProposal(Diamond.FacetCut[] _facetCuts, address _initAddress);

https://github.com/code-423n4/2022-10-zksync/blob/5a31c9db8ab32175dbd7264b05ce84931b6c0428/ethereum/contracts/zksync/interfaces/IDiamondCut.sol#L24 event DiamondCutProposalExecution(Diamond.DiamondCutData _diamondCut);

https://github.com/code-423n4/2022-10-zksync/blob/5a31c9db8ab32175dbd7264b05ce84931b6c0428/ethereum/contracts/zksync/interfaces/IDiamondCut.sol#L26 event EmergencyFreeze();

https://github.com/code-423n4/2022-10-zksync/blob/5a31c9db8ab32175dbd7264b05ce84931b6c0428/ethereum/contracts/zksync/interfaces/IDiamondCut.sol#L28 event Unfreeze(uint256 lastDiamondFreezeTimestamp);

https://github.com/code-423n4/2022-10-zksync/blob/5a31c9db8ab32175dbd7264b05ce84931b6c0428/ethereum/contracts/zksync/interfaces/IExecutor.sol#L85 event BlocksRevert(uint256 totalBlocksCommitted, uint256 totalBlocksVerified, uint256 totalBlocksExecuted);

https://github.com/code-423n4/2022-10-zksync/blob/5a31c9db8ab32175dbd7264b05ce84931b6c0428/ethereum/contracts/zksync/interfaces/IGovernance.sol#L32 event IsPorterAvailableStatusUpdate(bool isPorterAvailable);

https://github.com/code-423n4/2022-10-zksync/blob/5a31c9db8ab32175dbd7264b05ce84931b6c0428/ethereum/contracts/zksync/interfaces/IGovernance.sol#L48 event NewVerifierParams(VerifierParams oldVerifierParams, VerifierParams newVerifierParams);

https://github.com/code-423n4/2022-10-zksync/blob/5a31c9db8ab32175dbd7264b05ce84931b6c0428/ethereum/contracts/zksync/interfaces/IMailbox.sol#L95 event NewPriorityRequest(

Mitigation

Consider which event parameters could be particularly useful to off-chain tools and should be indexed.

Bad order of code

Summary

Clearness of the code is important for the readability and maintainability. As Solidity guidelines says about declaration order: 1.Type declarations 2.State variables 3.Events 4.Modifiers 5.Functions Also, state variables order affects to gas in the same way as ordering structs for saving storage slots