`GenesisUpgrade::upgrade` function can be called by anyone any no. of times, also through proxy due to no access control and can change important values in storage `protocolVersion` and `verifier`.

[c4-bot-10]

Lines of code

https://github.com/code-423n4/2024-03-zksync/blob/main/code/contracts/ethereum/contracts/upgrades/GenesisUpgrade.sol#L14-L17

Vulnerability details

Vulnerability Details

Since GenesisUpgrade inherits BaseZkSyncUpgradeGenesis and upgrade function is overridden from this. The comment in BaseZkSyncUpgradeGenesis::upgrade function says This is a virtual function and should be overridden by custom upgrade implementations. But when overriding no access control were introduced resulting in calling by everyone. Since parant function upgrade also doesn't have any access control when called by super. from child. Since this function changes important params in s storage struct variable like s.protocolVersion,s.verifier and many others. Due to no access control these can be changed by anyone and can be exploited by it.

Vulnerable Code

GenesisUpgrade::upgrade function

11:  contract GenesisUpgrade is BaseZkSyncUpgradeGenesis {
    /// @notice The main function that will be called by the upgrade proxy.
    /// @param _proposedUpgrade The upgrade to be executed.
    //@audit upgrade function doesn't have any access control
 14:   function upgrade(ProposedUpgrade calldata _proposedUpgrade) public override returns (bytes32) {
 15:       super.upgrade(_proposedUpgrade);//@audit calling parent's upgrade which also not has access control
        return Diamond.DIAMOND_INIT_SUCCESS_RETURN_VALUE;
    }

BaseZkSyncUpgradeGenesis::upgrade function

17:  abstract contract BaseZkSyncUpgradeGenesis is BaseZkSyncUpgrade {

      ...
//@audit No access control
47:  function upgrade(ProposedUpgrade calldata _proposedUpgrade) public virtual override returns (bytes32) {
        // Note that due to commitment delay, the timestamp of the L2 upgrade batch may be earlier than the timestamp
        // of the L1 block at which the upgrade occurred. This means that using timestamp as a signifier of "upgraded"
        // on the L2 side would be inaccurate. The effects of this "back-dating" of L2 upgrade batches will be reduced
        // as the permitted delay window is reduced in the future.
        require(block.timestamp >= _proposedUpgrade.upgradeTimestamp, "Upgrade is not ready yet");

54:        _setNewProtocolVersion(_proposedUpgrade.newProtocolVersion);//@audit changing imp. storage values
55:        _upgradeL1Contract(_proposedUpgrade.l1ContractsUpgradeCalldata);
56:        _upgradeVerifier(_proposedUpgrade.verifier, _proposedUpgrade.verifierParams);//@audit changing imp. storage values
57:        _setBaseSystemContracts(_proposedUpgrade.bootloaderHash, _proposedUpgrade.defaultAccountHash);

      ...

     }
}

Impact

Due to no access control Anyone can update important params in s storage struct variable like s.protocolVersion,s.verifier and many others. And functionality can be damaged.

Tools Used

Manual Review

Recommended Mitigation

Add some type of access control on GenesisUpgrade::upgrade function so only authorize person can call it. like adding onlyOwner

Assessed type

Access Control

[c4-sponsor]

saxenism (sponsor) disputed

[saxenism]

Invalid since it is an implementation contract to be called by a proxy.

[alex-ppg]

The Warden specifies that a critical function of the GenesisUpgrade contract does not impose access control. The contract is expected to be utilized ephemerally as a temporary implementation/facet and as such, access control is not mandatory rendering this exhibit invalid.

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Invalid

[thenua3bhai]

Hi @alex-ppg Thanks for Judging

I believe this is a valid High severity vulnerability. The attack is quite simple related to missing access control on critical function.

1. Any attacker calls upgrade function using his favorable data in param _proposedUpgrade on Proxy contract address.
2. Now proxy fallback function will be triggered since upgrade function is not present in proxy and here Diamond proxy pattern used so based on called function's selector the call will be re-directed to the GenesisUpgrade implementation contract using delegatecall.
3. Since implementation is called using delegatecall so storage var. will be changed inside Proxy contract using the logic of implementation contract.
4. Attacker can change those critical storage var. such as protocolVersion and verifier in s storage struct var.

5. Use those changed values in favour of his benefit Or disrupt the protocol functionality by changing those values randomly.

   It's likelihood is high and impact is also high. Anyone can call upgrade any time and change those values. Sponsor states that this is called through proxy but then proxy need to have access control but in this case proxy also doesn't have any access control on caller.(I have added the code of proxy below for extra context)

   Generally in upgradeable contracts proxy doesn't have access control the access control is implemented by implementation contract on critical functions defined inside that contract which can be called by specific role or admin. That's why I didn't emphasize about proxy code and didn't include proxy code in report but stated in finding that it can be exploited also through proxy contract. Since generally it's not the proxy's job to have access control. But implementation contract's critical functions should have access control. Since caller of proxy is treated as caller of implementation contract's function also if called through delegatecall from proxy.

   But sponsor said it is called thorough proxy that's why invalid. Then proxy need to have access control defined if implementation doesn't have acess control. Proxy doesn't protect code by default to be called from random caller. The access control need to be implemented but proxy also doesn't have any access control. Makes this finding clearly valid. You stated ephemerally but it can be called again by anyone to change those values. Proxy fallback function which also have not any access control on caller. Implementation already doesn't have any access control.

File : state-transition/chain-deps/DiamondProxy.sol

    /// @dev 1. Find the facet for the function that is called.
    /// @dev 2. Delegate the execution to the found facet via `delegatecall`.
    fallback() external payable {
        Diamond.DiamondStorage storage diamondStorage = Diamond.getDiamondStorage();
        // Check whether the data contains a "full" selector or it is empty.
        // Required because Diamond proxy finds a facet by function signature,
        // which is not defined for data length in range [1, 3].
        require(msg.data.length >= 4 || msg.data.length == 0, "Ut");
        // Get facet from function selector
        Diamond.SelectorToFacet memory facet = diamondStorage.selectorToFacet[msg.sig];
        address facetAddress = facet.facetAddress;
         //@audit No access control to prevent any random caller
 @>       require(facetAddress != address(0), "F"); // Proxy has no facet for this selector
 @>       require(!diamondStorage.isFrozen || !facet.isFreezable, "q1"); // Facet is frozen

        assembly {
            // The pointer to the free memory slot
            let ptr := mload(0x40)
            // Copy function signature and arguments from calldata at zero position into memory at pointer position
            calldatacopy(ptr, 0, calldatasize())
            // Delegatecall method of the implementation contract returns 0 on error
            let result := delegatecall(gas(), facetAddress, ptr, calldatasize(), 0, 0)
            // Get the size of the last return data
            let size := returndatasize()
            // Copy the size length of bytes from return data at zero position to pointer position
            returndatacopy(ptr, 0, size)
            // Depending on the result value
            switch result
            case 0 {
                // End execution and revert state changes
                revert(ptr, size)
            }
            default {
                // Return data with length of size at pointers position
                return(ptr, size)
            }
        }
    }

Implementation code is in the finding report itself So access control is neither in proxy fallback nor on implementation GenesisUpgrade::upgrade function. So this function can be called by anyone through proxy. So this vulnerability should be considered valid high.

Thanks

[alex-ppg]

Hey @thenua3bhai, thanks for contributing to the PJQA process! This finding is invalid, as it is a common pattern for proxy implementations that are transiently utilized to not impose any access control. Effectively, the Diamond instance will perform a delegatecall to the GenesisUpgrade style contracts to perform the upgrade and will never reference it as a facet permanently as it is an upgrade, i.e. an operation meant to be executed one time. I appreciate your effort in attempting to contextualize the exhibit further but the ruling will remain as-is and no further feedback is expected.