From 900f4d2fccd55aaa95c4c0d6ae05b4f989b34c6b Mon Sep 17 00:00:00 2001 From: nicholaspai Date: Tue, 13 Jun 2023 20:27:33 -0400 Subject: [PATCH 01/20] Create PermissionSplitter.sol --- contracts/PermissionSplitter.sol | 48 ++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+) create mode 100644 contracts/PermissionSplitter.sol diff --git a/contracts/PermissionSplitter.sol b/contracts/PermissionSplitter.sol new file mode 100644 index 000000000..7e28b7d83 --- /dev/null +++ b/contracts/PermissionSplitter.sol @@ -0,0 +1,48 @@ +// SPDX-License-Identifier: AGPL-3.0-only +pragma solidity ^0.8.0; + +import "@uma/core/contracts/common/implementation/MultiCaller.sol"; +import "@openzeppelin/contracts/access/AccessControl.sol"; + +contract PermissionSplitter is AccessControl, MultiCaller { + enum ProposalStatus { + None, + Pending, + Executed + } + + struct PermissionsTier { + bytes32 role; + uint32 liveness; // The minimum time between a proposal being made and executed. + uint8 minApprovals; // i.e. quorum amongst role members required to execute anything. + uint8 tier; + } + + struct Proposal { + address target; + bytes32 functionSig; + bytes params; + ProposalStatus status; + } + + // UUID for proposals. + uint256 nonce; + + // Tracks proposed proposals that passed quorum and can be executed after liveness. + mapping(uint256 => Proposal) public proposals; + + // Maps function signatures to permission tiers, making it convenient to change params en masse to different + // functions. + mapping(bytes32 => uint8) public permissionTierForFunctionSig; + + // Maps permission tiers to their configuration. + mapping(uint8 => PermissionsTier) public permissionsTiers; + + // General dictionary where admin can associate variables with specific L1 tokens, like the Rate Model and Token + // Transfer Thresholds. + mapping(address => string) public l1TokenConfig; + + // General dictionary where admin can store global variables like `MAX_POOL_REBALANCE_LEAF_SIZE` and + // `MAX_RELAYER_REPAYMENT_LEAF_SIZE` that off-chain agents can query. + mapping(bytes32 => string) public globalConfig; +} From 6a7b7030f54762daf9edfdeba6f4b50108f486d4 Mon Sep 17 00:00:00 2001 From: nicholaspai Date: Wed, 14 Jun 2023 10:47:11 -0400 Subject: [PATCH 02/20] add permission splitter examples Signed-off-by: nicholaspai --- contracts/PermissionSplitter.sol | 46 ++++++++++++++++++++++++-------- 1 file changed, 35 insertions(+), 11 deletions(-) diff --git a/contracts/PermissionSplitter.sol b/contracts/PermissionSplitter.sol index 7e28b7d83..30af9f1b9 100644 --- a/contracts/PermissionSplitter.sol +++ b/contracts/PermissionSplitter.sol @@ -12,37 +12,61 @@ contract PermissionSplitter is AccessControl, MultiCaller { } struct PermissionsTier { - bytes32 role; uint32 liveness; // The minimum time between a proposal being made and executed. - uint8 minApprovals; // i.e. quorum amongst role members required to execute anything. uint8 tier; } struct Proposal { address target; bytes32 functionSig; - bytes params; + bytes functionCallData; ProposalStatus status; + uint256 proposalTime; } // UUID for proposals. - uint256 nonce; + uint256 public nonce; // Tracks proposed proposals that passed quorum and can be executed after liveness. mapping(uint256 => Proposal) public proposals; - // Maps function signatures to permission tiers, making it convenient to change params en masse to different + // Maps function signatures to permission tiers, making it convenient to batch change params for multiple // functions. mapping(bytes32 => uint8) public permissionTierForFunctionSig; // Maps permission tiers to their configuration. mapping(uint8 => PermissionsTier) public permissionsTiers; - // General dictionary where admin can associate variables with specific L1 tokens, like the Rate Model and Token - // Transfer Thresholds. - mapping(address => string) public l1TokenConfig; + function setPermissionTier(uint8 tier, uint32 liveness) external onlyRole(DEFAULT_ADMIN_ROLE) { + permissionsTiers[tier] = PermissionsTier({ liveness: liveness, tier: tier }); + } + + // Only callable by someone who has correct role for the function sig they want to call. + function proposeAction( + bytes32 functionSig, + bytes memory functionCallData, + address target + ) external onlyRole(functionSig) { + proposals[nonce] = Proposal({ + target: target, + functionSig: functionSig, + functionCallData: functionCallData, + status: ProposalStatus.Pending, + proposalTime: block.timestamp + }); + nonce++; + } - // General dictionary where admin can store global variables like `MAX_POOL_REBALANCE_LEAF_SIZE` and - // `MAX_RELAYER_REPAYMENT_LEAF_SIZE` that off-chain agents can query. - mapping(bytes32 => string) public globalConfig; + // Can execute pending proposal after liveness period has elapsed. + function executeAction(uint256 proposalId) external { + // Need to handle fact that livenesses can change after a proposal is pending... + uint256 livenessForProposal = permissionsTiers[permissionTierForFunctionSig[proposals[proposalId].functionSig]] + .liveness; + require( + block.timestamp >= proposals[proposalId].proposalTime + livenessForProposal, + "Cannot execute proposal until liveness period has elapsed." + ); + proposals[nonce].status = ProposalStatus.Executed; + // Execute the action... + } } From 079eb785167b1d7f48eb5738ca432495d36b0e92 Mon Sep 17 00:00:00 2001 From: nicholaspai Date: Wed, 14 Jun 2023 11:44:30 -0400 Subject: [PATCH 03/20] Update PermissionSplitter.sol --- contracts/PermissionSplitter.sol | 2 ++ 1 file changed, 2 insertions(+) diff --git a/contracts/PermissionSplitter.sol b/contracts/PermissionSplitter.sol index 30af9f1b9..e61a02ecc 100644 --- a/contracts/PermissionSplitter.sol +++ b/contracts/PermissionSplitter.sol @@ -32,6 +32,8 @@ contract PermissionSplitter is AccessControl, MultiCaller { // Maps function signatures to permission tiers, making it convenient to batch change params for multiple // functions. + // Assumptions: Each function signature is unique, therefore the function signature itself can be used + // as the AccessControl.Role identifier. mapping(bytes32 => uint8) public permissionTierForFunctionSig; // Maps permission tiers to their configuration. From f308f6f2d42e7e810b3222a02ef810c7ff314aa4 Mon Sep 17 00:00:00 2001 From: nicholaspai Date: Fri, 16 Jun 2023 15:14:19 -0400 Subject: [PATCH 04/20] Update PermissionSplitter.sol --- contracts/PermissionSplitter.sol | 82 ++++++++------------------------ 1 file changed, 21 insertions(+), 61 deletions(-) diff --git a/contracts/PermissionSplitter.sol b/contracts/PermissionSplitter.sol index e61a02ecc..7d65454f3 100644 --- a/contracts/PermissionSplitter.sol +++ b/contracts/PermissionSplitter.sol @@ -4,71 +4,31 @@ pragma solidity ^0.8.0; import "@uma/core/contracts/common/implementation/MultiCaller.sol"; import "@openzeppelin/contracts/access/AccessControl.sol"; +// TODO: Make upgradeable. contract PermissionSplitter is AccessControl, MultiCaller { - enum ProposalStatus { - None, - Pending, - Executed - } - - struct PermissionsTier { - uint32 liveness; // The minimum time between a proposal being made and executed. - uint8 tier; - } - - struct Proposal { - address target; - bytes32 functionSig; - bytes functionCallData; - ProposalStatus status; - uint256 proposalTime; - } - - // UUID for proposals. - uint256 public nonce; + // Inherited admin role from AccessControl. Should be assigned to Across DAO multisig + // bytes32 public constant DEFAULT_ADMIN_ROLE = 0x00; - // Tracks proposed proposals that passed quorum and can be executed after liveness. - mapping(uint256 => Proposal) public proposals; - - // Maps function signatures to permission tiers, making it convenient to batch change params for multiple - // functions. + // Maps function signatures to role identifiers, which gatekeeps access to these functions to + // only role holders. // Assumptions: Each function signature is unique, therefore the function signature itself can be used - // as the AccessControl.Role identifier. - mapping(bytes32 => uint8) public permissionTierForFunctionSig; - - // Maps permission tiers to their configuration. - mapping(uint8 => PermissionsTier) public permissionsTiers; - - function setPermissionTier(uint8 tier, uint32 liveness) external onlyRole(DEFAULT_ADMIN_ROLE) { - permissionsTiers[tier] = PermissionsTier({ liveness: liveness, tier: tier }); - } - - // Only callable by someone who has correct role for the function sig they want to call. - function proposeAction( - bytes32 functionSig, - bytes memory functionCallData, - address target - ) external onlyRole(functionSig) { - proposals[nonce] = Proposal({ - target: target, - functionSig: functionSig, - functionCallData: functionCallData, - status: ProposalStatus.Pending, - proposalTime: block.timestamp - }); - nonce++; - } + // as the AccessControl.Role identifier. This seems like a weak assumption? + mapping(bytes4 => bytes32) public roleForFunctionSig; + // TODO: Implement this as a fallback. // Can execute pending proposal after liveness period has elapsed. - function executeAction(uint256 proposalId) external { - // Need to handle fact that livenesses can change after a proposal is pending... - uint256 livenessForProposal = permissionsTiers[permissionTierForFunctionSig[proposals[proposalId].functionSig]] - .liveness; - require( - block.timestamp >= proposals[proposalId].proposalTime + livenessForProposal, - "Cannot execute proposal until liveness period has elapsed." - ); - proposals[nonce].status = ProposalStatus.Executed; - // Execute the action... + function executeAction( + address target, + bytes4 functionSig, + bytes memory callData, + uint256 msgValue + ) external { + if (roleForFunctionSig[functionSig] == bytes32(0)) { + // pass through, no role required + } else { + require(hasRole(roleForFunctionSig[bytes32(functionSig)], msg.sender), "invalid role for function sig."); + + // execute functionSig with callData on target, optionally passing msgValue + } } } From e9d3e7efdc7126635daa53d7d940350257a8b779 Mon Sep 17 00:00:00 2001 From: Matt Rice Date: Fri, 16 Jun 2023 18:35:03 -0400 Subject: [PATCH 05/20] WIP Signed-off-by: Matt Rice --- contracts/PermissionSplitter.sol | 105 +++++++++++++++++++++++++------ 1 file changed, 87 insertions(+), 18 deletions(-) diff --git a/contracts/PermissionSplitter.sol b/contracts/PermissionSplitter.sol index 7d65454f3..d759feae4 100644 --- a/contracts/PermissionSplitter.sol +++ b/contracts/PermissionSplitter.sol @@ -5,30 +5,99 @@ import "@uma/core/contracts/common/implementation/MultiCaller.sol"; import "@openzeppelin/contracts/access/AccessControl.sol"; // TODO: Make upgradeable. -contract PermissionSplitter is AccessControl, MultiCaller { - // Inherited admin role from AccessControl. Should be assigned to Across DAO multisig +contract PermissionSplitterProxy is AccessControl, MultiCaller { + // Inherited admin role from AccessControl. Should be assigned to Across DAO Safe. // bytes32 public constant DEFAULT_ADMIN_ROLE = 0x00; // Maps function signatures to role identifiers, which gatekeeps access to these functions to // only role holders. - // Assumptions: Each function signature is unique, therefore the function signature itself can be used - // as the AccessControl.Role identifier. This seems like a weak assumption? - mapping(bytes4 => bytes32) public roleForFunctionSig; - - // TODO: Implement this as a fallback. - // Can execute pending proposal after liveness period has elapsed. - function executeAction( - address target, - bytes4 functionSig, - bytes memory callData, - uint256 msgValue - ) external { - if (roleForFunctionSig[functionSig] == bytes32(0)) { - // pass through, no role required + mapping(bytes4 => bytes32) public roleForSelector; + + address public target; + + constructor(address _target) { + _init(_target); + } + + // Public function! + // Note: these have two underscores in front to prevent any collisions with the target contract. + function __setTarget(address _target) public onlyRole(DEFAULT_ADMIN_ROLE) { + target = _target; + } + + // Public function! + // Note: these have two underscores in front to prevent any collisions with the target contract. + function __setRoleForSelector(bytes4 selector, bytes32 role) public onlyRole(DEFAULT_ADMIN_ROLE) { + roleForSelector[selector] = role; + } + + function _init(address _target) internal { + _grantRole(DEFAULT_ADMIN_ROLE, msg.sender); + target = _target; + } + + function _isCallAllowedToCall(address caller, bytes calldata callData) internal view virtual returns (bool) { + bytes4 selector; + if (callData.length < 4) { + // This handles any empty callData, which is a call to the fallback function. + selector = bytes4(0); } else { - require(hasRole(roleForFunctionSig[bytes32(functionSig)], msg.sender), "invalid role for function sig."); + selector = bytes4(callData[:4]); + } + return hasRole(DEFAULT_ADMIN_ROLE, caller) || hasRole(roleForSelector[selector], caller); + } + + /** + * @dev Forwards the current call to `implementation`. + * + * This function does not return to its internal call site, it will return directly to the external caller. + * Note: this function is a modified _delegate function here: + // https://github.com/OpenZeppelin/openzeppelin-contracts/blob/002a7c8812e73c282b91e14541ce9b93a6de1172/contracts/proxy/Proxy.sol#L22-L45 + */ + function _forward(address _target) internal virtual { + assembly { + // Copy msg.data. We take full control of memory in this inline assembly + // block because it will not return to Solidity code. We overwrite the + // Solidity scratch pad at memory position 0. + calldatacopy(0, 0, calldatasize()) + + // Call the implementation. + // out and outsize are 0 because we don't know the size yet. + let result := call(gas(), _target, callvalue(), 0, calldatasize(), 0, 0) - // execute functionSig with callData on target, optionally passing msgValue + // Copy the returned data. + returndatacopy(0, 0, returndatasize()) + + switch result + // call returns 0 on error. + case 0 { + revert(0, returndatasize()) + } + default { + return(0, returndatasize()) + } } } + + // Executes an action on the target. + function _executeAction() public payable { + require(_isCallAllowedToCall(msg.sender, msg.data), "Not allowed to call"); + _forward(target); + } + + /** + * @dev Fallback function that forwards calls to the target. Will run if no other + * function in the contract matches the call data. + */ + fallback() external payable virtual { + _executeAction(); + } + + /** + * @dev Fallback function that delegates calls to the target. Will run if call data + * is empty. + */ + receive() external payable virtual { + _executeAction(); + } } From c7afc5de06f6b2868cfc3f5426cd2b2f8fbf2349 Mon Sep 17 00:00:00 2001 From: Matt Rice Date: Fri, 16 Jun 2023 18:38:18 -0400 Subject: [PATCH 06/20] WIP Signed-off-by: Matt Rice --- contracts/PermissionSplitter.sol | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/contracts/PermissionSplitter.sol b/contracts/PermissionSplitter.sol index d759feae4..0c416ee48 100644 --- a/contracts/PermissionSplitter.sol +++ b/contracts/PermissionSplitter.sol @@ -31,12 +31,12 @@ contract PermissionSplitterProxy is AccessControl, MultiCaller { roleForSelector[selector] = role; } - function _init(address _target) internal { + function _init(address _target) internal virtual { _grantRole(DEFAULT_ADMIN_ROLE, msg.sender); target = _target; } - function _isCallAllowedToCall(address caller, bytes calldata callData) internal view virtual returns (bool) { + function _isAllowedToCall(address caller, bytes calldata callData) internal view virtual returns (bool) { bytes4 selector; if (callData.length < 4) { // This handles any empty callData, which is a call to the fallback function. @@ -54,7 +54,7 @@ contract PermissionSplitterProxy is AccessControl, MultiCaller { * Note: this function is a modified _delegate function here: // https://github.com/OpenZeppelin/openzeppelin-contracts/blob/002a7c8812e73c282b91e14541ce9b93a6de1172/contracts/proxy/Proxy.sol#L22-L45 */ - function _forward(address _target) internal virtual { + function _forward(address _target) internal { assembly { // Copy msg.data. We take full control of memory in this inline assembly // block because it will not return to Solidity code. We overwrite the @@ -80,8 +80,8 @@ contract PermissionSplitterProxy is AccessControl, MultiCaller { } // Executes an action on the target. - function _executeAction() public payable { - require(_isCallAllowedToCall(msg.sender, msg.data), "Not allowed to call"); + function _executeAction() internal virtual { + require(_isAllowedToCall(msg.sender, msg.data), "Not allowed to call"); _forward(target); } From f096eb6d1e68e7bddb0a9085782f86aeb18ce39a Mon Sep 17 00:00:00 2001 From: Matt Rice Date: Tue, 20 Jun 2023 19:03:57 -0400 Subject: [PATCH 07/20] Add tests Signed-off-by: Matt Rice --- ...litter.sol => PermissionSplitterProxy.sol} | 0 test/PermissionSplitterProxy.ts | 76 +++++++++++++++++++ 2 files changed, 76 insertions(+) rename contracts/{PermissionSplitter.sol => PermissionSplitterProxy.sol} (100%) create mode 100644 test/PermissionSplitterProxy.ts diff --git a/contracts/PermissionSplitter.sol b/contracts/PermissionSplitterProxy.sol similarity index 100% rename from contracts/PermissionSplitter.sol rename to contracts/PermissionSplitterProxy.sol diff --git a/test/PermissionSplitterProxy.ts b/test/PermissionSplitterProxy.ts new file mode 100644 index 000000000..2f77493a0 --- /dev/null +++ b/test/PermissionSplitterProxy.ts @@ -0,0 +1,76 @@ +import { + getContractFactory, + SignerWithAddress, + seedWallet, + expect, + Contract, + ethers, + randomAddress, + utf8ToHex, +} from "../utils/utils"; +import { + originChainId, + destinationChainId, + bondAmount, + zeroAddress, + mockTreeRoot, + mockSlowRelayRoot, + finalFeeUsdc, + finalFee, + totalBond, +} from "./constants"; +import { hubPoolFixture } from "./fixtures/HubPool.Fixture"; + +let hubPool: Contract, weth: Contract, usdc: Contract, permissionSplitter: Contract, hubPoolProxy: Contract; +let mockSpoke: Contract, mockAdapter: Contract, identifierWhitelist: Contract; +let owner: SignerWithAddress, other: SignerWithAddress, delegate: SignerWithAddress; +let delegateRole: string, defaultAdminRole: string; + +const enableTokenSelector = "0xb60c2d7d"; + +describe("PermissionSplitterProxy", function () { + beforeEach(async function () { + [owner, delegate, other] = await ethers.getSigners(); + ({ weth, hubPool, usdc, mockAdapter, mockSpoke, identifierWhitelist } = await hubPoolFixture()); + const permissionSplitterFactory = await getContractFactory("PermissionSplitterProxy", owner); + const hubPoolFactory = await getContractFactory("HubPool", owner); + + permissionSplitter = await permissionSplitterFactory.deploy(hubPool.address); + hubPoolProxy = hubPoolFactory.attach(permissionSplitter.address); + delegateRole = ethers.utils.keccak256(ethers.utils.toUtf8Bytes("DELEGATE_ROLE")); + permissionSplitter.connect(owner).grantRole(delegateRole, delegate.address); + defaultAdminRole = ethers.utils.hexZeroPad("0x00", 32); + await hubPool.transferOwnership(permissionSplitter.address); + }); + + it("Cannot run method until whitelisted", async function () { + await expect(hubPoolProxy.connect(delegate).enableL1TokenForLiquidityProvision(weth.address)).to.be.reverted; + await permissionSplitter.connect(owner).__setRoleForSelector(enableTokenSelector, delegateRole); + await hubPoolProxy.connect(delegate).enableL1TokenForLiquidityProvision(weth.address); + expect((await hubPool.callStatic.pooledTokens(weth.address)).isEnabled).to.equal(true); + }); + it("Owner can run without whitelisting", async function () { + await hubPoolProxy.connect(owner).enableL1TokenForLiquidityProvision(weth.address); + expect((await hubPool.callStatic.pooledTokens(weth.address)).isEnabled).to.equal(true); + }); + + it("Owner can revoke role", async function () { + await expect(hubPoolProxy.connect(delegate).enableL1TokenForLiquidityProvision(weth.address)).to.be.reverted; + await permissionSplitter.connect(owner).__setRoleForSelector(enableTokenSelector, delegateRole); + await hubPoolProxy.connect(delegate).enableL1TokenForLiquidityProvision(weth.address); + expect((await hubPool.callStatic.pooledTokens(weth.address)).isEnabled).to.equal(true); + + await permissionSplitter.connect(owner).revokeRole(delegateRole, delegate.address); + await expect(hubPoolProxy.connect(delegate).enableL1TokenForLiquidityProvision(usdc.address)).to.be.reverted; + }); + + it("Owner can revoke selector", async function () { + await expect(hubPoolProxy.connect(delegate).enableL1TokenForLiquidityProvision(weth.address)).to.be.reverted; + await permissionSplitter.connect(owner).__setRoleForSelector(enableTokenSelector, delegateRole); + await hubPoolProxy.connect(delegate).enableL1TokenForLiquidityProvision(weth.address); + expect((await hubPool.callStatic.pooledTokens(weth.address)).isEnabled).to.equal(true); + + await permissionSplitter.connect(owner).__setRoleForSelector(enableTokenSelector, defaultAdminRole); + await expect(hubPoolProxy.connect(delegate).enableL1TokenForLiquidityProvision(usdc.address)).to.be.reverted; + }); +}); From fe2fe4870182613f6ac696f5c376fd6f6333b39c Mon Sep 17 00:00:00 2001 From: Matt Rice Date: Tue, 20 Jun 2023 19:06:21 -0400 Subject: [PATCH 08/20] WIP Signed-off-by: Matt Rice --- test/PermissionSplitterProxy.ts | 29 ++++------------------------- 1 file changed, 4 insertions(+), 25 deletions(-) diff --git a/test/PermissionSplitterProxy.ts b/test/PermissionSplitterProxy.ts index 2f77493a0..d259b4f31 100644 --- a/test/PermissionSplitterProxy.ts +++ b/test/PermissionSplitterProxy.ts @@ -1,37 +1,16 @@ -import { - getContractFactory, - SignerWithAddress, - seedWallet, - expect, - Contract, - ethers, - randomAddress, - utf8ToHex, -} from "../utils/utils"; -import { - originChainId, - destinationChainId, - bondAmount, - zeroAddress, - mockTreeRoot, - mockSlowRelayRoot, - finalFeeUsdc, - finalFee, - totalBond, -} from "./constants"; +import { getContractFactory, SignerWithAddress, expect, Contract, ethers } from "../utils/utils"; import { hubPoolFixture } from "./fixtures/HubPool.Fixture"; let hubPool: Contract, weth: Contract, usdc: Contract, permissionSplitter: Contract, hubPoolProxy: Contract; -let mockSpoke: Contract, mockAdapter: Contract, identifierWhitelist: Contract; -let owner: SignerWithAddress, other: SignerWithAddress, delegate: SignerWithAddress; +let owner: SignerWithAddress, delegate: SignerWithAddress; let delegateRole: string, defaultAdminRole: string; const enableTokenSelector = "0xb60c2d7d"; describe("PermissionSplitterProxy", function () { beforeEach(async function () { - [owner, delegate, other] = await ethers.getSigners(); - ({ weth, hubPool, usdc, mockAdapter, mockSpoke, identifierWhitelist } = await hubPoolFixture()); + [owner, delegate] = await ethers.getSigners(); + ({ weth, hubPool, usdc } = await hubPoolFixture()); const permissionSplitterFactory = await getContractFactory("PermissionSplitterProxy", owner); const hubPoolFactory = await getContractFactory("HubPool", owner); From 8908e4e012d413a5f8b512fa65a9398567849a5c Mon Sep 17 00:00:00 2001 From: Matt Rice Date: Tue, 20 Jun 2023 19:07:35 -0400 Subject: [PATCH 09/20] WIP Signed-off-by: Matt Rice --- contracts/PermissionSplitterProxy.sol | 1 - 1 file changed, 1 deletion(-) diff --git a/contracts/PermissionSplitterProxy.sol b/contracts/PermissionSplitterProxy.sol index 0c416ee48..83d21ffae 100644 --- a/contracts/PermissionSplitterProxy.sol +++ b/contracts/PermissionSplitterProxy.sol @@ -4,7 +4,6 @@ pragma solidity ^0.8.0; import "@uma/core/contracts/common/implementation/MultiCaller.sol"; import "@openzeppelin/contracts/access/AccessControl.sol"; -// TODO: Make upgradeable. contract PermissionSplitterProxy is AccessControl, MultiCaller { // Inherited admin role from AccessControl. Should be assigned to Across DAO Safe. // bytes32 public constant DEFAULT_ADMIN_ROLE = 0x00; From bd55498502c0327292f335887a876edfe3f408c3 Mon Sep 17 00:00:00 2001 From: Matt Rice Date: Tue, 20 Jun 2023 19:09:25 -0400 Subject: [PATCH 10/20] add events Signed-off-by: Matt Rice --- contracts/PermissionSplitterProxy.sol | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/contracts/PermissionSplitterProxy.sol b/contracts/PermissionSplitterProxy.sol index 83d21ffae..5347da243 100644 --- a/contracts/PermissionSplitterProxy.sol +++ b/contracts/PermissionSplitterProxy.sol @@ -14,6 +14,9 @@ contract PermissionSplitterProxy is AccessControl, MultiCaller { address public target; + event TargetUpdated(address indexed newTarget); + event RoleForSelectorSet(bytes4 indexed selector, bytes32 indexed role); + constructor(address _target) { _init(_target); } @@ -22,17 +25,19 @@ contract PermissionSplitterProxy is AccessControl, MultiCaller { // Note: these have two underscores in front to prevent any collisions with the target contract. function __setTarget(address _target) public onlyRole(DEFAULT_ADMIN_ROLE) { target = _target; + emit TargetUpdated(_target); } // Public function! // Note: these have two underscores in front to prevent any collisions with the target contract. function __setRoleForSelector(bytes4 selector, bytes32 role) public onlyRole(DEFAULT_ADMIN_ROLE) { roleForSelector[selector] = role; + emit RoleForSelectorSet(selector, role); } function _init(address _target) internal virtual { _grantRole(DEFAULT_ADMIN_ROLE, msg.sender); - target = _target; + __setTarget(_target); } function _isAllowedToCall(address caller, bytes calldata callData) internal view virtual returns (bool) { From 536ad191e6f232aef6fec2246ba4f113ca0828e3 Mon Sep 17 00:00:00 2001 From: nicholaspai Date: Fri, 19 Jan 2024 09:56:10 -0500 Subject: [PATCH 11/20] wip foundry tests Signed-off-by: nicholaspai --- foundry.toml | 1 + hardhat.config.ts | 1 + lib/forge-std | 1 + package.json | 1 + test/foundry/PermissionSplitter.t.sol | 52 +++++++++++++++++++++++++++ yarn.lock | 7 ++++ 6 files changed, 63 insertions(+) create mode 160000 lib/forge-std create mode 100644 test/foundry/PermissionSplitter.t.sol diff --git a/foundry.toml b/foundry.toml index eb7a73831..89dbca4c9 100644 --- a/foundry.toml +++ b/foundry.toml @@ -1,6 +1,7 @@ [profile.default] src = "contracts" out = "artifacts" +test = "test/foundry" libs = ["node_modules", "lib"] remappings = [ "@across-protocol/=node_modules/@across-protocol/", diff --git a/hardhat.config.ts b/hardhat.config.ts index eec95f3a4..87d5f6d71 100644 --- a/hardhat.config.ts +++ b/hardhat.config.ts @@ -5,6 +5,7 @@ import { getNodeUrl, getMnemonic } from "@uma/common"; import "@nomicfoundation/hardhat-verify"; // Must be above hardhat-upgrades import "@nomiclabs/hardhat-waffle"; +import "@nomicfoundation/hardhat-foundry"; import "@typechain/hardhat"; import "@matterlabs/hardhat-zksync-solc"; import "@matterlabs/hardhat-zksync-verify"; diff --git a/lib/forge-std b/lib/forge-std new file mode 160000 index 000000000..066ff16c5 --- /dev/null +++ b/lib/forge-std @@ -0,0 +1 @@ +Subproject commit 066ff16c5c03e6f931cd041fd366bc4be1fae82a diff --git a/package.json b/package.json index 4eb6900ad..4b3c3cea3 100644 --- a/package.json +++ b/package.json @@ -56,6 +56,7 @@ "@matterlabs/hardhat-zksync-upgradable": "^0.1.0", "@matterlabs/hardhat-zksync-verify": "^0.2.0", "@matterlabs/zksync-contracts": "^0.2.4", + "@nomicfoundation/hardhat-foundry": "^1.1.1", "@nomicfoundation/hardhat-verify": "^1.0.3", "@nomiclabs/hardhat-ethers": "^2.2.3", "@nomiclabs/hardhat-waffle": "2.0.3", diff --git a/test/foundry/PermissionSplitter.t.sol b/test/foundry/PermissionSplitter.t.sol new file mode 100644 index 000000000..7a6f9170c --- /dev/null +++ b/test/foundry/PermissionSplitter.t.sol @@ -0,0 +1,52 @@ +// SPDX-License-Identifier: BUSL-1.1 +pragma solidity ^0.8.0; + +import { Test } from "forge-std/Test.sol"; +import "forge-std/console.sol"; + +import { Finder } from "@uma/core/contracts/data-verification-mechanism/implementation/Finder.sol"; +import { HubPool } from "../../contracts/HubPool.sol"; +import { LpTokenFactory } from "../../contracts/LpTokenFactory.sol"; +import { PermissionSplitterProxy } from "../../contracts/PermissionSplitterProxy.sol"; + +// Run this test to verify PermissionSplitter behavior when changing ownership of the HubPool +// to it. Therefore this test should be run as a fork test via: +// - forge test --fork-url +contract PermissionSplitterTest is Test { + HubPool hubPool; + PermissionSplitterProxy permissionSplitter; + + // defaultAdmin is the deployer of the PermissionSplitter and has authority + // to call any function on the HubPool. Therefore this should be a highly secure + // contract account such as a MultiSig contract. + address defaultAdmin; + // Pause admin should only be allowed to pause the HubPool. + address pauseAdmin; + + bytes32 constant PAUSE_ROLE = keccak256("PAUSE_ROLE"); + bytes4 constant PAUSE_SELECTOR = bytes4(keccak256("setPaused(bool)")); + + function setUp() public { + hubPool = HubPool(payable(0xc186fA914353c44b2E33eBE05f21846F1048bEda)); + + // For the purposes of this test, the default admin will be the current owner of the + // HubPool, which we can assume is a highly secured account. + defaultAdmin = hubPool.owner(); + pauseAdmin = vm.addr(1); + permissionSplitter.grantRole(PAUSE_ROLE, pauseAdmin); + + // Deploy PermissionSplitter from default admin account. + vm.prank(defaultAdmin); + permissionSplitter = new PermissionSplitterProxy(address(hubPool)); + + hubPool.transferOwnership(address(permissionSplitter)); + } + + function testMain() public { + // Grant anyone with the pause role the ability to call setPaused + vm.prank(defaultAdmin); + permissionSplitter.__setRoleForSelector(PAUSE_SELECTOR, PAUSE_ROLE); + vm.prank(pauseAdmin); + hubPool.setPaused(true); + } +} diff --git a/yarn.lock b/yarn.lock index d248ce7af..097a9f9ec 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1980,6 +1980,13 @@ mcl-wasm "^0.7.1" rustbn.js "~0.2.0" +"@nomicfoundation/hardhat-foundry@^1.1.1": + version "1.1.1" + resolved "https://registry.yarnpkg.com/@nomicfoundation/hardhat-foundry/-/hardhat-foundry-1.1.1.tgz#db72b1f33f9cfaecc27e67f69ad436f8710162d6" + integrity sha512-cXGCBHAiXas9Pg9MhMOpBVQCkWRYoRFG7GJJAph+sdQsfd22iRs5U5Vs9XmpGEQd1yEvYISQZMeE68Nxj65iUQ== + dependencies: + chalk "^2.4.2" + "@nomicfoundation/hardhat-verify@^1.0.3": version "1.0.3" resolved "https://registry.yarnpkg.com/@nomicfoundation/hardhat-verify/-/hardhat-verify-1.0.3.tgz#573b326fe0f58dcdc527e8c11bd5a6336eb52bc4" From 904f336a77fd2c042dc684ecd3524c87bd375311 Mon Sep 17 00:00:00 2001 From: nicholaspai Date: Fri, 19 Jan 2024 11:17:11 -0500 Subject: [PATCH 12/20] Update PermissionSplitter.t.sol --- test/foundry/PermissionSplitter.t.sol | 60 +++++++++++++++++++++++---- 1 file changed, 51 insertions(+), 9 deletions(-) diff --git a/test/foundry/PermissionSplitter.t.sol b/test/foundry/PermissionSplitter.t.sol index 7a6f9170c..d565dba2a 100644 --- a/test/foundry/PermissionSplitter.t.sol +++ b/test/foundry/PermissionSplitter.t.sol @@ -4,8 +4,8 @@ pragma solidity ^0.8.0; import { Test } from "forge-std/Test.sol"; import "forge-std/console.sol"; -import { Finder } from "@uma/core/contracts/data-verification-mechanism/implementation/Finder.sol"; import { HubPool } from "../../contracts/HubPool.sol"; +import { SpokePool } from "../../contracts/SpokePool.sol"; import { LpTokenFactory } from "../../contracts/LpTokenFactory.sol"; import { PermissionSplitterProxy } from "../../contracts/PermissionSplitterProxy.sol"; @@ -14,6 +14,8 @@ import { PermissionSplitterProxy } from "../../contracts/PermissionSplitterProxy // - forge test --fork-url contract PermissionSplitterTest is Test { HubPool hubPool; + HubPool hubPoolProxy; + SpokePool ethereumSpokePool; PermissionSplitterProxy permissionSplitter; // defaultAdmin is the deployer of the PermissionSplitter and has authority @@ -25,28 +27,68 @@ contract PermissionSplitterTest is Test { bytes32 constant PAUSE_ROLE = keccak256("PAUSE_ROLE"); bytes4 constant PAUSE_SELECTOR = bytes4(keccak256("setPaused(bool)")); + // Error emitted when non-owner calls onlyOwner HubPool function. + bytes constant OWNABLE_NOT_OWNER_ERROR = bytes("Ownable: caller is not the owner"); + // Error emitted when calling PermissionSplitterProxy function with incorrect role. + bytes constant PROXY_NOT_ALLOWED_TO_CALL_ERROR = bytes("Not allowed to call"); function setUp() public { + // Since this test file is designed to run against a mainnet fork, hardcode the following system + // contracts to skip the setup we'd usually need to run to use brand new contracts. hubPool = HubPool(payable(0xc186fA914353c44b2E33eBE05f21846F1048bEda)); + ethereumSpokePool = SpokePool(payable(0x5c7BCd6E7De5423a257D81B442095A1a6ced35C5)); // For the purposes of this test, the default admin will be the current owner of the // HubPool, which we can assume is a highly secured account. defaultAdmin = hubPool.owner(); pauseAdmin = vm.addr(1); - permissionSplitter.grantRole(PAUSE_ROLE, pauseAdmin); - // Deploy PermissionSplitter from default admin account. - vm.prank(defaultAdmin); + // Deploy PermissionSplitter from default admin account and then + // create and assign roles. + vm.startPrank(defaultAdmin); + // Default admin can call any ownable function, which no one else can call without + // the correct role. permissionSplitter = new PermissionSplitterProxy(address(hubPool)); + permissionSplitter.grantRole(PAUSE_ROLE, pauseAdmin); + // Grant anyone with the pause role the ability to call setPaused + permissionSplitter.__setRoleForSelector(PAUSE_SELECTOR, PAUSE_ROLE); + vm.stopPrank(); + vm.prank(defaultAdmin); hubPool.transferOwnership(address(permissionSplitter)); + hubPoolProxy = HubPool(payable(permissionSplitter)); } - function testMain() public { - // Grant anyone with the pause role the ability to call setPaused - vm.prank(defaultAdmin); - permissionSplitter.__setRoleForSelector(PAUSE_SELECTOR, PAUSE_ROLE); - vm.prank(pauseAdmin); + function testPause() public { + // Calling HubPool setPaused directly should fail, even if called by previous owner. + vm.startPrank(defaultAdmin); + vm.expectRevert(OWNABLE_NOT_OWNER_ERROR); hubPool.setPaused(true); + vm.stopPrank(); + + // Must call HubPool via PermissionSplitterProxy. + vm.prank(pauseAdmin); + hubPoolProxy.setPaused(true); + assertTrue(hubPool.paused()); + } + + function testCallSpokePoolFunction() public { + bytes32 fakeRoot = keccak256("new admin root"); + bytes memory spokeFunctionCallData = abi.encodeWithSignature( + "relayRootBundle(bytes32,bytes32)", + fakeRoot, + fakeRoot + ); + uint256 spokeChainId = 1; + + vm.expectRevert(PROXY_NOT_ALLOWED_TO_CALL_ERROR); + hubPoolProxy.relaySpokePoolAdminFunction(spokeChainId, spokeFunctionCallData); + vm.expectRevert(OWNABLE_NOT_OWNER_ERROR); + hubPool.relaySpokePoolAdminFunction(spokeChainId, spokeFunctionCallData); + + vm.startPrank(defaultAdmin); + vm.expectCall(address(ethereumSpokePool), spokeFunctionCallData); + hubPoolProxy.relaySpokePoolAdminFunction(spokeChainId, spokeFunctionCallData); + vm.stopPrank(); } } From af8716b52a3840e73073e6e04a42c0584aee2e7a Mon Sep 17 00:00:00 2001 From: nicholaspai Date: Fri, 19 Jan 2024 11:20:24 -0500 Subject: [PATCH 13/20] Delete forge-std --- lib/forge-std | 1 - 1 file changed, 1 deletion(-) delete mode 160000 lib/forge-std diff --git a/lib/forge-std b/lib/forge-std deleted file mode 160000 index 066ff16c5..000000000 --- a/lib/forge-std +++ /dev/null @@ -1 +0,0 @@ -Subproject commit 066ff16c5c03e6f931cd041fd366bc4be1fae82a From aaa08770248ba2bcd2c134ac1586839239d38f14 Mon Sep 17 00:00:00 2001 From: nicholaspai Date: Fri, 19 Jan 2024 11:24:52 -0500 Subject: [PATCH 14/20] Revert "Delete forge-std" This reverts commit af8716b52a3840e73073e6e04a42c0584aee2e7a. --- lib/forge-std | 1 + 1 file changed, 1 insertion(+) create mode 160000 lib/forge-std diff --git a/lib/forge-std b/lib/forge-std new file mode 160000 index 000000000..066ff16c5 --- /dev/null +++ b/lib/forge-std @@ -0,0 +1 @@ +Subproject commit 066ff16c5c03e6f931cd041fd366bc4be1fae82a From 338545ee0c1fc1150ba6b519cb396d417f398898 Mon Sep 17 00:00:00 2001 From: nicholaspai Date: Fri, 19 Jan 2024 11:26:00 -0500 Subject: [PATCH 15/20] Remove hardhat-foundry --- hardhat.config.ts | 1 - package.json | 1 - yarn.lock | 7 ------- 3 files changed, 9 deletions(-) diff --git a/hardhat.config.ts b/hardhat.config.ts index 87d5f6d71..eec95f3a4 100644 --- a/hardhat.config.ts +++ b/hardhat.config.ts @@ -5,7 +5,6 @@ import { getNodeUrl, getMnemonic } from "@uma/common"; import "@nomicfoundation/hardhat-verify"; // Must be above hardhat-upgrades import "@nomiclabs/hardhat-waffle"; -import "@nomicfoundation/hardhat-foundry"; import "@typechain/hardhat"; import "@matterlabs/hardhat-zksync-solc"; import "@matterlabs/hardhat-zksync-verify"; diff --git a/package.json b/package.json index 4b3c3cea3..4eb6900ad 100644 --- a/package.json +++ b/package.json @@ -56,7 +56,6 @@ "@matterlabs/hardhat-zksync-upgradable": "^0.1.0", "@matterlabs/hardhat-zksync-verify": "^0.2.0", "@matterlabs/zksync-contracts": "^0.2.4", - "@nomicfoundation/hardhat-foundry": "^1.1.1", "@nomicfoundation/hardhat-verify": "^1.0.3", "@nomiclabs/hardhat-ethers": "^2.2.3", "@nomiclabs/hardhat-waffle": "2.0.3", diff --git a/yarn.lock b/yarn.lock index 097a9f9ec..d248ce7af 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1980,13 +1980,6 @@ mcl-wasm "^0.7.1" rustbn.js "~0.2.0" -"@nomicfoundation/hardhat-foundry@^1.1.1": - version "1.1.1" - resolved "https://registry.yarnpkg.com/@nomicfoundation/hardhat-foundry/-/hardhat-foundry-1.1.1.tgz#db72b1f33f9cfaecc27e67f69ad436f8710162d6" - integrity sha512-cXGCBHAiXas9Pg9MhMOpBVQCkWRYoRFG7GJJAph+sdQsfd22iRs5U5Vs9XmpGEQd1yEvYISQZMeE68Nxj65iUQ== - dependencies: - chalk "^2.4.2" - "@nomicfoundation/hardhat-verify@^1.0.3": version "1.0.3" resolved "https://registry.yarnpkg.com/@nomicfoundation/hardhat-verify/-/hardhat-verify-1.0.3.tgz#573b326fe0f58dcdc527e8c11bd5a6336eb52bc4" From b45aaf887d6bc15beb87d2e8641482563e24b124 Mon Sep 17 00:00:00 2001 From: nicholaspai Date: Fri, 19 Jan 2024 12:35:23 -0500 Subject: [PATCH 16/20] add test --- .eslintignore | 1 + .prettierignore | 1 + .solhintignore | 1 + contracts/PermissionSplitterProxy.sol | 28 +++++++++++++++++++++++---- test/foundry/PermissionSplitter.t.sol | 24 +++++++++++++++++++++++ 5 files changed, 51 insertions(+), 4 deletions(-) diff --git a/.eslintignore b/.eslintignore index 8a2103827..4d2713385 100644 --- a/.eslintignore +++ b/.eslintignore @@ -6,3 +6,4 @@ gasReporterOutput.json typechain artifacts-zk cache-zk +lib diff --git a/.prettierignore b/.prettierignore index 59766e79e..74ef1d144 100644 --- a/.prettierignore +++ b/.prettierignore @@ -7,3 +7,4 @@ typechain dist artifacts-zk cache-zk +lib diff --git a/.solhintignore b/.solhintignore index 3c3629e64..9b26ed04f 100644 --- a/.solhintignore +++ b/.solhintignore @@ -1 +1,2 @@ node_modules +lib \ No newline at end of file diff --git a/contracts/PermissionSplitterProxy.sol b/contracts/PermissionSplitterProxy.sol index 5347da243..02d242bfe 100644 --- a/contracts/PermissionSplitterProxy.sol +++ b/contracts/PermissionSplitterProxy.sol @@ -4,6 +4,11 @@ pragma solidity ^0.8.0; import "@uma/core/contracts/common/implementation/MultiCaller.sol"; import "@openzeppelin/contracts/access/AccessControl.sol"; +/** + * @notice This contract is designed to own an Ownable "target" contract and gate access to specific + * function selectors based on specific roles. Practically, this contract should own the HubPool contract. + * All ownable function calls to target should be sent through this contract's fallback function. + */ contract PermissionSplitterProxy is AccessControl, MultiCaller { // Inherited admin role from AccessControl. Should be assigned to Across DAO Safe. // bytes32 public constant DEFAULT_ADMIN_ROLE = 0x00; @@ -12,24 +17,39 @@ contract PermissionSplitterProxy is AccessControl, MultiCaller { // only role holders. mapping(bytes4 => bytes32) public roleForSelector; + // The contract that should be owned by this contract and whose function selectors will be gated + // by roleForSelector. address public target; event TargetUpdated(address indexed newTarget); event RoleForSelectorSet(bytes4 indexed selector, bytes32 indexed role); + /** + * @notice constructs PermissionSplitterProxy + */ constructor(address _target) { _init(_target); } - // Public function! - // Note: these have two underscores in front to prevent any collisions with the target contract. + /** + * @notice Change target contract. + * @dev Public function! This has two underscores in front to prevent any collisions with the target contract. + * @dev Only callable by default admin role holder. + * @param _target New target contract. + */ function __setTarget(address _target) public onlyRole(DEFAULT_ADMIN_ROLE) { target = _target; emit TargetUpdated(_target); } - // Public function! - // Note: these have two underscores in front to prevent any collisions with the target contract. + /** + * @notice Change role ID for function selector. Only role holder can call function selector + * on target. + * @dev Public function! This has two underscores in front to prevent any collisions with the target contract. + * @dev Only callable by default admin role holder. + * @param selector Function selector to map to role. + * @param role Only this role holder can call function selector on the target + */ function __setRoleForSelector(bytes4 selector, bytes32 role) public onlyRole(DEFAULT_ADMIN_ROLE) { roleForSelector[selector] = role; emit RoleForSelectorSet(selector, role); diff --git a/test/foundry/PermissionSplitter.t.sol b/test/foundry/PermissionSplitter.t.sol index d565dba2a..3572ff506 100644 --- a/test/foundry/PermissionSplitter.t.sol +++ b/test/foundry/PermissionSplitter.t.sol @@ -91,4 +91,28 @@ contract PermissionSplitterTest is Test { hubPoolProxy.relaySpokePoolAdminFunction(spokeChainId, spokeFunctionCallData); vm.stopPrank(); } + + function testFallback() public { + // Calling a function that doesn't exist on target or PermissionSplitter calls the HubPool's + // fallback function which wraps any msg.value into wrapped native token. + uint256 balBefore = address(hubPool).balance; + + // Calling fake function as admin with no value succeeds and does nothing. + vm.prank(defaultAdmin); + (bool success1, ) = address(hubPoolProxy).call("doesNotExist()"); + assertTrue(success1); + + // Calling fake function as admin with value also succeeds and wraps the msg.value + // and then does nothing. + vm.deal(defaultAdmin, 1 ether); + vm.prank(defaultAdmin); + (bool success2, bytes memory reason) = address(hubPoolProxy).call{ value: 1 ether }("doesNotExist()"); + assertTrue(success2); + assertEq(address(hubPool).balance, balBefore); + } + + function testFunctionSelectorCollisions() public { + // TODO: Test that HubPool has no public function selector collisions with itself. + // TODO: Test that PermissionSplitter has no function selector collisions with HubPool. + } } From a1734638a66b4ef5c45493db87771510aaa3bc18 Mon Sep 17 00:00:00 2001 From: nicholaspai Date: Fri, 19 Jan 2024 15:15:33 -0500 Subject: [PATCH 17/20] Update PermissionSplitter.t.sol --- test/foundry/PermissionSplitter.t.sol | 90 +++++++++++++++++++++++++-- 1 file changed, 86 insertions(+), 4 deletions(-) diff --git a/test/foundry/PermissionSplitter.t.sol b/test/foundry/PermissionSplitter.t.sol index 3572ff506..65fa8e3ac 100644 --- a/test/foundry/PermissionSplitter.t.sol +++ b/test/foundry/PermissionSplitter.t.sol @@ -25,13 +25,79 @@ contract PermissionSplitterTest is Test { // Pause admin should only be allowed to pause the HubPool. address pauseAdmin; - bytes32 constant PAUSE_ROLE = keccak256("PAUSE_ROLE"); + // HubPool function selectors: bytes4 constant PAUSE_SELECTOR = bytes4(keccak256("setPaused(bool)")); + bytes4 constant DELETE_PROPOSAL_SELECTOR = bytes4(keccak256("emergencyDeleteProposal()")); + bytes4 constant RELAY_SPOKEPOOL_SELECTOR = bytes4(keccak256("relaySpokePoolAdminFunction(uint256,bytes)")); + bytes4 constant SET_PROTOCOL_FEE_SELECTOR = bytes4(keccak256("setProtocolFeeCapture(address,uint256)")); + bytes4 constant SET_BOND_SELECTOR = bytes4(keccak256("setBond(address,uint256)")); + bytes4 constant SET_LIVENESS_SELECTOR = bytes4(keccak256("setLiveness(uint32)")); + bytes4 constant SET_IDENTIFIER_SELECTOR = bytes4(keccak256("setIdentifier(bytes32)")); + bytes4 constant SET_XCHAIN_CONTRACTS_SELECTOR = + bytes4(keccak256("setCrossChainContracts(uint256,address,address)")); + bytes4 constant SET_POOL_REBALANCE_ROUTES_SELECTOR = + bytes4(keccak256("setPoolRebalanceRoute(uint256,address,address)")); + bytes4 constant SET_DEPOSIT_ROUTES_SELECTOR = bytes4(keccak256("setDepositRoute(uint256,uint256,address,bool)")); + bytes4 constant ENABLE_L1_TOKEN_SELECTOR = bytes4(keccak256("enableL1TokenForLiquidityProvision(address)")); + bytes4 constant DISABLE_L1_TOKEN_SELECTOR = bytes4(keccak256("disableL1TokenForLiquidityProvision(address)")); + bytes4 constant HAIRCUT_RESERVES_SELECTOR = bytes4(keccak256("haircutReserves(address,int256)")); + bytes4 constant ADD_LIQUIDITY_SELECTOR = bytes4(keccak256("addLiquidity(address,int256)")); + bytes4 constant REMOVE_LIQUIDITY_SELECTOR = bytes4(keccak256("removeLiquidity(address,int256,bool)")); + bytes4 constant EXCHANGE_RATE_SELECTOR = bytes4(keccak256("exchangeRateCurrent(address)")); + bytes4 constant UTILIZATION_SELECTOR = bytes4(keccak256("liquidityUtilizationCurrent(address)")); + bytes4 constant UTILIZATION_POST_RELAY_SELECTOR = + bytes4(keccak256("liquidityUtilizationPostRelay(address,uint256)")); + bytes4 constant SYNC_SELECTOR = bytes4(keccak256("sync(address)")); + bytes4 constant PROPOSE_SELECTOR = bytes4(keccak256("proposeRootBundle(uint256[],uint8,bytes32,bytes32,bytes32)")); + bytes4 constant EXECUTE_SELECTOR = + bytes4(keccak256("executeRootBundle(uint256,uint256,uint256[],int256[],int256[],uint8,address[],bytes32[])")); + bytes4 constant DISPUTE_SELECTOR = bytes4(keccak256("disputeRootBundle()")); + bytes4 constant CLAIM_FEES_SELECTOR = bytes4(keccak256("claimProtocolFeesCaptured(address)")); + bytes4 constant LOAD_ETH_SELECTOR = bytes4(keccak256("loadEthForL2Calls()")); + + // PermissionSplitterProxy function selectors: + bytes4 constant SET_ROLE_SELECTOR = bytes4(keccak256("__setRoleForSelector(bytes4,bytes32)")); + bytes4 constant SET_TARGET_SELECTOR = bytes4(keccak256("__setTarget(address)")); + + bytes4[] allSelectors = [ + PAUSE_SELECTOR, + DELETE_PROPOSAL_SELECTOR, + RELAY_SPOKEPOOL_SELECTOR, + SET_PROTOCOL_FEE_SELECTOR, + SET_BOND_SELECTOR, + SET_LIVENESS_SELECTOR, + SET_IDENTIFIER_SELECTOR, + SET_XCHAIN_CONTRACTS_SELECTOR, + SET_POOL_REBALANCE_ROUTES_SELECTOR, + SET_DEPOSIT_ROUTES_SELECTOR, + ENABLE_L1_TOKEN_SELECTOR, + DISABLE_L1_TOKEN_SELECTOR, + HAIRCUT_RESERVES_SELECTOR, + ADD_LIQUIDITY_SELECTOR, + REMOVE_LIQUIDITY_SELECTOR, + EXCHANGE_RATE_SELECTOR, + UTILIZATION_SELECTOR, + UTILIZATION_POST_RELAY_SELECTOR, + SYNC_SELECTOR, + PROPOSE_SELECTOR, + EXECUTE_SELECTOR, + DISPUTE_SELECTOR, + CLAIM_FEES_SELECTOR, + LOAD_ETH_SELECTOR, + SET_ROLE_SELECTOR, + SET_TARGET_SELECTOR + ]; + + bytes32 constant PAUSE_ROLE = keccak256("PAUSE_ROLE"); // Error emitted when non-owner calls onlyOwner HubPool function. bytes constant OWNABLE_NOT_OWNER_ERROR = bytes("Ownable: caller is not the owner"); // Error emitted when calling PermissionSplitterProxy function with incorrect role. bytes constant PROXY_NOT_ALLOWED_TO_CALL_ERROR = bytes("Not allowed to call"); + address constant WETHAddress = 0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2; + + error FuncSelectorCollision(); + function setUp() public { // Since this test file is designed to run against a mainnet fork, hardcode the following system // contracts to skip the setup we'd usually need to run to use brand new contracts. @@ -111,8 +177,24 @@ contract PermissionSplitterTest is Test { assertEq(address(hubPool).balance, balBefore); } - function testFunctionSelectorCollisions() public { - // TODO: Test that HubPool has no public function selector collisions with itself. - // TODO: Test that PermissionSplitter has no function selector collisions with HubPool. + /// forge-config: default.fuzz.runs = 300 + function testFunctionSelectorCollisions(uint256 funcSelectorIdx1, uint256 funcSelectorIdx2) public { + vm.assume(funcSelectorIdx1 < allSelectors.length); + vm.assume(funcSelectorIdx2 < allSelectors.length); + vm.assume(funcSelectorIdx1 != funcSelectorIdx2); + + // Assert that HubPool has no public function selector collisions with itself. + // Assert that PermissionSplitter has no function selector collisions with HubPool. + if (allSelectors[funcSelectorIdx1] == allSelectors[funcSelectorIdx2]) revert FuncSelectorCollision(); + } + + function testCallPublicFunction() public { + // Should be able to call public functions without any access modifiers via proxy as the default admin. + + vm.prank(defaultAdmin); + hubPoolProxy.sync(WETHAddress); + + vm.expectRevert(PROXY_NOT_ALLOWED_TO_CALL_ERROR); + hubPoolProxy.sync(WETHAddress); } } From 887295b8b7a54d53948cca80f70ace18d22fbfb7 Mon Sep 17 00:00:00 2001 From: nicholaspai Date: Sun, 21 Jan 2024 11:34:54 -0500 Subject: [PATCH 18/20] Respond to matt comments --- test/foundry/PermissionSplitter.t.sol | 27 ++++++++++++++++----------- 1 file changed, 16 insertions(+), 11 deletions(-) diff --git a/test/foundry/PermissionSplitter.t.sol b/test/foundry/PermissionSplitter.t.sol index 65fa8e3ac..4adea7034 100644 --- a/test/foundry/PermissionSplitter.t.sol +++ b/test/foundry/PermissionSplitter.t.sol @@ -59,7 +59,7 @@ contract PermissionSplitterTest is Test { bytes4 constant SET_ROLE_SELECTOR = bytes4(keccak256("__setRoleForSelector(bytes4,bytes32)")); bytes4 constant SET_TARGET_SELECTOR = bytes4(keccak256("__setTarget(address)")); - bytes4[] allSelectors = [ + bytes4[] hubPoolSelectors = [ PAUSE_SELECTOR, DELETE_PROPOSAL_SELECTOR, RELAY_SPOKEPOOL_SELECTOR, @@ -83,10 +83,9 @@ contract PermissionSplitterTest is Test { EXECUTE_SELECTOR, DISPUTE_SELECTOR, CLAIM_FEES_SELECTOR, - LOAD_ETH_SELECTOR, - SET_ROLE_SELECTOR, - SET_TARGET_SELECTOR + LOAD_ETH_SELECTOR ]; + bytes4[] proxySelectors = [SET_ROLE_SELECTOR, SET_TARGET_SELECTOR]; bytes32 constant PAUSE_ROLE = keccak256("PAUSE_ROLE"); // Error emitted when non-owner calls onlyOwner HubPool function. @@ -136,6 +135,11 @@ contract PermissionSplitterTest is Test { vm.prank(pauseAdmin); hubPoolProxy.setPaused(true); assertTrue(hubPool.paused()); + + // Can also call Proxy function via default admin. + vm.prank(defaultAdmin); + hubPoolProxy.setPaused(false); + assertFalse(hubPool.paused()); } function testCallSpokePoolFunction() public { @@ -177,15 +181,16 @@ contract PermissionSplitterTest is Test { assertEq(address(hubPool).balance, balBefore); } - /// forge-config: default.fuzz.runs = 300 - function testFunctionSelectorCollisions(uint256 funcSelectorIdx1, uint256 funcSelectorIdx2) public { - vm.assume(funcSelectorIdx1 < allSelectors.length); - vm.assume(funcSelectorIdx2 < allSelectors.length); - vm.assume(funcSelectorIdx1 != funcSelectorIdx2); + /// forge-config: default.fuzz.runs = 100 + function testFunctionSelectorCollisions(uint256 hubPoolFuncSelectorIdx, uint256 proxyFuncSelectorIdx) public { + vm.assume(hubPoolFuncSelectorIdx < hubPoolSelectors.length); + vm.assume(proxyFuncSelectorIdx < proxySelectors.length); - // Assert that HubPool has no public function selector collisions with itself. // Assert that PermissionSplitter has no function selector collisions with HubPool. - if (allSelectors[funcSelectorIdx1] == allSelectors[funcSelectorIdx2]) revert FuncSelectorCollision(); + // @dev Solidity compilation will fail if function selectors on the same contract collide. + // - https://ethereum.stackexchange.com/a/46188/47801 + if (hubPoolSelectors[hubPoolFuncSelectorIdx] == proxySelectors[proxyFuncSelectorIdx]) + revert FuncSelectorCollision(); } function testCallPublicFunction() public { From 70c36e1322a7cfdb86f10d1adab4ec170a69d471 Mon Sep 17 00:00:00 2001 From: nicholaspai Date: Mon, 22 Jan 2024 17:26:16 -0500 Subject: [PATCH 19/20] add more unit tests: transferring ownership back to default admin, more robust fuzz testing of fallback()/receive() Signed-off-by: nicholaspai --- test/foundry/PermissionSplitter.t.sol | 175 +++++++++++++++++--------- 1 file changed, 115 insertions(+), 60 deletions(-) diff --git a/test/foundry/PermissionSplitter.t.sol b/test/foundry/PermissionSplitter.t.sol index 4adea7034..3ee6f479d 100644 --- a/test/foundry/PermissionSplitter.t.sol +++ b/test/foundry/PermissionSplitter.t.sol @@ -24,70 +24,74 @@ contract PermissionSplitterTest is Test { address defaultAdmin; // Pause admin should only be allowed to pause the HubPool. address pauseAdmin; + address rando1; + address rando2; - // HubPool function selectors: + // We test the pause() selector specifically in the tests so we define it as a variable. bytes4 constant PAUSE_SELECTOR = bytes4(keccak256("setPaused(bool)")); - bytes4 constant DELETE_PROPOSAL_SELECTOR = bytes4(keccak256("emergencyDeleteProposal()")); - bytes4 constant RELAY_SPOKEPOOL_SELECTOR = bytes4(keccak256("relaySpokePoolAdminFunction(uint256,bytes)")); - bytes4 constant SET_PROTOCOL_FEE_SELECTOR = bytes4(keccak256("setProtocolFeeCapture(address,uint256)")); - bytes4 constant SET_BOND_SELECTOR = bytes4(keccak256("setBond(address,uint256)")); - bytes4 constant SET_LIVENESS_SELECTOR = bytes4(keccak256("setLiveness(uint32)")); - bytes4 constant SET_IDENTIFIER_SELECTOR = bytes4(keccak256("setIdentifier(bytes32)")); - bytes4 constant SET_XCHAIN_CONTRACTS_SELECTOR = - bytes4(keccak256("setCrossChainContracts(uint256,address,address)")); - bytes4 constant SET_POOL_REBALANCE_ROUTES_SELECTOR = - bytes4(keccak256("setPoolRebalanceRoute(uint256,address,address)")); - bytes4 constant SET_DEPOSIT_ROUTES_SELECTOR = bytes4(keccak256("setDepositRoute(uint256,uint256,address,bool)")); - bytes4 constant ENABLE_L1_TOKEN_SELECTOR = bytes4(keccak256("enableL1TokenForLiquidityProvision(address)")); - bytes4 constant DISABLE_L1_TOKEN_SELECTOR = bytes4(keccak256("disableL1TokenForLiquidityProvision(address)")); - bytes4 constant HAIRCUT_RESERVES_SELECTOR = bytes4(keccak256("haircutReserves(address,int256)")); - bytes4 constant ADD_LIQUIDITY_SELECTOR = bytes4(keccak256("addLiquidity(address,int256)")); - bytes4 constant REMOVE_LIQUIDITY_SELECTOR = bytes4(keccak256("removeLiquidity(address,int256,bool)")); - bytes4 constant EXCHANGE_RATE_SELECTOR = bytes4(keccak256("exchangeRateCurrent(address)")); - bytes4 constant UTILIZATION_SELECTOR = bytes4(keccak256("liquidityUtilizationCurrent(address)")); - bytes4 constant UTILIZATION_POST_RELAY_SELECTOR = - bytes4(keccak256("liquidityUtilizationPostRelay(address,uint256)")); - bytes4 constant SYNC_SELECTOR = bytes4(keccak256("sync(address)")); - bytes4 constant PROPOSE_SELECTOR = bytes4(keccak256("proposeRootBundle(uint256[],uint8,bytes32,bytes32,bytes32)")); - bytes4 constant EXECUTE_SELECTOR = - bytes4(keccak256("executeRootBundle(uint256,uint256,uint256[],int256[],int256[],uint8,address[],bytes32[])")); - bytes4 constant DISPUTE_SELECTOR = bytes4(keccak256("disputeRootBundle()")); - bytes4 constant CLAIM_FEES_SELECTOR = bytes4(keccak256("claimProtocolFeesCaptured(address)")); - bytes4 constant LOAD_ETH_SELECTOR = bytes4(keccak256("loadEthForL2Calls()")); - - // PermissionSplitterProxy function selectors: - bytes4 constant SET_ROLE_SELECTOR = bytes4(keccak256("__setRoleForSelector(bytes4,bytes32)")); - bytes4 constant SET_TARGET_SELECTOR = bytes4(keccak256("__setTarget(address)")); + bytes32 constant PAUSE_ROLE = keccak256("PAUSE_ROLE"); + // We hardcode all of the contract function selectors so we can stress test with possible function selectors and + // receive()/fallback() behavior when the proxy is called with random func selectors that it doesn't have. bytes4[] hubPoolSelectors = [ PAUSE_SELECTOR, - DELETE_PROPOSAL_SELECTOR, - RELAY_SPOKEPOOL_SELECTOR, - SET_PROTOCOL_FEE_SELECTOR, - SET_BOND_SELECTOR, - SET_LIVENESS_SELECTOR, - SET_IDENTIFIER_SELECTOR, - SET_XCHAIN_CONTRACTS_SELECTOR, - SET_POOL_REBALANCE_ROUTES_SELECTOR, - SET_DEPOSIT_ROUTES_SELECTOR, - ENABLE_L1_TOKEN_SELECTOR, - DISABLE_L1_TOKEN_SELECTOR, - HAIRCUT_RESERVES_SELECTOR, - ADD_LIQUIDITY_SELECTOR, - REMOVE_LIQUIDITY_SELECTOR, - EXCHANGE_RATE_SELECTOR, - UTILIZATION_SELECTOR, - UTILIZATION_POST_RELAY_SELECTOR, - SYNC_SELECTOR, - PROPOSE_SELECTOR, - EXECUTE_SELECTOR, - DISPUTE_SELECTOR, - CLAIM_FEES_SELECTOR, - LOAD_ETH_SELECTOR + bytes4(keccak256("emergencyDeleteProposal()")), + bytes4(keccak256("relaySpokePoolAdminFunction(uint256,bytes)")), + bytes4(keccak256("setProtocolFeeCapture(address,uint256)")), + bytes4(keccak256("setBond(address,uint256)")), + bytes4(keccak256("setLiveness(uint32)")), + bytes4(keccak256("setIdentifier(bytes32)")), + bytes4(keccak256("setCrossChainContracts(uint256,address,address)")), + bytes4(keccak256("setPoolRebalanceRoute(uint256,address,address)")), + bytes4(keccak256("setDepositRoute(uint256,uint256,address,bool)")), + bytes4(keccak256("enableL1TokenForLiquidityProvision(address)")), + bytes4(keccak256("disableL1TokenForLiquidityProvision(address)")), + bytes4(keccak256("haircutReserves(address,int256)")), + bytes4(keccak256("addLiquidity(address,int256)")), + bytes4(keccak256("removeLiquidity(address,int256,bool)")), + bytes4(keccak256("exchangeRateCurrent(address)")), + bytes4(keccak256("liquidityUtilizationCurrent(address)")), + bytes4(keccak256("liquidityUtilizationPostRelay(address,uint256)")), + bytes4(keccak256("sync(address)")), + bytes4(keccak256("proposeRootBundle(uint256[],uint8,bytes32,bytes32,bytes32)")), + bytes4(keccak256("executeRootBundle(uint256,uint256,uint256[],int256[],int256[],uint8,address[],bytes32[])")), + bytes4(keccak256("disputeRootBundle()")), + bytes4(keccak256("claimProtocolFeesCaptured(address)")), + bytes4(keccak256("loadEthForL2Calls()")), + bytes4(keccak256("rootBundleProposal()")), + bytes4(keccak256("pooledTokens(address)")), + bytes4(keccak256("crossChainContracts(uint256)")), + bytes4(keccak256("unclaimedAccumulatedProtocolFees(address)")), + bytes4(keccak256("paused()")), + bytes4(keccak256("protocolFeeCaptureAddress()")), + bytes4(keccak256("bondToken()")), + bytes4(keccak256("liveness()")), + bytes4(keccak256("identifier()")), + bytes4(keccak256("lpFeeRatePerSecond()")), + bytes4(keccak256("protocolFeeCapturePct()")), + bytes4(keccak256("bondAmount()")), + bytes4(keccak256("poolRebalanceRoute(uint256,address)")), + bytes4(keccak256("setCurrentTime(uint256)")), + bytes4(keccak256("getCurrentTime()")), + bytes4(keccak256("timerAddress()")), + bytes4(keccak256("multicall(bytes4[])")), + bytes4(keccak256("owner()")), + bytes4(keccak256("renounceOwnership()")), + bytes4(keccak256("transferOwnership(address)")) + ]; + bytes4[] proxySelectors = [ + bytes4(keccak256("__setRoleForSelector(bytes4,bytes32)")), + bytes4(keccak256("__setTarget(address)")), + bytes4(keccak256("target()")), + bytes4(keccak256("roleForSelector(bytes4)")), + bytes4(keccak256("supportsInterface(bytes4)")), + bytes4(keccak256("hasRole(bytes32,address)")), + bytes4(keccak256("getRoleAdmin(bytes32)")), + bytes4(keccak256("grantRole(bytes32,address)")), + bytes4(keccak256("revokeRole(bytes32,address)")), + bytes4(keccak256("renounceRole(bytes32,address)")) ]; - bytes4[] proxySelectors = [SET_ROLE_SELECTOR, SET_TARGET_SELECTOR]; - bytes32 constant PAUSE_ROLE = keccak256("PAUSE_ROLE"); // Error emitted when non-owner calls onlyOwner HubPool function. bytes constant OWNABLE_NOT_OWNER_ERROR = bytes("Ownable: caller is not the owner"); // Error emitted when calling PermissionSplitterProxy function with incorrect role. @@ -107,6 +111,8 @@ contract PermissionSplitterTest is Test { // HubPool, which we can assume is a highly secured account. defaultAdmin = hubPool.owner(); pauseAdmin = vm.addr(1); + rando1 = vm.addr(2); + rando2 = vm.addr(3); // Deploy PermissionSplitter from default admin account and then // create and assign roles. @@ -140,6 +146,18 @@ contract PermissionSplitterTest is Test { vm.prank(defaultAdmin); hubPoolProxy.setPaused(false); assertFalse(hubPool.paused()); + + // Multiple EOA's can be granted the pause role. + vm.startPrank(defaultAdmin); + permissionSplitter.grantRole(PAUSE_ROLE, rando1); + permissionSplitter.grantRole(PAUSE_ROLE, rando2); + vm.stopPrank(); + vm.prank(rando1); + hubPoolProxy.setPaused(true); + assertTrue(hubPool.paused()); + vm.prank(rando2); + hubPoolProxy.setPaused(false); + assertFalse(hubPool.paused()); } function testCallSpokePoolFunction() public { @@ -162,25 +180,62 @@ contract PermissionSplitterTest is Test { vm.stopPrank(); } - function testFallback() public { + function testTransferOwnership() public { + vm.expectRevert(PROXY_NOT_ALLOWED_TO_CALL_ERROR); + hubPoolProxy.transferOwnership(defaultAdmin); + + // Should be able to transfer ownership back to default admin in an emergency. + vm.startPrank(defaultAdmin); + hubPoolProxy.transferOwnership(defaultAdmin); + assertEq(hubPool.owner(), defaultAdmin); + + hubPool.transferOwnership(address(permissionSplitter)); + assertEq(hubPool.owner(), address(permissionSplitter)); + } + + /// forge-config: default.fuzz.runs = 300 + function testFallback(bytes4 randomFuncSelector, bytes memory randomCalldata) public { + // random funcSelector doesn't collide with hub pool: + for (uint256 i = 0; i < hubPoolSelectors.length; i++) { + if (hubPoolSelectors[i] == randomFuncSelector) vm.assume(false); + } + // random funcSelector doesn't collide with proxy: + for (uint256 i = 0; i < proxySelectors.length; i++) { + if (proxySelectors[i] == randomFuncSelector) vm.assume(false); + } + // Calling a function that doesn't exist on target or PermissionSplitter calls the HubPool's // fallback function which wraps any msg.value into wrapped native token. uint256 balBefore = address(hubPool).balance; // Calling fake function as admin with no value succeeds and does nothing. vm.prank(defaultAdmin); - (bool success1, ) = address(hubPoolProxy).call("doesNotExist()"); + (bool success1, ) = address(hubPoolProxy).call(abi.encodeWithSelector(randomFuncSelector, randomCalldata)); assertTrue(success1); // Calling fake function as admin with value also succeeds and wraps the msg.value // and then does nothing. vm.deal(defaultAdmin, 1 ether); vm.prank(defaultAdmin); - (bool success2, bytes memory reason) = address(hubPoolProxy).call{ value: 1 ether }("doesNotExist()"); + (bool success2, ) = address(hubPoolProxy).call{ value: 1 ether }( + abi.encodeWithSelector(randomFuncSelector, randomCalldata) + ); assertTrue(success2); assertEq(address(hubPool).balance, balBefore); } + function testReceive() public { + // Sending ETH to the proxy should trigger the target's receive() function, which on the HubPool wraps + // any msg.value into wrapped native token. + uint256 balBefore = address(hubPool).balance; + + vm.deal(defaultAdmin, 1 ether); + vm.prank(defaultAdmin); + (bool success, ) = address(hubPoolProxy).call{ value: 1 ether }(""); + assertTrue(success); + assertEq(address(hubPool).balance, balBefore); + } + /// forge-config: default.fuzz.runs = 100 function testFunctionSelectorCollisions(uint256 hubPoolFuncSelectorIdx, uint256 proxyFuncSelectorIdx) public { vm.assume(hubPoolFuncSelectorIdx < hubPoolSelectors.length); From 31d12dcbab92b491787def4b748a9bc9a722b540 Mon Sep 17 00:00:00 2001 From: nicholaspai Date: Thu, 25 Jan 2024 10:58:36 -0500 Subject: [PATCH 20/20] Update PermissionSplitter.t.sol --- test/foundry/PermissionSplitter.t.sol | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/test/foundry/PermissionSplitter.t.sol b/test/foundry/PermissionSplitter.t.sol index 3ee6f479d..46704c19b 100644 --- a/test/foundry/PermissionSplitter.t.sol +++ b/test/foundry/PermissionSplitter.t.sol @@ -222,6 +222,8 @@ contract PermissionSplitterTest is Test { ); assertTrue(success2); assertEq(address(hubPool).balance, balBefore); + // Proxy shouldn't hold any ETH + assertEq(address(hubPoolProxy).balance, 0); } function testReceive() public { @@ -234,6 +236,8 @@ contract PermissionSplitterTest is Test { (bool success, ) = address(hubPoolProxy).call{ value: 1 ether }(""); assertTrue(success); assertEq(address(hubPool).balance, balBefore); + // Proxy shouldn't hold any ETH + assertEq(address(hubPoolProxy).balance, 0); } /// forge-config: default.fuzz.runs = 100