From 195f151a7ce7ea5ff7aee0fdc038f6b97b1144f7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Muhammed=20Tanr=C4=B1kulu?= Date: Tue, 19 Nov 2024 14:42:09 +0100 Subject: [PATCH 1/5] test storage persistence after registry upgrade --- src/VerifiableFactory.sol | 38 ++++++++++++++++++--------------- test/VerifiableFactory.t.sol | 41 ++++++++++++++++++++++++++++++++++++ 2 files changed, 62 insertions(+), 17 deletions(-) diff --git a/src/VerifiableFactory.sol b/src/VerifiableFactory.sol index d21f805..4c12dba 100644 --- a/src/VerifiableFactory.sol +++ b/src/VerifiableFactory.sol @@ -36,8 +36,6 @@ contract VerifiableFactory { * @return proxy The address of the deployed `TransparentVerifiableProxy`. */ function deployProxy(address implementation, uint256 salt) external returns (address) { - console.log("deploys"); - console.logAddress(msg.sender); bytes32 outerSalt = keccak256(abi.encode(msg.sender, salt)); TransparentVerifiableProxy proxy = new TransparentVerifiableProxy{salt: outerSalt}(address(this)); @@ -74,26 +72,32 @@ contract VerifiableFactory { return false; } try IProxy(proxy).salt() returns (uint256 salt) { - try IProxy(proxy).creator() returns (address creator) { - // verify the creator matches this factory - if (address(this) != creator) { - return false; - } + try IProxy(proxy).owner() returns (address owner) { + try IProxy(proxy).creator() returns (address creator) { + return _verifyContract(proxy, creator, owner, salt); + } catch {} + } catch {} + } catch {} + + return false; + } - // reconstruct the address using CREATE2 and verify it matches - bytes32 outerSalt = keccak256(abi.encode(msg.sender, salt)); + function _verifyContract(address proxy, address creator, address owner, uint256 salt) private view returns (bool) { + // verify the creator matches this factory + if (address(this) != creator) { + return false; + } - // get creation bytecode with constructor arguments - bytes memory bytecode = - abi.encodePacked(type(TransparentVerifiableProxy).creationCode, abi.encode(address(this))); + // reconstruct the address using CREATE2 and verify it matches + bytes32 outerSalt = keccak256(abi.encode(owner, salt)); - address expectedProxyAddress = Create2.computeAddress(outerSalt, keccak256(bytecode), address(this)); + // get creation bytecode with constructor arguments + bytes memory bytecode = + abi.encodePacked(type(TransparentVerifiableProxy).creationCode, abi.encode(address(this))); - return expectedProxyAddress == proxy; - } catch {} - } catch {} + address expectedProxyAddress = Create2.computeAddress(outerSalt, keccak256(bytecode), address(this)); - return false; + return expectedProxyAddress == proxy; } function isContract(address account) internal view returns (bool) { diff --git a/test/VerifiableFactory.t.sol b/test/VerifiableFactory.t.sol index e1b17f4..8f1b02c 100644 --- a/test/VerifiableFactory.t.sol +++ b/test/VerifiableFactory.t.sol @@ -142,6 +142,47 @@ contract VerifiableFactoryTest is Test { assertEq(proxy.creator(), address(factory), "Wrong creator"); } + function test_StoragePersistenceAfterUpgrade() public { + uint256 salt = 1; + address testAccount = makeAddr("testAccount"); + + // deploy proxy + vm.prank(owner); + address proxyAddress = factory.deployProxy(address(implementation), salt); + + // initialize v1 implementation + MockRegistry proxyV1 = MockRegistry(proxyAddress); + + // initialize registry + vm.prank(owner); + proxyV1.initialize(owner); + assertEq(proxyV1.admin(), owner, "Admin should be set"); + + // register an address + vm.prank(owner); + proxyV1.register(testAccount); + assertTrue(proxyV1.registeredAddresses(testAccount), "Address should be registered in V1"); + assertEq(proxyV1.getRegistryVersion(), 1, "Should be V1 implementation"); + + // upgrade to v2 + vm.prank(owner); + factory.upgradeImplementation(proxyAddress, address(implementationV2), ""); + + // verify state persists after upgrade + MockRegistryV2 proxyV2 = MockRegistryV2(proxyAddress); + + // check storage persistence + assertTrue(proxyV2.registeredAddresses(testAccount), "Address registration should persist after upgrade"); + assertEq(proxyV2.admin(), owner, "Admin should persist after upgrade"); + assertEq(proxyV2.getRegistryVersion(), 2, "Should be V2 implementation"); + + // verify v2 functionality still works as it should be + address newTestAccount = makeAddr("newTestAccount"); + vm.prank(owner); + proxyV2.register(newTestAccount); + assertTrue(proxyV2.registeredAddresses(newTestAccount), "Should be able to register new address in V2"); + } + // ### Helpers function isContract(address account) internal view returns (bool) { uint256 size; From b7d1d1c1f9cd85e0a5654f44a8b64a48c140cc36 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Muhammed=20Tanr=C4=B1kulu?= Date: Thu, 21 Nov 2024 14:59:50 +0100 Subject: [PATCH 2/5] remove unnecessary creator check, since it's a part of address creation under CREATE2 --- src/VerifiableFactory.sol | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/src/VerifiableFactory.sol b/src/VerifiableFactory.sol index 4c12dba..bb9d96e 100644 --- a/src/VerifiableFactory.sol +++ b/src/VerifiableFactory.sol @@ -9,8 +9,6 @@ interface IProxy { function salt() external view returns (uint256); function owner() external view returns (address); - - function creator() external view returns (address); } contract VerifiableFactory { @@ -73,21 +71,14 @@ contract VerifiableFactory { } try IProxy(proxy).salt() returns (uint256 salt) { try IProxy(proxy).owner() returns (address owner) { - try IProxy(proxy).creator() returns (address creator) { - return _verifyContract(proxy, creator, owner, salt); - } catch {} + return _verifyContract(proxy, owner, salt); } catch {} } catch {} return false; } - function _verifyContract(address proxy, address creator, address owner, uint256 salt) private view returns (bool) { - // verify the creator matches this factory - if (address(this) != creator) { - return false; - } - + function _verifyContract(address proxy, address owner, uint256 salt) private view returns (bool) { // reconstruct the address using CREATE2 and verify it matches bytes32 outerSalt = keccak256(abi.encode(owner, salt)); From ea98561f61440105795c1ea420f1339f71626b16 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Muhammed=20Tanr=C4=B1kulu?= Date: Thu, 21 Nov 2024 18:40:53 +0100 Subject: [PATCH 3/5] implement common ITransParentVerifiableProxy interface --- src/ITransparentVerifiableProxy.sol | 10 ++++++++++ src/TransparentVerifiableProxy.sol | 3 ++- src/VerifiableFactory.sol | 13 ++++--------- 3 files changed, 16 insertions(+), 10 deletions(-) create mode 100644 src/ITransparentVerifiableProxy.sol diff --git a/src/ITransparentVerifiableProxy.sol b/src/ITransparentVerifiableProxy.sol new file mode 100644 index 0000000..c5fca5d --- /dev/null +++ b/src/ITransparentVerifiableProxy.sol @@ -0,0 +1,10 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.20; + +interface ITransParentVerifiableProxy { + function salt() external view returns (uint256); + + function owner() external view returns (address); + + function creator() external view returns (address); +} diff --git a/src/TransparentVerifiableProxy.sol b/src/TransparentVerifiableProxy.sol index d19de15..0aebffc 100644 --- a/src/TransparentVerifiableProxy.sol +++ b/src/TransparentVerifiableProxy.sol @@ -8,6 +8,7 @@ pragma solidity ^0.8.20; import {Proxy} from "@openzeppelin/contracts/proxy/Proxy.sol"; import {ERC1967Utils} from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Utils.sol"; import {Initializable} from "@openzeppelin/contracts/proxy/utils/Initializable.sol"; +import {ITransParentVerifiableProxy} from "./ITransparentVerifiableProxy.sol"; // EIP-2535 Diamond Storage pattern // ref: https://eips.ethereum.org/EIPS/eip-2535#storage @@ -24,7 +25,7 @@ library StorageSlot { } } -interface ITransparentVerifiableProxy { +interface ITransparentVerifiableProxy is ITransParentVerifiableProxy { /// @dev See {UUPSUpgradeable-upgradeToAndCall} function upgradeToAndCall(address newImplementation, bytes calldata data) external payable; } diff --git a/src/VerifiableFactory.sol b/src/VerifiableFactory.sol index bb9d96e..cfd427d 100644 --- a/src/VerifiableFactory.sol +++ b/src/VerifiableFactory.sol @@ -4,12 +4,7 @@ pragma solidity ^0.8.20; import {console} from "forge-std/console.sol"; import {Create2} from "@openzeppelin/contracts/utils/Create2.sol"; import {ITransparentVerifiableProxy, TransparentVerifiableProxy} from "./TransparentVerifiableProxy.sol"; - -interface IProxy { - function salt() external view returns (uint256); - - function owner() external view returns (address); -} +import {ITransParentVerifiableProxy} from "./ITransParentVerifiableProxy.sol"; contract VerifiableFactory { event ProxyDeployed(address indexed sender, address indexed proxyAddress, uint256 salt, address implementation); @@ -48,7 +43,7 @@ contract VerifiableFactory { // Function to upgrade the proxy's implementation (only owner of proxy can call this) function upgradeImplementation(address proxyAddress, address newImplementation, bytes memory data) external { - address owner = IProxy(proxyAddress).owner(); + address owner = ITransparentVerifiableProxy(proxyAddress).owner(); require(owner == msg.sender, "Only the owner can upgrade"); // Upgrade the proxy to point to the new implementation @@ -69,8 +64,8 @@ contract VerifiableFactory { if (!isContract(proxy)) { return false; } - try IProxy(proxy).salt() returns (uint256 salt) { - try IProxy(proxy).owner() returns (address owner) { + try ITransparentVerifiableProxy(proxy).salt() returns (uint256 salt) { + try ITransparentVerifiableProxy(proxy).owner() returns (address owner) { return _verifyContract(proxy, owner, salt); } catch {} } catch {} From aa39c67419af96251cf5eafbf6d316ae0f044481 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Muhammed=20Tanr=C4=B1kulu?= Date: Thu, 21 Nov 2024 18:49:54 +0100 Subject: [PATCH 4/5] fix interface --- src/ITransparentVerifiableProxy.sol | 5 ++++- src/TransparentVerifiableProxy.sol | 7 +------ src/VerifiableFactory.sol | 4 ++-- 3 files changed, 7 insertions(+), 9 deletions(-) diff --git a/src/ITransparentVerifiableProxy.sol b/src/ITransparentVerifiableProxy.sol index c5fca5d..f48345a 100644 --- a/src/ITransparentVerifiableProxy.sol +++ b/src/ITransparentVerifiableProxy.sol @@ -1,10 +1,13 @@ // SPDX-License-Identifier: MIT pragma solidity ^0.8.20; -interface ITransParentVerifiableProxy { +interface ITransparentVerifiableProxy { function salt() external view returns (uint256); function owner() external view returns (address); function creator() external view returns (address); + + /// @dev See {UUPSUpgradeable-upgradeToAndCall} + function upgradeToAndCall(address newImplementation, bytes calldata data) external payable; } diff --git a/src/TransparentVerifiableProxy.sol b/src/TransparentVerifiableProxy.sol index 2b9c1c0..37fd815 100644 --- a/src/TransparentVerifiableProxy.sol +++ b/src/TransparentVerifiableProxy.sol @@ -10,12 +10,7 @@ import {ERC1967Utils} from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Utils.s import {Initializable} from "@openzeppelin/contracts/proxy/utils/Initializable.sol"; import {StorageSlot} from "@openzeppelin/contracts/utils/StorageSlot.sol"; import {SlotDerivation} from "@openzeppelin/contracts/utils/SlotDerivation.sol"; -import {ITransParentVerifiableProxy} from "./ITransparentVerifiableProxy.sol"; - -interface ITransparentVerifiableProxy is ITransParentVerifiableProxy { - /// @dev See {UUPSUpgradeable-upgradeToAndCall} - function upgradeToAndCall(address newImplementation, bytes calldata data) external payable; -} +import {ITransparentVerifiableProxy} from "./ITransparentVerifiableProxy.sol"; contract TransparentVerifiableProxy is Proxy, Initializable { using StorageSlot for bytes32; diff --git a/src/VerifiableFactory.sol b/src/VerifiableFactory.sol index cfd427d..7f05e0a 100644 --- a/src/VerifiableFactory.sol +++ b/src/VerifiableFactory.sol @@ -3,8 +3,8 @@ pragma solidity ^0.8.20; import {console} from "forge-std/console.sol"; import {Create2} from "@openzeppelin/contracts/utils/Create2.sol"; -import {ITransparentVerifiableProxy, TransparentVerifiableProxy} from "./TransparentVerifiableProxy.sol"; -import {ITransParentVerifiableProxy} from "./ITransParentVerifiableProxy.sol"; +import {TransparentVerifiableProxy} from "./TransparentVerifiableProxy.sol"; +import {ITransparentVerifiableProxy} from "./ITransparentVerifiableProxy.sol"; contract VerifiableFactory { event ProxyDeployed(address indexed sender, address indexed proxyAddress, uint256 salt, address implementation); From 73242b9ea28656f3341a25e4926b4893cc038620 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Muhammed=20Tanr=C4=B1kulu?= Date: Mon, 25 Nov 2024 14:58:52 +0100 Subject: [PATCH 5/5] add slot derivation test --- src/TransparentVerifiableProxy.sol | 6 +-- test/TransparentVerifiableProxy.t.sol | 72 +++++++++++++++++++++++++++ 2 files changed, 75 insertions(+), 3 deletions(-) create mode 100644 test/TransparentVerifiableProxy.t.sol diff --git a/src/TransparentVerifiableProxy.sol b/src/TransparentVerifiableProxy.sol index 37fd815..1406232 100644 --- a/src/TransparentVerifiableProxy.sol +++ b/src/TransparentVerifiableProxy.sol @@ -27,9 +27,9 @@ contract TransparentVerifiableProxy is Proxy, Initializable { // ### EVENTS error ProxyDeniedOwnerAccess(); - // // Modifier that allows only the owner to call certain functions - // modifier onlyOwner() { - // require(msg.sender == owner, "Caller is not the owner"); + // modifier that allows only the creator address to call certain functions + // modifier onlyCreator() { + // require(msg.sender == creator, "Caller is not the creator address"); // _; // } diff --git a/test/TransparentVerifiableProxy.t.sol b/test/TransparentVerifiableProxy.t.sol new file mode 100644 index 0000000..d39f228 --- /dev/null +++ b/test/TransparentVerifiableProxy.t.sol @@ -0,0 +1,72 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.20; + +import "forge-std/Test.sol"; +import "../src/TransparentVerifiableProxy.sol"; +import {SlotDerivation} from "@openzeppelin/contracts/utils/SlotDerivation.sol"; +import {MockRegistry} from "../src/mock/MockRegistry.sol"; + +contract TransparentVerifiableProxyTest is Test { + using SlotDerivation for bytes32; + + TransparentVerifiableProxy proxy; + + address creator = address(0x1); + address owner = address(0x2); + address implementation = address(new MockRegistry()); + uint256 salt = 12345; + bytes emptyData; + + string internal constant _VERIFICATION_SLOT = "proxy.verifiable"; + string internal constant _SALT = "salt"; + string internal constant _OWNER = "owner"; + + function setUp() public { + proxy = new TransparentVerifiableProxy(creator); + } + + function testInitialize() public { + // initialize the proxy + proxy.initialize(salt, owner, implementation, emptyData); + + // check salt and owner values + assertEq(proxy.salt(), salt, "Salt mismatch"); + assertEq(proxy.owner(), owner, "Owner mismatch"); + } + + function testSaltStorage() public { + // initialize the proxy + proxy.initialize(salt, owner, implementation, emptyData); + + // compute the base slot + bytes32 baseSlot = SlotDerivation.erc7201Slot(_VERIFICATION_SLOT); + + // use SlotDerivation to compute the salt slot + bytes32 saltSlot = baseSlot.deriveMapping(_SALT); + + // directly manipulate the storage for the salt + uint256 newSalt = 54321; + vm.store(address(proxy), saltSlot, bytes32(newSalt)); + + // verify the updated salt + assertEq(proxy.salt(), newSalt, "Salt update failed"); + } + + function testOwnerStorage() public { + // initialize the proxy + proxy.initialize(salt, owner, implementation, emptyData); + + // compute the base slot + bytes32 baseSlot = SlotDerivation.erc7201Slot(_VERIFICATION_SLOT); + + // use SlotDerivation to compute the owner slot + bytes32 ownerSlot = baseSlot.deriveMapping(_OWNER); + + // directly manipulate the storage for the owner + address newOwner = address(0x4); + vm.store(address(proxy), ownerSlot, bytes32(uint256(uint160(newOwner)))); + + // verify the updated owner + assertEq(proxy.owner(), newOwner, "Owner update failed"); + } +}