Hi folks,
I'm a security auditor. I have a few questions about upgradeable-contracts:
-
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? -
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
}
}