Contracts size recoverfunds #303
Merged
Conversation
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
![korbit-ai[bot]](https://avatars.githubusercontent.com/in/322216?s=60&v=4){kind=small}
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @sirpy - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- recoverFunds function exposes fund recovery to any caller with registry role 0x00. (link)
General comments:
- Library functions like _sendReward, _claim, and others are declared public, which exposes internal logic as part of the library API—consider marking them internal (or private) to avoid unintended external calls and reduce gas overhead.
- Extract the repeated magic time constants (e.g. 60 * 60 * 24, 60 * 60 * 24 * 30) into named constants to improve readability and maintainability.
- The try/catch fallback for registerApp silently ignores failures, which may mask real issues in production—consider at least emitting an event or logging the error reason when falling back.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Library functions like _sendReward, _claim, and others are declared public, which exposes internal logic as part of the library API—consider marking them internal (or private) to avoid unintended external calls and reduce gas overhead.
- Extract the repeated magic time constants (e.g. 60 * 60 * 24, 60 * 60 * 24 * 30) into named constants to improve readability and maintainability.
- The try/catch fallback for registerApp silently ignores failures, which may mask real issues in production—consider at least emitting an event or logging the error reason when falling back.
## Individual Comments
### Comment 1
<location> `packages/contracts/contracts/GoodCollective/GoodCollectiveSuperApp.sol:366` </location>
<code_context>
TransferHelper.safeTransfer(address(superToken), recipient, fee);
}
+ function recoverFunds(address _recipient, uint256 amount) external {
+ HelperLibrary.recoverFunds(superToken, _recipient, amount);
+ }
</code_context>
<issue_to_address>
recoverFunds function exposes fund recovery to any caller with registry role 0x00.
Please verify that only trusted accounts can be assigned registry role 0x00 to prevent unauthorized fund transfers.
</issue_to_address>
### Comment 2
<location> `packages/contracts/contracts/DirectPayments/DirectPaymentsFactory.sol:141` </location>
<code_context>
// Register the app with the host
- IRegisterSuperapp(address(pool.host())).registerApp(address(pool), SuperAppDefinitions.APP_LEVEL_FINAL);
+ if (pool.host().isApp(pool) == false) {
+ try
+ IRegisterSuperapp(address(pool.host())).registerApp(address(pool), SuperAppDefinitions.APP_LEVEL_FINAL)
+ {} catch {
+ //fallback for older versions of superfluid used in unit tests
+ IRegisterSuperapp(address(pool.host())).registerAppByFactory(
+ address(pool),
+ SuperAppDefinitions.APP_LEVEL_FINAL
+ );
+ }
+ }
</code_context>
<issue_to_address>
Fallback to registerAppByFactory improves compatibility but may mask registration failures.
If both registration attempts fail, the error is ignored. Please add logging or revert to prevent silent misconfiguration.
</issue_to_address>
### Comment 3
<location> `packages/contracts/contracts/UBI/UBIPoolFactory.sol:128` </location>
<code_context>
// Register the app with the host
- IRegisterSuperapp(address(pool.host())).registerApp(address(pool), SuperAppDefinitions.APP_LEVEL_FINAL);
+ if (pool.host().isApp(pool) == false) {
+ try
+ IRegisterSuperapp(address(pool.host())).registerApp(address(pool), SuperAppDefinitions.APP_LEVEL_FINAL)
+ {} catch {
+ //fallback for older versions of superfluid used in unit tests
+ IRegisterSuperapp(address(pool.host())).registerAppByFactory(
+ address(pool),
+ SuperAppDefinitions.APP_LEVEL_FINAL
+ );
+ }
+ }
</code_context>
<issue_to_address>
Silent catch in app registration may hide critical errors.
If both registration methods fail, ensure the error is surfaced by emitting an event or reverting, rather than silently ignoring it.
Suggested implementation:
```
if (pool.host().isApp(pool) == false) {
try
IRegisterSuperapp(address(pool.host())).registerApp(address(pool), SuperAppDefinitions.APP_LEVEL_FINAL)
{} catch {
// fallback for older versions of superfluid used in unit tests
try
IRegisterSuperapp(address(pool.host())).registerAppByFactory(
address(pool),
SuperAppDefinitions.APP_LEVEL_FINAL
);
{} catch {
emit AppRegistrationFailed(address(pool), address(pool.host()));
revert("App registration failed: both registerApp and registerAppByFactory methods failed");
}
}
```
```
// Event to surface registration failures
event AppRegistrationFailed(address indexed app, address indexed host);
```
</issue_to_address>
### Comment 4
<location> `packages/contracts/test/DirectPayments/DirectPaymentsFactory.test.ts:141` </location>
<code_context>
await expect(pool.connect(signers[1]).mintNFT(pool.address, nftSample, false)).not.reverted;
});
+
+ it('should only allow factory admin to withdraw pool funds', async () => {
+ const tx = await factory.createPool('test', 'pool1', poolSettings, poolLimits, 0);
+ const poolAddr = (await (await tx).wait()).events?.find((_) => _.event === 'PoolCreated')?.args?.[0];
+ const pool = (await ethers.getContractAt('DirectPaymentsPool', poolAddr)) as DirectPaymentsPool;
+ await gdframework.GoodDollar.mint(pool.address, ethers.constants.WeiPerEther.mul(100000)).then((_: any) =>
+ _.wait()
+ );
+ await expect(pool.recoverFunds(signer.address, 1)).not.reverted;
+ await expect(pool.connect(signers[1]).recoverFunds(signer.address, 1)).revertedWith('not owner');
+ });
});
</code_context>
<issue_to_address>
Missing test for edge cases in recoverFunds (invalid recipient, zero amount, contract recipient, etc.)
Please add tests for zero address recipient, zero amount, exceeding available funds, and contract recipient to fully cover edge cases in recoverFunds.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
it('should only allow factory admin to withdraw pool funds', async () => {
const tx = await factory.createPool('test', 'pool1', poolSettings, poolLimits, 0);
const poolAddr = (await (await tx).wait()).events?.find((_) => _.event === 'PoolCreated')?.args?.[0];
const pool = (await ethers.getContractAt('DirectPaymentsPool', poolAddr)) as DirectPaymentsPool;
await gdframework.GoodDollar.mint(pool.address, ethers.constants.WeiPerEther.mul(100000)).then((_: any) =>
_.wait()
);
await expect(pool.recoverFunds(signer.address, 1)).not.reverted;
await expect(pool.connect(signers[1]).recoverFunds(signer.address, 1)).revertedWith('not owner');
});
=======
it('should only allow factory admin to withdraw pool funds', async () => {
const tx = await factory.createPool('test', 'pool1', poolSettings, poolLimits, 0);
const poolAddr = (await (await tx).wait()).events?.find((_) => _.event === 'PoolCreated')?.args?.[0];
const pool = (await ethers.getContractAt('DirectPaymentsPool', poolAddr)) as DirectPaymentsPool;
await gdframework.GoodDollar.mint(pool.address, ethers.constants.WeiPerEther.mul(100000)).then((_: any) =>
_.wait()
);
await expect(pool.recoverFunds(signer.address, 1)).not.reverted;
await expect(pool.connect(signers[1]).recoverFunds(signer.address, 1)).revertedWith('not owner');
});
it('should revert if recipient is zero address', async () => {
const tx = await factory.createPool('test', 'pool2', poolSettings, poolLimits, 0);
const poolAddr = (await (await tx).wait()).events?.find((_) => _.event === 'PoolCreated')?.args?.[0];
const pool = (await ethers.getContractAt('DirectPaymentsPool', poolAddr)) as DirectPaymentsPool;
await gdframework.GoodDollar.mint(pool.address, ethers.constants.WeiPerEther.mul(100000)).then((_: any) =>
_.wait()
);
await expect(pool.recoverFunds(ethers.constants.AddressZero, 1)).to.be.revertedWith('invalid recipient');
});
it('should revert if amount is zero', async () => {
const tx = await factory.createPool('test', 'pool3', poolSettings, poolLimits, 0);
const poolAddr = (await (await tx).wait()).events?.find((_) => _.event === 'PoolCreated')?.args?.[0];
const pool = (await ethers.getContractAt('DirectPaymentsPool', poolAddr)) as DirectPaymentsPool;
await gdframework.GoodDollar.mint(pool.address, ethers.constants.WeiPerEther.mul(100000)).then((_: any) =>
_.wait()
);
await expect(pool.recoverFunds(signer.address, 0)).to.be.revertedWith('invalid amount');
});
it('should revert if amount exceeds available funds', async () => {
const tx = await factory.createPool('test', 'pool4', poolSettings, poolLimits, 0);
const poolAddr = (await (await tx).wait()).events?.find((_) => _.event === 'PoolCreated')?.args?.[0];
const pool = (await ethers.getContractAt('DirectPaymentsPool', poolAddr)) as DirectPaymentsPool;
await gdframework.GoodDollar.mint(pool.address, ethers.constants.WeiPerEther.mul(100)).then((_: any) =>
_.wait()
);
await expect(pool.recoverFunds(signer.address, ethers.constants.WeiPerEther.mul(101))).to.be.revertedWith('insufficient funds');
});
it('should revert if recipient is a contract', async () => {
const tx = await factory.createPool('test', 'pool5', poolSettings, poolLimits, 0);
const poolAddr = (await (await tx).wait()).events?.find((_) => _.event === 'PoolCreated')?.args?.[0];
const pool = (await ethers.getContractAt('DirectPaymentsPool', poolAddr)) as DirectPaymentsPool;
await gdframework.GoodDollar.mint(pool.address, ethers.constants.WeiPerEther.mul(100000)).then((_: any) =>
_.wait()
);
// Deploy a dummy contract to use as recipient
const DummyRecipient = await ethers.getContractFactory('ProvableNFT');
const dummyRecipient = await upgrades.deployProxy(DummyRecipient, ['nft', 'cc'], {
kind: 'uups',
validEvents: [1, 2],
manager: signers[1].address,
membersValidator: ethers.constants.AddressZero,
rewardToken: gdframework.GoodDollar.address,
allowRewardOverride: false,
});
await dummyRecipient.deployed();
await expect(pool.recoverFunds(dummyRecipient.address, 1)).to.be.revertedWith('recipient is contract');
});
>>>>>>> REPLACE
</suggested_fix>
### Comment 5
<location> `packages/contracts/test/DirectPayments/DirectPayments.claim.test.ts:186` </location>
<code_context>
- await expect(pool['claim(uint256)'](nftId)).revertedWithCustomError(pool, 'OVER_MEMBER_LIMITS');
- } else await pool['claim(uint256)'](nftId);
- await time.increase(86400);
+ const claimTx = await pool['claim(uint256)'](nftId);
+ if (i >= 11) {
+ await expect(claimTx).to.emit(pool, 'NOT_MEMBER_OR_WHITELISTED_OR_LIMITS');
+ } else {
+ await expect(claimTx).not.to.emit(pool, 'NOT_MEMBER_OR_WHITELISTED_OR_LIMITS');
+ await time.increase(86400);
+ }
</code_context>
<issue_to_address>
Tests for claim limits now check for event emission, but do not verify state changes or balances.
Please add assertions to verify that member balances and pool state are unchanged after a failed claim to ensure no unintended side effects.
Suggested implementation:
```typescript
// Record member balance and pool state before claim
const memberAddress = await nft.ownerOf(nftId);
const balanceBefore = await pool.balanceOf(memberAddress);
const poolStateBefore = await pool.totalClaimed();
const claimTx = await pool['claim(uint256)'](nftId);
if (i >= 11) {
await expect(claimTx).to.emit(pool, 'NOT_MEMBER_OR_WHITELISTED_OR_LIMITS');
// Assert member balance and pool state unchanged
const balanceAfter = await pool.balanceOf(memberAddress);
const poolStateAfter = await pool.totalClaimed();
expect(balanceAfter).to.equal(balanceBefore);
expect(poolStateAfter).to.equal(poolStateBefore);
} else {
await expect(claimTx).not.to.emit(pool, 'NOT_MEMBER_OR_WHITELISTED_OR_LIMITS');
// Optionally, assert that balance or pool state has changed
const balanceAfter = await pool.balanceOf(memberAddress);
const poolStateAfter = await pool.totalClaimed();
expect(balanceAfter).to.be.above(balanceBefore);
expect(poolStateAfter).to.be.above(poolStateBefore);
await time.increase(86400);
}
```
- If `pool.balanceOf` or `pool.totalClaimed` do not exist or do not represent the correct state, replace them with the appropriate contract calls for your pool/member state.
- You may need to import `expect` from your test framework if not already present.
- Adjust the state checks to match your contract's actual state variables for claims and balances.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| TransferHelper.safeTransfer(address(superToken), recipient, fee); | ||
| } | ||
|
|
||
| function recoverFunds(address _recipient, uint256 amount) external { |
There was a problem hiding this comment.
🚨 issue (security): recoverFunds function exposes fund recovery to any caller with registry role 0x00.
Please verify that only trusted accounts can be assigned registry role 0x00 to prevent unauthorized fund transfers.
| pool = DirectPaymentsPool(address(new BeaconProxy(address(impl), initCall))); | ||
| } else { | ||
| pool = DirectPaymentsPool(address(new ERC1967Proxy(impl.implementation(), initCall))); | ||
| } |
There was a problem hiding this comment.
suggestion (bug_risk): Fallback to registerAppByFactory improves compatibility but may mask registration failures.
If both registration attempts fail, the error is ignored. Please add logging or revert to prevent silent misconfiguration.
| pool = UBIPool(address(new BeaconProxy(address(impl), initCall))); | ||
| } else { | ||
| pool = UBIPool(address(new ERC1967Proxy(impl.implementation(), initCall))); | ||
| } |
There was a problem hiding this comment.
suggestion (bug_risk): Silent catch in app registration may hide critical errors.
If both registration methods fail, ensure the error is surfaced by emitting an event or reverting, rather than silently ignoring it.
Suggested implementation:
if (pool.host().isApp(pool) == false) {
try
IRegisterSuperapp(address(pool.host())).registerApp(address(pool), SuperAppDefinitions.APP_LEVEL_FINAL)
{} catch {
// fallback for older versions of superfluid used in unit tests
try
IRegisterSuperapp(address(pool.host())).registerAppByFactory(
address(pool),
SuperAppDefinitions.APP_LEVEL_FINAL
);
{} catch {
emit AppRegistrationFailed(address(pool), address(pool.host()));
revert("App registration failed: both registerApp and registerAppByFactory methods failed");
}
}
// Event to surface registration failures
event AppRegistrationFailed(address indexed app, address indexed host);
Comment on lines
+141
to
+150
| it('should only allow factory admin to withdraw pool funds', async () => { | ||
| const tx = await factory.createPool('test', 'pool1', poolSettings, poolLimits, 0); | ||
| const poolAddr = (await (await tx).wait()).events?.find((_) => _.event === 'PoolCreated')?.args?.[0]; | ||
| const pool = (await ethers.getContractAt('DirectPaymentsPool', poolAddr)) as DirectPaymentsPool; | ||
| await gdframework.GoodDollar.mint(pool.address, ethers.constants.WeiPerEther.mul(100000)).then((_: any) => | ||
| _.wait() | ||
| ); | ||
| await expect(pool.recoverFunds(signer.address, 1)).not.reverted; | ||
| await expect(pool.connect(signers[1]).recoverFunds(signer.address, 1)).revertedWith('not owner'); | ||
| }); |
There was a problem hiding this comment.
suggestion (testing): Missing test for edge cases in recoverFunds (invalid recipient, zero amount, contract recipient, etc.)
Please add tests for zero address recipient, zero amount, exceeding available funds, and contract recipient to fully cover edge cases in recoverFunds.
Suggested change
| it('should only allow factory admin to withdraw pool funds', async () => { | |
| const tx = await factory.createPool('test', 'pool1', poolSettings, poolLimits, 0); | |
| const poolAddr = (await (await tx).wait()).events?.find((_) => _.event === 'PoolCreated')?.args?.[0]; | |
| const pool = (await ethers.getContractAt('DirectPaymentsPool', poolAddr)) as DirectPaymentsPool; | |
| await gdframework.GoodDollar.mint(pool.address, ethers.constants.WeiPerEther.mul(100000)).then((_: any) => | |
| _.wait() | |
| ); | |
| await expect(pool.recoverFunds(signer.address, 1)).not.reverted; | |
| await expect(pool.connect(signers[1]).recoverFunds(signer.address, 1)).revertedWith('not owner'); | |
| }); | |
| it('should only allow factory admin to withdraw pool funds', async () => { | |
| const tx = await factory.createPool('test', 'pool1', poolSettings, poolLimits, 0); | |
| const poolAddr = (await (await tx).wait()).events?.find((_) => _.event === 'PoolCreated')?.args?.[0]; | |
| const pool = (await ethers.getContractAt('DirectPaymentsPool', poolAddr)) as DirectPaymentsPool; | |
| await gdframework.GoodDollar.mint(pool.address, ethers.constants.WeiPerEther.mul(100000)).then((_: any) => | |
| _.wait() | |
| ); | |
| await expect(pool.recoverFunds(signer.address, 1)).not.reverted; | |
| await expect(pool.connect(signers[1]).recoverFunds(signer.address, 1)).revertedWith('not owner'); | |
| }); | |
| it('should revert if recipient is zero address', async () => { | |
| const tx = await factory.createPool('test', 'pool2', poolSettings, poolLimits, 0); | |
| const poolAddr = (await (await tx).wait()).events?.find((_) => _.event === 'PoolCreated')?.args?.[0]; | |
| const pool = (await ethers.getContractAt('DirectPaymentsPool', poolAddr)) as DirectPaymentsPool; | |
| await gdframework.GoodDollar.mint(pool.address, ethers.constants.WeiPerEther.mul(100000)).then((_: any) => | |
| _.wait() | |
| ); | |
| await expect(pool.recoverFunds(ethers.constants.AddressZero, 1)).to.be.revertedWith('invalid recipient'); | |
| }); | |
| it('should revert if amount is zero', async () => { | |
| const tx = await factory.createPool('test', 'pool3', poolSettings, poolLimits, 0); | |
| const poolAddr = (await (await tx).wait()).events?.find((_) => _.event === 'PoolCreated')?.args?.[0]; | |
| const pool = (await ethers.getContractAt('DirectPaymentsPool', poolAddr)) as DirectPaymentsPool; | |
| await gdframework.GoodDollar.mint(pool.address, ethers.constants.WeiPerEther.mul(100000)).then((_: any) => | |
| _.wait() | |
| ); | |
| await expect(pool.recoverFunds(signer.address, 0)).to.be.revertedWith('invalid amount'); | |
| }); | |
| it('should revert if amount exceeds available funds', async () => { | |
| const tx = await factory.createPool('test', 'pool4', poolSettings, poolLimits, 0); | |
| const poolAddr = (await (await tx).wait()).events?.find((_) => _.event === 'PoolCreated')?.args?.[0]; | |
| const pool = (await ethers.getContractAt('DirectPaymentsPool', poolAddr)) as DirectPaymentsPool; | |
| await gdframework.GoodDollar.mint(pool.address, ethers.constants.WeiPerEther.mul(100)).then((_: any) => | |
| _.wait() | |
| ); | |
| await expect(pool.recoverFunds(signer.address, ethers.constants.WeiPerEther.mul(101))).to.be.revertedWith('insufficient funds'); | |
| }); | |
| it('should revert if recipient is a contract', async () => { | |
| const tx = await factory.createPool('test', 'pool5', poolSettings, poolLimits, 0); | |
| const poolAddr = (await (await tx).wait()).events?.find((_) => _.event === 'PoolCreated')?.args?.[0]; | |
| const pool = (await ethers.getContractAt('DirectPaymentsPool', poolAddr)) as DirectPaymentsPool; | |
| await gdframework.GoodDollar.mint(pool.address, ethers.constants.WeiPerEther.mul(100000)).then((_: any) => | |
| _.wait() | |
| ); | |
| // Deploy a dummy contract to use as recipient | |
| const DummyRecipient = await ethers.getContractFactory('ProvableNFT'); | |
| const dummyRecipient = await upgrades.deployProxy(DummyRecipient, ['nft', 'cc'], { | |
| kind: 'uups', | |
| validEvents: [1, 2], | |
| manager: signers[1].address, | |
| membersValidator: ethers.constants.AddressZero, | |
| rewardToken: gdframework.GoodDollar.address, | |
| allowRewardOverride: false, | |
| }); | |
| await dummyRecipient.deployed(); | |
| await expect(pool.recoverFunds(dummyRecipient.address, 1)).to.be.revertedWith('recipient is contract'); | |
| }); |
Comment on lines
+186
to
+190
| const claimTx = await pool['claim(uint256)'](nftId); | ||
| if (i >= 11) { | ||
| await expect(claimTx).to.emit(pool, 'NOT_MEMBER_OR_WHITELISTED_OR_LIMITS'); | ||
| } else { | ||
| await expect(claimTx).not.to.emit(pool, 'NOT_MEMBER_OR_WHITELISTED_OR_LIMITS'); |
There was a problem hiding this comment.
suggestion (testing): Tests for claim limits now check for event emission, but do not verify state changes or balances.
Please add assertions to verify that member balances and pool state are unchanged after a failed claim to ensure no unintended side effects.
Suggested implementation:
// Record member balance and pool state before claim
const memberAddress = await nft.ownerOf(nftId);
const balanceBefore = await pool.balanceOf(memberAddress);
const poolStateBefore = await pool.totalClaimed();
const claimTx = await pool['claim(uint256)'](nftId);
if (i >= 11) {
await expect(claimTx).to.emit(pool, 'NOT_MEMBER_OR_WHITELISTED_OR_LIMITS');
// Assert member balance and pool state unchanged
const balanceAfter = await pool.balanceOf(memberAddress);
const poolStateAfter = await pool.totalClaimed();
expect(balanceAfter).to.equal(balanceBefore);
expect(poolStateAfter).to.equal(poolStateBefore);
} else {
await expect(claimTx).not.to.emit(pool, 'NOT_MEMBER_OR_WHITELISTED_OR_LIMITS');
// Optionally, assert that balance or pool state has changed
const balanceAfter = await pool.balanceOf(memberAddress);
const poolStateAfter = await pool.totalClaimed();
expect(balanceAfter).to.be.above(balanceBefore);
expect(poolStateAfter).to.be.above(poolStateBefore);
await time.increase(86400);
}- If
pool.balanceOforpool.totalClaimeddo not exist or do not represent the correct state, replace them with the appropriate contract calls for your pool/member state. - You may need to import
expectfrom your test framework if not already present. - Adjust the state checks to match your contract's actual state variables for claims and balances.
Contracts size recoverfunds
🚨 Report Summary
For more details view the full report in OpenZeppelin Code Inspector |
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.
Description by Korbit AI
What change is being made?
Refactor and enhance the DirectPayments smart contracts by introducing checks and balances for rewards, updating member limits, and improving error handling across the DirectPayments ecosystem. Add a recovery function to the GoodCollectiveSuperApp contracts for managing funds safely.
Why are these changes being made?
These changes are motivated by the need to improve the robustness and security of the DirectPayments contracts, ensuring that reward handling is properly managed with limits and errors, while also introducing new functionality such as fund recovery to enhance overall contract administration and compatibility. Adjustments to smart contract registration and library usage ensure backward compatibility and maintain efficient deployment processes.