feat: Implement SafeERC20 for secure ERC20 token transfers #69
Conversation
- Update Solidity version to ^0.8.20 - Import SafeERC20 from OpenZeppelin contracts - Implement safeTransferFrom in payInvoice function - Implement safeTransferFrom in payInvoicesBatch function - Add allowance validation before ERC20 transfers - Maintain CEI pattern for security
📝 WalkthroughWalkthroughThis pull request integrates OpenZeppelin's SafeERC20 library into the Chainvoice contract to enhance ERC20 token transfer safety. The Solidity compiler version is updated from 0.8.13 to 0.8.20, OpenZeppelin is added as a Git submodule with appropriate build system configuration, and token transfer operations are refactored to use SafeERC20 wrappers. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
contracts/src/Chainvoice.sol (1)
279-284: Potential DoS vector in batch native payments.In the batch payment loop for native tokens, if any recipient (
inv.from) is a contract that reverts or consumes excessive gas, the entire batch payment will fail. This could be exploited by a malicious invoice creator to grief payers attempting batch payments.Consider:
- Documenting this behavior so users are aware
- Using a pull-payment pattern for native transfers where recipients withdraw their funds
- Adding gas limits to the
.call{}to prevent gas griefing
🤖 Fix all issues with AI agents
In `@contracts/foundry.toml`:
- Line 8: The comment in contracts/foundry.toml contains a hardcoded private key
in the forge create command; remove or redact that secret by deleting the key or
replacing it with a placeholder such as YOUR_PRIVATE_KEY (and optionally
reference an env var like PRIVATE_KEY), and update the example command to
instruct using environment variables or a secrets manager rather than embedding
keys in comments (look for the forge create command line in the file and replace
the literal key).
🧹 Nitpick comments (4)
contracts/lib/openzeppelin-contracts (1)
1-1: Consider using the semantic version tag instead of the commit hash for clarity.The commit hash
fcbae5394ae8ad52d8e580a3477db99814b9d565corresponds to the official OpenZeppelin Contracts v5.5.0 release (published 2025-10-31), which is compatible with Solidity ^0.8.20 and has no known security vulnerabilities. However, referencing the stable release tagv5.5.0directly (or updating .gitmodules to point to it) would improve transparency and maintainability compared to using a commit hash..gitmodules (1)
7-9: Consider pinning the submodule to a specific release tag.The submodule addition is correct and aligns with the Foundry remappings. However, the submodule references the default branch without pinning to a specific version tag (e.g.,
v5.0.0). For reproducible builds and to avoid unexpected breaking changes, consider checking out a specific release tag after adding the submodule.contracts/src/Chainvoice.sol (1)
212-223: Redundant allowance check before safeTransferFrom.The explicit allowance check on lines 212-215 is redundant since
safeTransferFromwill revert internally if allowance is insufficient. While this provides a clearer custom error message, it adds gas overhead. Consider whether the improved error clarity justifies the extra gas cost.Optional: Remove redundant check
} else { require(msg.value == fee, "Must pay fee in native token"); - require( - IERC20(invoice.tokenAddress).allowance(msg.sender, address(this)) >= invoice.amountDue, - "Insufficient allowance" - ); accumulatedFees += fee; IERC20(invoice.tokenAddress).safeTransferFrom(contracts/test/Chainvoice.t.sol (1)
8-116: Consider adding ERC20 payment tests.The current test suite only covers native token flows. Since this PR introduces SafeERC20 for ERC20 transfers, consider adding tests that:
- Deploy a mock ERC20 token
- Test
payInvoicewith ERC20 tokens- Test
payInvoicesBatchwith ERC20 tokens- Verify behavior with non-standard ERC20 tokens (e.g., tokens that return false instead of reverting)
This would ensure the SafeERC20 integration works as expected.
Would you like me to help generate ERC20 payment test cases?
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
contracts/foundry.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
.gitmodulescontracts/.env.examplecontracts/foundry.tomlcontracts/lib/openzeppelin-contractscontracts/src/Chainvoice.solcontracts/test/Chainvoice.t.sol
💤 Files with no reviewable changes (1)
- contracts/.env.example
🔇 Additional comments (4)
contracts/foundry.toml (1)
5-6: Configuration looks good.The Solidity version bump to 0.8.20 and the OpenZeppelin remapping are correctly configured for SafeERC20 integration.
contracts/src/Chainvoice.sol (2)
2-10: SafeERC20 integration is well implemented.The import structure and
using SafeERC20 for IERC20;directive are correctly set up. This provides automatic revert handling for non-standard ERC20 tokens that don't return a boolean on transfer.
289-296: Batch ERC20 transfer implementation is correct.The SafeERC20 integration in the batch payment flow follows the same secure pattern. The allowance pre-check on lines 290-292 has the same redundancy mentioned earlier but provides consistent error handling.
contracts/test/Chainvoice.t.sol (1)
2-2: Pragma update is correct.The version bump aligns with the main contract.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| solc_version = '0.8.20' | ||
| remappings = ['@openzeppelin/=lib/openzeppelin-contracts/'] | ||
| # See more config options https://github.com/foundry-rs/foundry/blob/master/crates/config/README.md#all-options | ||
| # forge create --rpc-url https://eth-sepolia.g.alchemy.com/v2/nnY0qPUQLYsUvb5BKJM5bh81sI6O0PQG --private-key fba7342ef6879df2c735644c734ea69c140f423d84eb2d53fbdfd53fd5d7c586 src/Token.sol:MyToken --legacy |
There was a problem hiding this comment.
Critical: Remove or redact the private key from the comment.
Line 8 contains what appears to be a private key in a comment. Even if this is a test/example key, hardcoding private keys in version-controlled files is a serious security anti-pattern and can lead to accidental exposure of real credentials. Remove this line or replace the key with a placeholder like YOUR_PRIVATE_KEY.
Proposed fix
-# forge create --rpc-url https://eth-sepolia.g.alchemy.com/v2/nnY0qPUQLYsUvb5BKJM5bh81sI6O0PQG --private-key fba7342ef6879df2c735644c734ea69c140f423d84eb2d53fbdfd53fd5d7c586 src/Token.sol:MyToken --legacy
+# forge create --rpc-url <RPC_URL> --private-key <PRIVATE_KEY> src/Token.sol:MyToken --legacy📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # forge create --rpc-url https://eth-sepolia.g.alchemy.com/v2/nnY0qPUQLYsUvb5BKJM5bh81sI6O0PQG --private-key fba7342ef6879df2c735644c734ea69c140f423d84eb2d53fbdfd53fd5d7c586 src/Token.sol:MyToken --legacy | |
| # forge create --rpc-url <RPC_URL> --private-key <PRIVATE_KEY> src/Token.sol:MyToken --legacy |
🤖 Prompt for AI Agents
In `@contracts/foundry.toml` at line 8, The comment in contracts/foundry.toml
contains a hardcoded private key in the forge create command; remove or redact
that secret by deleting the key or replacing it with a placeholder such as
YOUR_PRIVATE_KEY (and optionally reference an env var like PRIVATE_KEY), and
update the example command to instruct using environment variables or a secrets
manager rather than embedding keys in comments (look for the forge create
command line in the file and replace the literal key).
1 participant
-why??
SafeERC20 provides critical security improvements:
boolon transferTesting
All existing tests pass:
Summary by CodeRabbit
Release Notes
Chores
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.