[ETHEREUM-CONTRACTS] add missing batch call operations (#1967)

* added batch call operations for upgradeTo and downgradeTo * added test case for connecting a pool via batch call * added OPERATION_TYPE_SIMPLE_FORWARD_CALL * added OPERATION_TYPE_ERC2771_FORWARD_CALL * forward native tokens with the first generic external call * fix contract size * appease linter * more comments, fixed value forwarding via 2771 * more verbose documentation * fix hotfuzz init * add dmzfwd to deploy script * adjust mock constructor * disable code to stay inside contract size limit * this is insanity * fix test * fix deployment test * disable only part of the replacePlaceholderCtx test * revert unrelated change * try failing test only * fixed the flakiness source and undid the undo * updated CHANGELOG * allow to withdraw native tokens stuck in DMZForwarder * made host.forwardBatchCall payable too * rename to GeneralDistributionAgreementV1.prop.t.sol * update foundry * use assembly destructor to have different error code * uncomment out test code * update foundry version * [ETHEREUM-CONTRACTS]: typo fixes * [ETHEREUM-CONTRACTS] rename prop.sol to prop.t.sol * fix build warning * ignore mocks from hardhat build * use assembly selfdestruct for now * fix build warning * update CHANGELOG --------- Co-authored-by: Miao, ZhiCheng <[email protected]>
superfluid-finance · Jul 2, 2024 · 7e943b7 · 7e943b7
1 parent 5875f1a
commit 7e943b7
Show file tree

Hide file tree

Showing 17 changed files with 725 additions and 43 deletions.
diff --git a/packages/ethereum-contracts/CHANGELOG.md b/packages/ethereum-contracts/CHANGELOG.md
@@ -9,6 +9,16 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 
 - ISuperfuidPool self-transfer is not allowed
 
+### Added
+
+- `batchCall` now supports 4 additional operation types:
+  - `OPERATION_TYPE_SUPERTOKEN_UPGRADE_TO`
+  - `OPERATION_TYPE_SUPERTOKEN_DOWNGRADE_TO`
+  - `OPERATION_TYPE_SIMPLE_FORWARD_CALL`
+  - `OPERATION_TYPE_ERC2771_FORWARD_CALL`
+
+  The latter 2 allow to add arbitrary contract calls to batch call.
+
 ### Changed
 
 - fix a few types and build warnings

diff --git a/packages/ethereum-contracts/contracts/interfaces/superfluid/Definitions.sol b/packages/ethereum-contracts/contracts/interfaces/superfluid/Definitions.sol
@@ -192,6 +192,24 @@ library BatchOperation {
      * )
      */
     uint32 constant internal OPERATION_TYPE_SUPERTOKEN_DOWNGRADE = 2 + 100;
+    /**
+     * @dev SuperToken.upgradeTo batch operation type
+     *
+     * Call spec:
+     * ISuperToken(target).operationUpgradeTo(
+     *     abi.decode(data, (address to, uint256 amount)
+     * )
+     */
+    uint32 constant internal OPERATION_TYPE_SUPERTOKEN_UPGRADE_TO = 3 + 100;
+    /**
+     * @dev SuperToken.downgradeTo batch operation type
+     *
+     * Call spec:
+     * ISuperToken(target).operationDowngradeTo(
+     *     abi.decode(data, (address to, uint256 amount)
+     * )
+     */
+    uint32 constant internal OPERATION_TYPE_SUPERTOKEN_DOWNGRADE_TO = 4 + 100;
     /**
      * @dev Superfluid.callAgreement batch operation type
      *
@@ -212,6 +230,41 @@ library BatchOperation {
      * )
      */
     uint32 constant internal OPERATION_TYPE_SUPERFLUID_CALL_APP_ACTION = 2 + 200;
+    /**
+     * @dev DMZForwarder.forwardCall batch operation type
+     *
+     * Call spec:
+     * forwardCall(
+     *     target,
+     *     data
+     * )
+     */
+    uint32 constant internal OPERATION_TYPE_SIMPLE_FORWARD_CALL = 1 + 300;
+    /**
+     * @dev DMZForwarder.forward2771Call batch operation type
+     *
+     * Call spec:
+     * forward2771Call(
+     *     target,
+     *     msgSender,
+     *     data
+     * )
+     *
+     * NOTE: In the context of this operation, the `DZMForwarder` contract acts as the
+     * _trusted forwarder_ which must be trusted by the _recipient contract_ (operation target).
+     * It shall do so by dynamically looking up the DMZForwarder used by the host, like this:
+     *
+     * function isTrustedForwarder(address forwarder) public view returns(bool) {
+     *     return forwarder == address(host.DMZ_FORWARDER());
+     * }
+     *
+     * If used in the context of a `forwardBatchCall`, we effectively have a chaining/nesting
+     * of ERC-2771 calls where the host acts as _recipient contract_ of the enveloping 2771 call
+     * and the DMZForwarder acts as the _trusted forwarder_ of the nested 2771 call(s).
+     * That's why `msgSender` could be either the actual `msg.sender` (if using `batchCall`)
+     * or the relayed sender address (if using `forwardBatchCall`).
+     */
+    uint32 constant internal OPERATION_TYPE_ERC2771_FORWARD_CALL = 2 + 300;
 }
 
 /**

diff --git a/packages/ethereum-contracts/contracts/interfaces/superfluid/ISuperToken.sol b/packages/ethereum-contracts/contracts/interfaces/superfluid/ISuperToken.sol
@@ -578,6 +578,28 @@ interface ISuperToken is ISuperfluidToken, IERC20Metadata, IERC777 {
     */
     function operationDowngrade(address account, uint256 amount) external;
 
+    /**
+    * @dev Upgrade ERC20 to SuperToken by host contract and transfer immediately.
+    * @param account The account to be changed.
+    * @param to The account to receive upgraded tokens
+    * @param amount Number of tokens to be upgraded (in 18 decimals)
+    *
+    * @custom:modifiers
+    *  - onlyHost
+    */
+    function operationUpgradeTo(address account, address to, uint256 amount) external;
+
+    /**
+    * @dev Downgrade ERC20 to SuperToken by host contract and transfer immediately.
+    * @param account The account to be changed.
+    * @param to The account to receive downgraded tokens
+    * @param amount Number of tokens to be downgraded (in 18 decimals)
+    *
+    * @custom:modifiers
+    *  - onlyHost
+    */
+    function operationDowngradeTo(address account, address to, uint256 amount) external;
+
     // Flow NFT events
     /**
      * @dev Constant Outflow NFT proxy created event

diff --git a/packages/ethereum-contracts/contracts/interfaces/superfluid/ISuperfluid.sol b/packages/ethereum-contracts/contracts/interfaces/superfluid/ISuperfluid.sol
@@ -622,14 +622,31 @@ interface ISuperfluid {
     /**
      * @dev Batch call function
      * @param operations Array of batch operations
+     *
+     * NOTE: `batchCall` is `payable, because there's limited support for sending
+     * native tokens to batch operation targets.
+     * If value is > 0, the whole amount is sent to the first operation matching any of:
+     * - OPERATION_TYPE_SUPERFLUID_CALL_APP_ACTION
+     * - OPERATION_TYPE_SIMPLE_FORWARD_CALL
+     * - OPERATION_TYPE_ERC2771_FORWARD_CALL
+     * If the first such operation does not allow receiving native tokens,
+     * the transaction will revert.
+     * It's currently not possible to send native tokens to multiple operations, or to
+     * any but the first operation of one of the above mentioned types.
+     * If no such operation is included, the native tokens will be sent back to the sender.
      */
     function batchCall(Operation[] calldata operations) external payable;
 
     /**
-     * @dev Batch call function for trusted forwarders (EIP-2771)
+     * @dev Batch call function with EIP-2771 encoded msgSender
      * @param operations Array of batch operations
+     *
+     * NOTE: This can be called only by contracts recognized as _trusted forwarder_
+     * by the host contract (see `Superfluid.isTrustedForwarder`).
+     * If native tokens are passed along, the same rules as for `batchCall` apply,
+     * with an optional refund going to the encoded msgSender.
      */
-    function forwardBatchCall(Operation[] calldata operations) external;
+    function forwardBatchCall(Operation[] calldata operations) external payable;
 
     /**************************************************************************
      * Function modifiers for access control and parameter validations

diff --git a/packages/ethereum-contracts/contracts/mocks/SuperfluidMock.sol b/packages/ethereum-contracts/contracts/mocks/SuperfluidMock.sol
@@ -11,7 +11,7 @@ import { CallUtils } from "../libs/CallUtils.sol";
 
 contract SuperfluidUpgradabilityTester is Superfluid {
 
-    constructor() Superfluid(false, false)
+    constructor() Superfluid(false, false, address(0))
     // solhint-disable-next-line no-empty-blocks
     {
     }
@@ -129,9 +129,8 @@ contract SuperfluidUpgradabilityTester is Superfluid {
 
 contract SuperfluidMock is Superfluid {
 
-
-    constructor(bool nonUpgradable, bool appWhiteListingEnabled)
-        Superfluid(nonUpgradable, appWhiteListingEnabled)
+    constructor(bool nonUpgradable, bool appWhiteListingEnabled, address dmzForwarder)
+        Superfluid(nonUpgradable, appWhiteListingEnabled, dmzForwarder)
     // solhint-disable-next-line no-empty-blocks
     {
     }
@@ -173,5 +172,4 @@ contract SuperfluidMock is Superfluid {
     {
         _jailApp(app, 6942);
     }
-
 }
diff --git a/packages/ethereum-contracts/contracts/superfluid/SuperToken.sol b/packages/ethereum-contracts/contracts/superfluid/SuperToken.sol
@@ -882,6 +882,20 @@ contract SuperToken is
         _downgrade(msg.sender, account, account, amount, "", "");
     }
 
+    function operationUpgradeTo(address account, address to, uint256 amount)
+        external virtual override
+        onlyHost
+    {
+        _upgrade(msg.sender, account, to, amount, "", "");
+    }
+
+    function operationDowngradeTo(address account, address to, uint256 amount)
+        external virtual override
+        onlyHost
+    {
+        _downgrade(msg.sender, account, to, amount, "", "");
+    }
+
     /**************************************************************************
     * Modifiers
     *************************************************************************/

diff --git a/packages/ethereum-contracts/contracts/superfluid/Superfluid.sol b/packages/ethereum-contracts/contracts/superfluid/Superfluid.sol
@@ -23,6 +23,7 @@ import { GeneralDistributionAgreementV1 } from "../agreements/gdav1/GeneralDistr
 import { SuperfluidUpgradeableBeacon } from "../upgradability/SuperfluidUpgradeableBeacon.sol";
 import { CallUtils } from "../libs/CallUtils.sol";
 import { BaseRelayRecipient } from "../libs/BaseRelayRecipient.sol";
+import { DMZForwarder } from "../utils/DMZForwarder.sol";
 
 /**
  * @dev The Superfluid host implementation.
@@ -51,6 +52,8 @@ contract Superfluid is
     // solhint-disable-next-line var-name-mixedcase
     bool immutable public APP_WHITE_LISTING_ENABLED;
 
+    DMZForwarder immutable public DMZ_FORWARDER;
+
     /**
      * @dev Maximum number of level of apps can be composed together
      *
@@ -95,9 +98,10 @@ contract Superfluid is
     /// function in its respective mock contract to ensure that it doesn't break anything or lead to unexpected
     /// behaviors/layout when upgrading
 
-    constructor(bool nonUpgradable, bool appWhiteListingEnabled) {
+    constructor(bool nonUpgradable, bool appWhiteListingEnabled, address dmzForwarderAddress) {
         NON_UPGRADABLE_DEPLOYMENT = nonUpgradable;
         APP_WHITE_LISTING_ENABLED = appWhiteListingEnabled;
+        DMZ_FORWARDER = DMZForwarder(dmzForwarderAddress);
     }
 
     ////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
@@ -312,7 +316,7 @@ contract Superfluid is
     ////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
     // Superfluid Upgradeable Beacon
     ////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
-    
+
     /// @inheritdoc ISuperfluid
     function updatePoolBeaconLogic(address newLogic) external override onlyGovernance {
         GeneralDistributionAgreementV1 gda = GeneralDistributionAgreementV1(
@@ -794,12 +798,11 @@ contract Superfluid is
     **************************************************************************/
 
     function _batchCall(
-        address msgSender,
+        address payable msgSender,
         Operation[] calldata operations
     )
        internal
     {
-        bool valueForwarded = false;
         for (uint256 i = 0; i < operations.length; ++i) {
             uint32 operationType = operations[i].operationType;
             if (operationType == BatchOperation.OPERATION_TYPE_ERC20_APPROVE) {
@@ -847,27 +850,61 @@ contract Superfluid is
                 ISuperToken(operations[i].target).operationDowngrade(
                     msgSender,
                     abi.decode(operations[i].data, (uint256))); // amount
+            } else if (operationType == BatchOperation.OPERATION_TYPE_SUPERTOKEN_UPGRADE_TO) {
+                (address to, uint256 amount) = abi.decode(operations[i].data, (address, uint256));
+                ISuperToken(operations[i].target).operationUpgradeTo(
+                    msgSender,
+                    to,
+                    amount);
+            } else if (operationType == BatchOperation.OPERATION_TYPE_SUPERTOKEN_DOWNGRADE_TO) {
+                (address to, uint256 amount) = abi.decode(operations[i].data, (address, uint256));
+                ISuperToken(operations[i].target).operationDowngradeTo(
+                    msgSender,
+                    to,
+                    amount);
             } else if (operationType == BatchOperation.OPERATION_TYPE_SUPERFLUID_CALL_AGREEMENT) {
                 (bytes memory callData, bytes memory userData) = abi.decode(operations[i].data, (bytes, bytes));
                 _callAgreement(
                     msgSender,
                     ISuperAgreement(operations[i].target),
                     callData,
                     userData);
-            } else if (operationType == BatchOperation.OPERATION_TYPE_SUPERFLUID_CALL_APP_ACTION) {
+            }
+            // The following operations for call proxies allow forwarding of native tokens.
+            // we use `address(this).balance` instead of `msg.value`, because the latter ist not
+            // updated after forwarding to the first operation, while `balance` is.
+            // The initial balance is equal to `msg.value` because there's no other path
+            // for the contract to receive native tokens.
+            else if (operationType == BatchOperation.OPERATION_TYPE_SUPERFLUID_CALL_APP_ACTION) {
                 _callAppAction(
                     msgSender,
                     ISuperApp(operations[i].target),
-                    valueForwarded ? 0 : msg.value,
+                    address(this).balance,
                     operations[i].data);
-                valueForwarded = true;
+            } else if (operationType == BatchOperation.OPERATION_TYPE_SIMPLE_FORWARD_CALL) {
+                (bool success, bytes memory returnData) =
+                    DMZ_FORWARDER.forwardCall{value: address(this).balance}(
+                        operations[i].target,
+                        operations[i].data);
+                if (!success) {
+                    CallUtils.revertFromReturnedData(returnData);
+                }
+            } else if (operationType == BatchOperation.OPERATION_TYPE_ERC2771_FORWARD_CALL) {
+                (bool success, bytes memory returnData) =
+                    DMZ_FORWARDER.forward2771Call{value: address(this).balance}(
+                        operations[i].target,
+                        msgSender,
+                        operations[i].data);
+                if (!success) {
+                    CallUtils.revertFromReturnedData(returnData);
+                }
             } else {
                revert HOST_UNKNOWN_BATCH_CALL_OPERATION_TYPE();
             }
         }
-        if (msg.value != 0 && !valueForwarded) {
-            // return ETH provided if not forwarded
-            payable(msg.sender).transfer(msg.value);
+        if (address(this).balance != 0) {
+            // return any native tokens left to the sender.
+            msgSender.transfer(address(this).balance);
         }
     }
 
@@ -877,12 +914,12 @@ contract Superfluid is
     )
        external override payable
     {
-        _batchCall(msg.sender, operations);
+        _batchCall(payable(msg.sender), operations);
     }
 
     /// @dev ISuperfluid.forwardBatchCall implementation
     function forwardBatchCall(Operation[] calldata operations)
-        external override
+        external override payable
     {
         _batchCall(_getTransactionSigner(), operations);
     }
@@ -899,7 +936,7 @@ contract Superfluid is
         ) != 0;
     }
 
-    /// @dev IRelayRecipient.isTrustedForwarder implementation
+    /// @dev IRelayRecipient.versionRecipient implementation
     function versionRecipient()
         external override pure
         returns (string memory)

diff --git a/packages/ethereum-contracts/contracts/utils/DMZForwarder.sol b/packages/ethereum-contracts/contracts/utils/DMZForwarder.sol
@@ -0,0 +1,50 @@
+// SPDX-License-Identifier: AGPLv3
+pragma solidity 0.8.23;
+
+import { Ownable } from "@openzeppelin/contracts/access/Ownable.sol";
+
+/**
+ * @title DMZForwarder
+ * @dev The purpose of this contract is to make arbitrary contract calls batchable
+ * alongside Superfluid specific batch operations.
+ * We route the calls through this dedicated contract in order to not have msg.sender set
+ * to the host contract, for security reasons.
+ * Forwarded calls can optionally use ERC-2771 to preserve the original msg.sender.
+ * If native tokens (msg.value) are provided, they are forwarded as well.
+ */
+contract DMZForwarder is Ownable {
+    /**
+     * @dev Forwards a call for which msg.sender doesn't matter
+     * @param target The target contract to call
+     * @param data The call data
+     */
+    function forwardCall(address target, bytes memory data)
+        external payable
+        returns(bool success, bytes memory returnData)
+    {
+        // solhint-disable-next-line avoid-low-level-calls
+        (success, returnData) = target.call{value: msg.value}(data);
+    }
+
+    /**
+     * @dev Forwards a call passing along the original msg.sender encoded as specified in ERC-2771.
+     * @param target The target contract to call
+     * @param msgSender The original msg.sender passed along by the trusted contract owner
+     * @param data The call data
+     */
+    function forward2771Call(address target, address msgSender, bytes memory data)
+        external payable onlyOwner
+        returns(bool success, bytes memory returnData)
+    {
+        // solhint-disable-next-line avoid-low-level-calls
+        (success, returnData) = target.call{value: msg.value}(abi.encodePacked(data, msgSender));
+    }
+
+    /**
+     * @dev Allows to withdraw native tokens (ETH) which got stuck in this contract.
+     * This could happen if a call fails, but the caller doesn't revert the tx.
+     */
+    function withdrawLostNativeTokens(address payable receiver) external onlyOwner {
+        receiver.transfer(address(this).balance);
+    }
+}