Skip to content

Complete unit test coverage for upgradeable renouncable proxy #112

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
Nov 25, 2024
Merged

Complete unit test coverage for upgradeable renouncable proxy #112

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
0ec31d9
(test/proxy): upgradeToAndCall unit test coverage
roleengineer Nov 20, 2024
ea76981
(test/proxy): renounceUpgradeability, implementation covered for admi…
roleengineer Nov 22, 2024
5c5b135
(test/proxy): added helper internal function _readImplementationSlot
roleengineer Nov 23, 2024
e6f858f
(test/proxy): implemented mock mint policy extended
roleengineer Nov 24, 2024
3ed7e67
(test/proxy): test upgradeToAndCall with calldata
roleengineer Nov 24, 2024
4936c10
(test/proxy): proxy admin delegatecall and test receive
roleengineer Nov 24, 2024
912e343
(test/proxy): 2 failed tests: anyone can change proxy admin and imple…
roleengineer Nov 24, 2024
f4ae7e7
Update test/groups/upgradeableProxy/adminOperationsUpgradeableRenounc…
roleengineer Nov 25, 2024
3bcaba6
Update test/groups/upgradeableProxy/adminOperationsUpgradeableRenounc…
roleengineer Nov 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
309 changes: 287 additions & 22 deletions test/groups/upgradeableProxy/adminOperationsUpgradeableRenounceableProxy.t.sol
View file
Open in desktop
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";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't believe I never realised using an absolute path is so much more readable and better. Internal style-guide updated !

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
Loading