Skip to content

Commit

Permalink
Merge pull request #112 from aboutcircles/v1.0.1-patch2-upgradable-gr…
Browse files Browse the repository at this point in the history 
…oup-policies

Complete unit test coverage for upgradeable renouncable proxy
  • Loading branch information
@benjaminbollen
benjaminbollen authored Nov 25, 2024
2 parents 8444981 + 3bcaba6 commit 097c254
Show file tree
Hide file tree
Showing 3 changed files with 438 additions and 22 deletions.
309 changes: 287 additions & 22 deletions test/groups/upgradeableProxy/adminOperationsUpgradeableRenounceableProxy.t.sol
View file
Original file line number Diff line number Diff line change
@@ -1,23 +1,37 @@
// 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 "../../../src/groups/UpgradeableRenounceableProxy.sol";
import "../../../src/errors/Errors.sol";
import "../groupSetup.sol";
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";
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

bytes32 internal constant ADMIN_SLOT = 0xb53127684a568b3173ae13b9f8a6016e243e63b6e8ee1178d6a717850b5d6103;
bytes32 internal constant IMPLEMENTATION_SLOT = 0x360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc;

// State variables

address public group;
IUpgradeableRenounceableProxy public proxy;
// copy of BaseMintPolicy
address public newMintPolicy;
address public mockMintPolicyWithSelectorClashes;
address public mockMintPolicyExtended;
address public whitelistAdmin;

// Constructor

Expand All @@ -43,31 +57,65 @@ 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());

// 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

function testGetImplementation() public {
// External implementation() returns(address)

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)

// let's upgrade to implementation with selector clashes
_upgradeToAndCall(mockMintPolicyWithSelectorClashes, "");

vm.prank(anyCaller);
implementation = proxy.implementation();
// should not return the mockPolicy value
assertTrue(
implementation != IMockMintPolicyWithSelectorClashes(mockMintPolicyWithSelectorClashes).implementation()
);
// should return the current implementation
assertEq(implementation, mockMintPolicyWithSelectorClashes);
assertEq(implementation, _readImplementationSlot());
}

/* todo: - test getting admin from proxy
* - test admin cannot be changed
* - test noone else can call upgradeToAndCall
* - test upgradeToAndCall with call data
/* todo: - test getting admin from proxy (DONE)
* - 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
* - test accessibility of interface functions from non-Admin callers (DONE)
*/

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
// despite the fact that current implementation has upgradeToAndCall function with different logic
_expectEmitUpgradedEvent(newMintPolicy);
vm.prank(group);
proxy.upgradeToAndCall(newMintPolicy, "");

Expand All @@ -79,24 +127,203 @@ contract adminOperationsUpgradeableRenounceableProxy is Test, GroupSetup {
_testGroupMintOwnCollateral(addresses[0], group, 1 * CRC);
}

function testRenounceAdmin() public {
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 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);
// 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), "");

// 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()

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
_expectEmitAdminChangedEvent(group, address(0x1));
vm.prank(group);
proxy.renounceUpgradeability();

// renounced admin
admin = address(uint160(uint256(vm.load(address(proxy), ADMIN_SLOT))));
admin = _readProxyAdminSlot();
assertEq(admin, address(0x1));
}

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

// expect revert when trying to upgrade to implementation 0xdead
// 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();
}

// External receive()

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);
}

// 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
Expand All @@ -120,4 +347,42 @@ contract adminOperationsUpgradeableRenounceableProxy is Test, GroupSetup {
uint256 balanceAfter = hub.balanceOf(_minter, tokenIdGroup);
assertEq(balanceAfter, balanceBefore + _amount);
}

/// @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);
vm.prank(group);
proxy.upgradeToAndCall(newImplementation, data);
// should be new implementation
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);
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))));
}

function _readImplementationSlot() internal view returns (address implementation) {
implementation = address(uint160(uint256(vm.load(address(proxy), IMPLEMENTATION_SLOT))));
}
}
Loading

0 comments on commit 097c254

Please sign in to comment.