Add petrifiable

[sohkai]

For tests, see #386.

Note that this also fixes #325.

---

From https://github.com/ConsenSys/aragonOS-audit-repo/issues/31:

It is recommended to initialise the baseKernel as part of the deployment or whitelist administrative contract addresses.

Our current base contracts on both rinkeby and mainnet are uninitialized. While this isn't a problem if everyone uses a proxy on top of them, it can lead to confusion as well as weird situations if someone transfers ETH or tokens to, e.g. the base vault.

---

Petrifiable is an attempt at making sure base contracts can be "turned off" as they're deployed. It allows us to disable base contracts without using another storage slot (for, e.g. an initializer).

For this to be safe, this requires all AragonApps to require initialization before they're used (even in apps that don't require it right now, e.g. Vault and Repo) by making sure all publicly accessible functions have isInitialized. This also requires changes in all the apps for including the constructor.

This also sidesteps the auth issue presented in aragon/aragon-apps#385, whereby an app can still be connected to a kernel without having to be behind a proxy.

[coveralls]

Coverage decreased (-98.7%) to 0.328% when pulling 5e46df1 on add-petrifiable into f3ba361 on dev.

[bingen]

It looks good to me. My only comment so far is that now we have 2 ways to set the kernel for an app: 1) through this new constructor and 2) through AppProxyBase. I think it's kind of confusing, so it would be good to put a more explicit comment over this new AragonApp constructor to explain the difference.

[sohkai]

Responding to offline questions from @bingen:

As of right now, the only possible states for our base and proxy contracts should be:

Base contract:

• Initialized and with a Kernel (safe)
• Petrified without a Kernel (safe)
• Not initialized but with a Kernel (not safe and should be avoided by factory deployments)

Proxies:

• Initialized and with a Kernel (safe)
• Petrified with a Kernel (safe, if for some reason the app implements a way to petrify itself instead of initializing)
• Not initialized but with a Kernel (not safe and should be avoided by factory deployments)

---

These cases:

• Initialized without a Kernel
• Not initialized and without a Kernel

Should be impossible with the change. Once we add isContract checks for places where we assume we get kernels (e.g. AppProxyBase, AragonApp), base contracts should either always be petrified or be attached to a kernel.

Initialized without a Kernel is impossible, because the proxy must be given a kernel, and the base will petrify itself on deployment if it doesn't receive one.

Not initialized and without a Kernel is also impossible, due to the same reasoning.

[izqui]

I like the idea of being able to freeze bases on deployment, as it reduces a vector of uncertainty.

However, this also opens the door for initializing bases with a Kernel that has an EVMScript executor that is able to selfdestruct bases (for apps that are forwarders and use runScript). This can be fatal if the organization has disabled upgrades or using a pinned AppProxy. @sohkai I disagree that a base contract Initialized and with a Kernel is safe for this reason.

My proposal is to petrify by default in the AragonApp constructor. This way apps don't need to have an explicit constructor and an initialize(...) function, which is quite confusing. If someone really wants to run an AragonApp without a proxy, they can write a wrapper contract like this:

contract VotingAppWrapper is VotingApp {
  function VotingAppWrapper(IKernel _kernel, address _token...) {
     initializationBlock = 0; // unpetrifies
     kernel = _kernel;
     initialize(_token...)
  }
}

Using the Kernel or any apps without a proxy is always going to require writing some custom code anyway, as deploying the contract and initializing it atomically is quite important.

IMO aragonOS should provide secure and sane defaults and allow for detecting phisy things in the code directly. By allowing to create bases that are either petrified or have a custom kernel, we are pushing extra uncertainty to the deployment process (knowing whether a base can be selfdestructed depends on the deployment + kernel state) and adding an extra aspect that app devs have to think about.

[bingen]

I agree about petrifying by default. But in essence this is again that question about if aragonOS/apps are intended to be used with all the ingredients (ACLs and Upgradeability) or if we are supporting also the use of ACLs only, without Proxies. I have said before that I like better the first option. If someone really wanted to use our apps without proxies, they would 3 options:

• Find another, simpler, solution
• Fork the and remove the proxy stuff
• Do some hack like that Voting wrapper above

But I would say that we are going the other way (and I'm fine with it, no strong opinion about it). But in that case we shouldn't petrify by default.
Anyway, I think we should clearly and explicitly answer that question first. It should even be in our documentation. If the answer is "No", it should say clearly that Proxies are not optional (and we still have Pinned apps), if it's a "Yes", it should explain clearly both ways of using apps (with and without Proxies) and the consequences of each mode.
PS: I love the name of "Petrify"! :')

[sohkai]

However, this also opens the door for initializing bases with a Kernel that has an EVMScript executor that is able to selfdestruct bases (for apps that are forwarders and use runScript). This can be fatal if the organization has disabled upgrades or using a pinned AppProxy.

I hadn't considered that someone could use their initialized base contract as an actual base for other proxies on top of it. Thinking about the potential consequences, we should make this path possible only through as much explicit effort as possible (e.g. writing explicit, custom code).

[sohkai]

@bingen on proxyless apps:

Given the above (someone mistakenly initializing a base, and then using that base to back other proxies), my current opinion is that we should still make it possible for someone to use an app without a proxy, but both they and the app developer should explicitly desire to do so.

My reasoning comes from:

1. There should be no difference when using an app behind a proxy or not (since they're both contracts implementing the same ABI), outside of gas costs and the initial set up (interface consistency)
2. We can enforce that all of our first party apps be used from behind a proxy,
3. We can design the interfaces so that developing apps meant to be only used behind a proxy is the default, easy, safe way of developing, and leave a "unsafe" mode to break out of this

[sohkai]

Bytecode diff:

                              CODE DEPOSIT COST    DEPLOYED BYTES     INITIALIZATION BYTES
ACL.json                      76400 more gas       +382               +353
APMRegistry.json              106200 more gas      +531               +353
AragonApp.json                79400 more gas       +397               +353
CallsScript.json              100800 more gas      +504               +331
ENSSubdomainRegistrar.json    76400 more gas       +382               +353
EVMScriptRegistry.json        76400 more gas       +382               +353
EVMScriptRegistryFactory.json Same                 0                  +1573
EVMScriptRunner.json          45000 more gas       +225               0
Initializable.json            22800 more gas       +114               +1
Kernel.json                   79200 more gas       +396               +403
Repo.json                     110000 more gas      +550               +353

[izqui]

I'm hesitant about adding an extra SLOAD for a function that will be called as often as kernel.hasPermission(...). In all 'write functions' we just revert if they are not initialized.

A cheaper way to do it is to check if the ACL address is 0 and if so return false.

[sohkai]

See c128077