[ETHEREUM-CONTRACTS] remove footgun from CFASuperAppBase

packages/ethereum-contracts/CHANGELOG.md

@@ -20,6 +20,13 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 - `SuperTokenV1Library.distributeFlow`: return `actualFlowRate` instead of a bool
 - `SuperTokenV1Library.distribute`: return `actualAmount` instead of a bool
 
+# Breaking
+- CFASuperAppBase: `onFlowDeleted` from now on only handles events related to incoming flows, while for events triggered by outgoing flows `onOutFlowDeleted` is invoked.
+  This is safer because the latter case is in many cases unexpected and may thus not be handled correctly, potentially leading to state corruption or SuperApp jailing.
+  The change is breaking because of a signature change in `onFlowDeleted`. The removal of the now unnecessary `receiver` argument also makes sure
+  that this change can't without notice break implementations which correctly handled the corner case of an outgoing flow with the previous implementation of CFASuperAppBase.
+  Many applications may not want/need to handle the case of outgoing flows being deleted, thus don't need to override the newly added `onOutFlowDeleted`.
+
 ## [v1.12.0]
 
 ### Added

packages/ethereum-contracts/contracts/apps/CFASuperAppBase.sol

@@ -8,20 +8,27 @@ import { SuperTokenV1Library } from "./SuperTokenV1Library.sol";
  * @title abstract base contract for SuperApps using CFA callbacks
  * @author Superfluid
  * @dev This contract provides a more convenient API for implementing CFA callbacks.
- * It allows to write more concise and readable SuperApps when the full flexibility
- * of the low-level agreement callbacks isn't needed.
- * The API is tailored for the most common use cases, with the "beforeX" and "afterX" callbacks being
+ * It allows to write more concise and readable SuperApps.
+ * The API is tailored for common use cases, with the "beforeX" and "afterX" callbacks being
  * abstrated into a single "onX" callback for create|update|delete flows.
- * For use cases requiring more flexibility (specifically if more data needs to be provided by the before callbacks)
- * it's recommended to implement the low-level callbacks directly instead of using this base contract.
+ * If the previous state provided by this API (`previousFlowRate` and `lastUpdated`) is not sufficient for you use case,
+ * you should implement the more generic low-level API of `ISuperApp` instead of using this base contract.
  */
 abstract contract CFASuperAppBase is ISuperApp {
     using SuperTokenV1Library for ISuperToken;
 
+    /// =================================================================================
+    /// CONSTANTS & IMMUTABLES
+    /// =================================================================================
+
     bytes32 public constant CFAV1_TYPE = keccak256("org.superfluid-finance.agreements.ConstantFlowAgreement.v1");
 
     ISuperfluid public immutable HOST;
 
+    /// =================================================================================
+    /// ERRORS
+    /// =================================================================================
+
     /// @dev Thrown when the callback caller is not the host.
     error UnauthorizedHost();
 
@@ -31,6 +38,10 @@ abstract contract CFASuperAppBase is ISuperApp {
     /// @dev Thrown when SuperTokens not accepted by the SuperApp are streamed to it
     error NotAcceptedSuperToken();
 
+    // =================================================================================
+    // SETUP
+    // =================================================================================
+
     /**
      * @dev Creates the contract tied to the provided Superfluid host
      * @param host_ the Superfluid host the SuperApp belongs to
@@ -49,7 +60,7 @@ abstract contract CFASuperAppBase is ISuperApp {
      *
      * Note: if the App self-registers on a network with permissioned SuperApp registration,
      * self-registration can be used only if the tx.origin (EOA) is whitelisted as deployer.
-     * If a whitelisted factory is used, it needs to call `host.registerApp()` itself.
+     * If instead a whitelisted factory is used, the factory needs to call `host.registerApp(address app)`.
      * For more details, see https://github.com/superfluid-finance/protocol-monorepo/wiki/Super-App-White-listing-Guide
      */
     function selfRegister(
@@ -72,7 +83,9 @@ abstract contract CFASuperAppBase is ISuperApp {
         bool activateOnUpdated,
         bool activateOnDeleted
     ) public pure returns (uint256 configWord) {
+        // since only 1 level is allowed by the protocol, we can hardcode APP_LEVEL_FINAL
         configWord = SuperAppDefinitions.APP_LEVEL_FINAL
+        // there's no information we want to carry over for create
             | SuperAppDefinitions.BEFORE_AGREEMENT_CREATED_NOOP;
         if (!activateOnCreated) {
             configWord |= SuperAppDefinitions.AFTER_AGREEMENT_CREATED_NOOP;
@@ -96,10 +109,9 @@ abstract contract CFASuperAppBase is ISuperApp {
         return true;
     }
 
-
-    // ---------------------------------------------------------------------------------------------
-    // CFA specific convenience callbacks
-    // to be overridden and implemented by inheriting SuperApps
+    // =================================================================================
+    // CFA SPECIFIC CALLBACKS - TO BE OVERRIDDEN BY INHERITING SUPERAPPS
+    // =================================================================================
 
     /// @dev override if the SuperApp shall have custom logic invoked when a new flow
     ///      to it is created.
@@ -130,6 +142,24 @@ abstract contract CFASuperAppBase is ISuperApp {
     function onFlowDeleted(
         ISuperToken /*superToken*/,
         address /*sender*/,
+        int96 /*previousFlowRate*/,
+        uint256 /*lastUpdated*/,
+        bytes calldata ctx
+    ) internal virtual returns (bytes memory /*newCtx*/) {
+        return ctx;
+    }
+
+    /// @dev override if the SuperApp shall have custom logic invoked when an outgoing flow
+    ///      is deleted by the receiver (it's not triggered when deleted by the SuperApp itself).
+    ///      A possible implementation is to make outflows "sticky" by simply reopening it.
+    ///      Like onFlowDeleted, this method is NOT allowed to revert.
+    ///      It's safe to not override this method if the SuperApp doesn't have outgoing flows,
+    ///      or if it doesn't want/need to know if an outgoing flow is deleted by its receiver.
+    /// Note: In theory this hook could also be triggered by a liquidation, but this would imply
+    /// that the SuperApp is insolvent, and would thus be jailed already.
+    /// Thus in practice this is triggered only when a receiver of an outgoing flow deletes that flow.
+    function onOutflowDeleted(
+        ISuperToken /*superToken*/,
         address /*receiver*/,
         int96 /*previousFlowRate*/,
         uint256 /*lastUpdated*/,
@@ -138,12 +168,16 @@ abstract contract CFASuperAppBase is ISuperApp {
         return ctx;
     }
 
+    // =================================================================================
+    // INTERNAL IMPLEMENTATION
+    // =================================================================================
 
-    // ---------------------------------------------------------------------------------------------
-    // Low-level callbacks
-    // Shall NOT be overriden by SuperApps when inheriting from this contract.
-    // The before-callbacks are implemented to forward data (flowrate, timestamp),
-    // the after-callbacks invoke the CFA specific specific convenience callbacks.
+    // The following methods SHALL NOT BE OVERRIDDEN by SuperApps inheriting from this contract.
+    // If more fine grained control than provided by the onX callbacks is needed,
+    // you should implement the more generic low-level API of `ISuperApp` instead of using this base contract.
+
+    // The before-callbacks are implemented to relay data (flowrate, timestamp) to the after-callbacks.
+    // The after-callbacks invoke the more convenient onX callbacks.
 
     // CREATED callback
 
@@ -167,7 +201,7 @@ abstract contract CFASuperAppBase is ISuperApp {
         bytes calldata ctx
     ) external override returns (bytes memory newCtx) {
         if (msg.sender != address(HOST)) revert UnauthorizedHost();
-        if (!isAcceptedAgreement(agreementClass)) return ctx;
+        if (!_isAcceptedAgreement(agreementClass)) return ctx;
         if (!isAcceptedSuperToken(superToken)) revert NotAcceptedSuperToken();
 
         (address sender, ) = abi.decode(agreementData, (address, address));
@@ -190,7 +224,7 @@ abstract contract CFASuperAppBase is ISuperApp {
         bytes calldata /*ctx*/
     ) external view override returns (bytes memory /*beforeData*/) {
         if (msg.sender != address(HOST)) revert UnauthorizedHost();
-        if (!isAcceptedAgreement(agreementClass)) return "0x";
+        if (!_isAcceptedAgreement(agreementClass)) return "0x";
         if (!isAcceptedSuperToken(superToken)) revert NotAcceptedSuperToken();
 
         (address sender, ) = abi.decode(agreementData, (address, address));
@@ -211,7 +245,7 @@ abstract contract CFASuperAppBase is ISuperApp {
         bytes calldata ctx
     ) external override returns (bytes memory newCtx) {
         if (msg.sender != address(HOST)) revert UnauthorizedHost();
-        if (!isAcceptedAgreement(agreementClass)) return ctx;
+        if (!_isAcceptedAgreement(agreementClass)) return ctx;
         if (!isAcceptedSuperToken(superToken)) revert NotAcceptedSuperToken();
 
         (address sender, ) = abi.decode(agreementData, (address, address));
@@ -238,7 +272,7 @@ abstract contract CFASuperAppBase is ISuperApp {
     ) external view override returns (bytes memory /*beforeData*/) {
         // we're not allowed to revert in this callback, thus just return empty beforeData on failing checks
         if (msg.sender != address(HOST)
-            || !isAcceptedAgreement(agreementClass)
+            || !_isAcceptedAgreement(agreementClass)
             || !isAcceptedSuperToken(superToken))
         {
             return "0x";
@@ -263,7 +297,7 @@ abstract contract CFASuperAppBase is ISuperApp {
     ) external override returns (bytes memory newCtx) {
         // we're not allowed to revert in this callback, thus just return ctx on failing checks
         if (msg.sender != address(HOST)
-            || !isAcceptedAgreement(agreementClass)
+            || !_isAcceptedAgreement(agreementClass)
             || !isAcceptedSuperToken(superToken))
         {
             return ctx;
@@ -272,15 +306,25 @@ abstract contract CFASuperAppBase is ISuperApp {
         (address sender, address receiver) = abi.decode(agreementData, (address, address));
         (uint256 lastUpdated, int96 previousFlowRate) = abi.decode(cbdata, (uint256, int96));
 
-        return
-            onFlowDeleted(
-                superToken,
-                sender,
-                receiver,
-                previousFlowRate,
-                lastUpdated,
-                ctx
-            );
+        if (receiver == address(this)) {
+            return
+                onFlowDeleted(
+                    superToken,
+                    sender,
+                    previousFlowRate,
+                    lastUpdated,
+                    ctx
+                );
+        } else {
+            return
+                onOutflowDeleted(
+                    superToken,
+                    receiver,
+                    previousFlowRate,
+                    lastUpdated,
+                    ctx
+                );
+        }
     }
 
 
@@ -292,7 +336,7 @@ abstract contract CFASuperAppBase is ISuperApp {
      *      This function can be overridden with custom logic and to revert if desired
      *      Current implementation expects ConstantFlowAgreement
      */
-    function isAcceptedAgreement(address agreementClass) internal view virtual returns (bool) {
+    function _isAcceptedAgreement(address agreementClass) internal view returns (bool) {
         return agreementClass == address(HOST.getAgreementClass(CFAV1_TYPE));
     }
 }

packages/ethereum-contracts/contracts/apps/SuperAppBase.sol

@@ -5,6 +5,11 @@ pragma solidity >= 0.8.11;
 // solhint-disable-next-line no-global-import
 import "../interfaces/superfluid/ISuperfluid.sol";
 
+/**
+ * @title [DEPRECATED] Base contract which provides a reverting implementation of all ISuperApp methods.
+ * @author Superfluid
+ * @custom:deprecated Use an agreement specific base contract (e.g. `CFASuperAppBase`) or implement `ISuperApp`.
+ */
 abstract contract SuperAppBase is ISuperApp {
 
     function beforeAgreementCreated(

packages/ethereum-contracts/test/foundry/apps/CFASuperAppBase.t.sol

@@ -194,7 +194,7 @@ contract CFASuperAppBaseTest is FoundrySuperfluidTester {
         vm.stopPrank();
     }
 
-    // test delete flow
+    // test delete flow to superApp (incoming flow)
     function testDeleteFlowToSuperApp(int96 flowRate) public {
         flowRate = int96(bound(flowRate, 1, int96(uint96(type(uint32).max))));
         vm.startPrank(alice);
@@ -207,13 +207,27 @@ contract CFASuperAppBaseTest is FoundrySuperfluidTester {
         superToken.deleteFlow(alice, superAppAddress);
         assertEq(superToken.getFlowRate(alice, superAppAddress), 0, "SuperAppBase: deleteFlow2 | flowRate incorrect");
         assertEq(superApp.afterSenderHolder(), alice, "SuperAppBase: deleteFlow2 | afterSenderHolder incorrect");
-        assertEq(
-            superApp.afterReceiverHolder(), superAppAddress, "SuperAppBase: deleteFlow2 | afterReceiverHolder incorrect"
-        );
         assertEq(superApp.oldFlowRateHolder(), flowRate, "SuperAppBase: deleteFlow2 | oldFlowRateHolder incorrect");
         vm.stopPrank();
     }
 
+    // test delete flow from superApp
+    function testDeleteFlowFromSuperApp(int96 flowRate) public {
+        flowRate = int96(bound(flowRate, 1, int96(uint96(type(uint32).max))));
+
+        vm.startPrank(alice);
+        // fund the superApp and start a stream from it to alice
+        superToken.transfer(superAppAddress, 1e18);
+        superApp.startStream(superToken, alice, flowRate);
+
+        // let alice delete the flow, triggering the onOutFlowDeleted callback
+        superToken.deleteFlow(superAppAddress, alice);
+        assertEq(superApp.lastUpdateHolder(), block.timestamp, "SuperAppBase: deleteFlow | lastUpdateHolder incorrect");
+        assertEq(superApp.oldFlowRateHolder(), flowRate, "SuperAppBase: deleteFlow | oldFlowRateHolder incorrect");
+        assertEq(superApp.afterReceiverHolder(), alice, "SuperAppBase: deleteFlow | afterReceiverHolder incorrect");
+        vm.stopPrank();
+    }
+
     function testMockBeforeAgreementCreated() public {
         vm.startPrank(alice);
         bytes memory data = superApp.beforeAgreementCreated(

packages/ethereum-contracts/test/foundry/apps/CFASuperAppBaseTester.t.sol

@@ -75,14 +75,25 @@ contract CFASuperAppBaseTester is CFASuperAppBase {
     function onFlowDeleted(
         ISuperToken, /*superToken*/
         address sender,
-        address receiver,
         int96 previousFlowRate,
         uint256 lastUpdated,
         bytes calldata ctx
     ) internal override returns (bytes memory newCtx) {
         lastUpdateHolder = lastUpdated;
         oldFlowRateHolder = previousFlowRate;
         afterSenderHolder = sender;
+        return ctx;
+    }
+
+    function onOutflowDeleted(
+        ISuperToken, /*superToken*/
+        address receiver,
+        int96 previousFlowRate,
+        uint256 lastUpdated,
+        bytes calldata ctx
+    ) internal override returns (bytes memory newCtx) {
+        lastUpdateHolder = lastUpdated;
+        oldFlowRateHolder = previousFlowRate;
         afterReceiverHolder = receiver;
         return ctx;
     }

packages/ethereum-contracts/test/foundry/apps/SuperAppTester/FlowSplitter.sol

@@ -132,7 +132,6 @@ contract FlowSplitter is CFASuperAppBase {
     function onFlowDeleted(
         ISuperToken superToken,
         address, /*sender*/
-        address receiver,
         int96 previousFlowRate,
         uint256, /*lastUpdated*/
         bytes calldata ctx
@@ -145,11 +144,6 @@ contract FlowSplitter is CFASuperAppBase {
                 + acceptedSuperToken.getFlowRate(address(this), sideReceiver)
         ) - previousFlowRate;
 
-        // handle "rogue recipients" with sticky stream - see readme
-        if (receiver == mainReceiver || receiver == sideReceiver) {
-            newCtx = superToken.createFlowWithCtx(receiver, previousFlowRate, newCtx);
-        }
-
         // if there is no more inflow, outflows should be deleted
         if (remainingInflow <= 0) {
             newCtx = superToken.deleteFlowWithCtx(address(this), mainReceiver, newCtx);
@@ -165,4 +159,19 @@ contract FlowSplitter is CFASuperAppBase {
             newCtx = superToken.updateFlowWithCtx(sideReceiver, (remainingInflow * sideReceiverPortion) / 1000, newCtx);
         }
     }
+
+    function onOutflowDeleted(
+        ISuperToken superToken,
+        address receiver,
+        int96 previousFlowRate,
+        uint256, /*lastUpdated*/
+        bytes calldata ctx
+    ) internal override returns (bytes memory newCtx) {
+        newCtx = ctx;
+
+        // handle "rogue recipients" with sticky stream - see readme
+        if (receiver == mainReceiver || receiver == sideReceiver) {
+            newCtx = superToken.createFlowWithCtx(receiver, previousFlowRate, newCtx);
+        }
+    }
 }