refactor: reduce gsn paymaster validation and allow additional callers

Author: SeanJCasey

contracts/release/infrastructure/gas-relayer/GasRelayPaymasterLib.sol

@@ -29,9 +29,17 @@ import {IGasRelayPaymasterDepositor} from "./IGasRelayPaymasterDepositor.sol";
 /// @title GasRelayPaymasterLib Contract
 /// @author Enzyme Council <[email protected]>
 /// @notice The core logic library for the "paymaster" contract which refunds GSN relayers
+/// @dev Allows any permissioned user of the fund to relay any call,
+/// without validation of the target of the call itself.
+/// Funds with untrusted permissioned users should monitor for abuse (i.e., relaying personal calls).
+/// The extent of abuse is throttled by `DEPOSIT_COOLDOWN` and `DEPOSIT_MAX_TOTAL`.
 contract GasRelayPaymasterLib is IGasRelayPaymaster, GasRelayPaymasterLibBase2 {
     using SafeMath for uint256;
 
+    event AdditionalRelayUserAdded(address indexed account);
+
+    event AdditionalRelayUserRemoved(address indexed account);
+
     // Immutable and constants
     // Sane defaults, subject to change after gas profiling
     uint256 private constant CALLDATA_SIZE_LIMIT = 10500;
@@ -50,11 +58,18 @@ contract GasRelayPaymasterLib is IGasRelayPaymaster, GasRelayPaymasterLibBase2 {
     address private immutable TRUSTED_FORWARDER;
     address private immutable WETH_TOKEN;
 
+    mapping(address => bool) private accountToIsAdditionalRelayUser;
+
     modifier onlyComptroller() {
         require(msg.sender == getParentComptroller(), "Can only be called by the parent comptroller");
         _;
     }
 
+    modifier onlyFundOwner() {
+        require(__msgSender() == IVault(getParentVault()).getOwner(), "Only the fund owner can call this function");
+        _;
+    }
+
     modifier relayHubOnly() {
         require(msg.sender == getHubAddr(), "Can only be called by RelayHub");
         _;
@@ -110,15 +125,19 @@ contract GasRelayPaymasterLib is IGasRelayPaymaster, GasRelayPaymasterLibBase2 {
         require(_relayRequest.relayData.baseRelayFee <= RELAY_FEE_MAX_BASE, "preRelayedCall: High baseRelayFee");
         require(_relayRequest.relayData.pctRelayFee <= RELAY_FEE_MAX_PERCENT, "preRelayedCall: High pctRelayFee");
 
-        address vaultProxy = getParentVault();
-        require(IVault(vaultProxy).canRelayCalls(_relayRequest.request.from), "preRelayedCall: Unauthorized caller");
+        // No Enzyme txs require msg.value
+        require(_relayRequest.request.value == 0, "preRelayedCall: Non-zero value");
 
-        bytes4 selector = __parseTxDataFunctionSelector(_relayRequest.request.data);
+        // Allow any transaction, as long as it's from a permissioned account for the fund
+        address vaultProxy = getParentVault();
         require(
-            __isAllowedCall(vaultProxy, _relayRequest.request.to, selector, _relayRequest.request.data),
-            "preRelayedCall: Function call not permitted"
+            IVault(vaultProxy).canRelayCalls(_relayRequest.request.from)
+                || isAdditionalRelayUser(_relayRequest.request.from),
+            "preRelayedCall: Unauthorized caller"
         );
 
+        bytes4 selector = __parseTxDataFunctionSelector(_relayRequest.request.data);
+
         return (abi.encode(_relayRequest.request.from, selector), false);
     }
 
@@ -143,8 +162,9 @@ contract GasRelayPaymasterLib is IGasRelayPaymaster, GasRelayPaymasterLibBase2 {
     /// @notice Send any deposited ETH back to the vault
     function withdrawBalance() external override {
         address vaultProxy = getParentVault();
+        address canonicalSender = __msgSender();
         require(
-            msg.sender == IVault(vaultProxy).getOwner() || msg.sender == __getComptrollerForVault(vaultProxy),
+            canonicalSender == IVault(vaultProxy).getOwner() || canonicalSender == __getComptrollerForVault(vaultProxy),
             "withdrawBalance: Only owner or comptroller is authorized"
         );
 
@@ -197,65 +217,18 @@ contract GasRelayPaymasterLib is IGasRelayPaymaster, GasRelayPaymasterLibBase2 {
         return IVault(_vaultProxy).getAccessor();
     }
 
-    /// @dev Helper to check if a contract call is allowed to be relayed using this paymaster
-    /// Allowed contracts are:
-    /// - VaultProxy
-    /// - ComptrollerProxy
-    /// - PolicyManager
-    /// - FundDeployer
-    function __isAllowedCall(address _vaultProxy, address _contract, bytes4 _selector, bytes calldata _txData)
-        private
-        view
-        returns (bool allowed_)
-    {
-        if (_contract == _vaultProxy) {
-            // All calls to the VaultProxy are allowed
-            return true;
-        }
-
-        address parentComptroller = __getComptrollerForVault(_vaultProxy);
-        if (_contract == parentComptroller) {
-            if (
-                _selector == IComptroller.callOnExtension.selector
-                    || _selector == IComptroller.vaultCallOnContract.selector
-                    || _selector == IComptroller.buyBackProtocolFeeShares.selector
-                    || _selector == IComptroller.depositToGasRelayPaymaster.selector
-                    || _selector == IComptroller.setAutoProtocolFeeSharesBuyback.selector
-            ) {
-                return true;
-            }
-        } else if (_contract == IComptroller(parentComptroller).getPolicyManager()) {
-            if (
-                _selector == IPolicyManager.updatePolicySettingsForFund.selector
-                    || _selector == IPolicyManager.enablePolicyForFund.selector
-                    || _selector == IPolicyManager.disablePolicyForFund.selector
-            ) {
-                return __parseTxDataFirstParameterAsAddress(_txData) == getParentComptroller();
+    /// @dev Helper to parse the canonical msg sender from trusted forwarder relayed calls
+    /// See https://github.com/opengsn/gsn/blob/da4222b76e3ae1968608dc5c5d80074dcac7c4be/packages/contracts/src/ERC2771Recipient.sol#L41-L53
+    function __msgSender() internal view returns (address canonicalSender_) {
+        if (msg.data.length >= 20 && msg.sender == TRUSTED_FORWARDER) {
+            assembly {
+                canonicalSender_ := shr(96, calldataload(sub(calldatasize(), 20)))
             }
-        } else if (_contract == IComptroller(parentComptroller).getFundDeployer()) {
-            if (
-                _selector == IFundDeployer.createReconfigurationRequest.selector
-                    || _selector == IFundDeployer.executeReconfiguration.selector
-                    || _selector == IFundDeployer.cancelReconfiguration.selector
-            ) {
-                return __parseTxDataFirstParameterAsAddress(_txData) == getParentVault();
-            }
-        }
 
-        return false;
-    }
-
-    /// @notice Parses the first parameter of tx data as an address
-    /// @param _txData The tx data to retrieve the address from
-    /// @return retrievedAddress_ The extracted address
-    function __parseTxDataFirstParameterAsAddress(bytes calldata _txData)
-        private
-        pure
-        returns (address retrievedAddress_)
-    {
-        require(_txData.length >= 36, "__parseTxDataFirstParameterAsAddress: _txData is not a valid length");
+            return canonicalSender_;
+        }
 
-        return abi.decode(_txData[4:36], (address));
+        return msg.sender;
     }
 
     /// @notice Parses the function selector from tx data
@@ -271,6 +244,36 @@ contract GasRelayPaymasterLib is IGasRelayPaymaster, GasRelayPaymasterLibBase2 {
         return functionSelector_;
     }
 
+    //////////////////////////////////////
+    // REGISTRY: ADDITIONAL RELAY USERS //
+    //////////////////////////////////////
+
+    /// @notice Adds additional relay users
+    /// @param _usersToAdd The users to add
+    function addAdditionalRelayUsers(address[] calldata _usersToAdd) external override onlyFundOwner {
+        for (uint256 i; i < _usersToAdd.length; i++) {
+            address user = _usersToAdd[i];
+            require(!isAdditionalRelayUser(user), "addAdditionalRelayUsers: User registered");
+
+            accountToIsAdditionalRelayUser[user] = true;
+
+            emit AdditionalRelayUserAdded(user);
+        }
+    }
+
+    /// @notice Removes additional relay users
+    /// @param _usersToRemove The users to remove
+    function removeAdditionalRelayUsers(address[] calldata _usersToRemove) external override onlyFundOwner {
+        for (uint256 i; i < _usersToRemove.length; i++) {
+            address user = _usersToRemove[i];
+            require(isAdditionalRelayUser(user), "removeAdditionalRelayUsers: User not registered");
+
+            accountToIsAdditionalRelayUser[user] = false;
+
+            emit AdditionalRelayUserRemoved(user);
+        }
+    }
+
     ///////////////////
     // STATE GETTERS //
     ///////////////////
@@ -313,6 +316,12 @@ contract GasRelayPaymasterLib is IGasRelayPaymaster, GasRelayPaymasterLibBase2 {
         return WETH_TOKEN;
     }
 
+    /// @notice Checks whether an account is an approved additional relayer user
+    /// @return isAdditionalRelayUser_ True if the account is an additional relayer user
+    function isAdditionalRelayUser(address _who) public view override returns (bool isAdditionalRelayUser_) {
+        return accountToIsAdditionalRelayUser[_who];
+    }
+
     /// @notice Gets the `TRUSTED_FORWARDER` variable value
     /// @return trustedForwarder_ The forwarder contract which is trusted to validated the relayed tx signature
     function trustedForwarder() external view override returns (address trustedForwarder_) {

contracts/release/infrastructure/gas-relayer/IGasRelayPaymaster.sol

@@ -17,6 +17,8 @@ import {IGsnPaymaster} from "../../../external-interfaces/IGsnPaymaster.sol";
 /// @title IGasRelayPaymaster Interface
 /// @author Enzyme Council <[email protected]>
 interface IGasRelayPaymaster is IGsnPaymaster {
+    function addAdditionalRelayUsers(address[] calldata _usersToAdd) external;
+
     function deposit() external;
 
     function getLastDepositTimestamp() external view returns (uint256 lastDepositTimestamp_);
@@ -29,5 +31,9 @@ interface IGasRelayPaymaster is IGsnPaymaster {
 
     function init(address _vault) external;
 
+    function isAdditionalRelayUser(address _who) external view returns (bool isAdditionalRelayUser_);
+
+    function removeAdditionalRelayUsers(address[] calldata _usersToRemove) external;
+
     function withdrawBalance() external;
 }

tests/interfaces/external/IGSNForwarder.sol

@@ -0,0 +1,16 @@
+// SPDX-License-Identifier: GPL-3.0
+pragma solidity >=0.6.0 <0.9.0;
+
+interface IGSNForwarder {
+    struct ForwardRequest {
+        address from;
+        address to;
+        uint256 value;
+        uint256 gas;
+        uint256 nonce;
+        bytes data;
+        uint256 validUntil;
+    }
+
+    function getNonce(address _from) external view returns (uint256 nonce_);
+}

tests/interfaces/external/IGSNPaymaster.sol

@@ -0,0 +1,6 @@
+// SPDX-License-Identifier: GPL-3.0
+pragma solidity >=0.6.0 <0.9.0;
+
+interface IGSNPaymaster {
+    function trustedForwarder() external view returns (address trustedForwarder_);
+}

tests/interfaces/external/IGSNRelayHub.sol

@@ -0,0 +1,14 @@
+// SPDX-License-Identifier: GPL-3.0
+pragma solidity >=0.6.0 <0.9.0;
+
+import {IGSNTypes} from "./IGSNTypes.sol";
+
+interface IGSNRelayHub {
+    function relayCall(
+        uint256 _maxAcceptanceBudget,
+        IGSNTypes.RelayRequest calldata _relayRequest,
+        bytes calldata _signature,
+        bytes calldata _approvalData,
+        uint256 _externalGasLimit
+    ) external returns (bool paymasterAccepted_, bytes memory returnValue_);
+}

tests/interfaces/external/IGSNTypes.sol

@@ -0,0 +1,22 @@
+// SPDX-License-Identifier: GPL-3.0
+pragma solidity >=0.6.0 <0.9.0;
+
+import {IGSNForwarder} from "./IGSNForwarder.sol";
+
+interface IGSNTypes {
+    struct RelayData {
+        uint256 gasPrice;
+        uint256 pctRelayFee;
+        uint256 baseRelayFee;
+        address relayWorker;
+        address paymaster;
+        address forwarder;
+        bytes paymasterData;
+        uint256 clientId;
+    }
+
+    struct RelayRequest {
+        IGSNForwarder.ForwardRequest request;
+        RelayData relayData;
+    }
+}