fix: harden DirectPayments and UBIPool contracts

package.json

@@ -39,7 +39,7 @@
     "shx": "^0.3.4",
     "syncpack": "^8.2.4",
     "ts-node": "^10.9.1",
-    "vercel": "^50.15.1"
+    "vercel": "^65.1.0"
   },
   "lint-staged": {
     "packages/app/**/*.{ts,tsx}": [

packages/contracts/contracts/DirectPayments/DirectPaymentsFactory.sol

@@ -63,6 +63,7 @@ contract DirectPaymentsFactory is AccessControlUpgradeable, UUPSUpgradeable {
     }
 
     modifier onlyPoolManager(DirectPaymentsPool pool) {
+        if (bytes(registry[address(pool)].projectId).length == 0) revert NOT_POOL();
         if (pool.hasRole(pool.MANAGER_ROLE(), msg.sender) == false) {
             revert NOT_PROJECT_MANAGER();
         }
@@ -208,6 +209,7 @@ contract DirectPaymentsFactory is AccessControlUpgradeable, UUPSUpgradeable {
             if (memberPools[member][i] == msg.sender) {
                 memberPools[member][i] = memberPools[member][memberPools[member].length - 1];
                 memberPools[member].pop();
+                break;
             }
         }
     }

packages/contracts/contracts/DirectPayments/DirectPaymentsLibrary.sol

@@ -24,26 +24,6 @@ library DirectPaymentsLibrary {
     error OVER_GLOBAL_LIMITS();
     error NO_BALANCE();
 
-    function _updateMemberLimits(DirectPaymentsPool.LimitsData storage memberStats, uint128 reward) public {
-        if (memberStats.lastReward + 60 * 60 * 24 < block.timestamp) //more than a day passed since last reward
-        {
-            memberStats.daily = reward;
-        } else {
-            memberStats.daily += reward;
-        }
-
-        if (memberStats.lastMonth < _month()) //month switched
-        {
-            memberStats.monthly = reward;
-        } else {
-            memberStats.monthly += reward;
-        }
-
-        memberStats.total += reward;
-        memberStats.lastReward = uint64(block.timestamp);
-        memberStats.lastMonth = _month();
-    }
-
     /**
      * @dev Updates the global limits with the new reward.
      * @param globalLimits The global limits data to update.
@@ -94,9 +74,19 @@ library DirectPaymentsLibrary {
         ) {
             return false;
         }
-        _updateMemberLimits(memberStats, reward);
+        uint128 daily = memberStats.lastReward / 1 days < block.timestamp / 1 days //new calendar day
+            ? reward
+            : memberStats.daily + reward;
+        uint128 monthly = memberStats.lastMonth < _month() ? reward : memberStats.monthly + reward;
 
-        if (memberStats.daily > limits.maxMemberPerDay || memberStats.monthly > limits.maxMemberPerMonth) return false;
+        // rejected rewards must not count towards the limits
+        if (daily > limits.maxMemberPerDay || monthly > limits.maxMemberPerMonth) return false;
+
+        memberStats.daily = daily;
+        memberStats.monthly = monthly;
+        memberStats.total += reward;
+        memberStats.lastReward = uint64(block.timestamp);
+        memberStats.lastMonth = _month();
 
         return true;
     }
@@ -157,8 +147,8 @@ library DirectPaymentsLibrary {
             ) * _data.events[i].quantity;
             if (reward > 0) {
                 totalRewards += reward;
-                if (totalRewards > rewardsBalance) revert NO_BALANCE();
-                rewardsBalance -= totalRewards;
+                if (reward > rewardsBalance) revert NO_BALANCE();
+                rewardsBalance -= reward;
 
                 _sendReward(
                     globalLimits,

packages/contracts/contracts/DirectPayments/DirectPaymentsPool.sol

@@ -189,6 +189,7 @@ contract DirectPaymentsPool is
 
     function claim(uint256 _nftId, ProvableNFT.NFTData memory _data) public {
         nft.proveNFTData(_nftId, _data);
+        if (_data.nftType != settings.nftType) revert UNSUPPORTED_NFT();
         if (claimedNfts[_nftId]) revert ALREADY_CLAIMED(_nftId);
         claimedNfts[_nftId] = true;
         // TODO: should pool own the NFTs?
@@ -303,9 +304,11 @@ contract DirectPaymentsPool is
         if (_settings.nftType != settings.nftType) revert NFTTYPE_CHANGED();
         if (_settings.manager == address(0)) revert EMPTY_MANAGER();
 
-        _revokeRole(DEFAULT_ADMIN_ROLE, settings.manager);
+        _revokeRole(MANAGER_ROLE, settings.manager);
+        _revokeRole(MINTER_ROLE, settings.manager);
         settings = _settings;
-        _setupRole(DEFAULT_ADMIN_ROLE, _settings.manager);
+        _setupRole(MANAGER_ROLE, _settings.manager);
+        _setupRole(MINTER_ROLE, _settings.manager);
         emit PoolSettingsChanged(_settings);
     }
 }

packages/contracts/contracts/GoodCollective/GoodCollectiveSuperApp.sol

@@ -151,11 +151,8 @@ abstract contract GoodCollectiveSuperApp is SuperAppBaseFlow {
      * @param _ctx The context of the transaction for superfluid in case this was used in superfluid batch. otherwise can be empty.
      * @return Returns the context of the transaction.
      */
-    function support(
-        address _sender,
-        uint256 _amount,
-        bytes memory _ctx
-    ) external onlyHostOrSender(_sender) returns (bytes memory) {
+    function support(address _sender, uint256 _amount, bytes memory _ctx) external returns (bytes memory) {
+        _onlyHostOrSender(_sender);
         if (_amount == 0) revert ZERO_AMOUNT();
 
         // Transfer the tokens from the sender to this contract
@@ -178,7 +175,9 @@ abstract contract GoodCollectiveSuperApp is SuperAppBaseFlow {
         address _sender,
         HelperLibrary.SwapData memory _customData,
         bytes memory _ctx
-    ) external onlyHostOrSender(_sender) returns (bytes memory) {
+    ) external returns (bytes memory) {
+        _onlyHostOrSender(_sender);
+
         uint256 balance = superToken.balanceOf(address(this));
         HelperLibrary.handleSwap(swapRouter, _customData, address(superToken), _sender, address(this));
         uint256 amountReceived = superToken.balanceOf(address(this)) - balance;
@@ -200,7 +199,9 @@ abstract contract GoodCollectiveSuperApp is SuperAppBaseFlow {
         HelperLibrary.SwapData memory _customData,
         address _sender,
         bytes memory _ctx
-    ) external onlyHostOrSender(_sender) returns (bytes memory) {
+    ) external returns (bytes memory) {
+        _onlyHostOrSender(_sender);
+
         HelperLibrary.handleSwap(swapRouter, _customData, address(superToken), _sender);
 
         // Return the context of the transaction
@@ -370,8 +371,8 @@ abstract contract GoodCollectiveSuperApp is SuperAppBaseFlow {
     /**
      * for methods that can be called via superfluid batch or directly
      */
-    modifier onlyHostOrSender(address _sender) {
-        if (msg.sender != _sender && msg.sender != address(host)) revert ONLY_HOST_OR_SENDER(msg.sender);
-        _;
+    function _onlyHostOrSender(address _sender) internal view {
+        if (_sender == address(this) || (msg.sender != _sender && msg.sender != address(host)))
+            revert ONLY_HOST_OR_SENDER(msg.sender);
     }
 }

packages/contracts/contracts/GoodCollective/SuperAppBaseFlow.sol

@@ -86,27 +86,26 @@ abstract contract SuperAppBaseFlow is ISuperApp {
         return ctx;
     }
 
+    // validation is done in afterAgreementCreated, no before data is needed
     function beforeAgreementCreated(
-        ISuperToken superToken,
-        address agreementClass,
+        ISuperToken /*superToken*/,
+        address /*agreementClass*/,
         bytes32 /*agreementId*/,
         bytes calldata /*agreementData*/,
         bytes calldata /*ctx*/
-    ) external view returns (bytes memory /*beforeData*/) {
-        if (msg.sender != address(host)) revert UnauthorizedHost();
-        if (!isAcceptedAgreement(agreementClass)) return "0x";
-        if (!isAcceptedSuperToken(superToken)) revert NotAcceptedSuperToken();
+    ) external pure returns (bytes memory /*beforeData*/) {
         return "0x";
     }
 
     function afterAgreementCreated(
         ISuperToken superToken,
-        address /*agreementClass*/,
+        address agreementClass,
         bytes32 /*agreementId*/,
         bytes calldata agreementData,
         bytes calldata /*cbdata*/,
         bytes calldata ctx
     ) external returns (bytes memory newCtx) {
+        if (!_isValidCallback(superToken, agreementClass, true)) return ctx;
         (address sender, ) = abi.decode(agreementData, (address, address));
 
         return
@@ -126,9 +125,7 @@ abstract contract SuperAppBaseFlow is ISuperApp {
         bytes calldata agreementData,
         bytes calldata /*ctx*/
     ) external view returns (bytes memory /*beforeData*/) {
-        if (msg.sender != address(host)) revert UnauthorizedHost();
-        if (!isAcceptedAgreement(agreementClass)) return "0x";
-        if (!isAcceptedSuperToken(superToken)) revert NotAcceptedSuperToken();
+        if (!_isValidCallback(superToken, agreementClass, true)) return "0x";
 
         (address sender, ) = abi.decode(agreementData, (address, address));
         (uint256 lastUpdated, int96 flowRate, , ) = superToken.getFlowInfo(sender, address(this));
@@ -138,12 +135,13 @@ abstract contract SuperAppBaseFlow is ISuperApp {
 
     function afterAgreementUpdated(
         ISuperToken superToken,
-        address /*agreementClass*/,
+        address agreementClass,
         bytes32 /*agreementId*/,
         bytes calldata agreementData,
         bytes calldata cbdata,
         bytes calldata ctx
     ) external returns (bytes memory newCtx) {
+        if (!_isValidCallback(superToken, agreementClass, true)) return ctx;
         (address sender, ) = abi.decode(agreementData, (address, address));
         (int96 previousFlowRate, uint256 lastUpdated) = abi.decode(cbdata, (int96, uint256));
 
@@ -167,9 +165,7 @@ abstract contract SuperAppBaseFlow is ISuperApp {
         bytes calldata /*ctx*/
     ) external view 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) || !isAcceptedSuperToken(superToken)) {
-            return "0x";
-        }
+        if (!_isValidCallback(superToken, agreementClass, false)) return "0x";
 
         (address sender, address receiver) = abi.decode(agreementData, (address, address));
         (uint256 lastUpdated, int96 flowRate, , ) = superToken.getFlowInfo(sender, receiver);
@@ -186,9 +182,7 @@ abstract contract SuperAppBaseFlow is ISuperApp {
         bytes calldata ctx
     ) external 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) || !isAcceptedSuperToken(superToken)) {
-            return ctx;
-        }
+        if (!_isValidCallback(superToken, agreementClass, false)) return ctx;
 
         (address sender, address receiver) = abi.decode(agreementData, (address, address));
         (uint256 lastUpdated, int96 previousFlowRate) = abi.decode(cbdata, (uint256, int96));
@@ -199,6 +193,24 @@ abstract contract SuperAppBaseFlow is ISuperApp {
     // ---------------------------------------------------------------------------------------------
     // HELPERS
 
+    /// @dev returns false for a non CFA agreement. host and token failures revert unless `canRevert` is false.
+    function _isValidCallback(
+        ISuperToken _superToken,
+        address agreementClass,
+        bool canRevert
+    ) internal view returns (bool) {
+        if (msg.sender != address(host)) {
+            if (canRevert) revert UnauthorizedHost();
+            return false;
+        }
+        if (!isAcceptedAgreement(agreementClass)) return false;
+        if (!isAcceptedSuperToken(_superToken)) {
+            if (canRevert) revert NotAcceptedSuperToken();
+            return false;
+        }
+        return true;
+    }
+
     /**
      * @dev Expect Super Agreement involved in callback to be an accepted one
      *      This function can be overridden with custom logic and to revert if desired

packages/contracts/contracts/UBI/UBIPool.sol

@@ -23,6 +23,8 @@ contract UBIPool is AccessControlUpgradeable, GoodCollectiveSuperApp, UUPSUpgrad
     error MAX_MEMBERS_REACHED();
     error MAX_PERIOD_CLAIMERS_REACHED(uint256 claimers);
     error LENGTH_MISMATCH();
+    error NO_UBI();
+    error INVALID_MAX_PERIOD_CLAIMERS();
 
     bytes32 public constant MANAGER_ROLE = keccak256("MANAGER_ROLE");
     bytes32 public constant MEMBER_ROLE = keccak256("MEMBER_ROLE");
@@ -125,8 +127,8 @@ contract UBIPool is AccessControlUpgradeable, GoodCollectiveSuperApp, UUPSUpgrad
         settings = _settings;
         ubiSettings = _ubiSettings;
         extendedSettings = _extendedSettings;
+        _verifyUBISettings(_ubiSettings, _extendedSettings);
         _verifyPoolSettings(_settings);
-        _verifyUBISettings(_ubiSettings);
         _setupRole(DEFAULT_ADMIN_ROLE, msg.sender); // when using factory this gives factory role which then set role to the real msg.sender
         _setupRole(MANAGER_ROLE, _settings.manager);
         setSuperToken(ISuperToken(address(settings.rewardToken)));
@@ -163,7 +165,6 @@ contract UBIPool is AccessControlUpgradeable, GoodCollectiveSuperApp, UUPSUpgrad
 
             status.currentDay = currentDay;
             status.dailyUbi = nextDailyUbi;
-            if (status.dailyUbi <= extendedSettings.minClaimAmount) status.dailyUbi = 0;
 
             emit UBICalculated(
                 currentDay,
@@ -188,7 +189,6 @@ contract UBIPool is AccessControlUpgradeable, GoodCollectiveSuperApp, UUPSUpgrad
                 currentBalance < (status.dailyCyclePool * (status.currentCycleLength - currentDayInCycle())));
 
         nextPeriodPool = status.dailyCyclePool;
-        nextDailyUbi;
         if ((currentDayInCycle() + 1) >= status.currentCycleLength || shouldStartEarlyCycle) {
             //start of cycle or first time
             nextPeriodPool = currentBalance / ubiSettings.cycleLengthDays;
@@ -240,12 +240,15 @@ contract UBIPool is AccessControlUpgradeable, GoodCollectiveSuperApp, UUPSUpgrad
         address whitelistedRoot = IIdentityV2(settings.uniquenessValidator).getWhitelistedRoot(claimer);
         if (whitelistedRoot == address(0)) revert NOT_WHITELISTED(claimer);
 
-        // if open for anyone but has limits, we add the first claimers as members to handle the max claimers
-        if ((ubiSettings.maxMembers > 0 && ubiSettings.onlyMembers == false)) _grantRole(MEMBER_ROLE, claimer);
+        // with a members cap, open pools auto join the whitelisted root so connected accounts share one slot
+        bool isMember = hasRole(MEMBER_ROLE, claimer) || hasRole(MEMBER_ROLE, whitelistedRoot);
+        if (isMember == false && ubiSettings.onlyMembers == false && ubiSettings.maxMembers > 0) {
+            _grantRole(MEMBER_ROLE, whitelistedRoot);
+            isMember = true;
+        }
 
         // check membership if has claimers limits or limited to members only
-        if ((ubiSettings.maxMembers > 0 || ubiSettings.onlyMembers) && hasRole(MEMBER_ROLE, claimer) == false)
-            revert NOT_MEMBER(claimer);
+        if ((ubiSettings.maxMembers > 0 || ubiSettings.onlyMembers) && isMember == false) revert NOT_MEMBER(claimer);
 
         if (extendedSettings.maxPeriodClaimers > 0 && status.periodClaimers >= extendedSettings.maxPeriodClaimers)
             revert MAX_PERIOD_CLAIMERS_REACHED(status.periodClaimers);
@@ -254,10 +257,16 @@ contract UBIPool is AccessControlUpgradeable, GoodCollectiveSuperApp, UUPSUpgrad
         // (new or active) will trigger the calculation with the active users count of the day before
         // and so on. the new or inactive users that will become active today, will not take into account
         // within the calculation.
+        uint256 previousDay = status.currentDay;
         uint256 dailyUbi = distributionFormula();
 
         // active user which has not claimed today yet, ie user last claimed < today
         if (status.lastClaimed[whitelistedRoot] == status.currentDay) revert ALREADY_CLAIMED(whitelistedRoot);
+        // a zero claim would use up the claim period and count as an active claimer
+        if (dailyUbi == 0) {
+            if (status.currentDay != previousDay) return;
+            revert NO_UBI();
+        }
 
         status.lastClaimed[whitelistedRoot] = status.currentDay;
         status.periodClaimers += 1;
@@ -305,23 +314,23 @@ contract UBIPool is AccessControlUpgradeable, GoodCollectiveSuperApp, UUPSUpgrad
      */
     function addMember(address member, bytes memory extraData) public returns (bool isMember) {
         bool success = _addMember(member, extraData);
-        
+
         if (!success) {
             // Determine the specific error to revert with
             if (address(settings.uniquenessValidator) != address(0)) {
                 address rootAddress = settings.uniquenessValidator.getWhitelistedRoot(member);
                 if (rootAddress == address(0)) revert NOT_WHITELISTED(member);
             }
-            
+
             if (address(settings.membersValidator) != address(0) && hasRole(MANAGER_ROLE, msg.sender) == false) {
                 revert NOT_MEMBER(member);
             }
-            
+
             if (ubiSettings.onlyMembers && hasRole(MANAGER_ROLE, msg.sender) == false) {
                 revert NOT_MANAGER(member);
             }
         }
-        
+
         return success;
     }
 
@@ -369,19 +378,25 @@ contract UBIPool is AccessControlUpgradeable, GoodCollectiveSuperApp, UUPSUpgrad
         UBISettings memory _ubiSettings,
         ExtendedSettings memory _extendedSettings
     ) public onlyRole(MANAGER_ROLE) {
-        _verifyUBISettings(_ubiSettings);
+        _verifyUBISettings(_ubiSettings, _extendedSettings);
         ubiSettings = _ubiSettings;
         extendedSettings = _extendedSettings;
         emit UBISettingsChanged(_ubiSettings);
     }
 
-    function _verifyUBISettings(UBISettings memory _ubiSettings) internal pure {
+    function _verifyUBISettings(
+        UBISettings memory _ubiSettings,
+        ExtendedSettings memory _extendedSettings
+    ) internal pure {
         if (
             _ubiSettings.claimPeriodDays == 0 ||
             _ubiSettings.cycleLengthDays == 0 ||
             _ubiSettings.minActiveUsers == 0 ||
             _ubiSettings.maxClaimAmount == 0
         ) revert INVALID_0_VALUE();
+
+        if (_ubiSettings.maxMembers > 0 && _extendedSettings.maxPeriodClaimers > _ubiSettings.maxMembers)
+            revert INVALID_MAX_PERIOD_CLAIMERS();
     }
 
     /**
@@ -435,7 +450,6 @@ contract UBIPool is AccessControlUpgradeable, GoodCollectiveSuperApp, UUPSUpgrad
         if (address(settings.uniquenessValidator) != address(0)) {
             whitelistedRoot = IIdentityV2(settings.uniquenessValidator).getWhitelistedRoot(_member);
         }
-        whitelistedRoot = IIdentityV2(settings.uniquenessValidator).getWhitelistedRoot(_member);
         return status.lastClaimed[whitelistedRoot] == getCurrentDay();
     }