c4-bot-7 commented 4 months ago

Lines of code

https://github.com/code-423n4/2024-03-zksync/blob/4f0ba34f34a864c354c7e8c47643ed8f4a250e13/code/contracts/zksync/contracts/bridge/L2StandardERC20.sol#L111-L128 https://github.com/code-423n4/2024-03-zksync/blob/4f0ba34f34a864c354c7e8c47643ed8f4a250e13/code/contracts/zksync/contracts/bridge/L2StandardERC20.sol#L134-L139

Vulnerability details

Proof of Concept

Take a look at https://github.com/code-423n4/2024-03-zksync/blob/4f0ba34f34a864c354c7e8c47643ed8f4a250e13/code/contracts/zksync/contracts/bridge/L2StandardERC20.sol#L111-L128

    function reinitializeToken(
        ERC20Getters calldata _availableGetters,
        string memory _newName,
        string memory _newSymbol,
        uint8 _version
    ) external onlyNextVersion(_version) reinitializer(_version) {
        // It is expected that this token is deployed as a beacon proxy, so we'll
        // allow the governor of the beacon to reinitialize the token.
        address beaconAddress = _getBeacon();
        require(msg.sender == UpgradeableBeacon(beaconAddress).owner(), "tt");

        __ERC20_init_unchained(_newName, _newSymbol);
        __ERC20Permit_init(_newName);
        availableGetters = _availableGetters;

        emit BridgeInitialize(l1Address, _newName, _newSymbol, decimals_);
    }

This function is used to reinitialize or update a token's metadata.

Firstly would be key to note that it passes the version to the onlyNextVersion() modifier to ensure that the governor does not accidentally disable future reinitialization of the token, https://github.com/code-423n4/2024-03-zksync/blob/4f0ba34f34a864c354c7e8c47643ed8f4a250e13/code/contracts/zksync/contracts/bridge/L2StandardERC20.sol#L134-L139

    modifier onlyNextVersion(uint8 _version) {
        // The version should be incremented by 1. Otherwise, the governor risks disabling
        // future reinitialization of the token by providing too large a version.
        require(_version == _getInitializedVersion() + 1, "v");
        _;
    }

Case is with how this whole logic is implemented, would be key to note that the function _getInitializedVersion() is inherited from Openzeppelin's implementation, i.e https://github.com/OpenZeppelin/openzeppelin-contracts/blob/0a5fba7a7ed726698477c7772be5d17afe69f118/contracts/proxy/utils/Initializable.sol#L208C1-L227C6

    function _getInitializedVersion() internal view returns (uint64) {
        return _getInitializableStorage()._initialized;
    }

    function _getInitializableStorage() private pure returns (InitializableStorage storage $) {
        assembly {
            $.slot := INITIALIZABLE_STORAGE
        }
    }

Now, evidently this function returns uint64 value, meaning that protocol instead limits it self to 255 versions instead of 18,446,744,073,709,551,615 which is over a > 99,9999% decrease on the amount of generations it should support.

Impact

Medium, this is high impact and somewhat low possibility (there needs to be 255 versions ), would be key to note that the current logic for reinitializing the tokens is a core functionality, as it directly syncing of the tokens and symbolizes with the signature logic of the token too, i.e not only is the decoded values for the name and symbol now going to be wrong, there is no possibility of passing in an EIP 712 signature for the token now, since __ERC20Permit_init(decodedName) is not going to get called with the correct new name.

Recommended Mitigation Steps

Use version as is stored from inherited implementations, i.e (uint64) apply these changes to https://github.com/code-423n4/2024-03-zksync/blob/4f0ba34f34a864c354c7e8c47643ed8f4a250e13/code/contracts/zksync/contracts/bridge/L2StandardERC20.sol#L111-L139

    function reinitializeToken(
        ERC20Getters calldata _availableGetters,
        string memory _newName,
        string memory _newSymbol,
-        uint8 _version
+        uint64 _version
    ) external onlyNextVersion(_version) reinitializer(_version) {
        // It is expected that this token is deployed as a beacon proxy, so we'll
        // allow the governor of the beacon to reinitialize the token.
        address beaconAddress = _getBeacon();
        require(msg.sender == UpgradeableBeacon(beaconAddress).owner(), "tt");

        __ERC20_init_unchained(_newName, _newSymbol);
        __ERC20Permit_init(_newName);
        availableGetters = _availableGetters;

        emit BridgeInitialize(l1Address, _newName, _newSymbol, decimals_);
    }

-    modifier onlyNextVersion(uint8 _version) {
+    modifier onlyNextVersion(uint64 _version) {
        // The version should be incremented by 1. Otherwise, the governor risks disabling
        // future reinitialization of the token by providing too large a version.
        require(_version == _getInitializedVersion() + 1, "v");
        _;
    }

Assessed type

DoS

razzorsec commented 3 months ago

We consider this as invalid. We use a different version of OZ, where u8 is returned.

c4-sponsor commented 3 months ago

razzorsec (sponsor) disputed

alex-ppg commented 2 months ago

The Warden attempts to specify a downcasting flaw in relation to the Initializable OpenZeppelin dependency, however, the version referenced does not match the one utilized by the contract. As such, the overall submission is invalid due to relying on a false premise.

c4-judge commented 2 months ago

alex-ppg marked the issue as unsatisfactory: Invalid

code-423n4 / 2024-03-zksync-findings