Skip to content

Commit 67a4450

Browse files
nicholaspaimrice32
nicholaspai
and
mrice32
authored
feat: Add PermissionSplitter test script scaffolding (#411)
* Create PermissionSplitter.sol * add permission splitter examples Signed-off-by: nicholaspai <npai.nyc@gmail.com> * Update PermissionSplitter.sol * Update PermissionSplitter.sol * WIP Signed-off-by: Matt Rice <matthewcrice32@gmail.com> * WIP Signed-off-by: Matt Rice <matthewcrice32@gmail.com> * Add tests Signed-off-by: Matt Rice <matthewcrice32@gmail.com> * WIP Signed-off-by: Matt Rice <matthewcrice32@gmail.com> * WIP Signed-off-by: Matt Rice <matthewcrice32@gmail.com> * add events Signed-off-by: Matt Rice <matthewcrice32@gmail.com> * wip foundry tests Signed-off-by: nicholaspai <npai.nyc@gmail.com> * Update PermissionSplitter.t.sol * Delete forge-std * Revert "Delete forge-std" This reverts commit af8716b. * Remove hardhat-foundry * add test * Update PermissionSplitter.t.sol * Respond to matt comments * add more unit tests: transferring ownership back to default admin, more robust fuzz testing of fallback()/receive() Signed-off-by: nicholaspai <npai.nyc@gmail.com> * Update PermissionSplitter.t.sol --------- Signed-off-by: nicholaspai <npai.nyc@gmail.com> Signed-off-by: Matt Rice <matthewcrice32@gmail.com> Co-authored-by: Matt Rice <matthewcrice32@gmail.com>
1 parent d17fa59 commit 67a4450

File tree

7 files changed

+293
-4
lines changed

7 files changed

+293
-4
lines changed

‎.eslintignore‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,3 +6,4 @@ gasReporterOutput.json
66
typechain
77
artifacts-zk
88
cache-zk
9+
lib

‎.prettierignore‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,3 +7,4 @@ typechain
77
dist
88
artifacts-zk
99
cache-zk
10+
lib

‎.solhintignore‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1 +1,2 @@
11
node_modules
2+
lib

‎contracts/PermissionSplitterProxy.sol‎

Lines changed: 24 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,11 @@ pragma solidity ^0.8.0;
44
import "@uma/core/contracts/common/implementation/MultiCaller.sol";
55
import "@openzeppelin/contracts/access/AccessControl.sol";
66

7+
/**
8+
* @notice This contract is designed to own an Ownable "target" contract and gate access to specific
9+
* function selectors based on specific roles. Practically, this contract should own the HubPool contract.
10+
* All ownable function calls to target should be sent through this contract's fallback function.
11+
*/
712
contract PermissionSplitterProxy is AccessControl, MultiCaller {
813
// Inherited admin role from AccessControl. Should be assigned to Across DAO Safe.
914
// bytes32 public constant DEFAULT_ADMIN_ROLE = 0x00;
@@ -12,24 +17,39 @@ contract PermissionSplitterProxy is AccessControl, MultiCaller {
1217
// only role holders.
1318
mapping(bytes4 => bytes32) public roleForSelector;
1419

20+
// The contract that should be owned by this contract and whose function selectors will be gated
21+
// by roleForSelector.
1522
address public target;
1623

1724
event TargetUpdated(address indexed newTarget);
1825
event RoleForSelectorSet(bytes4 indexed selector, bytes32 indexed role);
1926

27+
/**
28+
* @notice constructs PermissionSplitterProxy
29+
*/
2030
constructor(address _target) {
2131
_init(_target);
2232
}
2333

24-
// Public function!
25-
// Note: these have two underscores in front to prevent any collisions with the target contract.
34+
/**
35+
* @notice Change target contract.
36+
* @dev Public function! This has two underscores in front to prevent any collisions with the target contract.
37+
* @dev Only callable by default admin role holder.
38+
* @param _target New target contract.
39+
*/
2640
function __setTarget(address _target) public onlyRole(DEFAULT_ADMIN_ROLE) {
2741
target = _target;
2842
emit TargetUpdated(_target);
2943
}
3044

31-
// Public function!
32-
// Note: these have two underscores in front to prevent any collisions with the target contract.
45+
/**
46+
* @notice Change role ID for function selector. Only role holder can call function selector
47+
* on target.
48+
* @dev Public function! This has two underscores in front to prevent any collisions with the target contract.
49+
* @dev Only callable by default admin role holder.
50+
* @param selector Function selector to map to role.
51+
* @param role Only this role holder can call function selector on the target
52+
*/
3353
function __setRoleForSelector(bytes4 selector, bytes32 role) public onlyRole(DEFAULT_ADMIN_ROLE) {
3454
roleForSelector[selector] = role;
3555
emit RoleForSelectorSet(selector, role);

‎foundry.toml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
[profile.default]
22
src = "contracts"
33
out = "artifacts"
4+
test = "test/foundry"
45
libs = ["node_modules", "lib"]
56
remappings = [
67
"@across-protocol/=node_modules/@across-protocol/",

‎lib/forge-std‎

Submodule forge-std added at 066ff16

‎test/foundry/PermissionSplitter.t.sol‎

Lines changed: 264 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,264 @@
1+
// SPDX-License-Identifier: BUSL-1.1
2+
pragma solidity ^0.8.0;
3+
4+
import { Test } from "forge-std/Test.sol";
5+
import "forge-std/console.sol";
6+
7+
import { HubPool } from "../../contracts/HubPool.sol";
8+
import { SpokePool } from "../../contracts/SpokePool.sol";
9+
import { LpTokenFactory } from "../../contracts/LpTokenFactory.sol";
10+
import { PermissionSplitterProxy } from "../../contracts/PermissionSplitterProxy.sol";
11+
12+
// Run this test to verify PermissionSplitter behavior when changing ownership of the HubPool
13+
// to it. Therefore this test should be run as a fork test via:
14+
// - forge test --fork-url <MAINNET-RPC-URL>
15+
contract PermissionSplitterTest is Test {
16+
HubPool hubPool;
17+
HubPool hubPoolProxy;
18+
SpokePool ethereumSpokePool;
19+
PermissionSplitterProxy permissionSplitter;
20+
21+
// defaultAdmin is the deployer of the PermissionSplitter and has authority
22+
// to call any function on the HubPool. Therefore this should be a highly secure
23+
// contract account such as a MultiSig contract.
24+
address defaultAdmin;
25+
// Pause admin should only be allowed to pause the HubPool.
26+
address pauseAdmin;
27+
address rando1;
28+
address rando2;
29+
30+
// We test the pause() selector specifically in the tests so we define it as a variable.
31+
bytes4 constant PAUSE_SELECTOR = bytes4(keccak256("setPaused(bool)"));
32+
bytes32 constant PAUSE_ROLE = keccak256("PAUSE_ROLE");
33+
34+
// We hardcode all of the contract function selectors so we can stress test with possible function selectors and
35+
// receive()/fallback() behavior when the proxy is called with random func selectors that it doesn't have.
36+
bytes4[] hubPoolSelectors = [
37+
PAUSE_SELECTOR,
38+
bytes4(keccak256("emergencyDeleteProposal()")),
39+
bytes4(keccak256("relaySpokePoolAdminFunction(uint256,bytes)")),
40+
bytes4(keccak256("setProtocolFeeCapture(address,uint256)")),
41+
bytes4(keccak256("setBond(address,uint256)")),
42+
bytes4(keccak256("setLiveness(uint32)")),
43+
bytes4(keccak256("setIdentifier(bytes32)")),
44+
bytes4(keccak256("setCrossChainContracts(uint256,address,address)")),
45+
bytes4(keccak256("setPoolRebalanceRoute(uint256,address,address)")),
46+
bytes4(keccak256("setDepositRoute(uint256,uint256,address,bool)")),
47+
bytes4(keccak256("enableL1TokenForLiquidityProvision(address)")),
48+
bytes4(keccak256("disableL1TokenForLiquidityProvision(address)")),
49+
bytes4(keccak256("haircutReserves(address,int256)")),
50+
bytes4(keccak256("addLiquidity(address,int256)")),
51+
bytes4(keccak256("removeLiquidity(address,int256,bool)")),
52+
bytes4(keccak256("exchangeRateCurrent(address)")),
53+
bytes4(keccak256("liquidityUtilizationCurrent(address)")),
54+
bytes4(keccak256("liquidityUtilizationPostRelay(address,uint256)")),
55+
bytes4(keccak256("sync(address)")),
56+
bytes4(keccak256("proposeRootBundle(uint256[],uint8,bytes32,bytes32,bytes32)")),
57+
bytes4(keccak256("executeRootBundle(uint256,uint256,uint256[],int256[],int256[],uint8,address[],bytes32[])")),
58+
bytes4(keccak256("disputeRootBundle()")),
59+
bytes4(keccak256("claimProtocolFeesCaptured(address)")),
60+
bytes4(keccak256("loadEthForL2Calls()")),
61+
bytes4(keccak256("rootBundleProposal()")),
62+
bytes4(keccak256("pooledTokens(address)")),
63+
bytes4(keccak256("crossChainContracts(uint256)")),
64+
bytes4(keccak256("unclaimedAccumulatedProtocolFees(address)")),
65+
bytes4(keccak256("paused()")),
66+
bytes4(keccak256("protocolFeeCaptureAddress()")),
67+
bytes4(keccak256("bondToken()")),
68+
bytes4(keccak256("liveness()")),
69+
bytes4(keccak256("identifier()")),
70+
bytes4(keccak256("lpFeeRatePerSecond()")),
71+
bytes4(keccak256("protocolFeeCapturePct()")),
72+
bytes4(keccak256("bondAmount()")),
73+
bytes4(keccak256("poolRebalanceRoute(uint256,address)")),
74+
bytes4(keccak256("setCurrentTime(uint256)")),
75+
bytes4(keccak256("getCurrentTime()")),
76+
bytes4(keccak256("timerAddress()")),
77+
bytes4(keccak256("multicall(bytes4[])")),
78+
bytes4(keccak256("owner()")),
79+
bytes4(keccak256("renounceOwnership()")),
80+
bytes4(keccak256("transferOwnership(address)"))
81+
];
82+
bytes4[] proxySelectors = [
83+
bytes4(keccak256("__setRoleForSelector(bytes4,bytes32)")),
84+
bytes4(keccak256("__setTarget(address)")),
85+
bytes4(keccak256("target()")),
86+
bytes4(keccak256("roleForSelector(bytes4)")),
87+
bytes4(keccak256("supportsInterface(bytes4)")),
88+
bytes4(keccak256("hasRole(bytes32,address)")),
89+
bytes4(keccak256("getRoleAdmin(bytes32)")),
90+
bytes4(keccak256("grantRole(bytes32,address)")),
91+
bytes4(keccak256("revokeRole(bytes32,address)")),
92+
bytes4(keccak256("renounceRole(bytes32,address)"))
93+
];
94+
95+
// Error emitted when non-owner calls onlyOwner HubPool function.
96+
bytes constant OWNABLE_NOT_OWNER_ERROR = bytes("Ownable: caller is not the owner");
97+
// Error emitted when calling PermissionSplitterProxy function with incorrect role.
98+
bytes constant PROXY_NOT_ALLOWED_TO_CALL_ERROR = bytes("Not allowed to call");
99+
100+
address constant WETHAddress = 0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2;
101+
102+
error FuncSelectorCollision();
103+
104+
function setUp() public {
105+
// Since this test file is designed to run against a mainnet fork, hardcode the following system
106+
// contracts to skip the setup we'd usually need to run to use brand new contracts.
107+
hubPool = HubPool(payable(0xc186fA914353c44b2E33eBE05f21846F1048bEda));
108+
ethereumSpokePool = SpokePool(payable(0x5c7BCd6E7De5423a257D81B442095A1a6ced35C5));
109+
110+
// For the purposes of this test, the default admin will be the current owner of the
111+
// HubPool, which we can assume is a highly secured account.
112+
defaultAdmin = hubPool.owner();
113+
pauseAdmin = vm.addr(1);
114+
rando1 = vm.addr(2);
115+
rando2 = vm.addr(3);
116+
117+
// Deploy PermissionSplitter from default admin account and then
118+
// create and assign roles.
119+
vm.startPrank(defaultAdmin);
120+
// Default admin can call any ownable function, which no one else can call without
121+
// the correct role.
122+
permissionSplitter = new PermissionSplitterProxy(address(hubPool));
123+
permissionSplitter.grantRole(PAUSE_ROLE, pauseAdmin);
124+
// Grant anyone with the pause role the ability to call setPaused
125+
permissionSplitter.__setRoleForSelector(PAUSE_SELECTOR, PAUSE_ROLE);
126+
vm.stopPrank();
127+
128+
vm.prank(defaultAdmin);
129+
hubPool.transferOwnership(address(permissionSplitter));
130+
hubPoolProxy = HubPool(payable(permissionSplitter));
131+
}
132+
133+
function testPause() public {
134+
// Calling HubPool setPaused directly should fail, even if called by previous owner.
135+
vm.startPrank(defaultAdmin);
136+
vm.expectRevert(OWNABLE_NOT_OWNER_ERROR);
137+
hubPool.setPaused(true);
138+
vm.stopPrank();
139+
140+
// Must call HubPool via PermissionSplitterProxy.
141+
vm.prank(pauseAdmin);
142+
hubPoolProxy.setPaused(true);
143+
assertTrue(hubPool.paused());
144+
145+
// Can also call Proxy function via default admin.
146+
vm.prank(defaultAdmin);
147+
hubPoolProxy.setPaused(false);
148+
assertFalse(hubPool.paused());
149+
150+
// Multiple EOA's can be granted the pause role.
151+
vm.startPrank(defaultAdmin);
152+
permissionSplitter.grantRole(PAUSE_ROLE, rando1);
153+
permissionSplitter.grantRole(PAUSE_ROLE, rando2);
154+
vm.stopPrank();
155+
vm.prank(rando1);
156+
hubPoolProxy.setPaused(true);
157+
assertTrue(hubPool.paused());
158+
vm.prank(rando2);
159+
hubPoolProxy.setPaused(false);
160+
assertFalse(hubPool.paused());
161+
}
162+
163+
function testCallSpokePoolFunction() public {
164+
bytes32 fakeRoot = keccak256("new admin root");
165+
bytes memory spokeFunctionCallData = abi.encodeWithSignature(
166+
"relayRootBundle(bytes32,bytes32)",
167+
fakeRoot,
168+
fakeRoot
169+
);
170+
uint256 spokeChainId = 1;
171+
172+
vm.expectRevert(PROXY_NOT_ALLOWED_TO_CALL_ERROR);
173+
hubPoolProxy.relaySpokePoolAdminFunction(spokeChainId, spokeFunctionCallData);
174+
vm.expectRevert(OWNABLE_NOT_OWNER_ERROR);
175+
hubPool.relaySpokePoolAdminFunction(spokeChainId, spokeFunctionCallData);
176+
177+
vm.startPrank(defaultAdmin);
178+
vm.expectCall(address(ethereumSpokePool), spokeFunctionCallData);
179+
hubPoolProxy.relaySpokePoolAdminFunction(spokeChainId, spokeFunctionCallData);
180+
vm.stopPrank();
181+
}
182+
183+
function testTransferOwnership() public {
184+
vm.expectRevert(PROXY_NOT_ALLOWED_TO_CALL_ERROR);
185+
hubPoolProxy.transferOwnership(defaultAdmin);
186+
187+
// Should be able to transfer ownership back to default admin in an emergency.
188+
vm.startPrank(defaultAdmin);
189+
hubPoolProxy.transferOwnership(defaultAdmin);
190+
assertEq(hubPool.owner(), defaultAdmin);
191+
192+
hubPool.transferOwnership(address(permissionSplitter));
193+
assertEq(hubPool.owner(), address(permissionSplitter));
194+
}
195+
196+
/// forge-config: default.fuzz.runs = 300
197+
function testFallback(bytes4 randomFuncSelector, bytes memory randomCalldata) public {
198+
// random funcSelector doesn't collide with hub pool:
199+
for (uint256 i = 0; i < hubPoolSelectors.length; i++) {
200+
if (hubPoolSelectors[i] == randomFuncSelector) vm.assume(false);
201+
}
202+
// random funcSelector doesn't collide with proxy:
203+
for (uint256 i = 0; i < proxySelectors.length; i++) {
204+
if (proxySelectors[i] == randomFuncSelector) vm.assume(false);
205+
}
206+
207+
// Calling a function that doesn't exist on target or PermissionSplitter calls the HubPool's
208+
// fallback function which wraps any msg.value into wrapped native token.
209+
uint256 balBefore = address(hubPool).balance;
210+
211+
// Calling fake function as admin with no value succeeds and does nothing.
212+
vm.prank(defaultAdmin);
213+
(bool success1, ) = address(hubPoolProxy).call(abi.encodeWithSelector(randomFuncSelector, randomCalldata));
214+
assertTrue(success1);
215+
216+
// Calling fake function as admin with value also succeeds and wraps the msg.value
217+
// and then does nothing.
218+
vm.deal(defaultAdmin, 1 ether);
219+
vm.prank(defaultAdmin);
220+
(bool success2, ) = address(hubPoolProxy).call{ value: 1 ether }(
221+
abi.encodeWithSelector(randomFuncSelector, randomCalldata)
222+
);
223+
assertTrue(success2);
224+
assertEq(address(hubPool).balance, balBefore);
225+
// Proxy shouldn't hold any ETH
226+
assertEq(address(hubPoolProxy).balance, 0);
227+
}
228+
229+
function testReceive() public {
230+
// Sending ETH to the proxy should trigger the target's receive() function, which on the HubPool wraps
231+
// any msg.value into wrapped native token.
232+
uint256 balBefore = address(hubPool).balance;
233+
234+
vm.deal(defaultAdmin, 1 ether);
235+
vm.prank(defaultAdmin);
236+
(bool success, ) = address(hubPoolProxy).call{ value: 1 ether }("");
237+
assertTrue(success);
238+
assertEq(address(hubPool).balance, balBefore);
239+
// Proxy shouldn't hold any ETH
240+
assertEq(address(hubPoolProxy).balance, 0);
241+
}
242+
243+
/// forge-config: default.fuzz.runs = 100
244+
function testFunctionSelectorCollisions(uint256 hubPoolFuncSelectorIdx, uint256 proxyFuncSelectorIdx) public {
245+
vm.assume(hubPoolFuncSelectorIdx < hubPoolSelectors.length);
246+
vm.assume(proxyFuncSelectorIdx < proxySelectors.length);
247+
248+
// Assert that PermissionSplitter has no function selector collisions with HubPool.
249+
// @dev Solidity compilation will fail if function selectors on the same contract collide.
250+
// - https://ethereum.stackexchange.com/a/46188/47801
251+
if (hubPoolSelectors[hubPoolFuncSelectorIdx] == proxySelectors[proxyFuncSelectorIdx])
252+
revert FuncSelectorCollision();
253+
}
254+
255+
function testCallPublicFunction() public {
256+
// Should be able to call public functions without any access modifiers via proxy as the default admin.
257+
258+
vm.prank(defaultAdmin);
259+
hubPoolProxy.sync(WETHAddress);
260+
261+
vm.expectRevert(PROXY_NOT_ALLOWED_TO_CALL_ERROR);
262+
hubPoolProxy.sync(WETHAddress);
263+
}
264+
}

0 commit comments

Comments
 (0)