openzeppelin-labs icon indicating copy to clipboard operation
openzeppelin-labs copied to clipboard

Unstructure storage code doesn't work for onlyOwner methods.

Open ProphetDaniel opened this issue 7 years ago • 4 comments

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.

ProphetDaniel avatar Aug 28 '18 02:08 ProphetDaniel

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

spalladino avatar Aug 28 '18 13:08 spalladino

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

ProphetDaniel avatar Aug 30 '18 14:08 ProphetDaniel

@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 avatar Aug 30 '18 22:08 spalladino

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?

spalladino avatar Aug 30 '18 22:08 spalladino