From b3b285b174ee0eeef6d6268b2437d39d717020c7 Mon Sep 17 00:00:00 2001 From: Eric Lau Date: Fri, 20 Sep 2024 10:23:48 -0400 Subject: [PATCH] Add check to ensure `initialOwner` for `deployTransparentProxy` is not a ProxyAdmin (#76) --- CHANGELOG.md | 5 ++ docs/modules/api/pages/Options.adoc | 1 + src/Options.sol | 6 ++ src/Upgrades.sol | 12 +++ src/internal/Core.sol | 19 ++++- test/Upgrades.t.sol | 116 ++++++++++++++++++++++++---- test/contracts/HasOwner.sol | 40 ++++++++++ test/internal/Core.t.sol | 32 ++++++++ 8 files changed, 217 insertions(+), 14 deletions(-) create mode 100644 test/contracts/HasOwner.sol diff --git a/CHANGELOG.md b/CHANGELOG.md index a0defef..1e2ce82 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,10 @@ # Changelog +## 0.3.5 (2024-09-20) + +### Potentially breaking changes +- Adds a check to ensure `initialOwner` for `deployTransparentProxy` is not a ProxyAdmin contract. ([#76](https://github.com/OpenZeppelin/openzeppelin-foundry-upgrades/pull/76)) + ## 0.3.4 (2024-09-16) - Defender: Add `metadata` option. ([#75](https://github.com/OpenZeppelin/openzeppelin-foundry-upgrades/pull/75)) diff --git a/docs/modules/api/pages/Options.adoc b/docs/modules/api/pages/Options.adoc index eb71a59..7670e27 100644 --- a/docs/modules/api/pages/Options.adoc +++ b/docs/modules/api/pages/Options.adoc @@ -14,6 +14,7 @@ struct Options { bytes constructorData; string unsafeAllow; bool unsafeAllowRenames; + bool unsafeSkipProxyAdminCheck; bool unsafeSkipStorageCheck; bool unsafeSkipAllChecks; struct DefenderOptions defender; diff --git a/src/Options.sol b/src/Options.sol index 71225a6..2c87c0c 100644 --- a/src/Options.sol +++ b/src/Options.sol @@ -25,6 +25,12 @@ struct Options { * Configure storage layout check to allow variable renaming */ bool unsafeAllowRenames; + /* + * Skips checking the `initialOwner` parameter of `Upgrades.deployTransparentProxy`. + * When deploying a transparent proxy, the `initialOwner` must be the address of an EOA or a contract that can call functions on a ProxyAdmin. It must not be a ProxyAdmin contract itself. + * Use this if you encounter an error due to this check and are sure that the `initialOwner` is not a ProxyAdmin contract. + */ + bool unsafeSkipProxyAdminCheck; /* * Skips checking for storage layout compatibility errors. This is a dangerous option meant to be used as a last resort. */ diff --git a/src/Upgrades.sol b/src/Upgrades.sol index e354a37..90a4674 100644 --- a/src/Upgrades.sol +++ b/src/Upgrades.sol @@ -6,8 +6,10 @@ import {TransparentUpgradeableProxy} from "@openzeppelin/contracts/proxy/transpa import {UpgradeableBeacon} from "@openzeppelin/contracts/proxy/beacon/UpgradeableBeacon.sol"; import {BeaconProxy} from "@openzeppelin/contracts/proxy/beacon/BeaconProxy.sol"; +import {Vm} from "forge-std/Vm.sol"; import {Options} from "./Options.sol"; import {Core} from "./internal/Core.sol"; +import {Utils} from "./internal/Utils.sol"; /** * @dev Library for deploying and managing upgradeable contracts from Forge scripts or tests. @@ -60,6 +62,16 @@ library Upgrades { bytes memory initializerData, Options memory opts ) internal returns (address) { + if (!opts.unsafeSkipAllChecks && !opts.unsafeSkipProxyAdminCheck && Core.inferProxyAdmin(initialOwner)) { + revert( + string.concat( + "`initialOwner` must not be a ProxyAdmin contract. If the contract at address ", + Vm(Utils.CHEATCODE_ADDRESS).toString(initialOwner), + " is not a ProxyAdmin contract and you are sure that this contract is able to call functions on an actual ProxyAdmin, skip this check with the `unsafeSkipProxyAdminCheck` option." + ) + ); + } + address impl = deployImplementation(contractName, opts); return diff --git a/src/internal/Core.sol b/src/internal/Core.sol index 6d2b966..2163de3 100644 --- a/src/internal/Core.sol +++ b/src/internal/Core.sol @@ -307,7 +307,7 @@ library Core { * If the contract does not have the getter or the return data does not look like a string, this function returns an empty string. */ function getUpgradeInterfaceVersion(address addr) internal view returns (string memory) { - // Use staticcall to prevent forge from broadcasting it + // Use staticcall to prevent forge from broadcasting it, and to ensure no state changes (bool success, bytes memory returndata) = addr.staticcall( abi.encodeWithSignature("UPGRADE_INTERFACE_VERSION()") ); @@ -318,6 +318,23 @@ library Core { } } + /** + * @dev Infers whether the address might be a ProxyAdmin contract. + */ + function inferProxyAdmin(address addr) internal view returns (bool) { + return _hasOwner(addr); + } + + /** + * @dev Returns true if the address is a contract with an `owner()` function that is not state-changing and returns something that might be an address, + * otherwise returns false. + */ + function _hasOwner(address addr) private view returns (bool) { + // Use staticcall to prevent forge from broadcasting it, and to ensure no state changes + (bool success, bytes memory returndata) = addr.staticcall(abi.encodeWithSignature("owner()")); + return (success && returndata.length == 32); + } + function _validate(string memory contractName, Options memory opts, bool requireReference) private { if (opts.unsafeSkipAllChecks) { return; diff --git a/test/Upgrades.t.sol b/test/Upgrades.t.sol index d13eb68..c60f8e6 100644 --- a/test/Upgrades.t.sol +++ b/test/Upgrades.t.sol @@ -6,12 +6,17 @@ import {Test} from "forge-std/Test.sol"; import {Upgrades, Options} from "openzeppelin-foundry-upgrades/Upgrades.sol"; import {IBeacon} from "@openzeppelin/contracts/proxy/beacon/IBeacon.sol"; +import {ProxyAdmin} from "@openzeppelin/contracts/proxy/transparent/ProxyAdmin.sol"; +import {ITransparentUpgradeableProxy} from "@openzeppelin/contracts/proxy/transparent/TransparentUpgradeableProxy.sol"; import {Greeter} from "./contracts/Greeter.sol"; import {GreeterProxiable} from "./contracts/GreeterProxiable.sol"; import {GreeterV2} from "./contracts/GreeterV2.sol"; import {GreeterV2Proxiable} from "./contracts/GreeterV2Proxiable.sol"; import {WithConstructor, NoInitializer} from "./contracts/WithConstructor.sol"; +import {HasOwner} from "./contracts/HasOwner.sol"; + +import {strings} from "solidity-stringutils/src/strings.sol"; // Import additional contracts to include them for compilation import "./contracts/Validations.sol"; @@ -20,6 +25,8 @@ import "./contracts/Validations.sol"; * @dev Tests for the Upgrades library. */ contract UpgradesTest is Test { + using strings for *; + function testUUPS() public { address proxy = Upgrades.deployUUPSProxy( "GreeterProxiable.sol", @@ -139,8 +146,8 @@ contract UpgradesTest is Test { function testValidateImplementation() public { Options memory opts; - Validator v = new Validator(); - try v.validateImplementation("Validations.sol:Unsafe", opts) { + Invoker i = new Invoker(); + try i.validateImplementation("Validations.sol:Unsafe", opts) { fail(); } catch { // TODO: check error message @@ -150,8 +157,8 @@ contract UpgradesTest is Test { function testValidateLayout() public { Options memory opts; opts.referenceContract = "Validations.sol:LayoutV1"; - Validator v = new Validator(); - try v.validateUpgrade("Validations.sol:LayoutV2_Bad", opts) { + Invoker i = new Invoker(); + try i.validateUpgrade("Validations.sol:LayoutV2_Bad", opts) { fail(); } catch { // TODO: check error message @@ -160,8 +167,8 @@ contract UpgradesTest is Test { function testValidateLayoutUpgradesFrom() public { Options memory opts; - Validator v = new Validator(); - try v.validateUpgrade("Validations.sol:LayoutV2_UpgradesFrom_Bad", opts) { + Invoker i = new Invoker(); + try i.validateUpgrade("Validations.sol:LayoutV2_UpgradesFrom_Bad", opts) { fail(); } catch { // TODO: check error message @@ -171,8 +178,8 @@ contract UpgradesTest is Test { function testValidateNamespaced() public { Options memory opts; opts.referenceContract = "Validations.sol:NamespacedV1"; - Validator v = new Validator(); - try v.validateUpgrade("Validations.sol:NamespacedV2_Bad", opts) { + Invoker i = new Invoker(); + try i.validateUpgrade("Validations.sol:NamespacedV2_Bad", opts) { fail(); } catch { // TODO: check error message @@ -181,8 +188,8 @@ contract UpgradesTest is Test { function testValidateNamespacedUpgradesFrom() public { Options memory opts; - Validator v = new Validator(); - try v.validateUpgrade("Validations.sol:NamespacedV2_UpgradesFrom_Bad", opts) { + Invoker i = new Invoker(); + try i.validateUpgrade("Validations.sol:NamespacedV2_UpgradesFrom_Bad", opts) { fail(); } catch { // TODO: check error message @@ -202,9 +209,9 @@ contract UpgradesTest is Test { function testValidateNamespacedNoReference() public { Options memory opts; - Validator v = new Validator(); + Invoker i = new Invoker(); // validate upgrade without reference contract - an error is expected from upgrades-core CLI - try v.validateUpgrade("Validations.sol:NamespacedV2_Ok", opts) { + try i.validateUpgrade("Validations.sol:NamespacedV2_Ok", opts) { fail(); } catch { // TODO: check error message @@ -260,9 +267,92 @@ contract UpgradesTest is Test { address proxy = Upgrades.deployTransparentProxy("WithConstructor.sol:NoInitializer", msg.sender, "", opts); assertEq(WithConstructor(proxy).a(), 123); } + + function testProxyAdminCheck() public { + ProxyAdmin admin = new ProxyAdmin(msg.sender); + + Invoker i = new Invoker(); + try + i.deployTransparentProxy( + "Greeter.sol", + address(admin), // NOT SAFE + abi.encodeCall(Greeter.initialize, (msg.sender, "hello")) + ) + { + fail(); + } catch Error(string memory reason) { + strings.slice memory slice = reason.toSlice(); + assertTrue(slice.contains("`initialOwner` must not be a ProxyAdmin contract.".toSlice())); + assertTrue(slice.contains(vm.toString(address(admin)).toSlice())); + } + } + + function testProxyAdminCheck_emptyOpts() public { + HasOwner hasOwner = new HasOwner(msg.sender); + Options memory opts; + + Invoker i = new Invoker(); + try + i.deployTransparentProxy( + "Greeter.sol", + address(hasOwner), // false positive + abi.encodeCall(Greeter.initialize, (msg.sender, "hello")), + opts + ) + { + fail(); + } catch Error(string memory reason) { + strings.slice memory slice = reason.toSlice(); + assertTrue(slice.contains("`initialOwner` must not be a ProxyAdmin contract.".toSlice())); + assertTrue(slice.contains(vm.toString(address(hasOwner)).toSlice())); + } + } + + function testProxyAdminCheck_skip() public { + HasOwner hasOwner = new HasOwner(msg.sender); + Options memory opts; + opts.unsafeSkipProxyAdminCheck = true; + + Upgrades.deployTransparentProxy( + "Greeter.sol", + address(hasOwner), + abi.encodeCall(Greeter.initialize, (msg.sender, "hello")), + opts + ); + } + + function testProxyAdminCheck_skipAll() public { + HasOwner hasOwner = new HasOwner(msg.sender); + Options memory opts; + opts.unsafeSkipAllChecks = true; + + Upgrades.deployTransparentProxy( + "Greeter.sol", + address(hasOwner), + abi.encodeCall(Greeter.initialize, (msg.sender, "hello")), + opts + ); + } } -contract Validator { +contract Invoker { + function deployTransparentProxy( + string memory contractName, + address admin, + bytes memory data + ) public returns (address) { + return Upgrades.deployTransparentProxy(contractName, admin, data); + } + + function deployTransparentProxy( + string memory contractName, + address admin, + bytes memory data, + Options memory opts + ) public returns (address) { + return Upgrades.deployTransparentProxy(contractName, admin, data, opts); + } + function validateImplementation(string memory contractName, Options memory opts) public { Upgrades.validateImplementation(contractName, opts); } diff --git a/test/contracts/HasOwner.sol b/test/contracts/HasOwner.sol new file mode 100644 index 0000000..7c98a8a --- /dev/null +++ b/test/contracts/HasOwner.sol @@ -0,0 +1,40 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.20; + +// These contracts are for testing only, they are not safe for use in production. + +import {Ownable} from "@openzeppelin/contracts/access/Ownable.sol"; +import {ITransparentUpgradeableProxy} from "@openzeppelin/contracts/proxy/transparent/TransparentUpgradeableProxy.sol"; +import {ProxyAdmin} from "@openzeppelin/contracts/proxy/transparent/ProxyAdmin.sol"; + +// Basic but pointless contract that has its own owner and can call ProxyAdmin functions +contract HasOwner is Ownable { + constructor(address initialOwner) Ownable(initialOwner) {} + + function upgradeAndCall( + ProxyAdmin admin, + ITransparentUpgradeableProxy proxy, + address implementation, + bytes memory data + ) public payable virtual onlyOwner { + admin.upgradeAndCall{value: msg.value}(proxy, implementation, data); + } +} + +contract NoGetter {} + +contract StringOwner { + string public owner; + + constructor(string memory initialOwner) { + owner = initialOwner; + } +} + +contract StateChanging { + bool public triggered; + + function owner() public { + triggered = true; + } +} diff --git a/test/internal/Core.t.sol b/test/internal/Core.t.sol index cf50e33..3204907 100644 --- a/test/internal/Core.t.sol +++ b/test/internal/Core.t.sol @@ -6,6 +6,9 @@ import {Test} from "forge-std/Test.sol"; import {Core} from "openzeppelin-foundry-upgrades/internal/Core.sol"; import {UpgradeInterfaceVersionString, UpgradeInterfaceVersionNoGetter, UpgradeInterfaceVersionEmpty, UpgradeInterfaceVersionInteger, UpgradeInterfaceVersionVoid} from "../contracts/UpgradeInterfaceVersions.sol"; +import {HasOwner, NoGetter, StringOwner, StateChanging} from "../contracts/HasOwner.sol"; + +import {ProxyAdmin} from "@openzeppelin/contracts/proxy/transparent/ProxyAdmin.sol"; /** * @dev Tests the Core internal library. @@ -35,4 +38,33 @@ contract CoreTest is Test { UpgradeInterfaceVersionVoid u = new UpgradeInterfaceVersionVoid(); assertEq(Core.getUpgradeInterfaceVersion(address(u)), ""); } + + function testInferProxyAdmin() public { + ProxyAdmin admin = new ProxyAdmin(msg.sender); + assertEq(Core.inferProxyAdmin(address(admin)), true); + } + + function testInferProxyAdmin_hasOwner() public { + HasOwner c = new HasOwner(msg.sender); + assertEq(Core.inferProxyAdmin(address(c)), true); // not actually a proxy admin, but has an owner + } + + function testInferProxyAdmin_noOwner() public { + NoGetter c = new NoGetter(); + assertEq(Core.inferProxyAdmin(address(c)), false); + } + + function testInferProxyAdmin_stringOwner() public { + StringOwner c = new StringOwner("foo"); + assertEq(Core.inferProxyAdmin(address(c)), false); + } + + function testInferProxyAdmin_notContract() public view { + assertEq(Core.inferProxyAdmin(address(0)), false); + } + + function testInferProxyAdmin_stateChanging() public { + StateChanging c = new StateChanging(); + assertEq(Core.inferProxyAdmin(address(c)), false); + } }