Idempotence of OZ constructors

Hi folks,

I'm a security auditor. I have a few questions about upgradeable-contracts:

  1. Is it an architectural invariant that contracts' __{name}_init functions call __{name}_init_unchained functions in the (reverse) order of C3-linearization? If yes, do you a tool like Slither to ensure that that is true for all releases?

  2. Do all OZ contracts have idempotent initializers? Let me provide more detail on this one.

Lemma. It is not the case that if a set of methods is idempotent, then the method calling those methods in sequence is idempotent.

Proof. Consider this example:

contract C {
    uint public a;
    function some_idempotent_func() public {
        if (a == 4) {
            a = 8;
        }
    }
    
    function another_idempotent_func() public {
        if (a != 8) {
            a = 4;
        }
    }
    
    function not_idempotent() public {
        some_idempotent_func();
        another_idempotent_func();
    }
}

Based on that, the question arises:

Are all OZ __{c}_init_unchained functions idempotent, and all __{c}_init functions and all sequences consisting of calls to __{c}_init functions intended to be idempotent?

If not, I could see a vulnerability happening:

// OZ Initializable
abstract contract Initializable {
    /**
     * @dev Indicates that the contract has been initialized.
     */
    bool private _initialized;

    /**
     * @dev Indicates that the contract is in the process of being initialized.
     */
    bool private _initializing;

    /**
     * @dev Modifier to protect an initializer function from being invoked twice.
     */
    modifier initializer() {
        require(_initializing || !_initialized, "Initializable: contract is already initialized");

        bool isTopLevelCall = !_initializing;
        if (isTopLevelCall) {
            _initializing = true;
            _initialized = true;
        }

        _;

        if (isTopLevelCall) {
            _initializing = false;
        }
    }
}

// Mocks of OZ contracts
contract Storage {
    uint public a;
}

contract A is Initializable, Storage {
    function __A_init() public initializer {
        __A_init_unchained();
    }
    function __A_init_unchained() public initializer {
        ++a;
    }
}

contract B1 is A {
    function __B1_init() public initializer {
        __A_init_unchained();
        __B1_init_unchained();
    }
    function __B1_init_unchained() public initializer {
    }
}

contract B2 is A {
    function __B2_init() public initializer {
        __A_init_unchained();
        __B2_init_unchained();
    }
    function __B2_init_unchained() public initializer {
    }
}

// Some client's contract
// Client inherits OZ B1 and B2. They call B1.__B1._init and B2.__B2._init,
// oblivious to the fact that both will call A.__A_init_unchained
contract C is B1, B2 {
    function init() public initializer {
        __B1_init();
        __B2_init(); // disaster, A is initialized twice
    }
}

Our upgradeable contracts are generated using a transpiler that builds the sequence of initializer calls based on the linearization order as reported by solc, so they are guaranteed to be correct.

No, this is not the case. We recognize this is currently an error prone part of the library, and are planning to include a way to check correctness of initializers in our Upgrades Plugins for Hardhat and Truffle.