Validate that an upgraded contract keeps same storage layout as the original one

[spalladino]

(follows from zeppelinos/zos-lib#63)

The problem

For an upgrade to be safe, we need to ensure that the new contract shares the same storage layout as the previous version, and only appends new variables, without modifying the existing ones.

For instance, given:

contract V1 {
  uint a;
  uint b;
}

The following upgrade is valid, since it's only adding a new variable at the end of the storage:

contract V2 {
  uint a;
  uint b;
  uint c;
}

The following ones, on the other hand, are all invalid:

contract V2 {
  uint a;
  uint c; // New variable is inserted in the slot previously assigned to `b`
  uint b;
}

contract Base {
  uint base; // New `base` variable will be assigned the first slot
}
contract V2 is Base { 
  uint a;
  uint b;
}

contract V2 {
  uint a;
  string b; // Type of a variable is changed
}

Other potential breaking changes include modifying the fields of a struct, or changing inheritance order. Anything that alters the storage layout can potentially introduce bugs, and needs to be detected. To detect this, we need to figure out the storage layout, store it, and compare it when a new version is set.

Infering storage layout

The first step is to get the storage layout given a contract. The safest option to do this is to keep building on ethereum/solidity#4017, which is an enhancement to the Solidity compiler to provide this information directly. This requires considerable effort, and we'd need to wait until the new version of the compiler is released before we can make use of the feature.

A more lightweight alternative would be to extract this information from the AST. Assuming that the order of variables in storage is the same as the order in which they are declared (this is a safe assumption to make), we can walk the AST looking for nodes that correspond to state variables ("nodeType": "VariableDeclaration", "stateVariable": true), and store their order and type. Note that we'd also need to walk the linearizedBaseContracts (in the order specified) and also retrieve their state variables, to build a complete picture of the storage.

Ideally, we could output this information in a format as similar as possible as the one described here, so the transition to the information provided by solc is as smooth as possible.

Comparing storage layouts

Next step is to build a function that, given two storage layouts as extracted in the previous step, returns whether one is a valid extension of the other. It needs to check that variables types and names haven't changed, taking into account complex types (mappings, structs, etc) as well.

The output needs to be human-readable, so we can provide the user with information on where the problem is. Make sure the error is understandable when caused by a change in an ancestor: if a base contract changes its storage layout, it's something quite hidden to the user, so we need to be extra clear in those scenarios.

Recording storage info for a contract and checking

In order to compare a version with a new one, we need to store the storage layout information somewhere, and when the user proposes a new version, we perform the check. Ideally, we could to this when the user registers the new changes, but we need to have a git-like stage in place for that.

Instead, we can do that when the user runs zos push on a new version. On the first zos push, we store the storage layout info in the zos.network.json file (along with the bytecode hash). On subsequent ones, we recalculate the storage layout for the new contract, and compare it with the stored one. If it's ok, we allow the push and overwrite the storage layout. If not, we show the errors to the user. We'd also need to include an --ignore-warns-on-storage-layout (better name needed) flag so the user can push anyway, to work around false positives.

[spalladino]

For Gitcoin bounty hunters: this is an issue that has been raised by other teams also working with upgradeability solutions (such as Aragon, Livepeer, and Decentraland). As such, we'd accept contributions that just solve the first two items (infering and comparing storage layouts) independent of the ZeppelinOS CLI, so it can be reused by other teams. We can then work on the integration with the CLI on our own.

[gitcoinbot]

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

---

This issue now has a funding of 400.0 DAI (400.0 USD @ $1.0/DAI) attached to it.

• If you would like to work on this issue you can 'start work' on the Gitcoin Issue Details page.
• Want to chip in? Add your own contribution here.
• Questions? Checkout Gitcoin Help or the Gitcoin Slack
• $20,145.13 more funded OSS Work available on the Gitcoin Issue Explorer

[subramanianv]

I wrote this snippet for comparing storage layouts. @spalladino Let me know if this is a step in correct direction

const fs = require('fs');
const contracts = fs.readFileSync('./flattened.sol', 'utf8');
const _ = require('underscore');
let input = {
  'contracts': contracts
}


function traverseAST(_input, _elements) {
  if(_input.children) {
    for(var i=0;i<_input.children.length;i++) {
      traverseAST(_input.children[i], _elements);
    }
  }
  _elements.push(_input);
}

function compareStorageLayouts(oldLayout, newLayout) {
  function makeComp(x) {
    return [x.constant , x.name, x.scope, x.stateVariable, x.storageLocation, x.type, x.value, x.visibility].join(':');
  }
  if(newLayout.length < oldLayout.length) return false;
  for(var i=0;i<oldLayout.length;i++) {
    const a = oldLayout[i].attributes;
    const b = newLayout[i].attributes;
    const comp1 = makeComp(a)
    const comp2 = makeComp(b);
    if(comp1 != comp2) {
      return false;
    }
  }
  return true;
}

var output = solc.compile({ sources: input }, 1, _.noop);
const elements = [];
const AST = output.sources.contracts.AST;
traverseAST(AST, elements);

// filter out all Contract Definitions
const contractDefinitions = _.filter(elements, (e,i) =>  e.name == 'ContractDefinition');

// filter out all linearizedBaseContracts
// pick the last one as the last contract always has the full inheritance
const linearizedBaseContracts = _.last(_.map(contractDefinitions, e => e.attributes.linearizedBaseContracts));

// get all stateVariables
const stateVariables = _.filter(elements, e => e.attributes && e.attributes.stateVariable )

// group them by scope
const stateVariableMap = _.groupBy(stateVariables, e => e.attributes.scope);

orderedStateVariables = _.reduceRight(linearizedBaseContracts, (a, b) =>  {
  return a.concat(stateVariableMap[b] || [])
}, []);

//console.log(compareStorageLayouts(orderedStateVariables, orderedStateVariables));

[subramanianv]

@facuspagnuolo Is this task a priority now or has been put in the backlog ?