c4-bot-5 commented 8 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

A typo in stateTransistionManager.unfreezeChain would cause a DOS when the stateTransitionManager attempts to unfreeze a previously frozen chain. It would revert due to the check in Admin.freezeDiamond which requires that the chain is not already frozen. This means the stateTransitionManager would not be able to unfreeze any chain that had already been frozen using the stateTransitionManager.unfreezeChain. This would cause any chain which has been frozen to remain frozen until the contract or facets are redeployed, effectively DOSsing that chain.

src: code/contracts/ethereum/contracts/state-transition/StateTransitionManager.sol

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

Proof of Concept

The function stateTransitionManager.unfreezeChain which would be called by an admin/owner when attempting to unfreeze a previously frozen chain would always revert because this function instead would call the Admin.freezeDiamond instead of Admin.unfreezeDiamond. Due to checks in the freezeDiamond function, since this chain is already frozen, this would cause a revert.

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

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

        emit Freeze();
    }

Tools Used

Manual review

Recommended Mitigation Steps

Change freezeDiamond() to unfreezeDiamond which is the expected logic.

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

Assessed type

DoS

c4-judge commented 8 months ago

alex-ppg marked the issue as duplicate of #97

c4-judge commented 8 months ago

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

c4-judge commented 7 months ago

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