search

code-423n4 / 2022-10-zksync-findings

3 stars 0 forks source link

QA Report #126

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

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");

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)

TYPOS

    ///@audit: dellegate 
    /// @param initAddress The address that's dellegate called after setting up new facet changes
    ///@audit: Cotract 
    /// @title Diamond Proxy Cotract (EIP-2535)

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);
    event BlocksRevert(uint256 totalBlocksCommitted, uint256 totalBlocksVerified, uint256 totalBlocksExecuted);
    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(
    /// @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(

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:
GalloDaSballo commented 1 year ago

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

L

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

Use of assert() instead of require()

Disagree, in this case it's a dev check, it's fine

Cross-Chain Replay attack

Disputed, the variable is a DX tool, no storage / caching happens

open TODO comments

Disputed per Readme

TYPOS

NC

After further reading, with the timestamp report and the timelock report, am closing as unsatisfactory as I believe most of the report is unfocused and could have been better customized for the sponsor.

c4-judge commented 1 year ago

GalloDaSballo marked the issue as grade-c