OpenZeppelin / openzeppelin-labs

A space for the community to interact and exchange ideas on the OpenZeppelin platform. Do not use in production!
https://openzeppelin.com/
MIT License
375 stars 116 forks source link

Evaluate security of new fixed storage approach #82

Closed maraoz closed 6 years ago

maraoz commented 6 years ago

Can a user contract (a contract that is proxied using this storage mechanism) access the storage of the proxy? (i.e the proxyOwner and implementation addresses?)

frangio commented 6 years ago

Duplicate of https://github.com/zeppelinos/core/issues/26.

To be more specific, what we want to show is that a Solidity contract with no custom assembly (in particular, no custom sload and sstore instructions) cannot be made to read or write to our "fixed" storage slot(s).

Additionally we hold the assumption that keccak256 is collision-free.

maraoz commented 6 years ago

As discussed offline with @frangio, this is not a duplicate of https://github.com/zeppelinos/upgradeability-lib/issues/26. We still need to audit the security of the new approach.

frangio commented 6 years ago

After reviewing this we've concluded that Solidity will not clash with our proxy storage slots, e.g. keccak256("org.zeppelinos.proxy.owner"). More generally let's consider any string key, and resulting slot keccak256(key).

There's three ways in which Solidity accesses storage: 1) Simple state variables like uint x will be stored in sequential slot numbers in storage, starting from 0, so they will all be very low numbers like 0x00...007 (32 bytes). It is extremely unlikely that these numbers will clash with the result of a hash. 2) An array state variable uint[] y will have its length stored in y's slot number (y_slot) as per point 1, and its contents stored sequentially starting at keccak256(y_slot). Note that y_slot is a 32-byte value, so in order for the hash to coincide with ours (key's), the string should also be 32 bytes. Even if it were 32 bytes, key will be an ASCII string consisting of readable characters, and so it will have non-zero higher bits, and thus will be much larger than any possible y_slot. 3) A mapping state variable mapping (uint => uint) z will have the value z[k] stored in the slot keccak256(k, z_slot). The concatenation k, z_slot is a 64-byte value. Analogously to the previous point, key would have to be 64 bytes in order to coincide, and if it were, the part corresponding to z_slot will be much larger than it.

Whichever key we choose, as long as we use readable ASCII characters (particularly, if we do not use null bytes) will not clash with anything used by Solidity.