diff --git a/.gitmodules b/.gitmodules index 690924b..9296efd 100644 --- a/.gitmodules +++ b/.gitmodules @@ -4,3 +4,6 @@ [submodule "lib/openzeppelin-contracts"] path = lib/openzeppelin-contracts url = https://github.com/OpenZeppelin/openzeppelin-contracts +[submodule "lib/openzeppelin-contracts-upgradeable"] + path = lib/openzeppelin-contracts-upgradeable + url = https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable diff --git a/lib/openzeppelin-contracts b/lib/openzeppelin-contracts index a277d47..ccb39d2 160000 --- a/lib/openzeppelin-contracts +++ b/lib/openzeppelin-contracts @@ -1 +1 @@ -Subproject commit a277d472d657dbbcd88a3de5cae0c806130e2df2 +Subproject commit ccb39d27656fc36eea331e3403c52b057885634d diff --git a/lib/openzeppelin-contracts-upgradeable b/lib/openzeppelin-contracts-upgradeable new file mode 160000 index 0000000..3d5fa5c --- /dev/null +++ b/lib/openzeppelin-contracts-upgradeable @@ -0,0 +1 @@ +Subproject commit 3d5fa5c24c411112bab47bec25cfa9ad0af0e6e8 diff --git a/src/ITransparentVerifiableProxy.sol b/src/ITransparentVerifiableProxy.sol new file mode 100644 index 0000000..f48345a --- /dev/null +++ b/src/ITransparentVerifiableProxy.sol @@ -0,0 +1,13 @@ +// 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); + + /// @dev See {UUPSUpgradeable-upgradeToAndCall} + function upgradeToAndCall(address newImplementation, bytes calldata data) external payable; +} diff --git a/src/TransparentVerifiableProxy.sol b/src/TransparentVerifiableProxy.sol index 787457b..7793970 100644 --- a/src/TransparentVerifiableProxy.sol +++ b/src/TransparentVerifiableProxy.sol @@ -10,11 +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"; - -interface 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; @@ -31,12 +27,6 @@ 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"); - // _; - // } - constructor(address _creator) { creator = _creator; } @@ -56,6 +46,7 @@ contract TransparentVerifiableProxy is Proxy, Initializable { payable initializer { + require(msg.sender == creator, "Unauthorized initialization"); require(implementation != address(0), "New implementation cannot be the zero address"); bytes32 baseSlot = _VERIFICATION_SLOT.erc7201Slot(); @@ -85,7 +76,7 @@ contract TransparentVerifiableProxy is Proxy, Initializable { } /** - * @dev If caller is the owner, process the call internally, otherwise transparently fallback to the proxy behavior. + * @dev If caller is the cretor, process the call internally, otherwise transparently fallback to the proxy behavior. */ function _fallback() internal virtual override { if (msg.sender == creator) { diff --git a/src/VerifiableFactory.sol b/src/VerifiableFactory.sol index d21f805..7f05e0a 100644 --- a/src/VerifiableFactory.sol +++ b/src/VerifiableFactory.sol @@ -3,15 +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"; - -interface IProxy { - function salt() external view returns (uint256); - - function owner() external view returns (address); - - function creator() external view returns (address); -} +import {TransparentVerifiableProxy} from "./TransparentVerifiableProxy.sol"; +import {ITransparentVerifiableProxy} from "./ITransparentVerifiableProxy.sol"; contract VerifiableFactory { event ProxyDeployed(address indexed sender, address indexed proxyAddress, uint256 salt, address implementation); @@ -36,8 +29,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)); @@ -52,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 @@ -73,27 +64,26 @@ contract VerifiableFactory { 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; - } + try ITransparentVerifiableProxy(proxy).salt() returns (uint256 salt) { + try ITransparentVerifiableProxy(proxy).owner() returns (address owner) { + return _verifyContract(proxy, owner, salt); + } catch {} + } catch {} - // reconstruct the address using CREATE2 and verify it matches - bytes32 outerSalt = keccak256(abi.encode(msg.sender, salt)); + return false; + } - // get creation bytecode with constructor arguments - bytes memory bytecode = - abi.encodePacked(type(TransparentVerifiableProxy).creationCode, abi.encode(address(this))); + 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)); - 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/TransparentVerifiableProxy.t.sol b/test/TransparentVerifiableProxy.t.sol new file mode 100644 index 0000000..92cfd99 --- /dev/null +++ b/test/TransparentVerifiableProxy.t.sol @@ -0,0 +1,75 @@ +// 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 + vm.prank(creator); + 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 + vm.prank(creator); + 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 + vm.prank(creator); + 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"); + } +} 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;