Freezed Chain will never be unfreeze since `StateTransitionManager::unfreezeChain` is calling `freezeDiamond` instead of `unfreezeDiamond`.

[c4-bot-5]

Lines of code

https://github.com/code-423n4/2024-03-zksync/blob/main/code/contracts/ethereum/contracts/state-transition/StateTransitionManager.sol#L165-L167

Vulnerability details

Vulnerability Details & POC

StateTransitionManager::unfreezeChain function is meant for unfreeze the freezed chain of passed _chainId param. While freezeChain function is meant for freeze the chain according to passed _chainId. But freezeChain and unfreezeChain both functions are calling same function freezeDiamond by same line IZkSyncStateTransition(stateTransition[_chainId]).freezeDiamond() by mistake. So both these function will only freeze the chain.

Also there is no other function inside StateTransitionManager.sol contract which is calling unfreezeDiamond. unfreezeDiamond is function defined in Admin.sol where the call is going since IZkSyncStateTransition also inherits IAdmin which have freezeDiamond and unfreezeDiamond both functions. But unfreezeDiamond is not called from unfreezeChain function. So freezed chain will never be unfreeze.

unfreezeChain also have wrong comment instead of writing unfreezes it writes freezes. It seems like dev just copy pasted without doing required changes.

Vulnerable Code

code/contracts/ethereum/contracts/state-transition/StateTransitionManager.sol#L165-L167

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

164:    /// @dev freezes the specified chain
165:    function unfreezeChain(uint256 _chainId) external onlyOwner {
166:        IZkSyncStateTransition(stateTransition[_chainId]).freezeDiamond();//@audit `freezeDiamond` called instead of `unfreezeDiamond`

    }

(https://github.com/code-423n4/2024-03-zksync/blob/main/code/contracts/ethereum/contracts/state-transition/chain-interfaces/IZkSyncStateTransition.sol#L15)

IZkSyncStateTransition is inheriting IAdmin and by IZkSyncStateTransition wrapping instance is prepared to call freezeDiamond.

15: interface IZkSyncStateTransition is IAdmin, IExecutor, IGetters, IMailbox {

IAdmin interfaces have both functions

13:  interface IAdmin is IZkSyncStateTransitionBase {

    ....

52:  /// @notice Instantly pause the functionality of all freezable facets & their selectors
     /// @dev Only the governance mechanism may freeze Diamond Proxy
54:    function freezeDiamond() external;

     /// @notice Unpause the functionality of all freezable facets & their selectors
     /// @dev Both the admin and the STM can unfreeze Diamond Proxy
58:    function unfreezeDiamond() external;

It shows that by mistake unfreezeChain is calling freezeDiamond instaed of unfreezeDiamond which should be used to unfreeze th chain.

Impact

freezed chain will never be unfreeze. Since freezeChain and unfreezeChain both functions are calling same function freezeDiamond which is used to freeze the chain. And unfreezeDiamond no where called which should is made for unfreeze the freezed chain.

Tools Used

Manual Review

Recommended Mitigation

In StateTransitionManager::unfreezeChain function call unfreezeDiamond instead of freezeDiamond on IZkSyncStateTransition(stateTransition[_chainId]) instance.

File:  code/contracts/ethereum/contracts/state-transition/StateTransitionManager.sol
- 164:    /// @dev freezes the specified chain
+ 164:    /// @dev unfreezes the specified chain
165:    function unfreezeChain(uint256 _chainId) external onlyOwner {
- 166:        IZkSyncStateTransition(stateTransition[_chainId]).freezeDiamond();
+ 166:        IZkSyncStateTransition(stateTransition[_chainId]).unfreezeDiamond();
          }

Assessed type

Other

[c4-judge]

alex-ppg marked the issue as primary issue

[c4-judge]

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

[c4-sponsor]

saxenism (sponsor) confirmed

[c4-sponsor]

saxenism marked the issue as disagree with severity

[saxenism]

This is a good finding, but we consider this a medium severity issue because in the current codebase admin could also unfreeze (so no permanent freeze & so not high), but in the future we might wanna change this mechanism

[c4-judge]

alex-ppg marked the issue as satisfactory

[c4-judge]

alex-ppg marked the issue as selected for report

[alex-ppg]

The submission and its relevant duplicates have identified a mistype in the codebase that causes certain functionality that is expected to be accessible to behave oppositely.

The exhibit represents an actual error in the code resulting in functionality missing, however, the functionality does contain an alternative access path per the Sponsor's statement and as such I consider this exhibit to be of medium risk.

[c4-judge]

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