c4-bot-10 commented 7 months ago

Lines of code

https://github.com/code-423n4/2024-03-zksync/blob/main/code/contracts/ethereum/contracts/state-transition/StateTransitionManager.sol#L165 https://github.com/code-423n4/2024-03-zksync/blob/main/code/contracts/ethereum/contracts/state-transition/chain-deps/facets/Admin.sol#L143

Vulnerability details

Impact

Denial of Service When State Transition Manager wants to Unfreeze Chain as the unfreeze function calls freezeDiamond() again instead of calling unfreezeDiamond()

Proof of Concept

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

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

The code above from the StateTransitionManager contract shows how freezing and unfreezing is done, however there is a mistake with the implementation of the unfreeze function, it calls freezeDiamond() instead of unfreezeDiamond() which will cause Denial of Service and unfreezing would never be possible again. The code provided below is from the Admin.sol contract, it shows there is indeed an implementation for both freezing and unfreezing which proves that the implementation in the unfreezeChain(...) function in the StateTransitionManager contract was indeed an error which would break protocol functionality.

   /// @inheritdoc IAdmin
    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();
    }

Tools Used

Manual Review

Recommended Mitigation Steps

The protocol should make necessary correction as provided below

 /// @dev freezes the specified chain
    function freezeChain(uint256 _chainId) external onlyOwner {
        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();
    }

Assessed type

Error

c4-judge commented 7 months ago

alex-ppg marked the issue as duplicate of #97

c4-judge commented 6 months ago

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