Upgraded Q -> 2 from #122 [1714762888728]

[c4-judge]

Judge has assessed an item in Issue #122 as 2 risk. The relevant finding follows:

QA-01 A chain that gets frozen by the state transition manager can never be removed from the frozen state in StateTransitionManager.sol

Proof of Concept

Take a look at https://github.com/code-423n4/2024-03-zksync/blob/4f0ba34f34a864c354c7e8c47643ed8f4a250e13/code/contracts/ethereum/contracts/state-transition/StateTransitionManager.sol#L159-L163

    function freezeChain(uint256 _chainId) external onlyOwner {
        IZkSyncStateTransition(stateTransition[_chainId]).freezeDiamond();
    }

This is a special function used by the admin to freeze a specific chain Id, this would come in handy in different context and is placed as a safe measure to help ensure the chain could be frozen to be safe.

Now protocol, logic also includes an unfreezing functionality, but this has been wrongly implemented, see https://github.com/code-423n4/2024-03-zksync/blob/4f0ba34f34a864c354c7e8c47643ed8f4a250e13/code/contracts/ethereum/contracts/state-transition/StateTransitionManager.sol#L164-L168

    function unfreezeChain(uint256 _chainId) external onlyOwner {
        IZkSyncStateTransition(stateTransition[_chainId]).freezeDiamond();
    }

It's evident that where as this function is meant to be used for "unfreezing" the specified chain, it insteads attempt to call freezeDiamond(), now this means that an attempt to call on unfreezeChain() by the owner of the StateTransitionManager.sol contract would always revert since while freezeDiamond() is being executed it checks if the chain Id is frozen already and if yes it reverts (require(!diamondStorage.isFrozen, "a9");), see https://github.com/code-423n4/2024-03-zksync/blob/4f0ba34f34a864c354c7e8c47643ed8f4a250e13/code/contracts/ethereum/contracts/state-transition/chain-deps/facets/Admin.sol#L133-L150

    function freezeDiamond() external onlyAdminOrStateTransitionManager {
        Diamond.DiamondStorage storage diamondStorage = Diamond.getDiamondStorage();

        require(!diamondStorage.isFrozen, "a9"); // diamond proxy is frozen already
        diamondStorage.isFrozen = true;

        emit Freeze();
    }

    /// @inheritdoc IAdmin
    function unfreezeDiamond() external onlyAdminOrStateTransitionManager {
        Diamond.DiamondStorage storage diamondStorage = Diamond.getDiamondStorage();

        require(diamondStorage.isFrozen, "a7"); // diamond proxy is not frozen
        diamondStorage.isFrozen = false;

        emit Unfreeze();
    }

Evidently, this bug is probably due to a copy paste error (i.e copying the implementation of freezeChain() for unfreezeChain() ), as we can see that the dev comment is the same in both instances of freezing the chain.

Impact

Whereas the owner/admin can currently freeze a specified chain, the logic of unfreezing these chains is flawed and as such any chain that gets frozen can't get unfrozen by the state transition Manager.

This was submitted as a QA, cause even though this is a break in important functinoality, if we navigate to these instances in Admin.sol we can see that having the onlyAdminOrStateTransitionManager on the unfreeze() function means that whenver the chain id gets frozen and the state transition manager can't unfreeze it, the admin can just directly call Admin::freezeDiamond() since the modifier allows it

Recommended Mitigation Steps

Consider applying these changes to https://github.com/code-423n4/2024-03-zksync/blob/4f0ba34f34a864c354c7e8c47643ed8f4a250e13/code/contracts/ethereum/contracts/state-transition/StateTransitionManager.sol#L164-L168

-    /// @dev freezes the specified chain
+    /// @dev unfreezes the specified chain
    function unfreezeChain(uint256 _chainId) external onlyOwner {
-        IZkSyncStateTransition(stateTransition[_chainId]).freezeDiamond();
+        IZkSyncStateTransition(stateTransition[_chainId]).unfreezeDiamond();
    }

[c4-judge]

alex-ppg marked the issue as duplicate of #97

[c4-judge]

alex-ppg marked the issue as satisfactory