zkSync v2 contest - jayjonah8's results

Lines of code

https://github.com/code-423n4/2022-10-zksync/blob/main/ethereum/contracts/zksync/DiamondInit.sol#L25 https://github.com/code-423n4/2022-10-zksync/blob/main/ethereum/contracts/zksync/facets/DiamondCut.sol#L106

Vulnerability details

Impact

As stated in the Github repo documentation, there are 3 steps to a system upgrade in the `DiamondCutFacet.sol'.

1. propose an upgrade by the governor by calling the proposeDiamondCut function.
2. approve the upgrade by security council by calling the approveEmergencyDiamondCutAsSecurityCouncilMember function.
3. finalize the upgrade by the governor calling the executeDiamondCutProposal function

The problem is that the security council members are never added in DiamondInit.sol initialize function or anywhere else in the code base. The securityCouncilMembers mapping is never modified making it impossible to perform upgrades/adding or deleting new diamond cut facets in the correct manner.

Proof of Concept

https://github.com/code-423n4/2022-10-zksync/blob/main/ethereum/contracts/zksync/DiamondInit.sol#L25

https://github.com/code-423n4/2022-10-zksync/blob/main/ethereum/contracts/zksync/facets/DiamondCut.sol#L106

Tools Used

Manual code review

Recommended Mitigation Steps

Security council members should be added when the initialize function is called in DiamondInit.sol

function initialize(
        Verifier _verifier,
        address _governor,
        address _validator,
        bytes32 _genesisBlockHash,
        uint64 _genesisIndexRepeatedStorageChanges,
        bytes32 _genesisBlockCommitment,
        IAllowList _allowList,
        VerifierParams calldata _verifierParams,
        bool _zkPorterIsAvailable,
        bytes32 _l2BootloaderBytecodeHash,
        bytes32 _l2DefaultAccountBytecodeHash,
+      address[] _securityCouncilMembers
    ) external reentrancyGuardInitializer returns (bytes32) {
        s.verifier = _verifier;
        s.governor = _governor;
        s.validators[_validator] = true;


       // mapping(address => bool) securityCouncilMembers;
+     require(_securityCouncilMembers.length > 0, "Add security council");

+     for (uint256 i = 0; i < _securityCouncilMembers.length; i++) {
+
+          address councilMember = _securityCouncilMembers[i];
+          require(councilMember != address(0), "Zero Address");

+          s.diamondCutStorage.securityCouncilMembers[councilMember] = true;
+      }

        // We need to initialize the state hash because it is used in the commitment of the next block
        IExecutor.StoredBlockInfo memory storedBlockZero = IExecutor
            .StoredBlockInfo( // @audit-info check if this should be Executor.StoredBlockInfo instead of IExecutor.StoredBlockInfo since a Interface should not return any logic
            0,
            _genesisBlockHash,
            _genesisIndexRepeatedStorageChanges,
            0,
            EMPTY_STRING_KECCAK,
            DEFAULT_L2_LOGS_TREE_ROOT_HASH,
            0,
            _genesisBlockCommitment
        );

        s.storedBlockHashes[0] = keccak256(abi.encode(storedBlockZero));
        s.allowList = _allowList;
        s.verifierParams = _verifierParams;
        s.zkPorterIsAvailable = _zkPorterIsAvailable;
        s.l2BootloaderBytecodeHash = _l2BootloaderBytecodeHash;
        s.l2DefaultAccountBytecodeHash = _l2DefaultAccountBytecodeHash;

        return Diamond.DIAMOND_INIT_SUCCESS_RETURN_VALUE;
    }