c4-bot-7 commented 4 months ago

Lines of code

https://github.com/code-423n4/2024-03-zksync/blob/4f0ba34f34a864c354c7e8c47643ed8f4a250e13/code/contracts/ethereum/contracts/state-transition/StateTransitionManager.sol#L166

Vulnerability details

Impact

This bug hampers the StateTransitionManager's ability to manage chain freezing.

Proof of Concept

In StateTransitionManager.sol there are two functions available to freeze and unfreeze a chain:

freezeChain

/// @dev freezes the specified chain
function freezeChain(uint256 _chainId) external onlyOwner {
    IZkSyncStateTransition(stateTransition[_chainId]).freezeDiamond();
}

unfreezeChain

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

The problem is that both functions are calling freezeDiamond() which will freeze the chain. freezeDiamond():

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

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

    emit Freeze();
}

It means that if a chain gets freezed by StateTransitionManager or the Admin of a chain, StateTransitionManager won't be able to unfreeze the chain again.

Note that the admin of a chainId is able to call unfreezeDiamond directly from the Admin.sol facet, but this does not mitigate the bug in the unfreezeChain function. Despite the admin's capability to unfreeze the chain, the bug persists as the StateTransitionManager should adhere to the invariants by being able to both freeze and unfreeze a chain.

Tools Used

VSCode

Recommended Mitigation Steps

diff --git a/StateTransitionManager.sol.orig b/StateTransitionManager.sol
index 0c27439..c74df7a 100644
--- a/StateTransitionManager.sol.orig
+++ b/StateTransitionManager.sol
@@ -161,9 +161,9 @@ contract StateTransitionManager is IStateTransitionManager, ReentrancyGuard, Own
         IZkSyncStateTransition(stateTransition[_chainId]).freezeDiamond();
     }

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

     /// @dev reverts batches on the specified chain

Assessed type

Error

c4-judge commented 3 months ago

alex-ppg marked the issue as duplicate of #97

c4-judge commented 3 months ago

alex-ppg changed the severity to 3 (High Risk)

c4-judge commented 2 months ago

alex-ppg changed the severity to 2 (Med Risk)