Transparent Proxy #36

frangio · 2018-04-12T16:53:39Z

The idea explored here is to make the Proxy entirely transparent to a normal user, removing the conflicts between the public administrative interface of the proxy itself, and the interface of the underlying proxied contract.

For example, OwnedUpgradeabilityProxy cannot have a public function owner(), because the proxied contract may have a function with the same signature. If both the proxy and proxied contracts had a function owner(), any such call would be caught by the proxy and not be delegated.

We solve this situation essentially by modifying onlyProxyOwner to execute the administrative function if the caller is the proxyOwner, and otherwise delegate the call to the proxied contract.

https://github.com/zeppelinos/core/blob/8f81183f4de074fb2359ceab285c2cbf085a7c3a/contracts/upgradeability/OwnedUpgradeabilityProxy.sol#L30-L36

There is a caveat, which is that Proxy#implementation is now only accessible by the proxyOwner. When the proxy owner is a contract that manages upgrades, such as ProjectController, it should expose this information about its owned proxies.

https://github.com/zeppelinos/core/blob/8f81183f4de074fb2359ceab285c2cbf085a7c3a/contracts/ProjectController.sol#L163-L165

Similarly, OwnedUpgradeabilityProxy#proxyOwner cannot be queried by anyone, only by the proxyOwner. This makes it kind of redundant to expose proxyOwner via the owner contract (like we can do with implementation). If someone wished to query who is the owner for a particular proxy they would have to read the storage location directly, or look for proxy ownership events originating from its address.

I'd like to hear everyone's thoughts on this.

Possibly rename onlyProxyOwner as the semantics are different from the usual only* modifiers.
Remove getProxyOwner and test ownership changes differently.
Fix the failing OwnedUpgradeabilityProxy tests (see comment below).

frangio · 2018-04-12T18:27:08Z

The tests are failing because of the way OwnedUpgradeabilityProxy is being tested: the implementations behind the proxy are not actual contracts but account addresses.

But there is kind of a deeper reason why the previous tests may no longer be appropriate. For example one of the failing tests is "OwnedUpgradeabilityProxy upgradeTo: when the sender is not the owner reverts". This is no longer true in the "transparent" model. When the sender is not the owner the behavior will depend on the underlying implementation.

This fact makes me doubt if the transparent model is a good one.

spalladino · 2018-04-12T22:43:54Z

There is another reason for this feature, and that is that function signature clashes are not that uncommon. A method identifier is 4 bytes, which is only 2^32 possible values. For reference, the 4byte directory has currently about 2^15 registered function names. The main problem is that, without the compiler detecting the clash, the underlying function will be uncallable without the developer noticing.

An alternative is to build this into a linter to check this kind of issues, but I'd still like to ensure no clashes at a contract level.

Another alternative (at contract level) would be to require an extra 256-bit parameter to be included in every call that should be directed to the proxy itself, thus artificially increasing the length of the signature; but I think I prefer your implementation.

spalladino · 2018-04-12T22:44:51Z

contracts/upgradeability/OwnedUpgradeabilityProxy.sol

+   * @dev Redefines Proxy's fallback to disallow delegation when caller is the proxy owner.
+   */
+  function () public payable {
+    require(msg.sender != proxyOwner());


Wouldn't this break the createAndCall methods?

I think it shouldn't since the factory performs the call and then transfers the ownership to the requested owner

Still, I believe it's a bit too brittle. I'd consider removing this check.

I wouldn't remove the check for this reason alone. I think the proxyOwner should only be allowed to use the proxy opaquely. If this breaks our current implementation of initialization, we may need to revise that part of the model.

I agree that it is brittle in this way and that it needs to be tackled, but I'm going to think of other solutions.

fiiiu · 2018-05-07T21:12:51Z

contracts/upgradeability/OwnedUpgradeabilityProxy.sol

@@ -28,8 +28,11 @@ contract OwnedUpgradeabilityProxy is UpgradeabilityProxy {
  * @dev Throws if called by any account other than the owner.


update docs to reflect current caller-dependent behavior.

fiiiu · 2018-05-07T21:18:06Z

contracts/upgradeability/OwnedUpgradeabilityProxy.sol

+  function implementation() public view returns (address) {
+    return _implementation();
+  }
+
  /**
   * @dev Sets the address of the owner
   */


NIT: L59, setUpgradeabilityOwner -> _setUpgradeabilityOwner? See also calls in L24 & L73.

frangio · 2018-05-08T21:37:29Z

I've rebased the changes on master and I'm working on getting this running again. I still need to fix a few tests.

frangio · 2018-05-10T21:08:23Z

This is an important PR so please review it in detail. 🙏

frangio · 2018-05-10T21:18:37Z

The only thing I still have doubts about are the proxyOwner and getProxyOwner functions. The first one only works when the proxy owner calls it, and the second one returns the same AppManager address on which you must call the function, so it is all somewhat redundant. Someone other than the proxy owner can get this information by reading from storage:

zos-lib/test/upgradeability/OwnedUpgradeabilityProxy.test.js

Lines 298 to 299 in 5889da1

    
           const position = web3.sha3("org.zeppelinos.proxy.owner"); 
        
           const proxyOwner = await web3.eth.getStorageAt(this.proxyAddress, position);

We should provide in the zos-lib package a helper to read from storage at this position. We could use this same helper in the tests, instead of the redundant getProxyOwner.

Thoughts?

facuspagnuolo · 2018-05-10T21:42:00Z

@frangio would you mind rebasing from master?

…ransparent-proxy

facuspagnuolo

Thanks fran! Looking really good, left some minor comments

facuspagnuolo · 2018-05-11T15:28:39Z

.gitignore

@@ -3,3 +3,4 @@ build/
 lib/
 coverage/
 coverage.json
+.node-xmlhttprequest-sync-*


facuspagnuolo · 2018-05-11T15:30:40Z

contracts/mocks/ClashingImplementation.sol

+  function delegatedFunction() external pure returns (bool) {
+    return true;
+  }
+}


we should move all the mocks for testing purpose to a test folder and npm-ignore it

facuspagnuolo · 2018-05-11T15:36:04Z

contracts/application/management/BaseAppManager.sol

+   */
+  function getProxyOwner(OwnedUpgradeabilityProxy proxy) public view returns (address) {
+    return proxy.proxyOwner();
+  }


This is kind of redundant, isn't it? We are asking the owner of the proxy to tell us the owner of the proxy :p
But I guess we will need it for consistency tho

Yeah! See this comment: #36 (comment)

Agree with adding a helper to read the owner from storage directly :)

I'd go with both options. The helper is needed when BaseAppManager is not used, but it's somewhat obscure. If an app manager is used, the symmetry between getProxyOwner and getProxyImplementation would be helpful for newcomers.

facuspagnuolo · 2018-05-11T15:36:53Z

contracts/mocks/ClashingImplementation.sol

+pragma solidity ^0.4.21;
+
+
+contract ClashingImplementation {


we can add an inline comment here explaining what is clashing in this contract

facuspagnuolo · 2018-05-11T15:50:08Z

contracts/upgradeability/Proxy.sol

+   */
+  function () payable external {
+    _fallback();
+  }


can we move internal functions to the bottom of the contract?

facuspagnuolo · 2018-05-11T15:52:54Z

contracts/upgradeability/OwnedUpgradeabilityProxy.sol

  /**
   * @dev Sets the address of the owner
   */
-  function setUpgradeabilityOwner(address newProxyOwner) internal {
+  function _setUpgradeabilityOwner(address newProxyOwner) internal {


let's use setProxyOwner here

facuspagnuolo · 2018-05-11T15:58:04Z

contracts/upgradeability/UpgradeabilityProxyFactory.sol

    proxy.transferProxyOwnership(owner);
+    require(proxy.call.value(msg.value)(data));


Nice catch @frangio

facuspagnuolo · 2018-05-11T16:01:47Z

test/upgradeability/OwnedUpgradeabilityProxy.test.js


-          const proxyOwner = await this.proxy.proxyOwner()
+        it('assigns new proxy owner', async function () {
+          // [TODO] maybe we should test this by reading from storage


it would be too implementative in this context IMO... we are already checking storage works as expected

if you agree please remove the TODO note or add an issue to tackle it

facuspagnuolo · 2018-05-11T16:04:23Z

test/upgradeability/OwnedUpgradeabilityProxy.test.js

+        assert.equal(value, '0x0000000000000000000000000000000011111142')
+      });
+    });
+  });


…ransparent-proxy

fiiiu · 2018-05-11T15:59:38Z

contracts/upgradeability/Proxy.sol

 */
 contract Proxy {
  /**
-   * @dev Tells the address of the implementation where every call will be delegated.
-   * @return address of the implementation to which it will be delegated
+   * @return address of the implementation to which the fallback delegates all calls


Do we also want a @dev line here? I understand that it will be redundant, but concerned about automated doc generation..

fiiiu · 2018-05-11T16:33:03Z

contracts/mocks/ClashingImplementation.sol

+pragma solidity ^0.4.21;
+
+
+contract ClashingImplementation {


fiiiu · 2018-05-11T16:39:41Z

contracts/upgradeability/OwnedUpgradeabilityProxy.sol

  /**
   * @dev Sets the address of the owner
   */
-  function setUpgradeabilityOwner(address newProxyOwner) internal {
+  function _setUpgradeabilityOwner(address newProxyOwner) internal {


fiiiu · 2018-05-11T16:51:33Z

contracts/upgradeability/UpgradeabilityProxy.sol

@@ -52,7 +56,7 @@ contract UpgradeabilityProxy is Proxy {

  /**
   * @dev Upgrades the implementation address
-   * @param newImplementation The address of the new implementation to be set
+   * @param newImplementation representing the address of the new implementation to be set
   */
  function _upgradeTo(address newImplementation) internal {


Do we really need both _upgradeTo() and _setImplementation() or is this just a naming issue?

_setImplementation() should be a dumb setter for the org.zeppelinos.proxy.implementation slot, which is why it's private. I'm going to move the other logic to _upgradeTo and add comments explaining.

I ended up leaving the isContract check in the otherwise "dumb" setter because it's quite crucial and in fact it initially led me to a bug which was hard to pin down (but correctly caused the tests to fail).

frangio · 2018-05-12T18:23:51Z

@fiiiu @facuspagnuolo All review comments addressed! 🚀

maraoz · 2018-05-13T23:00:07Z

Reviewed the code and all pending comments. LGTM.
I think the test helper for extracting the proxyOwner can be left out of scope of this PR, so I opened an issue for it, and we can address it in the future: #100
Merging, great work @frangio!

…sparent-proxy Transparent Proxy

frangio added the status:in progress Pull Request is a work in progress label Apr 12, 2018

spalladino reviewed Apr 12, 2018

View reviewed changes

facuspagnuolo added status:blocked Blocked issue and removed status:in progress Pull Request is a work in progress labels Apr 17, 2018

facuspagnuolo added this to the v0.1.0 (Kernel MVP) milestone Apr 17, 2018

facuspagnuolo assigned frangio Apr 17, 2018

frangio modified the milestones: v0.1.0 (Kernel MVP), v0.2.0 Apr 24, 2018

fiiiu reviewed May 7, 2018

View reviewed changes

implement transparent proxy

9f590ce

frangio force-pushed the feature/transparent-proxy branch from 8f81183 to 9f590ce Compare May 8, 2018 21:35

frangio added status:in progress Pull Request is a work in progress and removed status:blocked Blocked issue labels May 8, 2018

frangio added 7 commits May 9, 2018 16:57

make remaining tests pass

fc45ac4

Merge remote-tracking branch 'origin' into feature/transparent-proxy

787c803

ignore artifact generated by tests

09c243b

add _willFallback extension point

bfb7f25

make all proxy functions external

64965f6

improve documentation

b904096

rename modifier to ifProxyOwner

7902a34

frangio requested a review from fiiiu May 10, 2018 18:59

frangio added 3 commits May 10, 2018 16:07

revert to public the only ContractProvider function

458a166

tweak inline documentation and some variable names

385f0d2

make _setImplementation private for more encapsulation

8577314

frangio added status:review PR waiting for review and removed status:in progress Pull Request is a work in progress labels May 10, 2018

frangio requested a review from facuspagnuolo May 10, 2018 21:04

add tests for transparent proxy behavior

3c1f279

frangio force-pushed the feature/transparent-proxy branch from e007c10 to 3c1f279 Compare May 10, 2018 21:07

Merge branch 'master' of github.com:zeppelinos/zos-lib into feature/t…

1085757

…ransparent-proxy

facuspagnuolo approved these changes May 11, 2018

View reviewed changes

Merge branch 'master' of github.com:zeppelinos/zos-lib into feature/t…

73b9b03

…ransparent-proxy

fiiiu reviewed May 11, 2018

View reviewed changes

address review comments

fd7dda1

fix inline docs after merge

954ef4a

maraoz mentioned this pull request May 13, 2018

Expose a JavaScript function to read the proxyOwner from any proxy's storage #100

Closed

maraoz merged commit 44851b1 into master May 13, 2018

frangio deleted the feature/transparent-proxy branch May 13, 2018 23:33

frangio mentioned this pull request May 14, 2018

Fixes after Nomic Labs audit #102

Merged

15 tasks

fiiiu mentioned this pull request May 23, 2018

docstrings: update the comments of the contracts #143

Merged

facuspagnuolo removed this from the Backlog milestone Jun 4, 2018

sohkai mentioned this pull request Jun 12, 2018

Transparent proxies? aragon/aragonOS#330

Closed

spalladino pushed a commit to OpenZeppelin/openzeppelin-sdk that referenced this pull request Aug 1, 2018

Merge pull request zeppelinos/zos-lib#36 from zeppelinos/feature/tran…

828ce47

…sparent-proxy Transparent Proxy

ascjones mentioned this pull request Feb 7, 2023

Custom Selectors and Proxy Selector Clashing Attacks use-ink/ink#1643

Closed

		@@ -28,8 +28,11 @@ contract OwnedUpgradeabilityProxy is UpgradeabilityProxy {
		* @dev Throws if called by any account other than the owner.

		proxy.transferProxyOwnership(owner);
		require(proxy.call.value(msg.value)(data));

Transparent Proxy #36

Transparent Proxy #36

Conversation

frangio commented Apr 12, 2018 • edited Loading

frangio commented Apr 12, 2018 • edited Loading

spalladino commented Apr 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fiiiu May 7, 2018 • edited Loading

Choose a reason for hiding this comment

frangio commented May 8, 2018

frangio commented May 10, 2018

frangio commented May 10, 2018 • edited Loading

facuspagnuolo commented May 10, 2018

facuspagnuolo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frangio May 11, 2018 • edited Loading

Choose a reason for hiding this comment

frangio commented May 12, 2018

maraoz commented May 13, 2018

frangio commented Apr 12, 2018 •

edited

Loading

frangio commented Apr 12, 2018 •

edited

Loading

fiiiu May 7, 2018 •

edited

Loading

frangio commented May 10, 2018 •

edited

Loading

frangio May 11, 2018 •

edited

Loading