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
376 stars 116 forks source link

Unstructure storage code doesn't work for onlyOwner methods. #130

Open ProphetDaniel opened 6 years ago

ProphetDaniel commented 6 years ago

If you try to call a method from the proxy with unstructured upgradeability pattern as follows:

    await aContractInstanceByProxy.anOnlyOnwerMethod(anAddress, {
      from: owner
    });

It gives you an error:

Error encountered, bailing. Network state unknown. Review successful transactions manually.
Error: VM Exception while processing transaction: revert
    at Object.InvalidResponse (C:\Users\Daniel\AppData\Roaming\npm\node_modules\truffle\build\webpack:\~\web3\lib\web3\errors.j
s:38:1)
    at C:\Users\Daniel\AppData\Roaming\npm\node_modules\truffle\build\webpack:\~\web3\lib\web3\requestmanager.js:86:1
    at C:\Users\Daniel\AppData\Roaming\npm\node_modules\truffle\build\webpack:\packages\truffle-migrate\index.js:225:1
    at C:\Users\Daniel\AppData\Roaming\npm\node_modules\truffle\build\webpack:\packages\truffle-provider\wrapper.js:134:1
    at XMLHttpRequest.request.onreadystatechange (C:\Users\Daniel\AppData\Roaming\npm\node_modules\truffle\build\webpack:\~\web
3\lib\web3\httpprovider.js:128:1)
    at XMLHttpRequestEventTarget.dispatchEvent (C:\Users\Daniel\AppData\Roaming\npm\node_modules\truffle\build\webpack:\~\xhr2\
lib\xhr2.js:64:1)
    at XMLHttpRequest._setReadyState (C:\Users\Daniel\AppData\Roaming\npm\node_modules\truffle\build\webpack:\~\xhr2\lib\xhr2.j
s:354:1)
    at XMLHttpRequest._onHttpResponseEnd (C:\Users\Daniel\AppData\Roaming\npm\node_modules\truffle\build\webpack:\~\xhr2\lib\xh
r2.js:509:1)
    at IncomingMessage.<anonymous> (C:\Users\Daniel\AppData\Roaming\npm\node_modules\truffle\build\webpack:\~\xhr2\lib\xhr2.js:
469:1)
    at emitNone (events.js:111:20)
    at IncomingMessage.emit (events.js:208:7)
    at endReadableNT (_stream_readable.js:1064:12)
    at _combinedTickCallback (internal/process/next_tick.js:138:11)
    at process._tickDomainCallback (internal/process/next_tick.js:218:9)

But if you call directly without the proxy it works:

    await aContractInstance.anOnlyOnwerMethod(anAddress, {
      from: owner
    });

Or if you edit the contract and remove onlyOwner from the metod then still call by proxy it also works.

spalladino commented 6 years ago

@ProphetDaniel thanks for the report! It's odd, since proxies should not interfere with Ownable. Could you share the actual code of your contracts?

ProphetDaniel commented 6 years ago

@spalladino You can check running tests on the unstructured_storage branch: https://github.com/CCEG-Blockchain-UN-Lab/upgradeable-proxy-plus/tree/unstructured_storage

spalladino commented 6 years ago

@ProphetDaniel the issue you're having is caused by using openzeppelin-solidity Ownable, instead of the one from openzeppelin-zos. The former sets the owner state variable in the constructor, while the latter has a separate initializer method to set it, which you should be using.

What's happenning is the following:

  1. You deploy an instance of your Ownable contract from an address
  2. The Ownable constructor sets the owner of the contract as that address
  3. You deploy a proxy for your contract
  4. You send a transaction to your proxy to invoke an onlyOwner method
  5. The proxy checks its owner state variable in its own storage which was never initialized so it is the zero address
  6. Since the msg.sender is not the zero address, the tx reverts

To fix this, make sure your contracts inherit from the openzeppelin-zos Ownable implementation, and explicitly call initialize in your proxy with the address that should be the owner. This doc has more info on the initializer vs constructor differences.

Let me know if this works!

spalladino commented 6 years ago

Also, and if you don't mind me asking: why are you using the Proxy contracts directly, instead of via the ZeppelinOS CLI? Do you have any particular use case that doesn't fit?