Repository navigation
Conversation
…p callbacks, member limits - claim reverts with UNSUPPORTED_NFT for an NFT of another type - remove DirectPaymentsPool.cancelIncomingFlow - consolidate callback validation in SuperAppBaseFlow and require the host in after callbacks - only record member limits for rewards that are paid, reset daily limit per calendar day - changePoolDetails requires a registered pool - add tests
…hangePoolDetails - drop HelperLibrary.cancelIncomingFlow, no longer used - add tests
…eriodClaimers - claim reverts with NO_UBI when there is nothing to claim - open pools with a members cap join the whitelisted root so connected accounts share a slot - daily ubi equal to minClaimAmount is claimable - maxPeriodClaimers must not exceed maxMembers - factories removeMember stops after the first match - add UBIPool claim tests
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 9 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="packages/contracts/contracts/utils/HelperLibrary.sol" line_range="58" />
<code_context>
) public returns (uint256 amountOut) {
+ if (_sender == address(this)) revert INVALID_PATH();
+ // make sure tokens are not used form this pool balance
+ uint preBalance = IERC20(_customData.swapFrom).balanceOf(address(this));
// Transfer the tokens from the sender to this contract
TransferHelper.safeTransferFrom(_customData.swapFrom, _sender, address(this), _customData.amount);
</code_context>
<issue_to_address>
**Pool tokens can be spent**
When a multi-hop swap uses a fee-on-transfer input token while the helper already holds that token, `exactInput` returns without checking the input-token balance against `preBalance`, so the swap can draw the shortfall from the helper’s existing balance and the pool loses funds.
Check the input-token balance after the multi-hop `exactInput` call and revert if it falls below `preBalance`.
</issue_to_address>
### Comment 2
<location path="packages/contracts/contracts/UBI/UBIPool.sol" line_range="246" />
<code_context>
+ // 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;
+ }
</code_context>
<issue_to_address>
**Open pool member cap is bypassed**
When an open pool has `maxMembers > 0` and whitelisted roots claim, the claim path grants `MEMBER_ROLE` directly without checking or incrementing `status.membersCount`, so each new whitelisted root joins regardless of `maxMembers` and the pool's membership cap is bypassed.
Enforce the cap and update `status.membersCount` when a root is auto-joined, using the same accounting as the member-add path.
</issue_to_address>
### Comment 3
<location path="packages/contracts/contracts/UBI/UBIPool.sol" line_range="244-245" />
<code_context>
- // 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;
</code_context>
<issue_to_address>
**Linked accounts use extra member slots**
When a connected claimer has `MEMBER_ROLE` but its `whitelistedRoot` does not, `_claim` treats the claimer’s membership as sufficient and skips granting `MEMBER_ROLE` to the root; a later linked account adds the root as another member, consuming an extra slot and potentially blocking claims at the cap.
Ensure the whitelisted root is enrolled for capped open pools even when the claimer already has `MEMBER_ROLE`.
</issue_to_address>
### Comment 4
<location path="packages/contracts/contracts/DirectPayments/DirectPaymentsPool.sol" line_range="307-309" />
<code_context>
if (_settings.manager == address(0)) revert EMPTY_MANAGER();
- _revokeRole(DEFAULT_ADMIN_ROLE, settings.manager);
+ _revokeRole(MANAGER_ROLE, settings.manager);
settings = _settings;
- _setupRole(DEFAULT_ADMIN_ROLE, _settings.manager);
+ _setupRole(MANAGER_ROLE, _settings.manager);
emit PoolSettingsChanged(_settings);
}
</code_context>
<issue_to_address>
**New managers cannot mint NFTs**
When the pool changes to a manager who does not already hold `MINTER_ROLE`, `setPoolSettings` changes `MANAGER_ROLE` without transferring `MINTER_ROLE`, so the former manager can still mint NFTs and the new manager cannot.
Transfer `MINTER_ROLE` from the outgoing manager to the incoming manager when changing managers.
</issue_to_address>
### Comment 5
<location path="packages/contracts/contracts/DirectPayments/DirectPaymentsFactory.sol" line_range="66" />
<code_context>
}
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();
</code_context>
<issue_to_address>
**Managers cannot update blank-ID pools**
When a pool is created with an empty project ID, `onlyPoolManager` treats an empty `registry[address(pool)].projectId` as proof the pool is unregistered and reverts with `NOT_POOL`, so its manager cannot change the pool details.
Check pool registration independently of whether its project ID is empty.
Also at `packages/contracts/contracts/UBI/UBIPoolFactory.sol:66`.
</issue_to_address>
### Comment 6
<location path="packages/contracts/contracts/DirectPayments/DirectPaymentsFactory.sol" line_range="212" />
<code_context>
if (memberPools[member][i] == msg.sender) {
memberPools[member][i] = memberPools[member][memberPools[member].length - 1];
memberPools[member].pop();
+ break;
}
}
</code_context>
<issue_to_address>
**Revoked members remain listed**
When an administrator grants `MEMBER_ROLE` to an account that already has it, then the role is revoked, `_grantRole` adds duplicate entries to `memberPools`, while `removeMember` removes only one matching entry and breaks; callers still see the pool listed after membership is revoked.
Prevent duplicate `memberPools` entries or remove every matching entry when membership is revoked.
</issue_to_address>
### Comment 7
<location path="packages/contracts/contracts/DirectPayments/DirectPaymentsLibrary.sol" line_range="83-87" />
<code_context>
- 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();
</code_context>
<issue_to_address>
**Old rejected rewards still count**
When a pool is upgraded while a member’s stored counters include rewards rejected under the previous implementation, existing `memberLimits` can already include rewards that the previous `_updateMemberLimits` recorded before returning false. The new guard still rejects subsequent rewards against those inflated daily or monthly values, leaving affected members unpaid until the relevant window resets; their stored total also remains overstated.
Migrate or otherwise account for pre-upgrade rejected-reward increments before enforcing the new limits against existing counters.
</issue_to_address>
### Comment 8
<location path="packages/contracts/contracts/UBI/UBIPool.sol" line_range="394-395" />
<code_context>
_ubiSettings.maxClaimAmount == 0
) revert INVALID_0_VALUE();
+
+ if (_ubiSettings.maxMembers > 0 && _extendedSettings.maxPeriodClaimers > _ubiSettings.maxMembers)
+ revert INVALID_MAX_PERIOD_CLAIMERS();
}
</code_context>
<issue_to_address>
**Settings form submits rejected limits**
When a manager enters a positive `maxMembers` and a larger `maxPeriodClaimers` in the app form, `handleSaveUbiSettings` submits the values without checking this relation, so `setUBISettings` reverts with `INVALID_MAX_PERIOD_CLAIMERS` and the manager cannot save the form values.
Add the same cross-field validation to the app form before it submits the settings transaction.
</issue_to_address>
### Comment 9
<location path="packages/contracts/contracts/UBI/UBIPool.sol" line_range="394-395" />
<code_context>
_ubiSettings.maxClaimAmount == 0
) revert INVALID_0_VALUE();
+
+ if (_ubiSettings.maxMembers > 0 && _extendedSettings.maxPeriodClaimers > _ubiSettings.maxMembers)
+ revert INVALID_MAX_PERIOD_CLAIMERS();
}
</code_context>
<issue_to_address>
**Existing pools retain invalid UBI limits**
When an existing pool is upgraded while its stored `maxPeriodClaimers` exceeds its nonzero `maxMembers`, the new limit relation is checked only during initialization and `setUBISettings`, so upgrading an existing pool leaves a previously stored `maxPeriodClaimers > maxMembers` configuration active and claims continue under that invalid configuration.
Revalidate or migrate stored settings during upgrades, or enforce the relationship at runtime before claims rely on it.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 9 findings to address first, and the updated claim and reward-limit logic can change how many reward tokens are transferred to claimants, and the role changes affect who can manage pools. A revert restores the prior behavior but cannot recover reward tokens already paid out.
Blocking findings: packages/contracts/contracts/utils/HelperLibrary.sol:58, packages/contracts/contracts/UBI/UBIPool.sol:246, packages/contracts/contracts/UBI/UBIPool.sol:245, packages/contracts/contracts/DirectPayments/DirectPaymentsPool.sol:309, packages/contracts/contracts/DirectPayments/DirectPaymentsFactory.sol:66, and 4 more
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved swap reserve protection, claim accounting and recovery, and default deployment selection defects affect correctness and transaction safety.
4 open issues
What changed in this PR
This PR hardens GoodCollective’s DirectPayments and UBI pools, focusing on claim validation, reward accounting, permissions, and Superfluid interactions.
Changes:
- Tightens NFT, membership, factory, callback, and swap validation.
- Revises reward limits, manager-role rotation, and UBI claim handling, with expanded regression tests.
- Updates verification tooling, deployment configuration, and production deployment records.
| File | Description |
|---|---|
yarn.lock |
Updates verification dependencies and related resolutions. |
packages/contracts/test/UBIPool/UBIPoolFactory.test.ts |
Tests factory access controls and callback authorization. |
packages/contracts/test/UBIPool/UBIPool.claim.test.ts |
Adds UBI claim, minimum-payment, and membership-cap tests. |
packages/contracts/test/UBIPool/UBIPool.bulkMembers.test.ts |
Aligns capped-pool settings and reformats tests. |
packages/contracts/test/Swap.e2e.test.ts |
Adds invalid swap-path coverage and transfer-error logging. |
packages/contracts/test/DirectPayments/DirectPaymentsFactory.test.ts |
Tests registered-pool and manager checks. |
packages/contracts/test/DirectPayments/DirectPayments.superapp.test.ts |
Adds swap and callback authorization tests. |
packages/contracts/test/DirectPayments/DirectPayments.claim.test.ts |
Tests NFT types and member-limit accounting. |
packages/contracts/releases/deployment.json |
Reorders deployments and updates production addresses and interfaces. |
packages/contracts/package.json |
Replaces the verification plugin dependency. |
packages/contracts/hardhat.config.ts |
Loads verification support and adjusts deployment gas prices. |
packages/contracts/deployments/production-celo/UBIPoolFactory.json |
Updates UBI factory deployment metadata. |
packages/contracts/deployments/production-celo/DirectPaymentsFactory.json |
Updates DirectPayments factory deployment metadata. |
packages/contracts/deploy/01.DirectPayments.deploy.ts |
Logs the deployer at deployment start. |
packages/contracts/contracts/utils/HelperLibrary.sol |
Adds swap-path and pool-balance guards. |
packages/contracts/contracts/UBI/UBIPoolFactory.sol |
Requires registered pools and stops removal after a match. |
packages/contracts/contracts/UBI/UBIPool.sol |
Revises claim eligibility, identity membership, and settings validation. |
packages/contracts/contracts/GoodCollective/SuperAppBaseFlow.sol |
Centralizes callback validation. |
packages/contracts/contracts/GoodCollective/GoodCollectiveSuperApp.sol |
Consolidates sender checks and rejects pool self-senders. |
packages/contracts/contracts/DirectPayments/DirectPaymentsPool.sol |
Validates NFT types and rotates manager/minter roles. |
packages/contracts/contracts/DirectPayments/DirectPaymentsLibrary.sol |
Revises daily limits and reward-balance accounting. |
packages/contracts/contracts/DirectPayments/DirectPaymentsFactory.sol |
Requires registered pools and stops removal after a match. |
Review effort: Balanced
| // 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) revert NO_UBI(); |
There was a problem hiding this comment.
Implemented in 4cee704. A fresh zero-UBI recalculation now persists the period rollover; a subsequent same-day zero-UBI claim still reverts with NO_UBI. Added regression coverage for recovery. Tests could not run because the locked dependency install failed cloning the existing one-webcrypto Git dependency.
Refactor swap execution to store result in a variable before returning. Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: sirpy <671095+sirpy@users.noreply.github.com>
Co-authored-by: sirpy <671095+sirpy@users.noreply.github.com>
Co-authored-by: sirpy <671095+sirpy@users.noreply.github.com>
Contributor
Author
|
@copilot fix test to pass for first user claiming when dialyubi is 0 but revert for following users |
Co-authored-by: sirpy <671095+sirpy@users.noreply.github.com>
Updated in |
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes from a code review of DirectPayments and UBIPool.
DirectPayments
claimreverts withUNSUPPORTED_NFTwhen the NFT type differs from the pool's typecancelIncomingFlow(DirectPaymentsPool, UBIPool andHelperLibrary)setPoolSettingsrotatesMANAGER_ROLEinstead of grantingDEFAULT_ADMIN_ROLEchangePoolDetailsrequires a registered pool (both factories)removeMemberstops after the first matchSuperAppBaseFlow
_isValidCallbackhelper for the host, agreement and token checksafterAgreementCreatedandafterAgreementUpdatednow require the hostUBIPool
NO_UBIwhen the daily UBI is 0, so it does not use up the claim periodminClaimAmountis claimablemaxPeriodClaimersmust not exceedmaxMembers(INVALID_MAX_PERIOD_CLAIMERS)Notes
maxPeriodClaimers <= maxMembersyet.releases/deployment.jsonand thedeployments/artifacts are not part of this PR.Tests
npx hardhat testinpackages/contracts: 93 passing, 9 pending. New tests cover each fix, includingUBIPool.claim.test.ts.Summary by Sourcery
Harden DirectPayments and UBIPool contracts and align SDK and deployment tooling with the updated security and validation behavior.
New Features:
Bug Fixes:
Enhancements:
Build:
Deployment:
Tests:
Chores: