'incorrect-initializer-order' warning fires spuriously for nested initializers

[kyzooghost]

What we observed

Since updating @openzeppelin/upgrades-core from 1.33.1 to 1.41.0, we have encountered a barrage of spurious warnings in the npx hardhat test logs.

Warning: Potentially unsafe deployment of src/_testing/unit/messaging/TestL1MessageService.sol:TestL1MessageService

    src/_testing/unit/messaging/TestL1MessageService.sol:23: Incorrect order of parent initializer calls.
    - Found initializer calls to parent contracts in the following order: PauseManager, RateLimiter, L1MessageService
    - Expected: PauseManager, L1MessageService
        Call parent initializers in linearized order

This warning is repeated in our test logs at least 20 times. See the Github CI logs for the 'Run smart contract tests and generate coverage report' job.

Conceptual description of issue

When we looked into issue, it seems that @openzeppelin/upgrades-core will only recognise parent initializers declared in the top-level initializer as expected parent contract initializers.

A discrepancy occurs when @openzeppelin/upgrades-core will recognise all parent initializers as found parent contract initializers.

The discrepancy in scope between expected and found causes this error to fire spuriously in cases where a nested initializer is present (and not declared in the top-level initializer).

Code example

Top-level contract

contract TestL1MessageService is L1MessageService {
  function initialize(
    uint256 _rateLimitPeriod,
    uint256 _rateLimitAmount,
    PauseTypeRole[] calldata _pauseTypeRoles,
    PauseTypeRole[] calldata _unpauseTypeRoles
  ) public initializer {
    _grantRole(DEFAULT_ADMIN_ROLE, msg.sender);
    __PauseManager_init(_pauseTypeRoles, _unpauseTypeRoles);
    __MessageService_init(_rateLimitPeriod, _rateLimitAmount);
  }
}

Inherited/Parent contract

abstract contract L1MessageService is ... {
  function __MessageService_init(uint256 _rateLimitPeriod, uint256 _rateLimitAmount) internal onlyInitializing {
    __ERC165_init();
    __Context_init();
    __AccessControl_init();
    __RateLimiter_init(_rateLimitPeriod, _rateLimitAmount);
  }
}

In this case RateLimiter is outside of the scope of the expected initializers, but is in scope of the found initializers. This triggers a spurious warning in our opinion.

Fixes explored

Putting /// @custom:oz-upgrades-unsafe-allow incorrect-initializer-order on top of each 'offending' initializer does silence the warnings. But we disagree with needing to modify the source code to silence spurious warnings, since modifying source code incurs a financial cost for re-audits of production code.

[ericglau]

Thanks for reporting, we will investigate this further.
You can also skip the warning by using the following option in your tests/scripts, to avoid needing to modify contract code:

{ unsafeAllow: ['incorrect-initializer-order'] }

For example:

await upgrades.validateUpgrade(proxyAddress, newContract, {
      kind: "transparent",
      unsafeAllow: ['incorrect-initializer-order'],
});

[kyzooghost]

Thank you @ericglau for the prompt and helpful suggestion! Have successfully used your suggestion to silence the warnings for now.

[vladimir-trifonov]

I get

      test/contracts/xxxV2.sol:20: Missing initializer
          Define an initializer function and use it to call the initializers of parent contracts
          https://zpl.in/upgrades/error-001

when I try to upgrade from v1 to v2 of the smart contract. This error happens when

npx @openzeppelin/upgrades-core@^1.42.0 validate out/build-info --contract test/contracts/xxxV2.sol:xxxV2 --requireReference

is executed.
I didn't have this error with @openzeppelin/upgrades-core v1.41.0.
My initializers are ok... Both in the v1 contract and in the v2 as well.

[ericglau]

@vladimir-trifonov Can you please open a separate issue, and include what your xxxV2.sol initializer looks like, in terms of function name and/or modifiers that it uses?