Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
spalladino opened this Issue Aug 22, 2018 · 9 comments

Comments

@spalladino
Copy link
Member

spalladino commented Aug 22, 2018

(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

This comment has been minimized.

Copy link
Member Author

spalladino commented Aug 24, 2018

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

This comment has been minimized.

Copy link

gitcoinbot commented Aug 24, 2018

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.

@facuspagnuolo facuspagnuolo modified the milestones: Backlog, v2.0.0 Sep 5, 2018

@subramanianv

This comment has been minimized.

Copy link

subramanianv commented Sep 7, 2018

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

This comment has been minimized.

Copy link

subramanianv commented Sep 8, 2018

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

@facuspagnuolo

This comment has been minimized.

Copy link
Contributor

facuspagnuolo commented Sep 10, 2018

@subramanianv thanks for the submission and the proposal. Unfortunately, we decided to start working on this issue with a different approach. We will re-assign this issue to ourselves, I hope you can help us with other issues if you want. Thanks!

@facuspagnuolo

This comment has been minimized.

Copy link
Contributor

facuspagnuolo commented Sep 10, 2018

@gitcoinbot we decided to start working on this issue ourselves, please remove the gitcoin bounty attached to it. Thanks!

@gitcoinbot

This comment has been minimized.

Copy link

gitcoinbot commented Sep 13, 2018

@subramanianv Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

  • warning (3 days)
  • escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

1 similar comment
@gitcoinbot

This comment has been minimized.

Copy link

gitcoinbot commented Sep 13, 2018

@subramanianv Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

  • warning (3 days)
  • escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

@spalladino spalladino referenced this issue Sep 13, 2018

Closed

Validate storage layout #92

2 of 4 tasks complete
@gitcoinbot

This comment has been minimized.

Copy link

gitcoinbot commented Sep 24, 2018

Issue Status: 1. Open 2. Cancelled


The funding of 400.0 DAI (400.0 USD @ $1.0/DAI) attached to this issue has been cancelled by the bounty submitter

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.