Skip to content

Latest commit

 

History

History
274 lines (210 loc) · 14.8 KB

B2-Q.md

File metadata and controls

274 lines (210 loc) · 14.8 KB

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

Missing checks for zero-addresses may lead to infunctional protocol, if the variable addresses are updated incorrectly.

        l1Bridge = _l1Bridge;

File: contracts/bridge/L2ETHBridge.sol (line 31)

       l1Bridge = _l1Bridge;

File: contracts/bridge/L2ERC20Bridge.sol (line 36)

Upgradeable contract is missing a __gap[50] storage variable to allow for new storage variables in later versions

While some contracts may not currently be sub-classed, adding the variable now protects against forgetting to add it in the future.

	contract L1ERC20Bridge is IL1Bridge, AllowListed, ReentrancyGuard {

File: contracts/bridge/L1ERC20Bridge.sol (line 21)

	contract L1EthBridge is IL1Bridge, AllowListed, ReentrancyGuard {

File: contracts/bridge/L1EthBridge.sol#L17 (line 17)

	Other instances of this issue are:

Use of assert() instead of require()

Contracts use assert() instead of require() in multiple places. This causes a Panic error on failure and prevents the use of error strings.

Prior to solc 0.8.0, assert() used the invalid opcode which used up all the remaining gas while require() used the revert opcode which refunded the gas and therefore the importance of using require() instead of assert() was greater. However, after 0.8.0, assert() uses revert opcode just like require() but creates a Panic(uint256) error instead of Error(string) created by require().

        assert(SECURITY_COUNCIL_APPROVALS_FOR_EMERGENCY_UPGRADE > 0);

File: contracts/zksync/facets/DiamondCut.sol (line 16)

Cross-Chain Replay attack

Storing the block.chainid is not safe.

        require(_chainId == block.chainid, "pr");
  • File: /contracts/zksync/DiamondProxy.sol (line 13)

open TODO comments

Code architecture, incentives, and error handling/reporting questions/issues should be resolved before deployment.

	 // TODO: change constant to the real root hash of empty Merkle tree (SMA-184)
  • File: contracts/zksync/Config.sol (line 28)
       // TODO: estimate gas for L1 execute
  • File: contracts/zksync/facets/Mailbox.sol (line 94)
       // TODO: Restore after stable priority op fee modeling. (SMA-1230)
  • File: contracts/zksync/facets/Mailbox.sol (line 127)
                layer2Tip: uint192(0) // TODO: Restore after fee modeling will be stable. (SMA-1230)
  • File: contracts/zksync/facets/Mailbox.sol (line 169)

TYPOS

    ///@audit: dellegate 
    /// @param initAddress The address that's dellegate called after setting up new facet changes
  • File: contracts/zksync/libraries/Diamond.sol (line 63)
    ///@audit: Cotract 
    /// @title Diamond Proxy Cotract (EIP-2535)
  • File: contracts/zksync/DiamondProxy.sol (line 7)
    ///@audit: refference 
    /// @dev It is standard implementation of ERC20 Bridge that can be used as a refference
  • File: contracts/bridge/L1ERC20Bridge.sol (line 19)
    ///@audit: exect 
    ///audit: initiailize 
    // We are expecting to see the exect two bytecodes that are needed to initiailize the bridge
  • File: contracts/bridge/L1ERC20Bridge.sol (line 76)

Event is missing indexed fields

Each event should use three indexed fields if there are three or more fields.

    event DiamondCut(FacetCut[] facetCuts, address initAddress, bytes initCalldata);
  • File: /contracts/zksync/libraries/Diamond.sol (line 16)
    event BlocksRevert(uint256 totalBlocksCommitted, uint256 totalBlocksVerified, uint256 totalBlocksExecuted);
  • File: contracts/zksync/interfaces/IExecutor.sol (line 85)
    event NewPriorityRequest(
        uint256 txId,
        bytes32 txHash,
        uint64 expirationBlock,
        L2CanonicalTransaction transaction,
        bytes[] factoryDeps

Use of block.timestamp

Block timestamps have historically been used for a variety of applications, such as entropy for random numbers, locking funds for periods of time, and various state-changing conditional statements that are time-dependent. Miners have the ability to adjust timestamps slightly, which can prove to be dangerous if block timestamps are used incorrectly in smart contracts.

        s.diamondCutStorage.proposedDiamondCutTimestamp = block.timestamp;

File: contracts/zksync/facets/DiamondCut.sol#L28 (line 28)

        bool upgradeNoticePeriodPassed = block.timestamp >=

File: contracts/zksync/facets/DiamondCut.sol (line 52)

        s.diamondCutStorage.lastDiamondFreezeTimestamp = block.timestamp;

File: contracts/zksync/facets/DiamondCut.sol (line 85)

            bool timestampNotTooSmall = block.timestamp - COMMIT_TIMESTAMP_NOT_OLDER <= l2BlockTimestamp;

File: contracts/zksync/facets/Executor.sol (line 50)

            bool timestampNotTooBig = l2BlockTimestamp <= block.timestamp + COMMIT_TIMESTAMP_APPROXIMATION_DELTA;

File: contracts/zksync/facets/Executor.sol (line 51)

Require /revert should have descriptive reason strings

        require(l2BlockTimestamp == _newBlock.timestamp);

File: contracts/zksync/facets/Executor.sol (line 45)

                revert(ptr, size)

File: contracts/zksync/DiamondProxy.sol (line 47)

	Other instances of the issue are:

Set garbage value in mapping for deleting that

If there is a mapping data structure present inside struct, then deleting the struct doesn't delete the mapping. Instead one should use lock to lock that data structure from further use.

        delete depositAmount[_depositSender][_l1Token][_l2TxHash];

File: contracts/bridge/L1ERC20Bridge.sol (line 212)

        delete depositAmount[_depositSender][_l2TxHash];

File: contracts/bridge/L1EthBridge.sol (line 169)

NatSpec is incomplete

    /// @audit Missing: '@param'
    /// @dev Add new functions to the diamond proxy
    /// NOTE: expect but NOT enforce that `_selectors` is NON-EMPTY array
    function _addFunctions(
  • File: contracts/zksync/libraries/Diamond.sol [(line 117-119)](
    /// @audit Missing: '@param'
    /// @dev Change associated facets to already known function selectors
    /// NOTE: expect but NOT enforce that `_selectors` is NON-EMPTY array
    function _replaceFunctions(

	Other instances of the issue are:

Missing event and timelock for critical parameter change

Events help non-contract tools to track changes, and events prevent users from being surprised by changes.

    ) external onlyOwner {

File: contracts/common/AllowList.sol (line 94)

    ) external onlyOwner {

File: /contracts/common/AllowList.sol (line 118)

	Other instances of the issue are: