From 54fd3865fc7f0a4616914876859f6fdc9c886de8 Mon Sep 17 00:00:00 2001 From: Didi Date: Tue, 19 Mar 2024 16:22:13 +0100 Subject: [PATCH] [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 --- .../autowrap/package.json | 4 +- .../scheduler/package.json | 4 +- packages/ethereum-contracts/CHANGELOG.md | 11 ++-- .../agreements/ConstantFlowAgreementV1.sol | 60 +++++-------------- .../gdav1/GeneralDistributionAgreementV1.sol | 19 +----- .../superfluid/ConstantOutflowNFT.sol | 1 - packages/ethereum-contracts/package.json | 4 +- .../test/foundry/FoundrySuperfluidTester.sol | 22 ++++--- .../agreements/ConstantFlowAgreementV1.t.sol | 47 ++++++++++++++- .../superfluid/ConstantOutflowNFT.t.sol | 6 +- packages/hot-fuzz/package.json | 2 +- packages/js-sdk/package.json | 4 +- packages/sdk-core/package.json | 4 +- packages/subgraph/package.json | 2 +- 14 files changed, 99 insertions(+), 91 deletions(-) diff --git a/packages/automation-contracts/autowrap/package.json b/packages/automation-contracts/autowrap/package.json index 581f7a8cac..1a266e07a3 100644 --- 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 index cf7c0a98a9..e75e30dc0f 100644 --- 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 index 5f11d6b420..76bd6f606e 100644 --- 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 index ce24f40091..cc85dc1456 100644 --- 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 index cc395b8f3c..21de10a40a 100644 --- 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 index ad7bdd67a8..534855ace6 100644 --- 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 index 2c0d45fafc..5dddfbbd5a 100644 --- 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 index af9479cb7e..ce0477ddea 100644 --- 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 index 66b3a3f58a..77ad32e71b 100644 --- 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 index 32c54fac9b..1084df2638 100644 --- 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 index 5f2c28f0fc..0741f7d3fa 100644 --- 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 index 4501106ef5..4b51d35651 100644 --- 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 index 9135fef0bd..9a4316747b 100644 --- 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 index 59a3750427..9df469ce84 100644 --- 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",