Recommendation for preventing interaction with logic contracts

Support Upgrades
1

Hi folks,

I'd like to ask what is your recommendation for preventing malicious actors from interacting with logic (implementation) contracts in the proxied pattern.

We recommend that clients adopt the following approach:

  1. Make sure that once the logic contract is constructed, its initialization function cannot be called.
  2. Ensure that no function on the logic contract can be called until its initialization function is called.

(1) can be achieved by adding the initializer modifier to the constructor.

(2) is more difficult. We currently recommend adding initialized = true to an initialize function, and protecting every non-view public entrypoint (public or external function) with require (initialized). Note that this is a work-around since Initializable's _initialized is private.

My question is: do you recommend a different way for dealing with this scenario, and if not, would you consider making _initialized internal in future package versions, to be able to protect logic functions? Thank you.

2

I see your point and usually between deploying a contract and initializing it, the gap is small enough that very few to none people should be able to squeeze in a transaction to other public functions. Besides, adding a require statement for a condition that can be false for only a short period of time is a bit costly.

3

Thanks for your response. While you are correct that this protection also protects against functions being called on the proxy before initialization, it is not the crux of my question. I'm concerned about interactions with logic (implementation) contracts by malicious parties. This has been a problem in the past, notably here and here.

4

Got you. Turning _initialized from private to internal can make the this state variable useful across all public functions, not just the initialize function, in the child contract.

5

I think the constructor code is executed once when a contract is created, so is there a need to add the modifier initializer to it.

6

Just found some comments in the contract: Initializable.sol:

An uninitialized contract can be taken over by an attacker. This applies to both 
a proxy and its implementation contract, which may impact the proxy.
To initialize the implementation contract, you can either invoke the initializer
manually, or you can include a constructor to automatically mark it as initialized
when it is deployed:
 constructor() initializer {}

So it seems like the contracts can be wrote like:

contract Test is Initializable {
  constructor () public initializer {
    initialize();
  }
  function initialize() public initializer {
    XXX;
  }
}

But I always write like this:

contract Test is Initializable {
  constructor () public { // <<<--- here!!!
    initialize();
  }
  function initialize() public initializer {
    XXX;
  }
}
7

Right. I agree. The problem is that it is generally bad practice to copy-paste code, and if you include it as an npm dependency, you have no option to overwrite the visibility. That is why we need OpenZeppelin to change it (or provide a better solution :)).

8

From my experience with OZ contracts, the security of each one is of highest priority and a broad applicability is of lower priority in the ranking of design principles. In order to achieve what you would ike to achieve, you may create a new initialized variable which would not overwrite the one in the initializer modifier and everybody is happy :slight_smile:

9

The general recommendation is to add constructor() initializer {} in the logic contract.

Generally I would say that (2) is not as necessary because lack of initialization is often enough to leave a contract unusable. Do you have examples where it is not enough and additional measures are needed? And does it cause a vulnerability?

I would avoid adding require(initialized) to every function because of the added SLOAD cost.

What you could do is use an onlyDelegated modifier that looks like this:

    address private immutable self = address(this);

    modifier onlyDelegated() {
        require(address(this) != self);
    }

This is a more direct way of preventing interaction with the logic contract.

I don't see security reasons to use this modifier in every function though.