a bug in agreement library causing non super app callbacks in IDA case (

#246) * a bug in agreement library causing non super app callbacks in IDA case 1. due to dividends rights token are no longer part of the test suite, this regression was not detected. 2. IDA full test suite should cover this * bump ethereum-contracts version
superfluid-org · Feb 8, 2021 · ec60bf6 · ec60bf6
1 parent 869fd77
commit ec60bf6
Show file tree

Hide file tree

Showing 4 changed files with 61 additions and 51 deletions.
diff --git a/packages/ethereum-contracts/contracts/agreements/AgreementLibrary.sol b/packages/ethereum-contracts/contracts/agreements/AgreementLibrary.sol
@@ -49,6 +49,7 @@ library AgreementLibrary {
      *************************************************************************/
 
     struct CallbackInputs {
+        bool isSuperApp;
         uint256 noopMask;
         ISuperfluidToken token;
         address account;
@@ -73,9 +74,14 @@ library AgreementLibrary {
         inputs.account = account;
         inputs.agreementId = agreementId;
         inputs.agreementData = agreementData;
-        (bool isSuperApp, bool isJailed, uint256 noopMask) = host.getAppManifest(ISuperApp(account));
-        // skip the callbacks if the app is already jailed
-        inputs.noopMask = isSuperApp && !isJailed ? noopMask : type(uint256).max;
+        {
+            bool isJailed;
+            (inputs.isSuperApp, isJailed, inputs.noopMask) = host.getAppManifest(ISuperApp(account));
+            // skip the callbacks if the app is already jailed
+            if (isJailed) {
+                inputs.noopMask = type(uint256).max;
+            }
+        }
     }
 
     function callAppBeforeCallback(
@@ -85,25 +91,27 @@ library AgreementLibrary {
         internal
         returns(bytes memory cbdata)
     {
-        // this will check composit app whitelisting, do not skip!
-        // otherwise an app could be trapped into an agreement
-        bytes memory appCtx = _pushCallbackStack(ctx, inputs);
-        if ((inputs.noopMask & inputs.noopBit) == 0) {
-            bytes memory callData = abi.encodeWithSelector(
-                _selectorFromNoopBit(inputs.noopBit),
-                inputs.token,
-                address(this) /* agreementClass */,
-                inputs.agreementId,
-                inputs.agreementData,
-                new bytes(0) // placeholder ctx
-            );
-            cbdata = ISuperfluid(msg.sender).callAppBeforeCallback(
-                ISuperApp(inputs.account),
-                callData,
-                inputs.noopBit == SuperAppDefinitions.BEFORE_AGREEMENT_TERMINATED_NOOP,
-                appCtx);
+        if (inputs.isSuperApp) {
+            // this will check composit app whitelisting, do not skip!
+            // otherwise an app could be trapped into an agreement:
+            bytes memory appCtx = _pushCallbackStack(ctx, inputs);
+            if ((inputs.noopMask & inputs.noopBit) == 0) {
+                bytes memory callData = abi.encodeWithSelector(
+                    _selectorFromNoopBit(inputs.noopBit),
+                    inputs.token,
+                    address(this) /* agreementClass */,
+                    inputs.agreementId,
+                    inputs.agreementData,
+                    new bytes(0) // placeholder ctx
+                );
+                cbdata = ISuperfluid(msg.sender).callAppBeforeCallback(
+                    ISuperApp(inputs.account),
+                    callData,
+                    inputs.noopBit == SuperAppDefinitions.BEFORE_AGREEMENT_TERMINATED_NOOP,
+                    appCtx);
+            }
+            _popCallbackStatck(ctx, 0);
         }
-        _popCallbackStatck(ctx, 0);
     }
 
     function callAppAfterCallback(
@@ -114,33 +122,35 @@ library AgreementLibrary {
         internal
         returns (ISuperfluid.Context memory appContext, bytes memory appCtx)
     {
-        // this will check composit app whitelisting, do not skip!
-        // otherwise an app could be trapped into an agreement
-        appCtx = _pushCallbackStack(ctx, inputs);
-        if ((inputs.noopMask & inputs.noopBit) == 0) {
-            bytes memory callData = abi.encodeWithSelector(
-                _selectorFromNoopBit(inputs.noopBit),
-                inputs.token,
-                address(this) /* agreementClass */,
-                inputs.agreementId,
-                inputs.agreementData,
-                cbdata,
-                new bytes(0) // placeholder ctx
-            );
-            appCtx = ISuperfluid(msg.sender).callAppAfterCallback(
-                ISuperApp(inputs.account),
-                callData,
-                inputs.noopBit == SuperAppDefinitions.AFTER_AGREEMENT_TERMINATED_NOOP,
-                appCtx);
-
-            appContext = ISuperfluid(msg.sender).decodeCtx(appCtx);
-
-            // adjust allowance used to the range [appAllowanceWanted..appAllowanceGranted]
-            appContext.appAllowanceUsed = max(0, min(
-                inputs.appAllowanceGranted.toInt256(),
-                max(appContext.appAllowanceWanted.toInt256(), appContext.appAllowanceUsed)));
-
-            appCtx = _popCallbackStatck(ctx, appContext.appAllowanceUsed);
+        if (inputs.isSuperApp) {
+            // this will check composit app whitelisting, do not skip!
+            // otherwise an app could be trapped into an agreement
+            appCtx = _pushCallbackStack(ctx, inputs);
+            if ((inputs.noopMask & inputs.noopBit) == 0) {
+                bytes memory callData = abi.encodeWithSelector(
+                    _selectorFromNoopBit(inputs.noopBit),
+                    inputs.token,
+                    address(this) /* agreementClass */,
+                    inputs.agreementId,
+                    inputs.agreementData,
+                    cbdata,
+                    new bytes(0) // placeholder ctx
+                );
+                appCtx = ISuperfluid(msg.sender).callAppAfterCallback(
+                    ISuperApp(inputs.account),
+                    callData,
+                    inputs.noopBit == SuperAppDefinitions.AFTER_AGREEMENT_TERMINATED_NOOP,
+                    appCtx);
+
+                appContext = ISuperfluid(msg.sender).decodeCtx(appCtx);
+
+                // adjust allowance used to the range [appAllowanceWanted..appAllowanceGranted]
+                appContext.appAllowanceUsed = max(0, min(
+                    inputs.appAllowanceGranted.toInt256(),
+                    max(appContext.appAllowanceWanted.toInt256(), appContext.appAllowanceUsed)));
+
+                appCtx = _popCallbackStatck(ctx, appContext.appAllowanceUsed);
+            }
         }
     }
 

diff --git a/packages/ethereum-contracts/package.json b/packages/ethereum-contracts/package.json
@@ -1,6 +1,6 @@
 {
     "name": "@superfluid-finance/ethereum-contracts",
-    "version": "0.2.4",
+    "version": "0.2.5",
     "description": " Ethereum contracts implementation for the Superfluid Protocol",
     "repository": "github:superfluid-finance/protocol-monorepo",
     "license": "AGPL-3.0",

diff --git a/packages/ethereum-contracts/test/contracts/superfluid/Superfluid.test.js b/packages/ethereum-contracts/test/contracts/superfluid/Superfluid.test.js
@@ -1055,7 +1055,7 @@ contract("Superfluid Host Contract", accounts => {
                     );
                 });
 
-                it("#6.24 beforeCreated try to burn just enough gas", async () => {
+                it("#6.24 beforeCreated try to burn just enough gas [ @skip-on-coverage ]", async () => {
                     const actionOverhead = 20000; /* some action overhead */
                     const setNextAction = async () => {
                         await app.setNextCallbackAction(

diff --git a/packages/js-sdk/package.json b/packages/js-sdk/package.json
@@ -42,7 +42,7 @@
         "@ethersproject/contracts": "^5.0.10",
         "@ethersproject/providers": "^5.0.21",
         "@openzeppelin/test-helpers": "0.5.10",
-        "@superfluid-finance/ethereum-contracts": "^0.2.4",
+        "@superfluid-finance/ethereum-contracts": "^0.2.5",
         "chai-as-promised": "7.1.1",
         "truffle": "5.1.49",
         "web3": "^1.3.3",