From 121f327d89a8359e8266420d72588653f7f88c48 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Muhammed=20Tanr=C4=B1kulu?= Date: Fri, 8 Nov 2024 14:13:43 +0100 Subject: [PATCH] change the owner, add creator immutable state, update test --- script/VerifiableFactory.s.sol | 9 +- src/TransparentVerifiableProxy.sol | 14 +- src/VerifiableFactory.sol | 79 +++++----- src/mock/MockRegistry.sol | 53 +++++++ src/mock/MockRegistryV2.sol | 10 ++ test/VerifiableFactory.t.sol | 235 ++++++++++++++++++++--------- 6 files changed, 281 insertions(+), 119 deletions(-) create mode 100644 src/mock/MockRegistry.sol create mode 100644 src/mock/MockRegistryV2.sol diff --git a/script/VerifiableFactory.s.sol b/script/VerifiableFactory.s.sol index 6a61e0f..0fc0768 100644 --- a/script/VerifiableFactory.s.sol +++ b/script/VerifiableFactory.s.sol @@ -11,14 +11,7 @@ contract CounterScript is Script { function run() public { vm.startBroadcast(); - - // TODO complete it - VerifiableFactory.Verifiers[] memory verifiers = new VerifiableFactory.Verifiers[](3); - verifiers[0] = VerifiableFactory.Verifiers({networkId: 1, verifier: address(0)}); - verifiers[1] = VerifiableFactory.Verifiers({networkId: 42, verifier: address(0)}); - verifiers[2] = VerifiableFactory.Verifiers({networkId: 137, verifier: address(0)}); - - factory = new VerifiableFactory(verifiers); + factory = new VerifiableFactory(); vm.stopBroadcast(); } diff --git a/src/TransparentVerifiableProxy.sol b/src/TransparentVerifiableProxy.sol index 193c3be..76c9cd7 100644 --- a/src/TransparentVerifiableProxy.sol +++ b/src/TransparentVerifiableProxy.sol @@ -18,8 +18,12 @@ interface ITransparentVerifiableProxy { } contract TransparentVerifiableProxy is Proxy, Initializable { - uint256 public salt; // Salt, being used creating the proxy - address public owner; // The owner of the proxy contract + // immutable variable (in bytecode) + address immutable public creator; + + // storage variables (in storage slots) + uint256 public salt; // Salt, being used creating the proxy (slot 0) + address public owner; // The owner of the proxy contract (slot 1) // ### EVENTS error ProxyDeniedOwnerAccess(); @@ -30,6 +34,10 @@ contract TransparentVerifiableProxy is Proxy, Initializable { // _; // } + constructor(address _creator) { + creator = _creator; + } + /** * @dev Initializes the verifiable proxy with an initial implementation specified by `implementation`. * @@ -78,7 +86,7 @@ contract TransparentVerifiableProxy is Proxy, Initializable { * @dev If caller is the owner, process the call internally, otherwise transparently fallback to the proxy behavior. */ function _fallback() internal virtual override { - if (msg.sender == owner) { + if (msg.sender == creator) { if ( msg.sig != ITransparentVerifiableProxy.upgradeToAndCall.selector ) { diff --git a/src/VerifiableFactory.sol b/src/VerifiableFactory.sol index df9a416..c140d88 100644 --- a/src/VerifiableFactory.sol +++ b/src/VerifiableFactory.sol @@ -1,6 +1,7 @@ // SPDX-License-Identifier: MIT 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"; @@ -8,10 +9,11 @@ interface IProxy { function salt() external view returns (uint256); function owner() external view returns (address); + + function creator() external view returns (address); } contract VerifiableFactory { - event ProxyDeployed( address indexed sender, address indexed proxyAddress, @@ -24,12 +26,12 @@ contract VerifiableFactory { /** * @dev Deploys a new `TransparentVerifiableProxy` contract at a deterministic address. * - * This function deploys a proxy contract using the CREATE2 opcode, ensuring a predictable - * address based on the sender's address and a provided salt. The deployed proxy is + * This function deploys a proxy contract using the CREATE2 opcode, ensuring a predictable + * address based on the sender's address and a provided salt. The deployed proxy is * controlled by the factory and is initialized to use a specific implementation. * * - A unique address for the proxy is generated using the caller's address and the salt. - * - After deployment, the proxy's `initialize` function is called to configure it with the given salt, + * - After deployment, the proxy's `initialize` function is called to configure it with the given salt, * the factory address, and the provided implementation address. * - The proxy is fully managed by the factory, which controls upgrades and other administrative methods. * - The event `ProxyDeployed` is emitted, logging details of the deployment including the sender, proxy address, salt, and implementation. @@ -42,15 +44,17 @@ contract VerifiableFactory { 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)); require(isContract(address(proxy)), "Proxy deployment failed"); - proxy.initialize(salt, address(this), implementation, ""); + proxy.initialize(salt, msg.sender, implementation, ""); emit ProxyDeployed(msg.sender, address(proxy), salt, implementation); return address(proxy); @@ -62,7 +66,8 @@ contract VerifiableFactory { address newImplementation, bytes memory data ) external { - require(verifyContract(proxyAddress), "Only the owner can upgrade"); + address owner = IProxy(proxyAddress).owner(); + require(owner == msg.sender, "Only the owner can upgrade"); // Upgrade the proxy to point to the new implementation ITransparentVerifiableProxy(payable(proxyAddress)).upgradeToAndCall( @@ -74,44 +79,44 @@ contract VerifiableFactory { /** * @dev Initiates verification of a proxy contract. * - * This function attempts to validate a proxy contract by retrieving its salt - * and reconstructing the address to ensure it was correctly deployed by the + * This function attempts to validate a proxy contract by retrieving its salt + * and reconstructing the address to ensure it was correctly deployed by the * current factory. * * @param proxy The address of the proxy contract being verified. * @return A boolean indicating whether the verification succeeded. */ function verifyContract(address proxy) public view returns (bool) { - // directly fetch storage - try IProxy(proxy).salt() returns (uint256 salt) { - address owner = IProxy(proxy).owner(); - - require( - address(this) == owner, - "Proxy owner does not match with factory address" - ); - - // reconstruct the address using CREATE2 and the original salt - bytes32 outerSalt = keccak256(abi.encode(msg.sender, salt)); - - // bytes memory bytecode = abi.encodePacked( - // type(TransparentVerifiableProxy).creationCode, - // abi.encode(salt, address(this)) - // ); - - // Compute the expected proxy address using the outerSalt - address expectedProxyAddress = Create2.computeAddress( - outerSalt, - keccak256(type(TransparentVerifiableProxy).creationCode) - ); - - // Verify if the computed address matches the proxy address - require(expectedProxyAddress == proxy, "Proxy address mismatch"); - - return true; - } catch { + if (!isContract(proxy)) { 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; + } + + // reconstruct the address using CREATE2 and verify it matches + bytes32 outerSalt = keccak256(abi.encode(msg.sender, salt)); + + // get creation bytecode with constructor arguments + bytes memory bytecode = abi.encodePacked( + type(TransparentVerifiableProxy).creationCode, + abi.encode(address(this)) + ); + + address expectedProxyAddress = Create2.computeAddress( + outerSalt, + keccak256(bytecode), + address(this) + ); + + return expectedProxyAddress == proxy; + } catch {} + } catch {} + + return false; } function isContract(address account) internal view returns (bool) { diff --git a/src/mock/MockRegistry.sol b/src/mock/MockRegistry.sol new file mode 100644 index 0000000..805a4f8 --- /dev/null +++ b/src/mock/MockRegistry.sol @@ -0,0 +1,53 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.20; + +/** + * @title MockRegistry + * @dev Simulates a registry implementation for testing + */ +contract MockRegistry { + mapping(address => bool) public registeredAddresses; + address public admin; + uint256 public constant version = 1; + + // ### Events + event AddressRegistered(address indexed account); + event AddressUnregistered(address indexed account); + event AdminChanged(address indexed oldAdmin, address indexed newAdmin); + + constructor() { + admin = msg.sender; + } + + function register(address account) external { + require(!registeredAddresses[account], "Address already registered"); + registeredAddresses[account] = true; + emit AddressRegistered(account); + } + + function unregister(address account) external { + require(registeredAddresses[account], "Address not registered"); + registeredAddresses[account] = false; + emit AddressUnregistered(account); + } + + function isRegistered(address account) external view returns (bool) { + return registeredAddresses[account]; + } + + function changeAdmin(address newAdmin) external { + require(msg.sender == admin, "Only admin can change admin"); + require(newAdmin != address(0), "New admin cannot be zero address"); + emit AdminChanged(admin, newAdmin); + admin = newAdmin; + } + + function getRegistryVersion() public pure virtual returns (uint256) { + return version; + } + + function initialize(address _admin) external { + require(admin == address(0), "Already initialized"); + admin = _admin; + } +} diff --git a/src/mock/MockRegistryV2.sol b/src/mock/MockRegistryV2.sol new file mode 100644 index 0000000..2631340 --- /dev/null +++ b/src/mock/MockRegistryV2.sol @@ -0,0 +1,10 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.20; + +import {MockRegistry} from './MockRegistry.sol'; + +contract MockRegistryV2 is MockRegistry { + function getRegistryVersion() public pure override returns (uint256) { + return 2; + } +} diff --git a/test/VerifiableFactory.t.sol b/test/VerifiableFactory.t.sol index a660006..16dc699 100644 --- a/test/VerifiableFactory.t.sol +++ b/test/VerifiableFactory.t.sol @@ -1,114 +1,207 @@ // SPDX-License-Identifier: MIT pragma solidity ^0.8.20; -import {Test} from "forge-std/Test.sol"; +import {Test, console2} from "forge-std/Test.sol"; +import {Create2} from "@openzeppelin/contracts/utils/Create2.sol"; + import {VerifiableFactory} from "../src/VerifiableFactory.sol"; -import {ProxyAdmin} from "@openzeppelin/contracts/proxy/transparent/ProxyAdmin.sol"; +import {TransparentVerifiableProxy} from "../src/TransparentVerifiableProxy.sol"; +import {MockRegistry} from "../src/mock/MockRegistry.sol"; +import {MockRegistryV2} from "../src/mock/MockRegistryV2.sol"; contract VerifiableFactoryTest is Test { + // contract instances VerifiableFactory public factory; - ProxyAdmin public proxyAdmin; - address public deployer; + MockRegistry public implementation; + MockRegistryV2 public implementationV2; + + // test addresses + address public owner; + address public user; + address public maliciousUser; + + // ### Events + event ProxyDeployed( + address indexed sender, + address indexed proxyAddress, + uint256 salt, + address implementation + ); function setUp() public { - deployer = address(this); + owner = makeAddr("owner"); + user = makeAddr("user"); + maliciousUser = makeAddr("malicious"); + // deploy contracts factory = new VerifiableFactory(); + implementation = new MockRegistry(); + implementationV2 = new MockRegistryV2(); + + vm.label(address(factory), "Factory"); + vm.label(address(implementation), "Implementation"); + vm.label(address(implementationV2), "ImplementationV2"); } - function testFactoryDeployment() public view { - assert(address(factory) != address(0)); + function test_FactoryInitialState() public view { + assertTrue(address(factory) != address(0), "Factory deployment failed"); + assertTrue( + address(implementation) != address(0), + "Implementation deployment failed" + ); } - function testDeployProxy() public { + function test_DeployProxy() public { uint256 salt = 1; - address proxyAddress = factory.deployProxy(address(0), salt); - // Check if proxy was deployed - uint256 codeSize; - assembly { - codeSize := extcodesize(proxyAddress) - } - assert(codeSize > 0); - emit log_named_address("Deployed Proxy Address", proxyAddress); + // test event emit + vm.expectEmit(true, true, true, true); + emit ProxyDeployed( + owner, + computeExpectedAddress(salt), + salt, + address(implementation) + ); - // Check if proxy was initialized properly with salt and proxyAdmin - (bool success, bytes memory result) = proxyAddress.call( - abi.encodeWithSignature("salt()") + vm.startPrank(owner); + address proxyAddress = factory.deployProxy( + address(implementation), + salt ); - assert(success); - uint256 deployedNonce = abi.decode(result, (uint256)); - assertEq(deployedNonce, salt); - } - function testUpdateRegistry() public { - uint256 salt = 1; - address proxyAddress = factory.deployProxy(address(0), salt); - address newRegistryAddress = address(0xBEEF); - - // Update the registry of the deployed proxy - (bool updateSuccess, ) = proxyAddress.call( - abi.encodeWithSignature( - "updateRegistry(address)", - newRegistryAddress - ) + vm.stopPrank(); + + // verify proxy deployment + assertTrue( + proxyAddress != address(0), + "Proxy address should not be zero" ); - assert(updateSuccess); + assertTrue(isContract(proxyAddress), "Proxy should be a contract"); - // Retrieve the registry from the proxy to confirm the update - (bool retrieveSuccess, bytes memory result) = proxyAddress.call( - abi.encodeWithSignature("registry()") + // verify proxy state + TransparentVerifiableProxy proxy = TransparentVerifiableProxy( + payable(proxyAddress) ); - assert(retrieveSuccess); - address updatedRegistryAddress = abi.decode(result, (address)); - assertEq(updatedRegistryAddress, newRegistryAddress); + assertEq(proxy.salt(), salt, "Proxy salt mismatch"); + assertEq(proxy.owner(), owner, "Proxy owner mismatch"); + assertEq(proxy.creator(), address(factory), "Proxy creator mismatch"); } - function testVerifyContract() public { + function test_DeployProxyWithSameSalt() public { uint256 salt = 1; - address proxyAddress = factory.deployProxy(address(0), salt); // tbd - factory.verifyContract(proxyAddress); + vm.startPrank(owner); - // If no revert, verification passed - assertTrue(true); + // deploy first proxy + factory.deployProxy(address(implementation), salt); + + // try to deploy another proxy with same salt - should fail + vm.expectRevert(); + factory.deployProxy(address(implementation), salt); + + vm.stopPrank(); } - function testUpgradeImplementation() public { + function test_UpgradeImplementation() public { uint256 salt = 1; - address proxyAddress = factory.deployProxy(address(0), salt); // tbd - // Upgrade the proxy to point to the new implementation - factory.upgradeImplementation(proxyAddress, address(0), ""); //tbd + // deploy proxy as owner + vm.prank(owner); + address proxyAddress = factory.deployProxy( + address(implementation), + salt + ); + + // try to upgrade as non-owner (should fail) + vm.prank(maliciousUser); + vm.expectRevert("Only the owner can upgrade"); + factory.upgradeImplementation( + proxyAddress, + address(implementationV2), + "" // add upgrade data if we need + ); + + // upgrade as owner (should pass) + vm.prank(owner); + factory.upgradeImplementation( + proxyAddress, + address(implementationV2), + "" // add upgrade data if we need + ); + + // verify new implementation + MockRegistryV2 upgradedProxy = MockRegistryV2(proxyAddress); + assertEq( + upgradedProxy.getRegistryVersion(), + 2, + "Implementation upgrade failed" + ); + } + + function test_VerifyContract() public { + uint256 salt = 1; - // Ensure the proxy's implementation was upgraded by calling a function in the new implementation - (bool success, bytes memory result) = proxyAddress.call( - abi.encodeWithSignature("getRegistryVersion()") + // deploy proxy + vm.prank(owner); + address proxyAddress = factory.deployProxy( + address(implementation), + salt ); - assert(success); - uint256 version = abi.decode(result, (uint256)); - assertEq(version, 1); // Assuming MockRegistry's version is 1 + + vm.prank(owner); + // verify the contract + bool isVerified = factory.verifyContract(proxyAddress); + assertTrue(isVerified, "Contract verification failed"); + + vm.prank(owner); + // try to verify non-existent contract + address randomAddress = makeAddr("random"); + bool shouldBeFalse = factory.verifyContract(randomAddress); + assertFalse(shouldBeFalse, "Non-existent contract should not verify"); } - function testUpgradeAndCall() public { + function test_ProxyInitialization() public { uint256 salt = 1; - address proxyAddress = factory.deployProxy(address(0), salt); - // ABI encode data for initializing the new implementation with a specific salt and registry - bytes memory data = abi.encodeWithSignature( - "initialize(uint256,address)", - 2, - address(proxyAdmin) + vm.prank(owner); + address proxyAddress = factory.deployProxy( + address(implementation), + salt ); - // Upgrade and initialize the new implementation - factory.upgradeImplementation(proxyAddress, address(0), ""); // tbd + // test proxy state + TransparentVerifiableProxy proxy = TransparentVerifiableProxy( + payable(proxyAddress) + ); - // Check if the new implementation was initialized correctly - (bool success, bytes memory result) = proxyAddress.call( - abi.encodeWithSignature("salt()") + assertEq(proxy.salt(), salt, "Wrong salt"); + assertEq(proxy.owner(), owner, "Wrong owner"); + assertEq(proxy.creator(), address(factory), "Wrong creator"); + } + + // ### Helpers + function isContract(address account) internal view returns (bool) { + uint256 size; + assembly { + size := extcodesize(account) + } + return size > 0; + } + + function computeExpectedAddress( + uint256 salt + ) internal view returns (address) { + bytes32 outerSalt = keccak256(abi.encode(owner, salt)); + + bytes memory bytecode = abi.encodePacked( + type(TransparentVerifiableProxy).creationCode, + abi.encode(address(factory)) ); - assert(success); - uint256 newNonce = abi.decode(result, (uint256)); - assertEq(newNonce, 2); + + return + Create2.computeAddress( + outerSalt, + keccak256(bytecode), + address(factory) + ); } }