From 0ec31d91b3ee1944fbc6e085ffbbf6a19172aa2b Mon Sep 17 00:00:00 2001 From: roleengineer Date: Wed, 20 Nov 2024 17:13:58 +0100 Subject: [PATCH 1/9] (test/proxy): upgradeToAndCall unit test coverage --- ...erationsUpgradeableRenounceableProxy.t.sol | 65 +++++++++++++++++-- .../MockMintPolicyWithSelectorClashes.sol | 31 +++++++++ 2 files changed, 89 insertions(+), 7 deletions(-) create mode 100644 test/groups/upgradeableProxy/mocks/MockMintPolicyWithSelectorClashes.sol diff --git a/test/groups/upgradeableProxy/adminOperationsUpgradeableRenounceableProxy.t.sol b/test/groups/upgradeableProxy/adminOperationsUpgradeableRenounceableProxy.t.sol index 5724938..42bf976 100644 --- a/test/groups/upgradeableProxy/adminOperationsUpgradeableRenounceableProxy.t.sol +++ b/test/groups/upgradeableProxy/adminOperationsUpgradeableRenounceableProxy.t.sol @@ -5,9 +5,15 @@ import {Test} from "forge-std/Test.sol"; import {StdCheats} from "forge-std/StdCheats.sol"; import {StorageSlot} from "@openzeppelin/contracts/utils/StorageSlot.sol"; import "forge-std/console.sol"; -import "../../../src/groups/UpgradeableRenounceableProxy.sol"; -import "../../../src/errors/Errors.sol"; -import "../groupSetup.sol"; +import "src/errors/Errors.sol"; +import "test/groups/groupSetup.sol"; +import { + UpgradeableRenounceableProxy, IUpgradeableRenounceableProxy +} from "src/groups/UpgradeableRenounceableProxy.sol"; +import { + MockMintPolicyWithSelectorClashes, + IMockMintPolicyWithSelectorClashes +} from "test/groups/upgradeableProxy/mocks/MockMintPolicyWithSelectorClashes.sol"; contract adminOperationsUpgradeableRenounceableProxy is Test, GroupSetup { // Constants @@ -18,6 +24,9 @@ contract adminOperationsUpgradeableRenounceableProxy is Test, GroupSetup { address public group; IUpgradeableRenounceableProxy public proxy; + // copy of BaseMintPolicy + address public newMintPolicy; + address public mockMintPolicyWithSelectorClashes; // Constructor @@ -43,10 +52,18 @@ contract adminOperationsUpgradeableRenounceableProxy is Test, GroupSetup { vm.prank(group); hub.trust(addresses[i], INDEFINITE_FUTURE); } + + // deploy a new copy of base mint policy + newMintPolicy = address(new MintPolicy()); + + // deploy a policy mock designed to simulate proxy selector clashes + mockMintPolicyWithSelectorClashes = address(new MockMintPolicyWithSelectorClashes()); } // Tests + // External implementation() returns(address) + function testGetImplementation() public { address implementation = proxy.implementation(); assertEq(implementation, mintPolicy); @@ -60,14 +77,16 @@ contract adminOperationsUpgradeableRenounceableProxy is Test, GroupSetup { * - test accessibility of interface functions from non-Admin callers */ - function testUpgradeToAndCall() public { + // External upgradeToAndCall(address,bytes) + + function testAdminUpgradeToAndCall() public { address originalImplementation = proxy.implementation(); assertEq(originalImplementation, mintPolicy); - // deploy a new copy of base mint policy - address newMintPolicy = address(new MintPolicy()); + // let's upgrade to implementation with selector clashes + _upgradeToAndCall(mockMintPolicyWithSelectorClashes, ""); - // upgrade the proxy to the new implementation + // should upgrade the proxy to the new implementation as called by admin vm.prank(group); proxy.upgradeToAndCall(newMintPolicy, ""); @@ -79,6 +98,29 @@ contract adminOperationsUpgradeableRenounceableProxy is Test, GroupSetup { _testGroupMintOwnCollateral(addresses[0], group, 1 * CRC); } + function testNonAdminUpgradeToAndCall(address nonAdmin) public { + vm.assume(nonAdmin != group); + + vm.prank(nonAdmin); + // should revert as proxy fallback after checking that caller is not admin + // redirects call to implementation, which doesn't have related selector + vm.expectRevert(); + proxy.upgradeToAndCall(newMintPolicy, ""); + + // let's upgrade to implementation with selector clashes + _upgradeToAndCall(mockMintPolicyWithSelectorClashes, ""); + + vm.prank(nonAdmin); + (address returnedAddress, bytes memory returnedBytes) = + IMockMintPolicyWithSelectorClashes(address(proxy)).upgradeToAndCall(newMintPolicy, "newMintPolicy"); + // should not call proxy native upgradeToAndCall as fallback must redirect call to the implementation, + // however should find a selector match inside the implementation and execute the implementation logic + assertEq(returnedAddress, newMintPolicy); + assertEq(keccak256(returnedBytes), keccak256("newMintPolicy")); + // implementation shouldn't be changed + assertEq(mockMintPolicyWithSelectorClashes, proxy.implementation(), "implementation has changed"); + } + function testRenounceAdmin() public { // current admin address admin = address(uint160(uint256(vm.load(address(proxy), ADMIN_SLOT)))); @@ -120,4 +162,13 @@ contract adminOperationsUpgradeableRenounceableProxy is Test, GroupSetup { uint256 balanceAfter = hub.balanceOf(_minter, tokenIdGroup); assertEq(balanceAfter, balanceBefore + _amount); } + + // @dev makes admin (set to group) upgradeToAndCall call until admin is not renounced + function _upgradeToAndCall(address newImplementation, bytes memory data) internal { + // upgrade the proxy to the new implementation + vm.prank(group); + proxy.upgradeToAndCall(newImplementation, data); + // should be new implementation + assertEq(newImplementation, proxy.implementation(), "upgrade to new implementation failed"); + } } diff --git a/test/groups/upgradeableProxy/mocks/MockMintPolicyWithSelectorClashes.sol b/test/groups/upgradeableProxy/mocks/MockMintPolicyWithSelectorClashes.sol new file mode 100644 index 0000000..4712233 --- /dev/null +++ b/test/groups/upgradeableProxy/mocks/MockMintPolicyWithSelectorClashes.sol @@ -0,0 +1,31 @@ +// SPDX-License-Identifier: AGPL-3.0-only +pragma solidity >=0.8.13; + +import {MintPolicy} from "src/groups/BaseMintPolicy.sol"; + +interface IMockMintPolicyWithSelectorClashes { + function upgradeToAndCall(address _newImplementation, bytes memory _data) + external + returns (address, bytes memory); + function renounceUpgradeability() external returns (bool); +} + +contract MockMintPolicyWithSelectorClashes is MintPolicy { + // External functions implemented to simulate clashes with proxy native selectors + + function implementation() external pure returns (address) { + return address(0xff); + } + + function upgradeToAndCall(address _newImplementation, bytes memory _data) + external + pure + returns (address, bytes memory) + { + return (_newImplementation, _data); + } + + function renounceUpgradeability() external pure returns (bool) { + return false; + } +} From ea769812e24283b42a9c5ba2ccbc84aa5c1feda7 Mon Sep 17 00:00:00 2001 From: roleengineer Date: Fri, 22 Nov 2024 17:28:00 +0100 Subject: [PATCH 2/9] (test/proxy): renounceUpgradeability, implementation covered for admin/non-admin with selector clashes --- ...erationsUpgradeableRenounceableProxy.t.sol | 88 ++++++++++++++++--- .../MockMintPolicyWithSelectorClashes.sol | 2 +- 2 files changed, 79 insertions(+), 11 deletions(-) diff --git a/test/groups/upgradeableProxy/adminOperationsUpgradeableRenounceableProxy.t.sol b/test/groups/upgradeableProxy/adminOperationsUpgradeableRenounceableProxy.t.sol index 42bf976..7b085ad 100644 --- a/test/groups/upgradeableProxy/adminOperationsUpgradeableRenounceableProxy.t.sol +++ b/test/groups/upgradeableProxy/adminOperationsUpgradeableRenounceableProxy.t.sol @@ -1,10 +1,7 @@ // SPDX-License-Identifier: AGPL-3.0-only pragma solidity >=0.8.13; -import {Test} from "forge-std/Test.sol"; -import {StdCheats} from "forge-std/StdCheats.sol"; -import {StorageSlot} from "@openzeppelin/contracts/utils/StorageSlot.sol"; -import "forge-std/console.sol"; +import {console2, Test} from "forge-std/Test.sol"; import "src/errors/Errors.sol"; import "test/groups/groupSetup.sol"; import { @@ -64,9 +61,22 @@ contract adminOperationsUpgradeableRenounceableProxy is Test, GroupSetup { // External implementation() returns(address) - function testGetImplementation() public { + function testGetImplementation(address anyCaller) public { address implementation = proxy.implementation(); assertEq(implementation, mintPolicy); + + // static call is hardcoded in the proxy, this means that function with the same + // selector in any implementation is unreachable (selector clashes) + + // let's upgrade to implementation with selector clashes + _upgradeToAndCall(mockMintPolicyWithSelectorClashes, ""); + + vm.prank(anyCaller); + implementation = proxy.implementation(); + // should not return the mockPolicy value + assertTrue(implementation != address(0xff)); + // should return the current implementation + assertEq(implementation, mockMintPolicyWithSelectorClashes); } /* todo: - test getting admin from proxy @@ -87,6 +97,7 @@ contract adminOperationsUpgradeableRenounceableProxy is Test, GroupSetup { _upgradeToAndCall(mockMintPolicyWithSelectorClashes, ""); // should upgrade the proxy to the new implementation as called by admin + // despite the fact that current implementation has upgradeToAndCall function with different logic vm.prank(group); proxy.upgradeToAndCall(newMintPolicy, ""); @@ -121,24 +132,77 @@ contract adminOperationsUpgradeableRenounceableProxy is Test, GroupSetup { assertEq(mockMintPolicyWithSelectorClashes, proxy.implementation(), "implementation has changed"); } - function testRenounceAdmin() public { + // External renounceUpgradeability() + + function testAdminRenounceUpgradeability() public { // current admin - address admin = address(uint160(uint256(vm.load(address(proxy), ADMIN_SLOT)))); + address admin = _readProxyAdminSlot(); assertEq(admin, group); - // renounce admin + // let's upgrade to implementation with selector clashes + _upgradeToAndCall(mockMintPolicyWithSelectorClashes, ""); + + // should renounce admin as called by admin + // despite the fact that current implementation has renounceUpgradeability function with different logic vm.prank(group); proxy.renounceUpgradeability(); // renounced admin - admin = address(uint160(uint256(vm.load(address(proxy), ADMIN_SLOT)))); + admin = _readProxyAdminSlot(); assertEq(admin, address(0x1)); + } - // expect revert when trying to upgrade to implementation 0xdead + function testNonAdminRenounceUpgradeability(address nonAdmin) public { + vm.assume(nonAdmin != group); + + vm.prank(nonAdmin); + // should revert as proxy fallback after checking that caller is not admin + // redirects call to implementation, which doesn't have related selector + vm.expectRevert(); + proxy.renounceUpgradeability(); + + // let's upgrade to implementation with selector clashes + _upgradeToAndCall(mockMintPolicyWithSelectorClashes, ""); + + vm.prank(nonAdmin); + bool returnedBool = IMockMintPolicyWithSelectorClashes(address(proxy)).renounceUpgradeability(); + // should not call proxy native renounceUpgradeability as fallback must redirect call to the implementation, + // however should find a selector match inside the implementation and execute the implementation logic + assertTrue(returnedBool); + // admin shouldn't be renounced + address admin = _readProxyAdminSlot(); + assertEq(admin, group); + } + + function testRenouncedAdminEqualNonAdmin() public { + // todo: update forge-std + uint256 snapshot = vm.snapshot(); + + // admin should experience same behaviour as non admin after renounced upgradeability + + // default implementation vm.startPrank(group); + proxy.renounceUpgradeability(); + vm.expectRevert(); + proxy.renounceUpgradeability(); vm.expectRevert(); proxy.upgradeToAndCall(address(0xdead), ""); vm.stopPrank(); + + // let's revert to initial state to upgrade to implementation with selector clashes + vm.revertTo(snapshot); + _upgradeToAndCall(mockMintPolicyWithSelectorClashes, ""); + + // implementation with selector clashes + vm.startPrank(group); + proxy.renounceUpgradeability(); + bool returnedBool = IMockMintPolicyWithSelectorClashes(address(proxy)).renounceUpgradeability(); + assertTrue(returnedBool); + (address returnedAddress, bytes memory returnedBytes) = + IMockMintPolicyWithSelectorClashes(address(proxy)).upgradeToAndCall(newMintPolicy, "newMintPolicy"); + assertEq(returnedAddress, newMintPolicy); + assertEq(keccak256(returnedBytes), keccak256("newMintPolicy")); + vm.stopPrank(); } // Internal functions @@ -171,4 +235,8 @@ contract adminOperationsUpgradeableRenounceableProxy is Test, GroupSetup { // should be new implementation assertEq(newImplementation, proxy.implementation(), "upgrade to new implementation failed"); } + + function _readProxyAdminSlot() internal view returns (address admin) { + admin = address(uint160(uint256(vm.load(address(proxy), ADMIN_SLOT)))); + } } diff --git a/test/groups/upgradeableProxy/mocks/MockMintPolicyWithSelectorClashes.sol b/test/groups/upgradeableProxy/mocks/MockMintPolicyWithSelectorClashes.sol index 4712233..4df8f8a 100644 --- a/test/groups/upgradeableProxy/mocks/MockMintPolicyWithSelectorClashes.sol +++ b/test/groups/upgradeableProxy/mocks/MockMintPolicyWithSelectorClashes.sol @@ -26,6 +26,6 @@ contract MockMintPolicyWithSelectorClashes is MintPolicy { } function renounceUpgradeability() external pure returns (bool) { - return false; + return true; } } From 5c5b135a75eaf7ababafc0d780d5f9a6851f77c8 Mon Sep 17 00:00:00 2001 From: roleengineer Date: Sat, 23 Nov 2024 15:59:10 +0100 Subject: [PATCH 3/9] (test/proxy): added helper internal function _readImplementationSlot --- .../adminOperationsUpgradeableRenounceableProxy.t.sol | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/test/groups/upgradeableProxy/adminOperationsUpgradeableRenounceableProxy.t.sol b/test/groups/upgradeableProxy/adminOperationsUpgradeableRenounceableProxy.t.sol index 7b085ad..15450b8 100644 --- a/test/groups/upgradeableProxy/adminOperationsUpgradeableRenounceableProxy.t.sol +++ b/test/groups/upgradeableProxy/adminOperationsUpgradeableRenounceableProxy.t.sol @@ -16,6 +16,7 @@ contract adminOperationsUpgradeableRenounceableProxy is Test, GroupSetup { // Constants bytes32 internal constant ADMIN_SLOT = 0xb53127684a568b3173ae13b9f8a6016e243e63b6e8ee1178d6a717850b5d6103; + bytes32 internal constant IMPLEMENTATION_SLOT = 0x360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc; // State variables @@ -64,6 +65,7 @@ contract adminOperationsUpgradeableRenounceableProxy is Test, GroupSetup { function testGetImplementation(address anyCaller) public { address implementation = proxy.implementation(); assertEq(implementation, mintPolicy); + assertEq(implementation, _readImplementationSlot()); // static call is hardcoded in the proxy, this means that function with the same // selector in any implementation is unreachable (selector clashes) @@ -77,14 +79,15 @@ contract adminOperationsUpgradeableRenounceableProxy is Test, GroupSetup { assertTrue(implementation != address(0xff)); // should return the current implementation assertEq(implementation, mockMintPolicyWithSelectorClashes); + assertEq(implementation, _readImplementationSlot()); } - /* todo: - test getting admin from proxy + /* todo: - test getting admin from proxy (DONE) * - test admin cannot be changed * - test noone else can call upgradeToAndCall * - test upgradeToAndCall with call data * - test renouncing admin (DONE) - * - test accessibility of interface functions from non-Admin callers + * - test accessibility of interface functions from non-Admin callers (DONE) */ // External upgradeToAndCall(address,bytes) @@ -239,4 +242,8 @@ contract adminOperationsUpgradeableRenounceableProxy is Test, GroupSetup { function _readProxyAdminSlot() internal view returns (address admin) { admin = address(uint160(uint256(vm.load(address(proxy), ADMIN_SLOT)))); } + + function _readImplementationSlot() internal view returns (address implementation) { + implementation = address(uint160(uint256(vm.load(address(proxy), IMPLEMENTATION_SLOT)))); + } } From e6f858f0b635190001fa6c6c942a6a2782ab21c7 Mon Sep 17 00:00:00 2001 From: roleengineer Date: Sun, 24 Nov 2024 17:22:50 +0100 Subject: [PATCH 4/9] (test/proxy): implemented mock mint policy extended --- ...erationsUpgradeableRenounceableProxy.t.sol | 18 +++ .../mocks/MockMintPolicyExtended.sol | 117 ++++++++++++++++++ 2 files changed, 135 insertions(+) create mode 100644 test/groups/upgradeableProxy/mocks/MockMintPolicyExtended.sol diff --git a/test/groups/upgradeableProxy/adminOperationsUpgradeableRenounceableProxy.t.sol b/test/groups/upgradeableProxy/adminOperationsUpgradeableRenounceableProxy.t.sol index 15450b8..a8e7b90 100644 --- a/test/groups/upgradeableProxy/adminOperationsUpgradeableRenounceableProxy.t.sol +++ b/test/groups/upgradeableProxy/adminOperationsUpgradeableRenounceableProxy.t.sol @@ -11,6 +11,10 @@ import { MockMintPolicyWithSelectorClashes, IMockMintPolicyWithSelectorClashes } from "test/groups/upgradeableProxy/mocks/MockMintPolicyWithSelectorClashes.sol"; +import { + MockMintPolicyExtended, + IMockMintPolicyExtended +} from "test/groups/upgradeableProxy/mocks/MockMintPolicyExtended.sol"; contract adminOperationsUpgradeableRenounceableProxy is Test, GroupSetup { // Constants @@ -25,6 +29,8 @@ contract adminOperationsUpgradeableRenounceableProxy is Test, GroupSetup { // copy of BaseMintPolicy address public newMintPolicy; address public mockMintPolicyWithSelectorClashes; + address public mockMintPolicyExtended; + address public whitelistAdmin; // Constructor @@ -56,6 +62,11 @@ contract adminOperationsUpgradeableRenounceableProxy is Test, GroupSetup { // deploy a policy mock designed to simulate proxy selector clashes mockMintPolicyWithSelectorClashes = address(new MockMintPolicyWithSelectorClashes()); + + // deploy a policy mock designed to test implementation with state and bypass renounce upgradeability + mockMintPolicyExtended = address(new MockMintPolicyExtended()); + // set whitelist admin for mock mint policy extended + whitelistAdmin = makeAddr("mockMintPolicyExtendedWhitelistAdmin"); } // Tests @@ -135,6 +146,13 @@ contract adminOperationsUpgradeableRenounceableProxy is Test, GroupSetup { assertEq(mockMintPolicyWithSelectorClashes, proxy.implementation(), "implementation has changed"); } + function testUpgradeToAndCallWithCalldata() public { + bytes4 initializeSelector = bytes4(keccak256("initialize(address,address[])")); + // encode whitelist admin and initial list of whitelisted addresses + bytes memory data = abi.encodeWithSelector(initializeSelector, whitelistAdmin, addresses); + _upgradeToAndCall(mockMintPolicyExtended, data); + } + // External renounceUpgradeability() function testAdminRenounceUpgradeability() public { diff --git a/test/groups/upgradeableProxy/mocks/MockMintPolicyExtended.sol b/test/groups/upgradeableProxy/mocks/MockMintPolicyExtended.sol new file mode 100644 index 0000000..c84b68b --- /dev/null +++ b/test/groups/upgradeableProxy/mocks/MockMintPolicyExtended.sol @@ -0,0 +1,117 @@ +// SPDX-License-Identifier: AGPL-3.0-only +pragma solidity >=0.8.20; + +import {MintPolicy, IMintPolicy} from "src/groups/BaseMintPolicy.sol"; +import {Initializable} from "@openzeppelin/contracts/proxy/utils/Initializable.sol"; +import {ERC1967Utils} from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Utils.sol"; + +interface IMockMintPolicyExtended is IMintPolicy { + function setProxyImplementation(address newImplementation, bytes memory data) external; + function changeWhitelistAdmin(address newWhitelistAdmin) external; + function setWhitelisted(address minter, bool whitelist) external; + function getWhitelistAdmin() external view returns (address); + function isWhitelisted(address minter) external view returns (bool); +} + +contract MockMintPolicyExtended is Initializable, MintPolicy { + /// @custom:storage-location erc7201:circles.storage.MockMintPolicyExtended + struct MockWhitelistStorage { + address whitelistAdmin; + mapping(address minter => bool) whitelisted; + } + + // keccak256(abi.encode(uint256(keccak256("circles.storage.MockWhitelist")) - 1)) & ~bytes32(uint256(0xff)) + bytes32 private constant MockWhitelistStorageLocation = + 0xad8f981846947e2d39c57b5b7bd1e5ee93f80ef26284ffb0224c576ef51a7c00; + + /// @custom:oz-upgrades-unsafe-allow constructor + constructor() { + _disableInitializers(); + } + + function initialize(address _admin, address[] calldata _initWhitelist) external initializer { + _setWhitelistAdmin(_admin); + + for (uint256 i; i < _initWhitelist.length;) { + _setWhitelisted(_initWhitelist[i], true); + unchecked { + ++i; + } + } + } + + // Modifiers + + modifier onlyWhitelistAdmin() { + require(msg.sender == _getWhitelistAdmin()); + _; + } + + // External functions + + function beforeMintPolicy( + address _minter, + address, /*_group*/ + uint256[] calldata, /*_collateral*/ + uint256[] calldata, /*_amounts*/ + bytes calldata /*_data*/ + ) external virtual override returns (bool) { + return _isWhitelisted(_minter); + } + + function changeWhitelistAdmin(address newWhitelistAdmin) external onlyWhitelistAdmin { + _setWhitelistAdmin(newWhitelistAdmin); + } + + function setWhitelisted(address minter, bool whitelist) external onlyWhitelistAdmin { + _setWhitelisted(minter, whitelist); + } + + // Functions to bypass proxy admin functions + + function setProxyAdmin(address newAdmin) external onlyWhitelistAdmin { + ERC1967Utils.changeAdmin(newAdmin); + } + + function setProxyImplementation(address newImplementation, bytes memory data) external onlyWhitelistAdmin { + ERC1967Utils.upgradeToAndCall(newImplementation, data); + } + + // View functions + + function getWhitelistAdmin() external view returns (address) { + return _getWhitelistAdmin(); + } + + function isWhitelisted(address minter) external view returns (bool) { + return _isWhitelisted(minter); + } + + // Private functions + + function _getMockWhitelistStorage() private pure returns (MockWhitelistStorage storage $) { + assembly { + $.slot := MockWhitelistStorageLocation + } + } + + function _getWhitelistAdmin() private view returns (address) { + MockWhitelistStorage storage $ = _getMockWhitelistStorage(); + return $.whitelistAdmin; + } + + function _setWhitelistAdmin(address admin) private { + MockWhitelistStorage storage $ = _getMockWhitelistStorage(); + $.whitelistAdmin = admin; + } + + function _isWhitelisted(address minter) private view returns (bool) { + MockWhitelistStorage storage $ = _getMockWhitelistStorage(); + return $.whitelisted[minter]; + } + + function _setWhitelisted(address minter, bool whitelist) private { + MockWhitelistStorage storage $ = _getMockWhitelistStorage(); + $.whitelisted[minter] = whitelist; + } +} From 3ed7e676bd2bcf9c2805da276d56141b679576c3 Mon Sep 17 00:00:00 2001 From: roleengineer Date: Sun, 24 Nov 2024 20:00:21 +0100 Subject: [PATCH 5/9] (test/proxy): test upgradeToAndCall with calldata --- ...erationsUpgradeableRenounceableProxy.t.sol | 57 ++++++++++++++++++- .../mocks/MockMintPolicyExtended.sol | 2 + .../MockMintPolicyWithSelectorClashes.sol | 1 + 3 files changed, 58 insertions(+), 2 deletions(-) diff --git a/test/groups/upgradeableProxy/adminOperationsUpgradeableRenounceableProxy.t.sol b/test/groups/upgradeableProxy/adminOperationsUpgradeableRenounceableProxy.t.sol index a8e7b90..53fce02 100644 --- a/test/groups/upgradeableProxy/adminOperationsUpgradeableRenounceableProxy.t.sol +++ b/test/groups/upgradeableProxy/adminOperationsUpgradeableRenounceableProxy.t.sol @@ -4,6 +4,7 @@ pragma solidity >=0.8.13; import {console2, Test} from "forge-std/Test.sol"; import "src/errors/Errors.sol"; import "test/groups/groupSetup.sol"; +import {ERC1967Utils} from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Utils.sol"; import { UpgradeableRenounceableProxy, IUpgradeableRenounceableProxy } from "src/groups/UpgradeableRenounceableProxy.sol"; @@ -87,7 +88,9 @@ contract adminOperationsUpgradeableRenounceableProxy is Test, GroupSetup { vm.prank(anyCaller); implementation = proxy.implementation(); // should not return the mockPolicy value - assertTrue(implementation != address(0xff)); + assertTrue( + implementation != IMockMintPolicyWithSelectorClashes(mockMintPolicyWithSelectorClashes).implementation() + ); // should return the current implementation assertEq(implementation, mockMintPolicyWithSelectorClashes); assertEq(implementation, _readImplementationSlot()); @@ -96,7 +99,7 @@ contract adminOperationsUpgradeableRenounceableProxy is Test, GroupSetup { /* todo: - test getting admin from proxy (DONE) * - test admin cannot be changed * - test noone else can call upgradeToAndCall - * - test upgradeToAndCall with call data + * - test upgradeToAndCall with call data (DONE) * - test renouncing admin (DONE) * - test accessibility of interface functions from non-Admin callers (DONE) */ @@ -112,6 +115,7 @@ contract adminOperationsUpgradeableRenounceableProxy is Test, GroupSetup { // should upgrade the proxy to the new implementation as called by admin // despite the fact that current implementation has upgradeToAndCall function with different logic + _expectEmitUpgradedEvent(newMintPolicy); vm.prank(group); proxy.upgradeToAndCall(newMintPolicy, ""); @@ -151,6 +155,41 @@ contract adminOperationsUpgradeableRenounceableProxy is Test, GroupSetup { // encode whitelist admin and initial list of whitelisted addresses bytes memory data = abi.encodeWithSelector(initializeSelector, whitelistAdmin, addresses); _upgradeToAndCall(mockMintPolicyExtended, data); + // proxy state should be initialized + assertEq(whitelistAdmin, IMockMintPolicyExtended(address(proxy)).getWhitelistAdmin()); + for (uint256 i; i < addresses.length;) { + assertTrue(IMockMintPolicyExtended(address(proxy)).isWhitelisted(addresses[i])); + unchecked { + ++i; + } + } + // initialize should not be called twice + vm.expectRevert(); + IMockMintPolicyExtended(address(proxy)).initialize(group, addresses); + + // beforeMintPolicy should be overridden correctly + uint256[] memory empty; + address random = makeAddr("random"); + assertTrue(!IMockMintPolicyExtended(address(proxy)).beforeMintPolicy(random, group, empty, empty, "")); + assertTrue(IMockMintPolicyExtended(address(proxy)).beforeMintPolicy(addresses[1], group, empty, empty, "")); + + // new functions should work correctly + vm.prank(whitelistAdmin); + IMockMintPolicyExtended(address(proxy)).setWhitelisted(random, true); + assertTrue(IMockMintPolicyExtended(address(proxy)).beforeMintPolicy(random, group, empty, empty, "")); + + vm.prank(whitelistAdmin); + IMockMintPolicyExtended(address(proxy)).changeWhitelistAdmin(group); + assertEq(group, IMockMintPolicyExtended(address(proxy)).getWhitelistAdmin()); + // not whitelisted admin + vm.expectRevert(); + IMockMintPolicyExtended(address(proxy)).setWhitelisted(address(this), true); + vm.expectRevert(); + IMockMintPolicyExtended(address(proxy)).changeWhitelistAdmin(address(this)); + vm.expectRevert(); + IMockMintPolicyExtended(address(proxy)).setProxyAdmin(address(this)); + vm.expectRevert(); + IMockMintPolicyExtended(address(proxy)).setProxyImplementation(address(this), ""); } // External renounceUpgradeability() @@ -165,6 +204,7 @@ contract adminOperationsUpgradeableRenounceableProxy is Test, GroupSetup { // should renounce admin as called by admin // despite the fact that current implementation has renounceUpgradeability function with different logic + _expectEmitAdminChangedEvent(group, address(0x1)); vm.prank(group); proxy.renounceUpgradeability(); @@ -251,12 +291,25 @@ contract adminOperationsUpgradeableRenounceableProxy is Test, GroupSetup { // @dev makes admin (set to group) upgradeToAndCall call until admin is not renounced function _upgradeToAndCall(address newImplementation, bytes memory data) internal { // upgrade the proxy to the new implementation + _expectEmitUpgradedEvent(newImplementation); vm.prank(group); proxy.upgradeToAndCall(newImplementation, data); // should be new implementation assertEq(newImplementation, proxy.implementation(), "upgrade to new implementation failed"); } + /// @dev should emit Upgraded event + function _expectEmitUpgradedEvent(address newImplementation) internal { + vm.expectEmit(true, true, true, true); + emit ERC1967Utils.Upgraded(newImplementation); + } + + /// @dev should emit AdminChanged event + function _expectEmitAdminChangedEvent(address previousAdmin, address newAdmin) internal { + vm.expectEmit(true, true, true, true); + emit ERC1967Utils.AdminChanged(previousAdmin, newAdmin); + } + function _readProxyAdminSlot() internal view returns (address admin) { admin = address(uint160(uint256(vm.load(address(proxy), ADMIN_SLOT)))); } diff --git a/test/groups/upgradeableProxy/mocks/MockMintPolicyExtended.sol b/test/groups/upgradeableProxy/mocks/MockMintPolicyExtended.sol index c84b68b..f1f2a4c 100644 --- a/test/groups/upgradeableProxy/mocks/MockMintPolicyExtended.sol +++ b/test/groups/upgradeableProxy/mocks/MockMintPolicyExtended.sol @@ -6,6 +6,8 @@ import {Initializable} from "@openzeppelin/contracts/proxy/utils/Initializable.s import {ERC1967Utils} from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Utils.sol"; interface IMockMintPolicyExtended is IMintPolicy { + function initialize(address _admin, address[] calldata _initWhitelist) external; + function setProxyAdmin(address newAdmin) external; function setProxyImplementation(address newImplementation, bytes memory data) external; function changeWhitelistAdmin(address newWhitelistAdmin) external; function setWhitelisted(address minter, bool whitelist) external; diff --git a/test/groups/upgradeableProxy/mocks/MockMintPolicyWithSelectorClashes.sol b/test/groups/upgradeableProxy/mocks/MockMintPolicyWithSelectorClashes.sol index 4df8f8a..eb4ae8c 100644 --- a/test/groups/upgradeableProxy/mocks/MockMintPolicyWithSelectorClashes.sol +++ b/test/groups/upgradeableProxy/mocks/MockMintPolicyWithSelectorClashes.sol @@ -4,6 +4,7 @@ pragma solidity >=0.8.13; import {MintPolicy} from "src/groups/BaseMintPolicy.sol"; interface IMockMintPolicyWithSelectorClashes { + function implementation() external pure returns (address); function upgradeToAndCall(address _newImplementation, bytes memory _data) external returns (address, bytes memory); From 4936c10c5cf439ebce533b3b6b08890b381f22a5 Mon Sep 17 00:00:00 2001 From: roleengineer Date: Sun, 24 Nov 2024 20:46:12 +0100 Subject: [PATCH 6/9] (test/proxy): proxy admin delegatecall and test receive --- ...dminOperationsUpgradeableRenounceableProxy.t.sol | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/test/groups/upgradeableProxy/adminOperationsUpgradeableRenounceableProxy.t.sol b/test/groups/upgradeableProxy/adminOperationsUpgradeableRenounceableProxy.t.sol index 53fce02..8da5e77 100644 --- a/test/groups/upgradeableProxy/adminOperationsUpgradeableRenounceableProxy.t.sol +++ b/test/groups/upgradeableProxy/adminOperationsUpgradeableRenounceableProxy.t.sol @@ -190,6 +190,11 @@ contract adminOperationsUpgradeableRenounceableProxy is Test, GroupSetup { IMockMintPolicyExtended(address(proxy)).setProxyAdmin(address(this)); vm.expectRevert(); IMockMintPolicyExtended(address(proxy)).setProxyImplementation(address(this), ""); + + // proxy admin should be able to make delegate call to implementation + vm.prank(group); + IMockMintPolicyExtended(address(proxy)).setWhitelisted(address(this), true); + assertTrue(IMockMintPolicyExtended(address(proxy)).isWhitelisted(address(this))); } // External renounceUpgradeability() @@ -266,6 +271,14 @@ contract adminOperationsUpgradeableRenounceableProxy is Test, GroupSetup { vm.stopPrank(); } + // External receive() + + function testReceive(address anyAddress) public { + vm.deal(anyAddress, 1 ether); + vm.expectRevert(UpgradeableRenounceableProxy.BlockReceive.selector); + address(proxy).call{value: 1 ether}(""); + } + // Internal functions // todo: this is a duplicate; test helpers can be better structured From 912e34342353f4d935d26df767225cba26368af0 Mon Sep 17 00:00:00 2001 From: roleengineer Date: Sun, 24 Nov 2024 21:35:03 +0100 Subject: [PATCH 7/9] (test/proxy): 2 failed tests: anyone can change proxy admin and implementation --- ...erationsUpgradeableRenounceableProxy.t.sol | 62 +++++++++++++++++-- 1 file changed, 58 insertions(+), 4 deletions(-) diff --git a/test/groups/upgradeableProxy/adminOperationsUpgradeableRenounceableProxy.t.sol b/test/groups/upgradeableProxy/adminOperationsUpgradeableRenounceableProxy.t.sol index 8da5e77..6a2f13b 100644 --- a/test/groups/upgradeableProxy/adminOperationsUpgradeableRenounceableProxy.t.sol +++ b/test/groups/upgradeableProxy/adminOperationsUpgradeableRenounceableProxy.t.sol @@ -97,8 +97,8 @@ contract adminOperationsUpgradeableRenounceableProxy is Test, GroupSetup { } /* todo: - test getting admin from proxy (DONE) - * - test admin cannot be changed - * - test noone else can call upgradeToAndCall + * - test admin cannot be changed (FAILED) + * - test noone else can call upgradeToAndCall (FAILED) * - test upgradeToAndCall with call data (DONE) * - test renouncing admin (DONE) * - test accessibility of interface functions from non-Admin callers (DONE) @@ -276,7 +276,53 @@ contract adminOperationsUpgradeableRenounceableProxy is Test, GroupSetup { function testReceive(address anyAddress) public { vm.deal(anyAddress, 1 ether); vm.expectRevert(UpgradeableRenounceableProxy.BlockReceive.selector); - address(proxy).call{value: 1 ether}(""); + (bool success,) = address(proxy).call{value: 1 ether}(""); + console2.log(success); + } + + // Test admin cannot be changed + + // failed test demonstrates that admin can be changed to any address, however it doesn't make sense + // since newAdmin != ADMIN_INIT. it make sense only when upgradeability is renounced to revert this action. + function testFail_AdminCannotBeChanged(address newAdmin) public { + vm.assume(newAdmin != group); + // upgrade to mock mint policy extended + _upgradeToAndCall(mockMintPolicyExtended, _defaultMockExtendedData(whitelistAdmin)); + + address proxyAdmin = _readProxyAdminSlot(); + assertEq(proxyAdmin, group); + + // should not change admin + vm.prank(whitelistAdmin); + // vm.expectRevert(); + IMockMintPolicyExtended(address(proxy)).setProxyAdmin(newAdmin); + + proxyAdmin = _readProxyAdminSlot(); + assertTrue(proxyAdmin != newAdmin); + } + + // Test noone else can call upgradeToAndCall + + // failed test demonstrates that implementation can be changed by any address (everyone can call upgradeToAndCall) + function testFail_NonAdminCannotCallUpgradeToAndCall(address anyAddress) public { + vm.assume(anyAddress != group); + // upgrade to mock mint policy extended + _upgradeToAndCall(mockMintPolicyExtended, _defaultMockExtendedData(whitelistAdmin)); + + address implementation = _readImplementationSlot(); + assertEq(implementation, mockMintPolicyExtended); + + // make any address whitelist admin + vm.prank(whitelistAdmin); + IMockMintPolicyExtended(address(proxy)).changeWhitelistAdmin(anyAddress); + + // any address should not be able to call upgradeToAndCall + vm.prank(anyAddress); + // vm.expectRevert(); + IMockMintPolicyExtended(address(proxy)).setProxyImplementation(mintPolicy, ""); + + implementation = _readImplementationSlot(); + assertTrue(implementation != mintPolicy); } // Internal functions @@ -301,7 +347,7 @@ contract adminOperationsUpgradeableRenounceableProxy is Test, GroupSetup { assertEq(balanceAfter, balanceBefore + _amount); } - // @dev makes admin (set to group) upgradeToAndCall call until admin is not renounced + /// @dev makes admin (set to group) upgradeToAndCall call until admin is not renounced function _upgradeToAndCall(address newImplementation, bytes memory data) internal { // upgrade the proxy to the new implementation _expectEmitUpgradedEvent(newImplementation); @@ -311,6 +357,14 @@ contract adminOperationsUpgradeableRenounceableProxy is Test, GroupSetup { assertEq(newImplementation, proxy.implementation(), "upgrade to new implementation failed"); } + /// @dev returns encoded data for mock mint policy extended initialization + function _defaultMockExtendedData(address _whitelistAdmin) internal pure returns (bytes memory data) { + bytes4 initializeSelector = bytes4(keccak256("initialize(address,address[])")); + address[] memory emptyList; + // encode whitelist admin and empty initial list + data = abi.encodeWithSelector(initializeSelector, _whitelistAdmin, emptyList); + } + /// @dev should emit Upgraded event function _expectEmitUpgradedEvent(address newImplementation) internal { vm.expectEmit(true, true, true, true); From f4ae7e7ce30536882884e21d7598d86afc41e4bd Mon Sep 17 00:00:00 2001 From: Yevgeniy <35062472+roleengineer@users.noreply.github.com> Date: Mon, 25 Nov 2024 15:09:23 +0100 Subject: [PATCH 8/9] Update test/groups/upgradeableProxy/adminOperationsUpgradeableRenounceableProxy.t.sol improve comment clarity for internal function _upgradeToAndCall --- .../adminOperationsUpgradeableRenounceableProxy.t.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/groups/upgradeableProxy/adminOperationsUpgradeableRenounceableProxy.t.sol b/test/groups/upgradeableProxy/adminOperationsUpgradeableRenounceableProxy.t.sol index 6a2f13b..bbab458 100644 --- a/test/groups/upgradeableProxy/adminOperationsUpgradeableRenounceableProxy.t.sol +++ b/test/groups/upgradeableProxy/adminOperationsUpgradeableRenounceableProxy.t.sol @@ -347,7 +347,7 @@ contract adminOperationsUpgradeableRenounceableProxy is Test, GroupSetup { assertEq(balanceAfter, balanceBefore + _amount); } - /// @dev makes admin (set to group) upgradeToAndCall call until admin is not renounced + /// @dev calls the upgradeToAndCall function by the proxy admin (works until admin is renounced) function _upgradeToAndCall(address newImplementation, bytes memory data) internal { // upgrade the proxy to the new implementation _expectEmitUpgradedEvent(newImplementation); From 3bcaba68ec16d3b85659ee05b44fa41c1c20d64b Mon Sep 17 00:00:00 2001 From: Yevgeniy <35062472+roleengineer@users.noreply.github.com> Date: Mon, 25 Nov 2024 15:12:02 +0100 Subject: [PATCH 9/9] Update test/groups/upgradeableProxy/adminOperationsUpgradeableRenounceableProxy.t.sol fix missed vm.prank --- .../adminOperationsUpgradeableRenounceableProxy.t.sol | 1 + 1 file changed, 1 insertion(+) diff --git a/test/groups/upgradeableProxy/adminOperationsUpgradeableRenounceableProxy.t.sol b/test/groups/upgradeableProxy/adminOperationsUpgradeableRenounceableProxy.t.sol index bbab458..75b175f 100644 --- a/test/groups/upgradeableProxy/adminOperationsUpgradeableRenounceableProxy.t.sol +++ b/test/groups/upgradeableProxy/adminOperationsUpgradeableRenounceableProxy.t.sol @@ -276,6 +276,7 @@ contract adminOperationsUpgradeableRenounceableProxy is Test, GroupSetup { function testReceive(address anyAddress) public { vm.deal(anyAddress, 1 ether); vm.expectRevert(UpgradeableRenounceableProxy.BlockReceive.selector); + vm.prank(anyAddress); (bool success,) = address(proxy).call{value: 1 ether}(""); console2.log(success); }