Merge pull request #114 from aboutcircles/v1.0.1-patch2-upgradable-gr…

…oup-policies

Patch UpgradeableRenounceableProxy to strictly enforce promises

src/groups/UpgradeableRenounceableProxy.sol

@@ -6,7 +6,7 @@ import {ERC1967Utils} from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Utils.s
 
 interface IUpgradeableRenounceableProxy {
     function implementation() external view returns (address);
-    function upgradeToAndCall(address _newImplementation, bytes memory _data) external;
+    function upgradeToAndCall(address newImplementation, bytes memory data) external;
     function renounceUpgradeability() external;
 }
 
@@ -15,18 +15,14 @@ contract UpgradeableRenounceableProxy is ERC1967Proxy {
 
     error BlockReceive();
 
-    // Constants
-
-    /// @dev Initial proxy admin.
-    address internal immutable ADMIN_INIT;
+    /// Triggered when the delegatecall modifies values, indicating a violation of proxy-native functionality.
+    error ProxyNative();
 
     // Constructor
 
     constructor(address _implementation, bytes memory _data) ERC1967Proxy(_implementation, _data) {
         // set the admin to the deployer
         ERC1967Utils.changeAdmin(msg.sender);
-        // set the admin as immutable
-        ADMIN_INIT = msg.sender;
     }
 
     /// @dev Handles proxy function calls: attempts to dispatch to a specific
@@ -42,10 +38,51 @@ contract UpgradeableRenounceableProxy is ERC1967Proxy {
             }
         }
         // dispatch if caller is admin, otherwise delegate to the implementation
-        if (msg.sender == ADMIN_INIT && msg.sender == ERC1967Utils.getAdmin()) {
+        if (msg.sender == ERC1967Utils.getAdmin()) {
             _dispatchAdmin();
         } else {
-            super._fallback();
+            // in principle this can allow the admin to reenter the proxy,
+            // and hot swap the implementation.
+            _delegate(_implementation());
+        }
+    }
+
+    /// @dev Overrides the function to add a check that prevents rewriting of admin and implementation slots.
+    function _delegate(address implementation) internal virtual override {
+        bytes32 adminSlot = ERC1967Utils.ADMIN_SLOT;
+        bytes32 implementationSlot = ERC1967Utils.IMPLEMENTATION_SLOT;
+        bytes32 errorProxyNative = ProxyNative.selector;
+        assembly {
+            // put the admin value on the stack before delegatecall (the implementation value has already been read and is on the stack)
+            let originalAdminValue := sload(adminSlot)
+            // Copy msg.data. We take full control of memory in this inline assembly
+            // block because it will not return to Solidity code. We overwrite the
+            // Solidity scratch pad at memory position 0.
+            calldatacopy(0, 0, calldatasize())
+
+            // Call the implementation.
+            // out and outsize are 0 because we don't know the size yet.
+            let result := delegatecall(gas(), implementation, 0, calldatasize(), 0, 0)
+
+            // Copy the returned data.
+            returndatacopy(0, 0, returndatasize())
+            switch result
+            // delegatecall returns 0 on error.
+            case 0 { revert(0, returndatasize()) }
+            default {
+                // read the values after the delegatecall
+                let currentAdminValue := sload(adminSlot)
+                let currentImplementationValue := sload(implementationSlot)
+                // check that the values remain unchanged
+                if iszero(
+                    and(eq(originalAdminValue, currentAdminValue), eq(implementation, currentImplementationValue))
+                ) {
+                    // revert with ProxyNative error, as delegatecall has modified values (proxy-native functionality)
+                    mstore(0, errorProxyNative)
+                    revert(0, 0x04)
+                }
+                return(0, returndatasize())
+            }
         }
     }
 

test/groups/upgradeableProxy/adminOperationsUpgradeableRenounceableProxy.t.sol

@@ -96,14 +96,6 @@ contract adminOperationsUpgradeableRenounceableProxy is Test, GroupSetup {
         assertEq(implementation, _readImplementationSlot());
     }
 
-    /* todo: - test getting admin from proxy (DONE)
-     *       - test admin cannot be changed (FAILED)
-     *       - test noone else can call upgradeToAndCall (FAILED)
-     *       - test upgradeToAndCall with call data (DONE)
-     *       - test renouncing admin (DONE)
-     *       - test accessibility of interface functions from non-Admin callers (DONE)
-     */
-
     // External upgradeToAndCall(address,bytes)
 
     function testAdminUpgradeToAndCall() public {
@@ -281,12 +273,10 @@ contract adminOperationsUpgradeableRenounceableProxy is Test, GroupSetup {
         console2.log(success);
     }
 
-    // Test admin cannot be changed
+    // Test admin cannot be modified
 
-    // failed test demonstrates that admin can be changed to any address, however it doesn't make sense
-    // since newAdmin != ADMIN_INIT. it make sense only when upgradeability is renounced to revert this action.
-    function testFail_AdminCannotBeChanged(address newAdmin) public {
-        vm.assume(newAdmin != group);
+    function testAdminCannotBeModified(address newAdmin) public {
+        vm.assume(newAdmin != group && newAdmin != address(0));
         // upgrade to mock mint policy extended
         _upgradeToAndCall(mockMintPolicyExtended, _defaultMockExtendedData(whitelistAdmin));
 
@@ -295,7 +285,7 @@ contract adminOperationsUpgradeableRenounceableProxy is Test, GroupSetup {
 
         // should not change admin
         vm.prank(whitelistAdmin);
-        // vm.expectRevert();
+        vm.expectRevert(UpgradeableRenounceableProxy.ProxyNative.selector);
         IMockMintPolicyExtended(address(proxy)).setProxyAdmin(newAdmin);
 
         proxyAdmin = _readProxyAdminSlot();
@@ -304,8 +294,7 @@ contract adminOperationsUpgradeableRenounceableProxy is Test, GroupSetup {
 
     // Test noone else can call upgradeToAndCall
 
-    // failed test demonstrates that implementation can be changed by any address (everyone can call upgradeToAndCall)
-    function testFail_NonAdminCannotCallUpgradeToAndCall(address anyAddress) public {
+    function testNonAdminCannotCallUpgradeToAndCall(address anyAddress) public {
         vm.assume(anyAddress != group);
         // upgrade to mock mint policy extended
         _upgradeToAndCall(mockMintPolicyExtended, _defaultMockExtendedData(whitelistAdmin));
@@ -319,7 +308,7 @@ contract adminOperationsUpgradeableRenounceableProxy is Test, GroupSetup {
 
         // any address should not be able to call upgradeToAndCall
         vm.prank(anyAddress);
-        // vm.expectRevert();
+        vm.expectRevert(UpgradeableRenounceableProxy.ProxyNative.selector);
         IMockMintPolicyExtended(address(proxy)).setProxyImplementation(mintPolicy, "");
 
         implementation = _readImplementationSlot();