diff --git a/.changeset/empty-taxis-kiss.md b/.changeset/empty-taxis-kiss.md new file mode 100644 index 000000000..fbb2e8c4e --- /dev/null +++ b/.changeset/empty-taxis-kiss.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': major +--- + +`UUPSUpgradeable`, `ProxyAdmin`: Removed `upgradeTo` and `upgrade` functions, and made `upgradeToAndCall` and `upgradeAndCall` ignore the data argument if it is empty. It is no longer possible to invoke the receive function along with an upgrade. diff --git a/.changeset/tender-shirts-turn.md b/.changeset/tender-shirts-turn.md new file mode 100644 index 000000000..9c98e6e2b --- /dev/null +++ b/.changeset/tender-shirts-turn.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': major +--- + +`BeaconProxy`: Reject value in initialization unless a payable function is explicitly invoked. diff --git a/contracts/mocks/proxy/ClashingImplementation.sol b/contracts/mocks/proxy/ClashingImplementation.sol index 89904b91f..4cb5d2326 100644 --- a/contracts/mocks/proxy/ClashingImplementation.sol +++ b/contracts/mocks/proxy/ClashingImplementation.sol @@ -9,7 +9,7 @@ pragma solidity ^0.8.19; contract ClashingImplementation { event ClashingImplementationCall(); - function upgradeTo(address) external payable { + function upgradeToAndCall(address, bytes calldata) external payable { emit ClashingImplementationCall(); } diff --git a/contracts/mocks/proxy/UUPSUpgradeableMock.sol b/contracts/mocks/proxy/UUPSUpgradeableMock.sol index 4333e63e8..769c89946 100644 --- a/contracts/mocks/proxy/UUPSUpgradeableMock.sol +++ b/contracts/mocks/proxy/UUPSUpgradeableMock.sol @@ -23,12 +23,8 @@ contract UUPSUpgradeableMock is NonUpgradeableMock, UUPSUpgradeable { } contract UUPSUpgradeableUnsafeMock is UUPSUpgradeableMock { - function upgradeTo(address newImplementation) public override { - ERC1967Utils.upgradeToAndCall(newImplementation, bytes(""), false); - } - function upgradeToAndCall(address newImplementation, bytes memory data) public payable override { - ERC1967Utils.upgradeToAndCall(newImplementation, data, false); + ERC1967Utils.upgradeToAndCall(newImplementation, data); } } diff --git a/contracts/proxy/ERC1967/ERC1967Proxy.sol b/contracts/proxy/ERC1967/ERC1967Proxy.sol index e7c90c706..786d20fcd 100644 --- a/contracts/proxy/ERC1967/ERC1967Proxy.sol +++ b/contracts/proxy/ERC1967/ERC1967Proxy.sol @@ -20,7 +20,7 @@ contract ERC1967Proxy is Proxy { * function call, and allows initializing the storage of the proxy like a Solidity constructor. */ constructor(address _logic, bytes memory _data) payable { - ERC1967Utils.upgradeToAndCall(_logic, _data, false); + ERC1967Utils.upgradeToAndCall(_logic, _data); } /** diff --git a/contracts/proxy/ERC1967/ERC1967Utils.sol b/contracts/proxy/ERC1967/ERC1967Utils.sol index 3ad93ff3c..c9fa06c70 100644 --- a/contracts/proxy/ERC1967/ERC1967Utils.sol +++ b/contracts/proxy/ERC1967/ERC1967Utils.sol @@ -52,6 +52,11 @@ library ERC1967Utils { */ error ERC1967InvalidBeacon(address beacon); + /** + * @dev An upgrade function sees `msg.value > 0` that may be lost. + */ + error ERC1967NonPayable(); + /** * @dev Returns the current implementation address. */ @@ -70,24 +75,18 @@ library ERC1967Utils { } /** - * @dev Perform implementation upgrade + * @dev Perform implementation upgrade with additional setup call. * * Emits an {IERC1967-Upgraded} event. */ - function upgradeTo(address newImplementation) internal { + function upgradeToAndCall(address newImplementation, bytes memory data) internal { _setImplementation(newImplementation); emit Upgraded(newImplementation); - } - /** - * @dev Perform implementation upgrade with additional setup call. - * - * Emits an {IERC1967-Upgraded} event. - */ - function upgradeToAndCall(address newImplementation, bytes memory data, bool forceCall) internal { - upgradeTo(newImplementation); - if (data.length > 0 || forceCall) { + if (data.length > 0) { Address.functionDelegateCall(newImplementation, data); + } else { + _checkNonPayable(); } } @@ -161,7 +160,7 @@ library ERC1967Utils { } /** - * @dev Change the beacon and trigger a setup call. + * @dev Change the beacon and trigger a setup call if data is nonempty. * * Emits an {IERC1967-BeaconUpgraded} event. * @@ -169,11 +168,20 @@ library ERC1967Utils { * it uses an immutable beacon without looking at the value of the ERC-1967 beacon slot for * efficiency. */ - function upgradeBeaconToAndCall(address newBeacon, bytes memory data, bool forceCall) internal { + function upgradeBeaconToAndCall(address newBeacon, bytes memory data) internal { _setBeacon(newBeacon); emit BeaconUpgraded(newBeacon); - if (data.length > 0 || forceCall) { + + if (data.length > 0) { Address.functionDelegateCall(IBeacon(newBeacon).implementation(), data); + } else { + _checkNonPayable(); + } + } + + function _checkNonPayable() private { + if (msg.value > 0) { + revert ERC1967NonPayable(); } } } diff --git a/contracts/proxy/beacon/BeaconProxy.sol b/contracts/proxy/beacon/BeaconProxy.sol index 14a8d1b66..6ad7e9d94 100644 --- a/contracts/proxy/beacon/BeaconProxy.sol +++ b/contracts/proxy/beacon/BeaconProxy.sol @@ -36,7 +36,7 @@ contract BeaconProxy is Proxy { * - `beacon` must be a contract with the interface {IBeacon}. */ constructor(address beacon, bytes memory data) payable { - ERC1967Utils.upgradeBeaconToAndCall(beacon, data, false); + ERC1967Utils.upgradeBeaconToAndCall(beacon, data); _beacon = beacon; } diff --git a/contracts/proxy/transparent/ProxyAdmin.sol b/contracts/proxy/transparent/ProxyAdmin.sol index 344c942f2..9a3aa90d1 100644 --- a/contracts/proxy/transparent/ProxyAdmin.sol +++ b/contracts/proxy/transparent/ProxyAdmin.sol @@ -3,7 +3,7 @@ pragma solidity ^0.8.19; -import {ITransparentUpgradeableProxy, TransparentUpgradeableProxy} from "./TransparentUpgradeableProxy.sol"; +import {ITransparentUpgradeableProxy} from "./TransparentUpgradeableProxy.sol"; import {Ownable} from "../../access/Ownable.sol"; /** @@ -12,24 +12,24 @@ import {Ownable} from "../../access/Ownable.sol"; */ contract ProxyAdmin is Ownable { /** - * @dev Sets the initial owner who can perform upgrades. + * @dev The version of the upgrade interface of the contract. If this getter is missing, both `upgrade(address)` + * and `upgradeAndCall(address,bytes)` are present, and `upgradeTo` must be used if no function should be called, + * while `upgradeAndCall` will invoke the `receive` function if the second argument is the empty byte string. + * If the getter returns `"5.0.0"`, only `upgradeAndCall(address,bytes)` is present, and the second argument must + * be the empty byte string if no function should be called, being impossible to invoke the `receive` function + * during an upgrade. */ - constructor(address initialOwner) Ownable(initialOwner) {} + // solhint-disable-next-line private-vars-leading-underscore + string internal constant UPGRADE_INTERFACE_VERSION = "5.0.0"; /** - * @dev Upgrades `proxy` to `implementation`. See {TransparentUpgradeableProxy-upgradeTo}. - * - * Requirements: - * - * - This contract must be the admin of `proxy`. + * @dev Sets the initial owner who can perform upgrades. */ - function upgrade(ITransparentUpgradeableProxy proxy, address implementation) public virtual onlyOwner { - proxy.upgradeTo(implementation); - } + constructor(address initialOwner) Ownable(initialOwner) {} /** - * @dev Upgrades `proxy` to `implementation` and calls a function on the new implementation. See - * {TransparentUpgradeableProxy-upgradeToAndCall}. + * @dev Upgrades `proxy` to `implementation` and calls a function on the new implementation. + * See {TransparentUpgradeableProxy-_dispatchUpgradeToAndCall}. * * Requirements: * diff --git a/contracts/proxy/transparent/TransparentUpgradeableProxy.sol b/contracts/proxy/transparent/TransparentUpgradeableProxy.sol index 3d6e11f38..419dbc13b 100644 --- a/contracts/proxy/transparent/TransparentUpgradeableProxy.sol +++ b/contracts/proxy/transparent/TransparentUpgradeableProxy.sol @@ -6,21 +6,20 @@ pragma solidity ^0.8.19; import {ERC1967Utils} from "../ERC1967/ERC1967Utils.sol"; import {ERC1967Proxy} from "../ERC1967/ERC1967Proxy.sol"; import {IERC1967} from "../../interfaces/IERC1967.sol"; +import {ProxyAdmin} from "./ProxyAdmin.sol"; /** * @dev Interface for {TransparentUpgradeableProxy}. In order to implement transparency, {TransparentUpgradeableProxy} - * does not implement this interface directly, and some of its functions are implemented by an internal dispatch + * does not implement this interface directly, and its upgradeability mechanism is implemented by an internal dispatch * mechanism. The compiler is unaware that these functions are implemented by {TransparentUpgradeableProxy} and will not * include them in the ABI so this interface must be used to interact with it. */ interface ITransparentUpgradeableProxy is IERC1967 { - function upgradeTo(address) external; - - function upgradeToAndCall(address, bytes memory) external payable; + function upgradeToAndCall(address, bytes calldata) external payable; } /** - * @dev This contract implements a proxy that is upgradeable by an immutable admin. + * @dev This contract implements a proxy that is upgradeable through an associated {ProxyAdmin} instance. * * To avoid https://medium.com/nomic-labs-blog/malicious-backdoors-in-ethereum-proxies-62629adf3357[proxy selector * clashing], which can potentially be used in an attack, this contract uses the @@ -28,23 +27,22 @@ interface ITransparentUpgradeableProxy is IERC1967 { * things that go hand in hand: * * 1. If any account other than the admin calls the proxy, the call will be forwarded to the implementation, even if - * that call matches one of the admin functions exposed by the proxy itself. - * 2. If the admin calls the proxy, it can access the admin functions, but its calls will never be forwarded to the + * that call matches the {ITransparentUpgradeableProxy-upgradeToAndCall} function exposed by the proxy itself. + * 2. If the admin calls the proxy, it can call the `upgradeToAndCall` function but any other call won't be forwarded to the * implementation. If the admin tries to call a function on the implementation it will fail with an error indicating the * proxy admin cannot fallback to the target implementation. * * These properties mean that the admin account can only be used for upgrading the proxy, so it's best if it's a dedicated * account that is not used for anything else. This will avoid headaches due to sudden errors when trying to call a function - * from the proxy implementation. - * - * Our recommendation is for the dedicated account to be an instance of the {ProxyAdmin} contract. If set up this way, - * you should think of the `ProxyAdmin` instance as the real administrative interface of your proxy, which extends from the - * {Ownable} contract to allow for changing the proxy's admin owner. + * from the proxy implementation. For this reason, the proxy deploys an instance of {ProxyAdmin} and allows upgrades + * only if they come through it. + * You should think of the `ProxyAdmin` instance as the administrative interface of the proxy, including the ability to + * change who can trigger upgrades by transferring ownership. * * NOTE: The real interface of this proxy is that defined in `ITransparentUpgradeableProxy`. This contract does not - * inherit from that interface, and instead the admin functions are implicitly implemented using a custom dispatch - * mechanism in `_fallback`. Consequently, the compiler will not produce an ABI for this contract. This is necessary to - * fully implement transparency without decoding reverts caused by selector clashes between the proxy and the + * inherit from that interface, and instead `upgradeToAndCall` is implicitly implemented using a custom dispatch mechanism + * in `_fallback`. Consequently, the compiler will not produce an ABI for this contract. This is necessary to fully + * implement transparency without decoding reverts caused by selector clashes between the proxy and the * implementation. * * IMPORTANT: This contract avoids unnecessary storage reads by setting the admin only during construction as an immutable variable, @@ -55,10 +53,10 @@ interface ITransparentUpgradeableProxy is IERC1967 { * WARNING: It is not recommended to extend this contract to add additional external functions. If you do so, the compiler * will not check that there are no selector conflicts, due to the note above. A selector clash between any new function * and the functions declared in {ITransparentUpgradeableProxy} will be resolved in favor of the new one. This could - * render the admin operations inaccessible, which could prevent upgradeability. Transparency may also be compromised. + * render the `upgradeToAndCall` function inaccessible, preventing upgradeability and compromising transparency. */ contract TransparentUpgradeableProxy is ERC1967Proxy { - // An immutable address for the admin avoid unnecessary SLOADs before each call + // An immutable address for the admin to avoid unnecessary SLOADs before each call // at the expense of removing the ability to change the admin once it's set. // This is acceptable if the admin is always a ProxyAdmin instance or similar contract // with its own ability to transfer the permissions to another account. @@ -69,19 +67,14 @@ contract TransparentUpgradeableProxy is ERC1967Proxy { */ error ProxyDeniedAdminAccess(); - /** - * @dev msg.value is not 0. - */ - error ProxyNonPayableFunction(); - /** * @dev Initializes an upgradeable proxy managed by `_admin`, backed by the implementation at `_logic`, and * optionally initialized with `_data` as explained in {ERC1967Proxy-constructor}. */ - constructor(address _logic, address admin_, bytes memory _data) payable ERC1967Proxy(_logic, _data) { - _admin = admin_; + constructor(address _logic, address initialOwner, bytes memory _data) payable ERC1967Proxy(_logic, _data) { + _admin = address(new ProxyAdmin(initialOwner)); // Set the storage value and emit an event for ERC-1967 compatibility - ERC1967Utils.changeAdmin(admin_); + ERC1967Utils.changeAdmin(_admin); } /** @@ -89,18 +82,11 @@ contract TransparentUpgradeableProxy is ERC1967Proxy { */ function _fallback() internal virtual override { if (msg.sender == _admin) { - bytes memory ret; - bytes4 selector = msg.sig; - if (selector == ITransparentUpgradeableProxy.upgradeTo.selector) { - ret = _dispatchUpgradeTo(); - } else if (selector == ITransparentUpgradeableProxy.upgradeToAndCall.selector) { - ret = _dispatchUpgradeToAndCall(); + if (msg.sig == ITransparentUpgradeableProxy.upgradeToAndCall.selector) { + _dispatchUpgradeToAndCall(); } else { revert ProxyDeniedAdminAccess(); } - assembly { - return(add(ret, 0x20), mload(ret)) - } } else { super._fallback(); } @@ -109,34 +95,8 @@ contract TransparentUpgradeableProxy is ERC1967Proxy { /** * @dev Upgrade the implementation of the proxy. */ - function _dispatchUpgradeTo() private returns (bytes memory) { - _requireZeroValue(); - - address newImplementation = abi.decode(msg.data[4:], (address)); - ERC1967Utils.upgradeToAndCall(newImplementation, bytes(""), false); - - return ""; - } - - /** - * @dev Upgrade the implementation of the proxy, and then call a function from the new implementation as specified - * by `data`, which should be an encoded function call. This is useful to initialize new storage variables in the - * proxied contract. - */ - function _dispatchUpgradeToAndCall() private returns (bytes memory) { + function _dispatchUpgradeToAndCall() private { (address newImplementation, bytes memory data) = abi.decode(msg.data[4:], (address, bytes)); - ERC1967Utils.upgradeToAndCall(newImplementation, data, true); - - return ""; - } - - /** - * @dev To keep this contract fully transparent, the fallback is payable. This helper is here to enforce - * non-payability of function implemented through dispatchers while still allowing value to pass through. - */ - function _requireZeroValue() private { - if (msg.value != 0) { - revert ProxyNonPayableFunction(); - } + ERC1967Utils.upgradeToAndCall(newImplementation, data); } } diff --git a/contracts/proxy/utils/UUPSUpgradeable.sol b/contracts/proxy/utils/UUPSUpgradeable.sol index f4045aa6f..3564ed246 100644 --- a/contracts/proxy/utils/UUPSUpgradeable.sol +++ b/contracts/proxy/utils/UUPSUpgradeable.sol @@ -20,6 +20,17 @@ abstract contract UUPSUpgradeable is IERC1822Proxiable { /// @custom:oz-upgrades-unsafe-allow state-variable-immutable state-variable-assignment address private immutable __self = address(this); + /** + * @dev The version of the upgrade interface of the contract. If this getter is missing, both `upgradeTo(address)` + * and `upgradeToAndCall(address,bytes)` are present, and `upgradeTo` must be used if no function should be called, + * while `upgradeToAndCall` will invoke the `receive` function if the second argument is the empty byte string. + * If the getter returns `"5.0.0"`, only `upgradeToAndCall(address,bytes)` is present, and the second argument must + * be the empty byte string if no function should be called, being impossible to invoke the `receive` function + * during an upgrade. + */ + // solhint-disable-next-line private-vars-leading-underscore + string internal constant UPGRADE_INTERFACE_VERSION = "5.0.0"; + /** * @dev The call is from an unauthorized context. */ @@ -71,20 +82,6 @@ abstract contract UUPSUpgradeable is IERC1822Proxiable { return ERC1967Utils.IMPLEMENTATION_SLOT; } - /** - * @dev Upgrade the implementation of the proxy to `newImplementation`. - * - * Calls {_authorizeUpgrade}. - * - * Emits an {Upgraded} event. - * - * @custom:oz-upgrades-unsafe-allow-reachable delegatecall - */ - function upgradeTo(address newImplementation) public virtual onlyProxy { - _authorizeUpgrade(newImplementation); - _upgradeToAndCallUUPS(newImplementation, new bytes(0), false); - } - /** * @dev Upgrade the implementation of the proxy to `newImplementation`, and subsequently execute the function call * encoded in `data`. @@ -97,17 +94,17 @@ abstract contract UUPSUpgradeable is IERC1822Proxiable { */ function upgradeToAndCall(address newImplementation, bytes memory data) public payable virtual onlyProxy { _authorizeUpgrade(newImplementation); - _upgradeToAndCallUUPS(newImplementation, data, true); + _upgradeToAndCallUUPS(newImplementation, data); } /** * @dev Function that should revert when `msg.sender` is not authorized to upgrade the contract. Called by - * {upgradeTo} and {upgradeToAndCall}. + * {upgradeToAndCall}. * * Normally, this function will use an xref:access.adoc[access control] modifier such as {Ownable-onlyOwner}. * * ```solidity - * function _authorizeUpgrade(address) internal onlyOwner {} + * function _authorizeUpgrade(address) internal onlyOwner {} * ``` */ function _authorizeUpgrade(address newImplementation) internal virtual; @@ -117,12 +114,12 @@ abstract contract UUPSUpgradeable is IERC1822Proxiable { * * Emits an {IERC1967-Upgraded} event. */ - function _upgradeToAndCallUUPS(address newImplementation, bytes memory data, bool forceCall) private { + function _upgradeToAndCallUUPS(address newImplementation, bytes memory data) private { try IERC1822Proxiable(newImplementation).proxiableUUID() returns (bytes32 slot) { if (slot != ERC1967Utils.IMPLEMENTATION_SLOT) { revert UUPSUnsupportedProxiableUUID(slot); } - ERC1967Utils.upgradeToAndCall(newImplementation, data, forceCall); + ERC1967Utils.upgradeToAndCall(newImplementation, data); } catch { // The implementation is not UUPS revert ERC1967Utils.ERC1967InvalidImplementation(newImplementation); diff --git a/test/governance/compatibility/GovernorCompatibilityBravo.test.js b/test/governance/compatibility/GovernorCompatibilityBravo.test.js index 4182dfb4e..26255342f 100644 --- a/test/governance/compatibility/GovernorCompatibilityBravo.test.js +++ b/test/governance/compatibility/GovernorCompatibilityBravo.test.js @@ -1,6 +1,6 @@ const { expectEvent } = require('@openzeppelin/test-helpers'); const { expect } = require('chai'); -const RLP = require('rlp'); +const { computeCreateAddress } = require('../../helpers/create'); const Enums = require('../../helpers/enums'); const { GovernorHelper } = require('../../helpers/governance'); const { clockFromReceipt } = require('../../helpers/time'); @@ -12,15 +12,6 @@ const CallReceiver = artifacts.require('CallReceiverMock'); const { shouldBehaveLikeEIP6372 } = require('../utils/EIP6372.behavior'); -function makeContractAddress(creator, nonce) { - return web3.utils.toChecksumAddress( - web3.utils - .sha3(RLP.encode([creator, nonce])) - .slice(12) - .substring(14), - ); -} - const TOKENS = [ { Token: artifacts.require('$ERC20Votes'), mode: 'blocknumber' }, { Token: artifacts.require('$ERC20VotesTimestampMock'), mode: 'timestamp' }, @@ -58,7 +49,7 @@ contract('GovernorCompatibilityBravo', function (accounts) { // Need to predict governance address to set it as timelock admin with a delayed transfer const nonce = await web3.eth.getTransactionCount(deployer); - const predictGovernor = makeContractAddress(deployer, nonce + 1); + const predictGovernor = computeCreateAddress(deployer, nonce + 1); this.timelock = await Timelock.new(predictGovernor, 2 * 86400); this.mock = await Governor.new( diff --git a/test/governance/extensions/GovernorTimelockCompound.test.js b/test/governance/extensions/GovernorTimelockCompound.test.js index c406baf8d..7a2cb0abd 100644 --- a/test/governance/extensions/GovernorTimelockCompound.test.js +++ b/test/governance/extensions/GovernorTimelockCompound.test.js @@ -1,10 +1,10 @@ const { constants, expectEvent, expectRevert } = require('@openzeppelin/test-helpers'); const { expect } = require('chai'); -const RLP = require('rlp'); const Enums = require('../../helpers/enums'); const { GovernorHelper, proposalStatesToBitMap } = require('../../helpers/governance'); const { expectRevertCustomError } = require('../../helpers/customError'); +const { computeCreateAddress } = require('../../helpers/create'); const { shouldSupportInterfaces } = require('../../utils/introspection/SupportsInterface.behavior'); @@ -14,15 +14,6 @@ const CallReceiver = artifacts.require('CallReceiverMock'); const ERC721 = artifacts.require('$ERC721'); const ERC1155 = artifacts.require('$ERC1155'); -function makeContractAddress(creator, nonce) { - return web3.utils.toChecksumAddress( - web3.utils - .sha3(RLP.encode([creator, nonce])) - .slice(12) - .substring(14), - ); -} - const TOKENS = [ { Token: artifacts.require('$ERC20Votes'), mode: 'blocknumber' }, { Token: artifacts.require('$ERC20VotesTimestampMock'), mode: 'timestamp' }, @@ -49,7 +40,7 @@ contract('GovernorTimelockCompound', function (accounts) { // Need to predict governance address to set it as timelock admin with a delayed transfer const nonce = await web3.eth.getTransactionCount(deployer); - const predictGovernor = makeContractAddress(deployer, nonce + 1); + const predictGovernor = computeCreateAddress(deployer, nonce + 1); this.timelock = await Timelock.new(predictGovernor, 2 * 86400); this.mock = await Governor.new( diff --git a/test/helpers/account.js b/test/helpers/account.js new file mode 100644 index 000000000..1b01a7214 --- /dev/null +++ b/test/helpers/account.js @@ -0,0 +1,14 @@ +const { web3 } = require('hardhat'); +const { impersonateAccount, setBalance } = require('@nomicfoundation/hardhat-network-helpers'); + +// Hardhat default balance +const DEFAULT_BALANCE = web3.utils.toBN('10000000000000000000000'); + +async function impersonate(account, balance = DEFAULT_BALANCE) { + await impersonateAccount(account); + await setBalance(account, balance); +} + +module.exports = { + impersonate, +}; diff --git a/test/helpers/create.js b/test/helpers/create.js new file mode 100644 index 000000000..e0cea66e2 --- /dev/null +++ b/test/helpers/create.js @@ -0,0 +1,23 @@ +const { rlp } = require('ethereumjs-util'); + +function computeCreateAddress(deployer, nonce) { + return web3.utils.toChecksumAddress(web3.utils.sha3(rlp.encode([deployer.address ?? deployer, nonce])).slice(-40)); +} + +function computeCreate2Address(saltHex, bytecode, deployer) { + return web3.utils.toChecksumAddress( + web3.utils + .sha3( + '0x' + + ['ff', deployer.address ?? deployer, saltHex, web3.utils.soliditySha3(bytecode)] + .map(x => x.replace(/0x/, '')) + .join(''), + ) + .slice(-40), + ); +} + +module.exports = { + computeCreateAddress, + computeCreate2Address, +}; diff --git a/test/helpers/create2.js b/test/helpers/create2.js deleted file mode 100644 index afe07dae3..000000000 --- a/test/helpers/create2.js +++ /dev/null @@ -1,11 +0,0 @@ -function computeCreate2Address(saltHex, bytecode, deployer) { - return web3.utils.toChecksumAddress( - `0x${web3.utils - .sha3(`0x${['ff', deployer, saltHex, web3.utils.soliditySha3(bytecode)].map(x => x.replace(/0x/, '')).join('')}`) - .slice(-40)}`, - ); -} - -module.exports = { - computeCreate2Address, -}; diff --git a/test/proxy/Clones.test.js b/test/proxy/Clones.test.js index 2edd1999c..14a70e20b 100644 --- a/test/proxy/Clones.test.js +++ b/test/proxy/Clones.test.js @@ -1,5 +1,5 @@ const { expectEvent } = require('@openzeppelin/test-helpers'); -const { computeCreate2Address } = require('../helpers/create2'); +const { computeCreate2Address } = require('../helpers/create'); const { expect } = require('chai'); const { expectRevertCustomError } = require('../helpers/customError'); diff --git a/test/proxy/ERC1967/ERC1967Proxy.test.js b/test/proxy/ERC1967/ERC1967Proxy.test.js index 22df960ff..4cbc6c71d 100644 --- a/test/proxy/ERC1967/ERC1967Proxy.test.js +++ b/test/proxy/ERC1967/ERC1967Proxy.test.js @@ -3,11 +3,9 @@ const shouldBehaveLikeProxy = require('../Proxy.behaviour'); const ERC1967Proxy = artifacts.require('ERC1967Proxy'); contract('ERC1967Proxy', function (accounts) { - const [proxyAdminOwner] = accounts; - - const createProxy = async function (implementation, _admin, initData, opts) { + const createProxy = async function (implementation, initData, opts) { return ERC1967Proxy.new(implementation, initData, opts); }; - shouldBehaveLikeProxy(createProxy, undefined, proxyAdminOwner); + shouldBehaveLikeProxy(createProxy, accounts); }); diff --git a/test/proxy/Proxy.behaviour.js b/test/proxy/Proxy.behaviour.js index 0867b93c9..7ad014e3e 100644 --- a/test/proxy/Proxy.behaviour.js +++ b/test/proxy/Proxy.behaviour.js @@ -2,15 +2,18 @@ const { expectRevert } = require('@openzeppelin/test-helpers'); const { getSlot, ImplementationSlot } = require('../helpers/erc1967'); const { expect } = require('chai'); +const { expectRevertCustomError } = require('../helpers/customError'); const DummyImplementation = artifacts.require('DummyImplementation'); -module.exports = function shouldBehaveLikeProxy(createProxy, proxyAdminAddress, proxyCreator) { +module.exports = function shouldBehaveLikeProxy(createProxy, accounts) { + const [proxyCreator] = accounts; + it('cannot be initialized with a non-contract address', async function () { const nonContractAddress = proxyCreator; const initializeData = Buffer.from(''); await expectRevert.unspecified( - createProxy(nonContractAddress, proxyAdminAddress, initializeData, { + createProxy(nonContractAddress, initializeData, { from: proxyCreator, }), ); @@ -43,7 +46,7 @@ module.exports = function shouldBehaveLikeProxy(createProxy, proxyAdminAddress, describe('when not sending balance', function () { beforeEach('creating proxy', async function () { this.proxy = ( - await createProxy(this.implementation, proxyAdminAddress, initializeData, { + await createProxy(this.implementation, initializeData, { from: proxyCreator, }) ).address; @@ -55,16 +58,16 @@ module.exports = function shouldBehaveLikeProxy(createProxy, proxyAdminAddress, describe('when sending some balance', function () { const value = 10e5; - beforeEach('creating proxy', async function () { - this.proxy = ( - await createProxy(this.implementation, proxyAdminAddress, initializeData, { + it('reverts', async function () { + await expectRevertCustomError( + createProxy(this.implementation, initializeData, { from: proxyCreator, value, - }) - ).address; + }), + 'ERC1967NonPayable', + [], + ); }); - - assertProxyInitialization({ value: 0, balance: value }); }); }); @@ -76,7 +79,7 @@ module.exports = function shouldBehaveLikeProxy(createProxy, proxyAdminAddress, describe('when not sending balance', function () { beforeEach('creating proxy', async function () { this.proxy = ( - await createProxy(this.implementation, proxyAdminAddress, initializeData, { + await createProxy(this.implementation, initializeData, { from: proxyCreator, }) ).address; @@ -93,7 +96,7 @@ module.exports = function shouldBehaveLikeProxy(createProxy, proxyAdminAddress, it('reverts', async function () { await expectRevert.unspecified( - createProxy(this.implementation, proxyAdminAddress, initializeData, { from: proxyCreator, value }), + createProxy(this.implementation, initializeData, { from: proxyCreator, value }), ); }); }); @@ -106,7 +109,7 @@ module.exports = function shouldBehaveLikeProxy(createProxy, proxyAdminAddress, describe('when not sending balance', function () { beforeEach('creating proxy', async function () { this.proxy = ( - await createProxy(this.implementation, proxyAdminAddress, initializeData, { + await createProxy(this.implementation, initializeData, { from: proxyCreator, }) ).address; @@ -123,7 +126,7 @@ module.exports = function shouldBehaveLikeProxy(createProxy, proxyAdminAddress, beforeEach('creating proxy', async function () { this.proxy = ( - await createProxy(this.implementation, proxyAdminAddress, initializeData, { + await createProxy(this.implementation, initializeData, { from: proxyCreator, value, }) @@ -148,7 +151,7 @@ module.exports = function shouldBehaveLikeProxy(createProxy, proxyAdminAddress, describe('when not sending balance', function () { beforeEach('creating proxy', async function () { this.proxy = ( - await createProxy(this.implementation, proxyAdminAddress, initializeData, { + await createProxy(this.implementation, initializeData, { from: proxyCreator, }) ).address; @@ -165,7 +168,7 @@ module.exports = function shouldBehaveLikeProxy(createProxy, proxyAdminAddress, it('reverts', async function () { await expectRevert.unspecified( - createProxy(this.implementation, proxyAdminAddress, initializeData, { from: proxyCreator, value }), + createProxy(this.implementation, initializeData, { from: proxyCreator, value }), ); }); }); @@ -180,7 +183,7 @@ module.exports = function shouldBehaveLikeProxy(createProxy, proxyAdminAddress, describe('when not sending balance', function () { beforeEach('creating proxy', async function () { this.proxy = ( - await createProxy(this.implementation, proxyAdminAddress, initializeData, { + await createProxy(this.implementation, initializeData, { from: proxyCreator, }) ).address; @@ -197,7 +200,7 @@ module.exports = function shouldBehaveLikeProxy(createProxy, proxyAdminAddress, beforeEach('creating proxy', async function () { this.proxy = ( - await createProxy(this.implementation, proxyAdminAddress, initializeData, { + await createProxy(this.implementation, initializeData, { from: proxyCreator, value, }) @@ -216,7 +219,7 @@ module.exports = function shouldBehaveLikeProxy(createProxy, proxyAdminAddress, it('reverts', async function () { await expectRevert( - createProxy(this.implementation, proxyAdminAddress, initializeData, { from: proxyCreator }), + createProxy(this.implementation, initializeData, { from: proxyCreator }), 'DummyImplementation reverted', ); }); diff --git a/test/proxy/beacon/BeaconProxy.test.js b/test/proxy/beacon/BeaconProxy.test.js index 63d982397..d583d0ffb 100644 --- a/test/proxy/beacon/BeaconProxy.test.js +++ b/test/proxy/beacon/BeaconProxy.test.js @@ -59,9 +59,8 @@ contract('BeaconProxy', function (accounts) { it('no initialization', async function () { const data = Buffer.from(''); - const balance = '10'; - this.proxy = await BeaconProxy.new(this.beacon.address, data, { value: balance }); - await this.assertInitialized({ value: '0', balance }); + this.proxy = await BeaconProxy.new(this.beacon.address, data); + await this.assertInitialized({ value: '0', balance: '0' }); }); it('non-payable initialization', async function () { @@ -79,7 +78,16 @@ contract('BeaconProxy', function (accounts) { await this.assertInitialized({ value, balance }); }); - it('reverting initialization', async function () { + it('reverting initialization due to value', async function () { + const data = Buffer.from(''); + await expectRevertCustomError( + BeaconProxy.new(this.beacon.address, data, { value: '1' }), + 'ERC1967NonPayable', + [], + ); + }); + + it('reverting initialization function', async function () { const data = this.implementationV0.contract.methods.reverts().encodeABI(); await expectRevert(BeaconProxy.new(this.beacon.address, data), 'DummyImplementation reverted'); }); diff --git a/test/proxy/transparent/ProxyAdmin.test.js b/test/proxy/transparent/ProxyAdmin.test.js index 23f7ce9b2..3283d74de 100644 --- a/test/proxy/transparent/ProxyAdmin.test.js +++ b/test/proxy/transparent/ProxyAdmin.test.js @@ -8,6 +8,7 @@ const ITransparentUpgradeableProxy = artifacts.require('ITransparentUpgradeableP const { getAddressInSlot, ImplementationSlot } = require('../../helpers/erc1967'); const { expectRevertCustomError } = require('../../helpers/customError'); +const { computeCreateAddress } = require('../../helpers/create'); contract('ProxyAdmin', function (accounts) { const [proxyAdminOwner, anotherAccount] = accounts; @@ -19,12 +20,12 @@ contract('ProxyAdmin', function (accounts) { beforeEach(async function () { const initializeData = Buffer.from(''); - this.proxyAdmin = await ProxyAdmin.new(proxyAdminOwner); - const proxy = await TransparentUpgradeableProxy.new( - this.implementationV1.address, - this.proxyAdmin.address, - initializeData, - ); + const proxy = await TransparentUpgradeableProxy.new(this.implementationV1.address, proxyAdminOwner, initializeData); + + const proxyNonce = await web3.eth.getTransactionCount(proxy.address); + const proxyAdminAddress = computeCreateAddress(proxy.address, proxyNonce - 1); // Nonce already used + this.proxyAdmin = await ProxyAdmin.at(proxyAdminAddress); + this.proxy = await ITransparentUpgradeableProxy.at(proxy.address); }); @@ -32,11 +33,13 @@ contract('ProxyAdmin', function (accounts) { expect(await this.proxyAdmin.owner()).to.equal(proxyAdminOwner); }); - describe('#upgrade', function () { + describe('without data', function () { context('with unauthorized account', function () { it('fails to upgrade', async function () { await expectRevertCustomError( - this.proxyAdmin.upgrade(this.proxy.address, this.implementationV2.address, { from: anotherAccount }), + this.proxyAdmin.upgradeAndCall(this.proxy.address, this.implementationV2.address, '0x', { + from: anotherAccount, + }), 'OwnableUnauthorizedAccount', [anotherAccount], ); @@ -45,7 +48,9 @@ contract('ProxyAdmin', function (accounts) { context('with authorized account', function () { it('upgrades implementation', async function () { - await this.proxyAdmin.upgrade(this.proxy.address, this.implementationV2.address, { from: proxyAdminOwner }); + await this.proxyAdmin.upgradeAndCall(this.proxy.address, this.implementationV2.address, '0x', { + from: proxyAdminOwner, + }); const implementationAddress = await getAddressInSlot(this.proxy, ImplementationSlot); expect(implementationAddress).to.be.equal(this.implementationV2.address); @@ -53,7 +58,7 @@ contract('ProxyAdmin', function (accounts) { }); }); - describe('#upgradeAndCall', function () { + describe('with data', function () { context('with unauthorized account', function () { it('fails to upgrade', async function () { const callData = new ImplV1('').contract.methods.initializeNonPayableWithValue(1337).encodeABI(); diff --git a/test/proxy/transparent/TransparentUpgradeableProxy.behaviour.js b/test/proxy/transparent/TransparentUpgradeableProxy.behaviour.js index 1a03b84db..cb9ad9a6f 100644 --- a/test/proxy/transparent/TransparentUpgradeableProxy.behaviour.js +++ b/test/proxy/transparent/TransparentUpgradeableProxy.behaviour.js @@ -5,6 +5,8 @@ const { expectRevertCustomError } = require('../../helpers/customError'); const { expect } = require('chai'); const { web3 } = require('hardhat'); +const { computeCreateAddress } = require('../../helpers/create'); +const { impersonate } = require('../../helpers/account'); const Implementation1 = artifacts.require('Implementation1'); const Implementation2 = artifacts.require('Implementation2'); @@ -17,8 +19,21 @@ const InitializableMock = artifacts.require('InitializableMock'); const DummyImplementation = artifacts.require('DummyImplementation'); const ClashingImplementation = artifacts.require('ClashingImplementation'); -module.exports = function shouldBehaveLikeTransparentUpgradeableProxy(createProxy, accounts) { - const [proxyAdminAddress, proxyAdminOwner, anotherAccount] = accounts; +module.exports = function shouldBehaveLikeTransparentUpgradeableProxy(createProxy, initialOwner, accounts) { + const [proxyCreator, anotherAccount] = accounts; + + async function createProxyWithImpersonatedProxyAdmin(logic, initData, opts) { + const proxy = await createProxy(logic, initData, { from: proxyCreator, ...opts }); + + // Expect proxy admin to be the first and only contract created by the proxy + const proxyAdminAddress = computeCreateAddress(proxy.address, 1); + await impersonate(proxyAdminAddress); + + return { + proxy, + proxyAdminAddress, + }; + } before(async function () { this.implementationV0 = (await DummyImplementation.new()).address; @@ -27,10 +42,12 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy(createProx beforeEach(async function () { const initializeData = Buffer.from(''); - this.proxy = await createProxy(this.implementationV0, proxyAdminAddress, initializeData, { - from: proxyAdminOwner, - }); - this.proxyAddress = this.proxy.address; + const { proxy, proxyAdminAddress } = await createProxyWithImpersonatedProxyAdmin( + this.implementationV0, + initializeData, + ); + this.proxy = proxy; + this.proxyAdminAddress = proxyAdminAddress; }); describe('implementation', function () { @@ -40,7 +57,7 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy(createProx }); it('delegates to the implementation', async function () { - const dummy = new DummyImplementation(this.proxyAddress); + const dummy = new DummyImplementation(this.proxy.address); const value = await dummy.get(); expect(value).to.equal(true); @@ -49,64 +66,31 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy(createProx describe('proxy admin', function () { it('emits AdminChanged event during construction', async function () { - expectEvent.inConstruction(this.proxy, 'AdminChanged', { + await expectEvent.inConstruction(this.proxy, 'AdminChanged', { previousAdmin: ZERO_ADDRESS, - newAdmin: proxyAdminAddress, + newAdmin: this.proxyAdminAddress, }); }); - it('sets the admin in the storage', async function () { - expect(await getAddressInSlot(this.proxy, AdminSlot)).to.be.equal(proxyAdminAddress); + it('sets the proxy admin in storage', async function () { + expect(await getAddressInSlot(this.proxy, AdminSlot)).to.be.equal(this.proxyAdminAddress); }); it('can overwrite the admin by the implementation', async function () { - const dummy = new DummyImplementation(this.proxyAddress); + const dummy = new DummyImplementation(this.proxy.address); await dummy.unsafeOverrideAdmin(anotherAccount); const ERC1967AdminSlotValue = await getAddressInSlot(this.proxy, AdminSlot); expect(ERC1967AdminSlotValue).to.be.equal(anotherAccount); // Still allows previous admin to execute admin operations - expect(ERC1967AdminSlotValue).to.not.equal(proxyAdminAddress); - expectEvent(await this.proxy.upgradeTo(this.implementationV1, { from: proxyAdminAddress }), 'Upgraded', { - implementation: this.implementationV1, - }); - }); - }); - - describe('upgradeTo', function () { - describe('when the sender is the admin', function () { - const from = proxyAdminAddress; - - describe('when the given implementation is different from the current one', function () { - it('upgrades to the requested implementation', async function () { - await this.proxy.upgradeTo(this.implementationV1, { from }); - - const implementationAddress = await getAddressInSlot(this.proxy, ImplementationSlot); - expect(implementationAddress).to.be.equal(this.implementationV1); - }); - - it('emits an event', async function () { - expectEvent(await this.proxy.upgradeTo(this.implementationV1, { from }), 'Upgraded', { - implementation: this.implementationV1, - }); - }); - }); - - describe('when the given implementation is the zero address', function () { - it('reverts', async function () { - await expectRevertCustomError(this.proxy.upgradeTo(ZERO_ADDRESS, { from }), 'ERC1967InvalidImplementation', [ - ZERO_ADDRESS, - ]); - }); - }); - }); - - describe('when the sender is not the admin', function () { - const from = anotherAccount; - - it('reverts', async function () { - await expectRevert.unspecified(this.proxy.upgradeTo(this.implementationV1, { from })); - }); + expect(ERC1967AdminSlotValue).to.not.equal(this.proxyAdminAddress); + expectEvent( + await this.proxy.upgradeToAndCall(this.implementationV1, '0x', { from: this.proxyAdminAddress }), + 'Upgraded', + { + implementation: this.implementationV1, + }, + ); }); }); @@ -120,11 +104,13 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy(createProx const initializeData = new InitializableMock('').contract.methods['initializeWithX(uint256)'](42).encodeABI(); describe('when the sender is the admin', function () { - const from = proxyAdminAddress; const value = 1e5; beforeEach(async function () { - this.receipt = await this.proxy.upgradeToAndCall(this.behavior.address, initializeData, { from, value }); + this.receipt = await this.proxy.upgradeToAndCall(this.behavior.address, initializeData, { + from: this.proxyAdminAddress, + value, + }); }); it('upgrades to the requested implementation', async function () { @@ -137,13 +123,13 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy(createProx }); it('calls the initializer function', async function () { - const migratable = new InitializableMock(this.proxyAddress); + const migratable = new InitializableMock(this.proxy.address); const x = await migratable.x(); expect(x).to.be.bignumber.equal('42'); }); it('sends given value to the proxy', async function () { - const balance = await web3.eth.getBalance(this.proxyAddress); + const balance = await web3.eth.getBalance(this.proxy.address); expect(balance.toString()).to.be.bignumber.equal(value.toString()); }); @@ -151,7 +137,7 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy(createProx // storage layout should look as follows: // - 0: Initializable storage ++ initializerRan ++ onlyInitializingRan // - 1: x - const storedValue = await web3.eth.getStorageAt(this.proxyAddress, 1); + const storedValue = await web3.eth.getStorageAt(this.proxy.address, 1); expect(parseInt(storedValue)).to.eq(42); }); }); @@ -170,7 +156,7 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy(createProx it('reverts', async function () { await expectRevert.unspecified( - this.proxy.upgradeToAndCall(this.behavior.address, initializeData, { from: proxyAdminAddress }), + this.proxy.upgradeToAndCall(this.behavior.address, initializeData, { from: this.proxyAdminAddress }), ); }); }); @@ -178,7 +164,6 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy(createProx describe('with migrations', function () { describe('when the sender is the admin', function () { - const from = proxyAdminAddress; const value = 1e5; describe('when upgrading to V1', function () { @@ -186,8 +171,11 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy(createProx beforeEach(async function () { this.behaviorV1 = await MigratableMockV1.new(); - this.balancePreviousV1 = new BN(await web3.eth.getBalance(this.proxyAddress)); - this.receipt = await this.proxy.upgradeToAndCall(this.behaviorV1.address, v1MigrationData, { from, value }); + this.balancePreviousV1 = new BN(await web3.eth.getBalance(this.proxy.address)); + this.receipt = await this.proxy.upgradeToAndCall(this.behaviorV1.address, v1MigrationData, { + from: this.proxyAdminAddress, + value, + }); }); it('upgrades to the requested version and emits an event', async function () { @@ -197,12 +185,12 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy(createProx }); it("calls the 'initialize' function and sends given value to the proxy", async function () { - const migratable = new MigratableMockV1(this.proxyAddress); + const migratable = new MigratableMockV1(this.proxy.address); const x = await migratable.x(); expect(x).to.be.bignumber.equal('42'); - const balance = await web3.eth.getBalance(this.proxyAddress); + const balance = await web3.eth.getBalance(this.proxy.address); expect(new BN(balance)).to.be.bignumber.equal(this.balancePreviousV1.addn(value)); }); @@ -211,9 +199,9 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy(createProx beforeEach(async function () { this.behaviorV2 = await MigratableMockV2.new(); - this.balancePreviousV2 = new BN(await web3.eth.getBalance(this.proxyAddress)); + this.balancePreviousV2 = new BN(await web3.eth.getBalance(this.proxy.address)); this.receipt = await this.proxy.upgradeToAndCall(this.behaviorV2.address, v2MigrationData, { - from, + from: this.proxyAdminAddress, value, }); }); @@ -225,7 +213,7 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy(createProx }); it("calls the 'migrate' function and sends given value to the proxy", async function () { - const migratable = new MigratableMockV2(this.proxyAddress); + const migratable = new MigratableMockV2(this.proxy.address); const x = await migratable.x(); expect(x).to.be.bignumber.equal('10'); @@ -233,7 +221,7 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy(createProx const y = await migratable.y(); expect(y).to.be.bignumber.equal('42'); - const balance = new BN(await web3.eth.getBalance(this.proxyAddress)); + const balance = new BN(await web3.eth.getBalance(this.proxy.address)); expect(balance).to.be.bignumber.equal(this.balancePreviousV2.addn(value)); }); @@ -242,9 +230,9 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy(createProx beforeEach(async function () { this.behaviorV3 = await MigratableMockV3.new(); - this.balancePreviousV3 = new BN(await web3.eth.getBalance(this.proxyAddress)); + this.balancePreviousV3 = new BN(await web3.eth.getBalance(this.proxy.address)); this.receipt = await this.proxy.upgradeToAndCall(this.behaviorV3.address, v3MigrationData, { - from, + from: this.proxyAdminAddress, value, }); }); @@ -256,7 +244,7 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy(createProx }); it("calls the 'migrate' function and sends given value to the proxy", async function () { - const migratable = new MigratableMockV3(this.proxyAddress); + const migratable = new MigratableMockV3(this.proxy.address); const x = await migratable.x(); expect(x).to.be.bignumber.equal('42'); @@ -264,7 +252,7 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy(createProx const y = await migratable.y(); expect(y).to.be.bignumber.equal('10'); - const balance = new BN(await web3.eth.getBalance(this.proxyAddress)); + const balance = new BN(await web3.eth.getBalance(this.proxy.address)); expect(balance).to.be.bignumber.equal(this.balancePreviousV3.addn(value)); }); }); @@ -289,15 +277,18 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy(createProx const initializeData = Buffer.from(''); this.clashingImplV0 = (await ClashingImplementation.new()).address; this.clashingImplV1 = (await ClashingImplementation.new()).address; - this.proxy = await createProxy(this.clashingImplV0, proxyAdminAddress, initializeData, { - from: proxyAdminOwner, - }); + const { proxy, proxyAdminAddress } = await createProxyWithImpersonatedProxyAdmin( + this.clashingImplV0, + initializeData, + ); + this.proxy = proxy; + this.proxyAdminAddress = proxyAdminAddress; this.clashing = new ClashingImplementation(this.proxy.address); }); it('proxy admin cannot call delegated functions', async function () { await expectRevertCustomError( - this.clashing.delegatedFunction({ from: proxyAdminAddress }), + this.clashing.delegatedFunction({ from: this.proxyAdminAddress }), 'ProxyDeniedAdminAccess', [], ); @@ -305,26 +296,16 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy(createProx describe('when function names clash', function () { it('executes the proxy function if the sender is the admin', async function () { - const receipt = await this.proxy.upgradeTo(this.clashingImplV1, { from: proxyAdminAddress, value: 0 }); + const receipt = await this.proxy.upgradeToAndCall(this.clashingImplV1, '0x', { + from: this.proxyAdminAddress, + }); expectEvent(receipt, 'Upgraded', { implementation: this.clashingImplV1 }); }); it('delegates the call to implementation when sender is not the admin', async function () { - const receipt = await this.proxy.upgradeTo(this.clashingImplV1, { from: anotherAccount, value: 0 }); - expectEvent.notEmitted(receipt, 'Upgraded'); - expectEvent.inTransaction(receipt.tx, this.clashing, 'ClashingImplementationCall'); - }); - - it('requires 0 value calling upgradeTo by proxy admin', async function () { - await expectRevertCustomError( - this.proxy.upgradeTo(this.clashingImplV1, { from: proxyAdminAddress, value: 1 }), - 'ProxyNonPayableFunction', - [], - ); - }); - - it('allows calling with value if sender is not the admin', async function () { - const receipt = await this.proxy.upgradeTo(this.clashingImplV1, { from: anotherAccount, value: 1 }); + const receipt = await this.proxy.upgradeToAndCall(this.clashingImplV1, '0x', { + from: anotherAccount, + }); expectEvent.notEmitted(receipt, 'Upgraded'); expectEvent.inTransaction(receipt.tx, this.clashing, 'ClashingImplementationCall'); }); @@ -336,13 +317,16 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy(createProx it('should add new function', async () => { const instance1 = await Implementation1.new(); - const proxy = await createProxy(instance1.address, proxyAdminAddress, initializeData, { from: proxyAdminOwner }); + const { proxy, proxyAdminAddress } = await createProxyWithImpersonatedProxyAdmin( + instance1.address, + initializeData, + ); const proxyInstance1 = new Implementation1(proxy.address); await proxyInstance1.setValue(42); const instance2 = await Implementation2.new(); - await proxy.upgradeTo(instance2.address, { from: proxyAdminAddress }); + await proxy.upgradeToAndCall(instance2.address, '0x', { from: proxyAdminAddress }); const proxyInstance2 = new Implementation2(proxy.address); const res = await proxyInstance2.getValue(); @@ -351,7 +335,10 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy(createProx it('should remove function', async () => { const instance2 = await Implementation2.new(); - const proxy = await createProxy(instance2.address, proxyAdminAddress, initializeData, { from: proxyAdminOwner }); + const { proxy, proxyAdminAddress } = await createProxyWithImpersonatedProxyAdmin( + instance2.address, + initializeData, + ); const proxyInstance2 = new Implementation2(proxy.address); await proxyInstance2.setValue(42); @@ -359,7 +346,7 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy(createProx expect(res.toString()).to.eq('42'); const instance1 = await Implementation1.new(); - await proxy.upgradeTo(instance1.address, { from: proxyAdminAddress }); + await proxy.upgradeToAndCall(instance1.address, '0x', { from: proxyAdminAddress }); const proxyInstance1 = new Implementation2(proxy.address); await expectRevert.unspecified(proxyInstance1.getValue()); @@ -367,13 +354,16 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy(createProx it('should change function signature', async () => { const instance1 = await Implementation1.new(); - const proxy = await createProxy(instance1.address, proxyAdminAddress, initializeData, { from: proxyAdminOwner }); + const { proxy, proxyAdminAddress } = await createProxyWithImpersonatedProxyAdmin( + instance1.address, + initializeData, + ); const proxyInstance1 = new Implementation1(proxy.address); await proxyInstance1.setValue(42); const instance3 = await Implementation3.new(); - await proxy.upgradeTo(instance3.address, { from: proxyAdminAddress }); + await proxy.upgradeToAndCall(instance3.address, '0x', { from: proxyAdminAddress }); const proxyInstance3 = new Implementation3(proxy.address); const res = await proxyInstance3.getValue(8); @@ -383,10 +373,13 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy(createProx it('should add fallback function', async () => { const initializeData = Buffer.from(''); const instance1 = await Implementation1.new(); - const proxy = await createProxy(instance1.address, proxyAdminAddress, initializeData, { from: proxyAdminOwner }); + const { proxy, proxyAdminAddress } = await createProxyWithImpersonatedProxyAdmin( + instance1.address, + initializeData, + ); const instance4 = await Implementation4.new(); - await proxy.upgradeTo(instance4.address, { from: proxyAdminAddress }); + await proxy.upgradeToAndCall(instance4.address, '0x', { from: proxyAdminAddress }); const proxyInstance4 = new Implementation4(proxy.address); const data = '0x'; @@ -398,10 +391,13 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy(createProx it('should remove fallback function', async () => { const instance4 = await Implementation4.new(); - const proxy = await createProxy(instance4.address, proxyAdminAddress, initializeData, { from: proxyAdminOwner }); + const { proxy, proxyAdminAddress } = await createProxyWithImpersonatedProxyAdmin( + instance4.address, + initializeData, + ); const instance2 = await Implementation2.new(); - await proxy.upgradeTo(instance2.address, { from: proxyAdminAddress }); + await proxy.upgradeToAndCall(instance2.address, '0x', { from: proxyAdminAddress }); const data = '0x'; await expectRevert.unspecified(web3.eth.sendTransaction({ to: proxy.address, from: anotherAccount, data })); diff --git a/test/proxy/transparent/TransparentUpgradeableProxy.test.js b/test/proxy/transparent/TransparentUpgradeableProxy.test.js index d60a31a21..764d28d0a 100644 --- a/test/proxy/transparent/TransparentUpgradeableProxy.test.js +++ b/test/proxy/transparent/TransparentUpgradeableProxy.test.js @@ -5,13 +5,14 @@ const TransparentUpgradeableProxy = artifacts.require('TransparentUpgradeablePro const ITransparentUpgradeableProxy = artifacts.require('ITransparentUpgradeableProxy'); contract('TransparentUpgradeableProxy', function (accounts) { - const [proxyAdminAddress, proxyAdminOwner] = accounts; + const [owner, ...otherAccounts] = accounts; - const createProxy = async function (logic, admin, initData, opts) { - const { address } = await TransparentUpgradeableProxy.new(logic, admin, initData, opts); - return ITransparentUpgradeableProxy.at(address); + const createProxy = async function (logic, initData, opts = {}) { + const { address, transactionHash } = await TransparentUpgradeableProxy.new(logic, owner, initData, opts); + const instance = await ITransparentUpgradeableProxy.at(address); + return { ...instance, transactionHash }; }; - shouldBehaveLikeProxy(createProxy, proxyAdminAddress, proxyAdminOwner); - shouldBehaveLikeTransparentUpgradeableProxy(createProxy, accounts); + shouldBehaveLikeProxy(createProxy, otherAccounts); + shouldBehaveLikeTransparentUpgradeableProxy(createProxy, owner, otherAccounts); }); diff --git a/test/proxy/utils/UUPSUpgradeable.test.js b/test/proxy/utils/UUPSUpgradeable.test.js index 6a8104248..8304264ff 100644 --- a/test/proxy/utils/UUPSUpgradeable.test.js +++ b/test/proxy/utils/UUPSUpgradeable.test.js @@ -26,7 +26,7 @@ contract('UUPSUpgradeable', function () { }); it('upgrade to upgradeable implementation', async function () { - const { receipt } = await this.instance.upgradeTo(this.implUpgradeOk.address); + const { receipt } = await this.instance.upgradeToAndCall(this.implUpgradeOk.address, '0x'); expect(receipt.logs.filter(({ event }) => event === 'Upgraded').length).to.be.equal(1); expectEvent(receipt, 'Upgraded', { implementation: this.implUpgradeOk.address }); expect(await getAddressInSlot(this.instance, ImplementationSlot)).to.be.equal(this.implUpgradeOk.address); @@ -48,7 +48,7 @@ contract('UUPSUpgradeable', function () { it('calling upgradeTo on the implementation reverts', async function () { await expectRevertCustomError( - this.implInitial.upgradeTo(this.implUpgradeOk.address), + this.implInitial.upgradeToAndCall(this.implUpgradeOk.address, '0x'), 'UUPSUnauthorizedCallContext', [], ); @@ -72,7 +72,7 @@ contract('UUPSUpgradeable', function () { ); await expectRevertCustomError( - instance.upgradeTo(this.implUpgradeUnsafe.address), + instance.upgradeToAndCall(this.implUpgradeUnsafe.address, '0x'), 'UUPSUnauthorizedCallContext', [], ); @@ -93,14 +93,14 @@ contract('UUPSUpgradeable', function () { it('rejects upgrading to an unsupported UUID', async function () { await expectRevertCustomError( - this.instance.upgradeTo(this.implUnsupportedUUID.address), + this.instance.upgradeToAndCall(this.implUnsupportedUUID.address, '0x'), 'UUPSUnsupportedProxiableUUID', [web3.utils.keccak256('invalid UUID')], ); }); it('upgrade to and unsafe upgradeable implementation', async function () { - const { receipt } = await this.instance.upgradeTo(this.implUpgradeUnsafe.address); + const { receipt } = await this.instance.upgradeToAndCall(this.implUpgradeUnsafe.address, '0x'); expectEvent(receipt, 'Upgraded', { implementation: this.implUpgradeUnsafe.address }); expect(await getAddressInSlot(this.instance, ImplementationSlot)).to.be.equal(this.implUpgradeUnsafe.address); }); @@ -108,7 +108,7 @@ contract('UUPSUpgradeable', function () { // delegate to a non existing upgradeTo function causes a low level revert it('reject upgrade to non uups implementation', async function () { await expectRevertCustomError( - this.instance.upgradeTo(this.implUpgradeNonUUPS.address), + this.instance.upgradeToAndCall(this.implUpgradeNonUUPS.address, '0x'), 'ERC1967InvalidImplementation', [this.implUpgradeNonUUPS.address], ); @@ -118,8 +118,10 @@ contract('UUPSUpgradeable', function () { const { address } = await ERC1967Proxy.new(this.implInitial.address, '0x'); const otherInstance = await UUPSUpgradeableMock.at(address); - await expectRevertCustomError(this.instance.upgradeTo(otherInstance.address), 'ERC1967InvalidImplementation', [ - otherInstance.address, - ]); + await expectRevertCustomError( + this.instance.upgradeToAndCall(otherInstance.address, '0x'), + 'ERC1967InvalidImplementation', + [otherInstance.address], + ); }); }); diff --git a/test/utils/Create2.test.js b/test/utils/Create2.test.js index f88d5504c..afbcc3db9 100644 --- a/test/utils/Create2.test.js +++ b/test/utils/Create2.test.js @@ -1,5 +1,5 @@ const { balance, ether, expectEvent, expectRevert, send } = require('@openzeppelin/test-helpers'); -const { computeCreate2Address } = require('../helpers/create2'); +const { computeCreate2Address } = require('../helpers/create'); const { expect } = require('chai'); const { expectRevertCustomError } = require('../helpers/customError');