[ETHEREUM-CONTRACTS] Fix nfthooks outofgas (#1880)

* WIP * fuzz test covering all 3 FlowNFT hooks * cleanup * fix: remove try/catch * appease linter * fix gda test * also change in GDA, don't fail if minting a pre-existing FlowNFT (in case a previously existing one wasn't burned on flow delete) * disabled test which doesn't allow minting of existing NFTs * update CHANGELOG & bump version
superfluid-finance · Mar 19, 2024 · 54fd386 · 54fd386
1 parent bb5ba5e
commit 54fd386
Show file tree

Hide file tree

Showing 14 changed files with 99 additions and 91 deletions.
diff --git a/packages/automation-contracts/autowrap/package.json b/packages/automation-contracts/autowrap/package.json
@@ -13,8 +13,8 @@
         "check-updates": "ncu --target minor"
     },
     "devDependencies": {
-        "@superfluid-finance/metadata": "^1.1.27",
         "@openzeppelin/contracts": "4.9.6",
-        "@superfluid-finance/ethereum-contracts": "1.9.0"
+        "@superfluid-finance/ethereum-contracts": "^1.9.1",
+        "@superfluid-finance/metadata": "^1.1.28"
     }
 }
diff --git a/packages/automation-contracts/scheduler/package.json b/packages/automation-contracts/scheduler/package.json
@@ -13,8 +13,8 @@
         "check-updates": "ncu --target minor"
     },
     "devDependencies": {
-        "@superfluid-finance/metadata": "^1.1.27",
         "@openzeppelin/contracts": "4.9.6",
-        "@superfluid-finance/ethereum-contracts": "1.9.0"
+        "@superfluid-finance/ethereum-contracts": "^1.9.1",
+        "@superfluid-finance/metadata": "^1.1.28"
     }
 }
diff --git a/packages/ethereum-contracts/CHANGELOG.md b/packages/ethereum-contracts/CHANGELOG.md
@@ -3,14 +3,12 @@ All notable changes to the ethereum-contracts will be documented in this file.
 
 This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
-## Unreleased
+## [v1.9.1] - 2024-03-19
 
 ### Breaking
 
-- The abstract base contract`SuperAppBaseFlow` was renamed to `CFASuperAppBase`.
-Initialization is now split between constructor and a method `_initialize`, with self-registration
-made optional.
-This allows the contract to be used with a SuperApp factory pattern (disable self-registration on networks with permissioned SuperApps) and for logic contracts in the context of the proxy pattern.
+- The abstract base contract`SuperAppBaseFlow` was renamed to `CFASuperAppBase` and doesn't self-register in the constructor anymore.
+This allows the contract to be used with a SuperApp factory pattern and by logic contracts in the context of the proxy pattern.
 Note: this will NOT break any deployed contracts, only affects undeployed Super Apps in case the ethereum-contracts dependency is updated.
 - `UniversalIndexData`, `PoolMemberData` and `FlowDistributionData` structs moved from `IGeneralDistributionAgreementV1.sol` to `GeneralDistributionAgreementV1.sol`
 - `PoolIndexData`, `MemberData` structs moved from `ISuperfluidPool.sol` to `SuperfluidPool.sol`
@@ -34,6 +32,9 @@ Note: this will NOT break any deployed contracts, only affects undeployed Super
 - bump solc to 0.8.23
 - `superTokenV1Library.getNetFlowInfo` sums CFA and GDA net flow info
 
+### Fixes
+
+- FlowNFT hooks can't revert with outofgas anymore
 
 ## [v1.9.0] - 2024-01-09
 

diff --git a/packages/ethereum-contracts/contracts/agreements/ConstantFlowAgreementV1.sol b/packages/ethereum-contracts/contracts/agreements/ConstantFlowAgreementV1.sol
@@ -17,7 +17,6 @@ import {
 import { AgreementBase } from "./AgreementBase.sol";
 import { SafeCast } from "@openzeppelin/contracts/utils/math/SafeCast.sol";
 import { AgreementLibrary } from "./AgreementLibrary.sol";
-import { SafeGasLibrary } from "../libs/SafeGasLibrary.sol";
 import { SolvencyHelperLibrary } from "../libs/SolvencyHelperLibrary.sol";
 
 /**
@@ -460,69 +459,40 @@ contract ConstantFlowAgreementV1 is
     function _handleOnCreateHook(
         _StackVars_createOrUpdateFlow memory flowVars
     ) internal {
-        uint256 gasLeftBefore = gasleft();
-
         address constantOutflowNFTAddress = _canCallNFTHook(flowVars.token);
 
         if (constantOutflowNFTAddress != address(0)) {
-            try
-                IConstantOutflowNFT(constantOutflowNFTAddress).onCreate(
-                    flowVars.token,
-                    flowVars.sender,
-                    flowVars.receiver
-                )
-            // solhint-disable-next-line no-empty-blocks
-            {
-
-            } catch {
-                SafeGasLibrary._revertWhenOutOfGas(gasLeftBefore);
-            }
+            IConstantOutflowNFT(constantOutflowNFTAddress).onCreate(
+                flowVars.token,
+                flowVars.sender,
+                flowVars.receiver
+            );
         }
     }
 
     function _handleOnUpdateHook(
         _StackVars_createOrUpdateFlow memory flowVars
     ) internal {
-        uint256 gasLeftBefore = gasleft();
-
         address constantOutflowNFTAddress = _canCallNFTHook(flowVars.token);
-
         if (constantOutflowNFTAddress != address(0)) {
-            try
-                IConstantOutflowNFT(constantOutflowNFTAddress).onUpdate(
-                    flowVars.token,
-                    flowVars.sender,
-                    flowVars.receiver
-                )
-            // solhint-disable-next-line no-empty-blocks
-            {
-
-            } catch {
-                SafeGasLibrary._revertWhenOutOfGas(gasLeftBefore);
-            }
+            IConstantOutflowNFT(constantOutflowNFTAddress).onUpdate(
+                flowVars.token,
+                flowVars.sender,
+                flowVars.receiver
+            );
         }
     }
 
     function _handleOnDeleteHook(
         _StackVars_createOrUpdateFlow memory flowVars
     ) internal {
-        uint256 gasLeftBefore = gasleft();
-
         address constantOutflowNFTAddress = _canCallNFTHook(flowVars.token);
-
         if (constantOutflowNFTAddress != address(0)) {
-            try
-                IConstantOutflowNFT(constantOutflowNFTAddress).onDelete(
-                    flowVars.token,
-                    flowVars.sender,
-                    flowVars.receiver
-                )
-            // solhint-disable-next-line no-empty-blocks
-            {
-
-            } catch {
-                SafeGasLibrary._revertWhenOutOfGas(gasLeftBefore);
-            }
+            IConstantOutflowNFT(constantOutflowNFTAddress).onDelete(
+                flowVars.token,
+                flowVars.sender,
+                flowVars.receiver
+            );
         }
     }
 

diff --git a/packages/ethereum-contracts/contracts/agreements/gdav1/GeneralDistributionAgreementV1.sol b/packages/ethereum-contracts/contracts/agreements/gdav1/GeneralDistributionAgreementV1.sol
@@ -576,35 +576,22 @@ contract GeneralDistributionAgreementV1 is AgreementBase, TokenMonad, IGeneralDi
             address constantOutflowNFTAddress = _getConstantOutflowNFTAddress(token);
 
             if (constantOutflowNFTAddress != address(0)) {
-                uint256 gasLeftBefore;
                 // create flow (mint)
                 if (requestedFlowRate > 0 && FlowRate.unwrap(flowVars.oldFlowRate) == 0) {
-                    gasLeftBefore = gasleft();
                     // solhint-disable-next-line no-empty-blocks
-                    try IConstantOutflowNFT(constantOutflowNFTAddress).onCreate(token, from, address(pool)) { }
-                    catch {
-                        SafeGasLibrary._revertWhenOutOfGas(gasLeftBefore);
-                    }
+                    IConstantOutflowNFT(constantOutflowNFTAddress).onCreate(token, from, address(pool));
                 }
 
                 // update flow (update metadata)
                 if (requestedFlowRate > 0 && FlowRate.unwrap(flowVars.oldFlowRate) > 0) {
-                    gasLeftBefore = gasleft();
                     // solhint-disable-next-line no-empty-blocks
-                    try IConstantOutflowNFT(constantOutflowNFTAddress).onUpdate(token, from, address(pool)) { }
-                    catch {
-                        SafeGasLibrary._revertWhenOutOfGas(gasLeftBefore);
-                    }
+                    IConstantOutflowNFT(constantOutflowNFTAddress).onUpdate(token, from, address(pool));
                 }
 
                 // delete flow (burn)
                 if (requestedFlowRate == 0) {
-                    gasLeftBefore = gasleft();
                     // solhint-disable-next-line no-empty-blocks
-                    try IConstantOutflowNFT(constantOutflowNFTAddress).onDelete(token, from, address(pool)) { }
-                    catch {
-                        SafeGasLibrary._revertWhenOutOfGas(gasLeftBefore);
-                    }
+                    IConstantOutflowNFT(constantOutflowNFTAddress).onDelete(token, from, address(pool));
                 }
             }
         }

diff --git a/packages/ethereum-contracts/contracts/superfluid/ConstantOutflowNFT.sol b/packages/ethereum-contracts/contracts/superfluid/ConstantOutflowNFT.sol
@@ -137,7 +137,6 @@ contract ConstantOutflowNFT is FlowNFTBase, IConstantOutflowNFT {
     function _mint(address superToken, address flowSender, address flowReceiver, uint256 newTokenId) internal {
         assert(flowSender != address(0));
         assert(flowSender != flowReceiver);
-        assert(!_exists(newTokenId));
 
         // update mapping for new NFT to be minted
         _flowDataByTokenId[newTokenId] = FlowNFTData(

diff --git a/packages/ethereum-contracts/package.json b/packages/ethereum-contracts/package.json
@@ -1,6 +1,6 @@
 {
     "name": "@superfluid-finance/ethereum-contracts",
-    "version": "1.9.0",
+    "version": "1.9.1",
     "description": " Ethereum contracts implementation for the Superfluid Protocol",
     "homepage": "https://github.com/superfluid-finance/protocol-monorepo/tree/dev/packages/ethereum-contracts#readme",
     "repository": {
@@ -92,7 +92,7 @@
         "@safe-global/safe-service-client": "^2.0.3",
         "@safe-global/safe-web3-lib": "^1.9.4",
         "@superfluid-finance/js-sdk": "^0.6.3",
-        "@superfluid-finance/metadata": "^1.1.27",
+        "@superfluid-finance/metadata": "^1.1.28",
         "async": "^3.2.5",
         "csv-writer": "^1.6.0",
         "ethers": "^5.7.2",

diff --git a/packages/ethereum-contracts/test/foundry/FoundrySuperfluidTester.sol b/packages/ethereum-contracts/test/foundry/FoundrySuperfluidTester.sol
@@ -742,6 +742,8 @@ contract FoundrySuperfluidTester is Test {
             _assertAccountFlowInfo(receiver, flowRateDelta, receiverFlowInfoBefore, false);
         }
 
+        _assertFlowNftState(superToken_, sender, receiver, flowRate);
+
         // Assert RTB for all users
         _assertRealTimeBalances(superToken_);
         _assertGlobalInvariants();
@@ -1948,7 +1950,7 @@ contract FoundrySuperfluidTester is Test {
 
         // Assert Outflow NFT is minted to distributor
         // Assert Inflow NFT is minted to pool
-        _assertFlowNftOnDistributeFlow(superToken_, pool_, from, requestedFlowRate);
+        _assertFlowNftState(superToken_, from, address(pool_), requestedFlowRate);
 
         {
             if (members.length == 0) return;
@@ -2294,27 +2296,29 @@ contract FoundrySuperfluidTester is Test {
         }
     }
 
-    function _assertFlowNftOnDistributeFlow(
+    function _assertFlowNftState(
         ISuperfluidToken _superToken,
-        ISuperfluidPool _pool,
-        address _distributor,
+        address _senderOrDistributor,
+        address _receiverOrPool,
         int96 _newFlowRate
     ) internal {
         IConstantOutflowNFT constantOutflowNFT = SuperToken(address(_superToken)).CONSTANT_OUTFLOW_NFT();
         IConstantInflowNFT constantInflowNFT = SuperToken(address(_superToken)).CONSTANT_INFLOW_NFT();
-        uint256 tokenId = constantOutflowNFT.getTokenId(address(_superToken), address(_distributor), address(_pool));
+        uint256 tokenId = constantOutflowNFT.getTokenId(address(_superToken), address(_senderOrDistributor), address(_receiverOrPool));
         if (_newFlowRate > 0) {
+            // positive flowrate: NFT exists and is owned by sender/distributor and receiver/pool
             assertEq(
                 constantOutflowNFT.ownerOf(tokenId),
-                _distributor,
-                "_assertFlowNftOnDistributeFlow: distributor doesn't own outflow NFT"
+                _senderOrDistributor,
+                "_assertFlowNftState: sender/distributor doesn't own outflow NFT"
             );
             assertEq(
                 constantInflowNFT.ownerOf(tokenId),
-                address(_pool),
-                "_assertFlowNftOnDistributeFlow: distributor doesn't own inflow NFT"
+                address(_receiverOrPool),
+                "_assertFlowNftState: receiver/pool doesn't own inflow NFT"
             );
         } else {
+            // zero flowrate: NFT doesn't exist (never minted or already burned)
             vm.expectRevert(IFlowNFTBase.CFA_NFT_INVALID_TOKEN_ID.selector);
             constantOutflowNFT.ownerOf(tokenId);
 

diff --git a/packages/ethereum-contracts/test/foundry/agreements/ConstantFlowAgreementV1.t.sol b/packages/ethereum-contracts/test/foundry/agreements/ConstantFlowAgreementV1.t.sol
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: AGPLv3
 pragma solidity 0.8.23;
 
+import { console } from "forge-std/Test.sol";
 import { FoundrySuperfluidTester, SuperTokenV1Library } from "../FoundrySuperfluidTester.sol";
 import { ISuperToken } from "../../../contracts/superfluid/SuperToken.sol";
 
@@ -17,11 +18,55 @@ contract ConstantFlowAgreementV1IntegrationTest is FoundrySuperfluidTester {
 
     function testBobAliceLoop(int96 flowRate) public {
         _helperCreateFlow(superToken, alice, bob, flowRate);
-        
+
         _warpAndAssertAll(superToken);
 
         _helperCreateFlow(superToken, bob, alice, flowRate);
 
         _warpAndAssertAll(superToken);
     }
+
+    // there should be no gas limit which causes the NFT hook to fail with the tx succeeding
+    function testNFTHookOutOfGasRevertsWholeTx(uint256 gasLimit) public {
+        gasLimit = bound(gasLimit, 350000, 550000);
+
+        console.log("trying createFlow...");
+        int96 fr = 1;
+        try this.__external_createFlow{gas: gasLimit}(superToken, alice, bob, fr) {
+            // if the tx does not revert, the NFT hook isn't allowed to revert with outofgas,
+            // which we can check by verifying the FlowNFT state
+            _assertFlowNftState(superToken, alice, bob, fr);
+        } catch { } // revert of the tx is ok
+
+        console.log("trying updateFlow...");
+        fr = 2;
+        try this.__external_updateFlow{gas: gasLimit}(superToken, alice, bob, fr) {
+            _assertFlowNftState(superToken, alice, bob, fr);
+        } catch { }
+
+        console.log("trying deleteFlow...");
+        try this.__external_deleteFlow{gas: gasLimit}(superToken, alice, bob) {
+            _assertFlowNftState(superToken, alice, bob, 0);
+        } catch { }
+    }
+
+    // helper functions wrapping internal calls into external calls (needed for try/catch)
+
+    function __external_createFlow(ISuperToken superToken, address sender, address receiver, int96 fr) external {
+        vm.startPrank(sender);
+        superToken.createFlow(receiver, fr);
+        vm.stopPrank();
+    }
+
+    function __external_updateFlow(ISuperToken superToken, address sender, address receiver, int96 fr) external {
+        vm.startPrank(sender);
+        superToken.updateFlow(receiver, fr);
+        vm.stopPrank();
+    }
+
+    function __external_deleteFlow(ISuperToken superToken, address sender, address receiver) external {
+        vm.startPrank(sender);
+        superToken.deleteFlow(sender, receiver);
+        vm.stopPrank();
+    }
 }
diff --git a/packages/ethereum-contracts/test/foundry/superfluid/ConstantOutflowNFT.t.sol b/packages/ethereum-contracts/test/foundry/superfluid/ConstantOutflowNFT.t.sol
@@ -34,7 +34,9 @@ contract ConstantOutflowNFTTest is FlowNFTBaseTest {
         constantOutflowNFT.mockMint(address(superTokenMock), address(0), _flowReceiver, nftId);
     }
 
-    function testRevertIfInternalMintTokenThatExists(address _flowSender, address _flowReceiver) public {
+    // test disabled -we do now explicitly allow this in order to not revert
+    // if previous onDelete hooks failed to execute
+    function noTestRevertIfInternalMintTokenThatExists(address _flowSender, address _flowReceiver) public {
         _assumeSenderNEQReceiverAndNeitherAreZeroAddress(_flowSender, _flowReceiver);
 
         uint256 nftId = _helperGetNFTID(address(superTokenMock), _flowSender, _flowReceiver);
@@ -85,7 +87,7 @@ contract ConstantOutflowNFTTest is FlowNFTBaseTest {
         vm.expectRevert();
         constantInflowNFT.tokenURI(nftId);
     }
-    
+
     function testRevertIfYouTryToTransferOutflowNFT(address _flowSender, address _flowReceiver) public {
         _assumeSenderNEQReceiverAndNeitherAreZeroAddress(_flowSender, _flowReceiver);
 

diff --git a/packages/hot-fuzz/package.json b/packages/hot-fuzz/package.json
@@ -25,7 +25,7 @@
         "@superfluid-finance/ethereum-contracts": "1.8.0"
     },
     "devDependencies": {
-        "@superfluid-finance/ethereum-contracts": "^1.9.0"
+        "@superfluid-finance/ethereum-contracts": "^1.9.1"
     },
     "license": "AGPL-3.0",
     "bugs": {

diff --git a/packages/js-sdk/package.json b/packages/js-sdk/package.json
@@ -43,13 +43,13 @@
         "cloc": "sh tasks/cloc.sh"
     },
     "dependencies": {
-        "@superfluid-finance/metadata": "^1.1.27",
+        "@superfluid-finance/metadata": "^1.1.28",
         "@truffle/contract": "4.6.31",
         "auto-bind": "4.0.0",
         "node-fetch": "2.7.0"
     },
     "devDependencies": {
-        "@superfluid-finance/ethereum-contracts": "^1.9.0",
+        "@superfluid-finance/ethereum-contracts": "^1.9.1",
         "chai-as-promised": "^7.1.1",
         "webpack": "^5.90.1",
         "webpack-bundle-analyzer": "^4.10.1",

diff --git a/packages/sdk-core/package.json b/packages/sdk-core/package.json
@@ -56,8 +56,8 @@
         "url": "https://github.com/superfluid-finance/protocol-monorepo/issues"
     },
     "dependencies": {
-        "@superfluid-finance/ethereum-contracts": "^1.9.0",
-        "@superfluid-finance/metadata": "^1.1.27",
+        "@superfluid-finance/ethereum-contracts": "^1.9.1",
+        "@superfluid-finance/metadata": "^1.1.28",
         "browserify": "^17.0.0",
         "graphql-request": "^6.1.0",
         "lodash": "^4.17.21",

diff --git a/packages/subgraph/package.json b/packages/subgraph/package.json
@@ -56,7 +56,7 @@
         "mustache": "^4.2.0"
     },
     "devDependencies": {
-        "@superfluid-finance/metadata": "^1.1.27",
+        "@superfluid-finance/metadata": "^1.1.28",
         "coingecko-api": "^1.0.10",
         "graphql": "^16.8.1",
         "graphql-request": "^6.1.0",