Key Admin facet functions are invalidated, due to vulnerable access-control implementations

[c4-bot-7]

Lines of code

https://github.com/code-423n4/2024-03-zksync/blob/4f0ba34f34a864c354c7e8c47643ed8f4a250e13/code/contracts/ethereum/contracts/state-transition/chain-deps/facets/Admin.sol#L51 https://github.com/code-423n4/2024-03-zksync/blob/4f0ba34f34a864c354c7e8c47643ed8f4a250e13/code/contracts/ethereum/contracts/state-transition/chain-deps/facets/Admin.sol#L58

Vulnerability details

Impact

Key Admin facet functions(setPorterAvailability, setPriorityTxMaxGasLimit) are invalidated, due to vulnerable access-control implementations

Proof of Concept

Key admin facet functions such as setPorterAvailability and setPriorityTxMaxGasLimit are now onlyStateTransitionManager controlled. However, (1) StateTransitionManger.sol is unable to call setPorterAvailability in any flows. (2) StateTransitionManger.sol is unable to change PriorityTxMaxGasLimit if needed.

(1) s.zkPorterIsAvailable on Admin facet is not initializable in DiamondInit.sol. Neither is it callable by designated StateTransitionManger.sol because ST manager contract does not have any functions or flows that will invoke setPorterAvailability. The result is s.zkPorterIsAvailable will always be default false value and cannot be set or reset if needed.

//code/contracts/ethereum/contracts/state-transition/chain-deps/facets/Admin.sol
    //@audit s.zkPorterIsAvailable will not be initialized by DiamondInit.sol, neither can it be set by StateTransitionManager.sol, due to the contract has not methods or flows to set it.
    function setPorterAvailability(
        bool _zkPorterIsAvailable
|>    ) external onlyStateTransitionManager {
        // Change the porter availability
        s.zkPorterIsAvailable = _zkPorterIsAvailable;
        emit IsPorterAvailableStatusUpdate(_zkPorterIsAvailable);
    }

(https://github.com/code-423n4/2024-03-zksync/blob/4f0ba34f34a864c354c7e8c47643ed8f4a250e13/code/contracts/ethereum/contracts/state-transition/chain-deps/facets/Admin.sol#L51) (2) Although s.priorityTxMaxGasLimit is initializalbe by DiamondInit.sol, it is not callable by StateTransitionManager.sol due to the contract doesn't have methods or flows to call it. As a result, s.priorityTxMaxGasLimit cannot be changed if needed.

//code/contracts/ethereum/contracts/state-transition/chain-deps/facets/Admin.sol
    //@audit s.priorityTxMaxGasLimit will only be set during initialization by DiamondInit.sol. But setPriorityTxMaxGasLimit cannot be called by StateTransitionManger.sol because the contract doesn't have methods or flows to call it. s.priorityTxMaxGasLimit cannot be updated.
    function setPriorityTxMaxGasLimit(
        uint256 _newPriorityTxMaxGasLimit
|>    ) external onlyStateTransitionManager {
        require(_newPriorityTxMaxGasLimit <= MAX_GAS_PER_TRANSACTION, "n5");
        uint256 oldPriorityTxMaxGasLimit = s.priorityTxMaxGasLimit;
        s.priorityTxMaxGasLimit = _newPriorityTxMaxGasLimit;
        emit NewPriorityTxMaxGasLimit(
            oldPriorityTxMaxGasLimit,
            _newPriorityTxMaxGasLimit
        );
    }

(https://github.com/code-423n4/2024-03-zksync/blob/4f0ba34f34a864c354c7e8c47643ed8f4a250e13/code/contracts/ethereum/contracts/state-transition/chain-deps/facets/Admin.sol#L58)

In addition, setValidator() in Admin.sol is also not callable by StateTransitionMager or chain Admin to change the ValidatorTimelock contract if needed.

Tools Used

Manual

Recommended Mitigation Steps

Consider adding related methods to update chain-specific s.zkPorterIsAvailable and s.priorityTxMaxGasLimit in StateTransitionManger.sol, to allow StateTransitionManger's owner to call an ST to change these parameters when needed.

Assessed type

Other

[c4-judge]

alex-ppg marked the issue as primary issue

[c4-sponsor]

saxenism marked the issue as disagree with severity

[saxenism]

We consider this issue QA at best because these functions are not really needed for the initial version and are already fixed.

[c4-sponsor]

razzorsec (sponsor) confirmed

[c4-sponsor]

razzorsec (sponsor) acknowledged

[alex-ppg]

The Warden specifies how functions implemented are currently not integrated by the state transition manager despite what their access control implies.

These functions mutate variables that are not utilized within the project, meaning that the maximum impact of such an exhibit is QA.

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Overinflated severity