sherlock-audit / 2024-08-perennial-v2-update-3-judging

4 stars 2 forks source link

panprog - Upgrade to v2.3 will revert due to incorrect initializer version in `OracleFactory` #33

Open sherlock-admin4 opened 2 months ago

sherlock-admin4 commented 2 months ago

panprog

Medium

Upgrade to v2.3 will revert due to incorrect initializer version in OracleFactory

Summary OracleFactory is part of the v2.3 upgrade process. It should be re-initialized after upgrading the implementation (to store a newly introduced OracleParameter which was not present in previous implementation):

    function initialize() external initializer(3) {
        // Re-initialize if owner is unset
        if (owner() == address(0)) __Factory__initialize();

        _parameter.store(OracleParameter(1, UFixed6Lib.ZERO, UFixed6Lib.ZERO));
    }

The issue is that initializer version used for new implementation is 3 - the same version as used in the previous initialization. This can be checked in live deployment events: https://arbiscan.io/address/0x8cda59615c993f925915d3eb4394badb3feef413#events (see the last Initialized (uint256 version) event parameter, which is 3). The modifier will revert when the initializer version is the same as the one already used:

    /// @dev Can only be called once per version, `version` is 1-indexed
    modifier initializer(uint256 version) {
        if (version == 0) revert InitializableZeroVersionError();
        if (_version.read() >= version) revert InitializableAlreadyInitializedError(version);

This means the whole upgrade process will fail, breaking contract core functionality (upgrade).

Root Cause Incorrect initializer version: https://github.com/sherlock-audit/2024-08-perennial-v2-update-3/blob/main/perennial-v2/packages/perennial-oracle/contracts/OracleFactory.sol#L39

Internal pre-conditions None.

External pre-conditions None.

Attack Path The whole upgrade process will revert when trying to initialize OracleFactory

Impact Core contract functionality (upgrade) broken

PoC Not needed.

Mitigation Use initializer version 4.

arjun-io commented 1 month ago

The re-initialize isn't strictly necessary for the migration to complete, as long as the parameter is updated in the same transaction as the implementation upgrade (which is possible via the Timelock's executeBatch functionality)