Improve safety checks for the upgradeability mechanism

[facuspagnuolo]

The current upgradeability model only prevents you from registering or upgrading to a zero address. Apart from that, it does not check if you upgrade a contract to a version registered for another contract, or if the contract that you are upgrading follows whatever was required by your previous version (e.g. storage or function signatures), etc.

We should explore how we can improve current model to handle most of these scenarios preventing projects from registering and/or upgrading to undesired addresses.

[facuspagnuolo]

@frangio you raised this issue, please add your ideas or edit the description directly, thx!

[frangio]

it does not ensure whether you upgrade a contract to a version registered for another contract

This refers to the fact that the upgradeTo function signature allows to change a proxy's implementation to any other contract, by sending an unrelated contractName.
https://github.com/zeppelinos/core/blob/519f4a0005113b4c0c7a6f315c30f56032239911/contracts/ProjectController.sol#L87

I see this as a problem in the conceptual model. Proxies' implementations should only be upgraded to other compatible implementations.

The current notion of compatibility between older and newer implementations seems to be that they have the same contractName. Thus, the solution would be to change upgradeTo to have signature upgradeTo(proxy, version) (or possibly upgradeTo(proxy, distribution, version)). ProjectController will need to keep track of the contractName corresponding to each Proxy.

This leaves out the possibility of upgrading from ERC721Basic to ERC721Full, so we may need to define a more general notion of compatibility. This ties into the idea of migrations. We could define compatibility between X and Y as: there is a migration from X to Y. I feel like this is a very powerful abstraction, but it may also be too complex.

[frangio]

if the contract that you are upgrading follows whatever was required by your previous version (e.g. storage or function signatures)

This is not possible to do on-chain. Off-chain, we could and should definitely check some of these things. Storage layout (#63) is the obvious one. I think these checks should part of our toolchain to register new versions.

[frangio]

Additionally, a very simple one that we should add is checking that an implementation has any code in it (via extcodesize), when it is registered.

[frangio]

Simple safety check added in #53.

Opened a spin-off issue in #63.

I'm leaving this issue open because it's something that we should keep thinking about.

[frangio]

See zeppelinos/zos-cli#320 for another safety issue to tackle.

[facuspagnuolo]

I'm closing this issue since we are already exploring some improvements in separate issues. In case we decide to tackle any other particular edge case or scenario, we could open another issue for that. Improving safety is something that never ends :)