Validate fromToken and toToken on construction#322
Conversation
✅ Deploy Preview for confidential-tokens ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughAdded validation in the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contracts/finance/BatcherConfidential.sol`:
- Around line 103-105: The constructor currently calls
fromToken_.supportsInterface(...) directly which can revert for non-ERC165
contracts; update the constructor in BatcherConfidential to probe ERC165 safely
(e.g. use ERC165Checker.supportsInterface or perform a low-level/staticcall to
supportsInterface and check the returned bool) before requiring
IERC7984ERC20Wrapper support and emit InvalidFromToken() on failure; apply
analogous validation for toToken_ if it should also implement
IERC7984ERC20Wrapper; add regression tests that pass a plain ERC20Mock and
address(0) to the constructor to assert safe rejections (and retain the existing
ERC7984Mock rejection test).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4c33ad44-516e-4d92-a6fd-432417f842d7
📒 Files selected for processing (2)
contracts/finance/BatcherConfidential.soltest/finance/BatcherConfidential.test.ts
| constructor(ERC7984ERC20Wrapper fromToken_, ERC7984ERC20Wrapper toToken_) { | ||
| require(fromToken_.supportsInterface(type(IERC7984ERC20Wrapper).interfaceId), InvalidFromToken()); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find and examine the target file
fd -t f "BatcherConfidential.sol" | head -5Repository: OpenZeppelin/openzeppelin-confidential-contracts
Length of output: 133
🏁 Script executed:
# Check repository structure and dependencies
ls -la | grep -E "(package.json|foundry.toml|remappings.txt|hardhat.config)"Repository: OpenZeppelin/openzeppelin-confidential-contracts
Length of output: 358
🏁 Script executed:
# Look for imports of ERC165Checker or ERC165 in the project
rg -i "ERC165Checker|ERC165" --type solidity | head -20Repository: OpenZeppelin/openzeppelin-confidential-contracts
Length of output: 124
🏁 Script executed:
# Read the target contract around lines 103-105
cat -n contracts/finance/BatcherConfidential.sol | sed -n '95,115p'Repository: OpenZeppelin/openzeppelin-confidential-contracts
Length of output: 1197
🏁 Script executed:
# Check package.json for OpenZeppelin contracts dependency
cat package.json | grep -A 5 "@openzeppelin"Repository: OpenZeppelin/openzeppelin-confidential-contracts
Length of output: 659
🏁 Script executed:
# Search for ERC165 usage in the codebase
rg "ERC165" contracts/Repository: OpenZeppelin/openzeppelin-confidential-contracts
Length of output: 1665
🏁 Script executed:
# Check if ERC165Checker exists in the local openzeppelin-contracts
find lib/openzeppelin-contracts -name "*ERC165Checker*" -type fRepository: OpenZeppelin/openzeppelin-confidential-contracts
Length of output: 74
🏁 Script executed:
# Read ERC165Checker implementation to understand its behavior
find lib/openzeppelin-contracts -name "ERC165Checker.sol" -exec cat -n {} \;Repository: OpenZeppelin/openzeppelin-confidential-contracts
Length of output: 74
🏁 Script executed:
# Check the IERC165 interface to understand supportsInterface behavior
rg -A 10 "interface IERC165" lib/openzeppelin-contractsRepository: OpenZeppelin/openzeppelin-confidential-contracts
Length of output: 74
🏁 Script executed:
# Check if toToken_ has similar validation or if there are tests for this
rg -B 5 -A 10 "supportsInterface" contracts/finance/BatcherConfidential.solRepository: OpenZeppelin/openzeppelin-confidential-contracts
Length of output: 782
🏁 Script executed:
# Search more broadly for ERC165Checker in openzeppelin-contracts
rg "ERC165Checker" lib/openzeppelin-contracts --type solRepository: OpenZeppelin/openzeppelin-confidential-contracts
Length of output: 119
🏁 Script executed:
# List the contents of the introspection directory
ls -la lib/openzeppelin-contracts/contracts/utils/introspection/ 2>/dev/null || echo "Directory not found"Repository: OpenZeppelin/openzeppelin-confidential-contracts
Length of output: 111
🏁 Script executed:
# Check the actual import statement to see what version/source is being used
head -30 contracts/finance/BatcherConfidential.sol | cat -nRepository: OpenZeppelin/openzeppelin-confidential-contracts
Length of output: 2432
🏁 Script executed:
# Verify the type of fromToken_ parameter and how it's used elsewhere
rg "ERC7984ERC20Wrapper" contracts/finance/BatcherConfidential.solRepository: OpenZeppelin/openzeppelin-confidential-contracts
Length of output: 961
🏁 Script executed:
# Check if lib/openzeppelin-contracts directory exists and explore its structure
ls -la lib/ | head -20Repository: OpenZeppelin/openzeppelin-confidential-contracts
Length of output: 358
🏁 Script executed:
# Look for ERC165Checker more broadly
find lib -name "*.sol" -type f 2>/dev/null | xargs grep -l "ERC165Checker" 2>/dev/null | head -5Repository: OpenZeppelin/openzeppelin-confidential-contracts
Length of output: 74
🏁 Script executed:
# Check the openzeppelin-contracts version being used
cat lib/openzeppelin-contracts/package.json 2>/dev/null | grep -A 2 '"version"' || cat lib/openzeppelin-contracts/package.json 2>/dev/null | head -20Repository: OpenZeppelin/openzeppelin-confidential-contracts
Length of output: 74
🏁 Script executed:
# Let's understand the actual behavior: check if there are any tests for BatcherConfidential
find . -name "*test*" -o -name "*spec*" | grep -i batcherRepository: OpenZeppelin/openzeppelin-confidential-contracts
Length of output: 134
🏁 Script executed:
# Check what IERC7984ERC20Wrapper actually is
cat -n contracts/token/ERC7984/extensions/ERC7984ERC20Wrapper.sol | head -50Repository: OpenZeppelin/openzeppelin-confidential-contracts
Length of output: 2852
🏁 Script executed:
# Check the test file to understand how BatcherConfidential is tested
cat -n test/finance/BatcherConfidential.test.ts | head -100Repository: OpenZeppelin/openzeppelin-confidential-contracts
Length of output: 4039
🏁 Script executed:
# Let's check what's in lib/openzeppelin-contracts directly
ls -la lib/openzeppelin-contracts/ | head -20Repository: OpenZeppelin/openzeppelin-confidential-contracts
Length of output: 198
🏁 Script executed:
# Search for ERC165Checker in the entire lib directory more broadly
find lib -type f -name "*.sol" 2>/dev/null | xargs grep -l "library ERC165Checker" 2>/dev/nullRepository: OpenZeppelin/openzeppelin-confidential-contracts
Length of output: 76
🏁 Script executed:
# Check the IERC165 standard to understand supportsInterface behavior
cat lib/openzeppelin-contracts/IERC165.sol 2>/dev/null || rg -A 5 "interface IERC165" lib/ 2>/dev/null | head -30Repository: OpenZeppelin/openzeppelin-confidential-contracts
Length of output: 74
🏁 Script executed:
# Search web resources or documentation about ERC165Checker behavior
echo "Searching for ERC165Checker in OpenZeppelin v5 contracts"Repository: OpenZeppelin/openzeppelin-confidential-contracts
Length of output: 148
🏁 Script executed:
# Check node_modules for OpenZeppelin contracts
ls -la node_modules/@openzeppelin/contracts/utils/introspection/ 2>/dev/null | head -20Repository: OpenZeppelin/openzeppelin-confidential-contracts
Length of output: 74
🏁 Script executed:
# Look for ERC165Checker in node_modules
find node_modules/@openzeppelin -name "ERC165Checker.sol" 2>/dev/nullRepository: OpenZeppelin/openzeppelin-confidential-contracts
Length of output: 74
🏁 Script executed:
# Check if tests have any coverage for invalid fromToken scenarios
grep -n "InvalidFromToken\|revert\|expect.*throw" test/finance/BatcherConfidential.test.tsRepository: OpenZeppelin/openzeppelin-confidential-contracts
Length of output: 990
🏁 Script executed:
# Understand the actual runtime behavior: check what happens when you call a missing function
# Let's look at the IERC7984ERC20Wrapper interface to understand what's being checked
cat -n contracts/interfaces/IERC7984ERC20Wrapper.sol | head -50Repository: OpenZeppelin/openzeppelin-confidential-contracts
Length of output: 1458
🏁 Script executed:
# Check the actual test that triggers InvalidFromToken
grep -B 10 -A 5 "InvalidFromToken" test/finance/BatcherConfidential.test.ts | head -40Repository: OpenZeppelin/openzeppelin-confidential-contracts
Length of output: 687
🏁 Script executed:
# Look for how the batcher is deployed in the test with invalid tokens
grep -B 5 -A 15 "deployContract.*BatcherConfidentialSwapMock" test/finance/BatcherConfidential.test.tsRepository: OpenZeppelin/openzeppelin-confidential-contracts
Length of output: 2113
🏁 Script executed:
# Check if there are any tests that pass an invalid fromToken (non-ERC165)
grep -C 10 "expect.*InvalidFromToken\|should.*revert.*InvalidFromToken" test/finance/BatcherConfidential.test.tsRepository: OpenZeppelin/openzeppelin-confidential-contracts
Length of output: 74
🏁 Script executed:
# Understand the behavioral difference: check if direct supportsInterface calls vs safe calls differ
# Let's look at the ERC7984.sol implementation to see how supportsInterface is implemented
cat -n contracts/token/ERC7984/ERC7984.sol | grep -A 10 "supportsInterface"Repository: OpenZeppelin/openzeppelin-confidential-contracts
Length of output: 639
🏁 Script executed:
# Check remappings to understand the OpenZeppelin import paths
cat remappings.txtRepository: OpenZeppelin/openzeppelin-confidential-contracts
Length of output: 154
🏁 Script executed:
# Check if ERC165Checker is available in OpenZeppelin v5
web_search "ERC165Checker OpenZeppelin Contracts v5"Repository: OpenZeppelin/openzeppelin-confidential-contracts
Length of output: 142
🏁 Script executed:
# Let's look at what contracts are imported from OpenZeppelin to understand the version
grep -r "@openzeppelin/contracts" contracts/ | head -20Repository: OpenZeppelin/openzeppelin-confidential-contracts
Length of output: 2573
🏁 Script executed:
# Check the package.json to see if there's version pinning for `@openzeppelin/contracts`
cat package.json | grep -A 10 "dependencies"Repository: OpenZeppelin/openzeppelin-confidential-contracts
Length of output: 74
🏁 Script executed:
# Let's verify the actual behavior: check if the test with ERC7984Mock (non-wrapper) truly expects InvalidFromToken
grep -B 5 -A 15 "should reject invalid fromToken" test/finance/BatcherConfidential.test.tsRepository: OpenZeppelin/openzeppelin-confidential-contracts
Length of output: 804
🏁 Script executed:
# Check the lib/openzeppelin-contracts directory structure to understand what's available
find lib/openzeppelin-contracts/contracts/utils -type d | head -20Repository: OpenZeppelin/openzeppelin-confidential-contracts
Length of output: 169
🏁 Script executed:
# Look for introspection utilities in the available lib
find lib/openzeppelin-contracts/contracts/utils -name "*165*" -o -name "*introspection*"Repository: OpenZeppelin/openzeppelin-confidential-contracts
Length of output: 169
🏁 Script executed:
# Let's check the package.json version info more carefully
cat lib/openzeppelin-contracts/package.json 2>/dev/null | grep -E '"version"|"name"' | head -5Repository: OpenZeppelin/openzeppelin-confidential-contracts
Length of output: 74
🏁 Script executed:
# Check what utilities are in the introspection folder
ls -la lib/openzeppelin-contracts/contracts/utils/introspection/ 2>/dev/null || echo "Checking alternative path" && find lib/openzeppelin-contracts -type f -path "*introspection*" 2>/dev/nullRepository: OpenZeppelin/openzeppelin-confidential-contracts
Length of output: 117
🏁 Script executed:
# Search for any safe interface checking patterns already used in the codebase
rg "supportsInterface|AddressChecker|ERC165" contracts/ --max-count 20Repository: OpenZeppelin/openzeppelin-confidential-contracts
Length of output: 2815
🏁 Script executed:
# Let's check if the lib was properly set up or if it's a stub
ls -la lib/openzeppelin-contracts/ 2>/dev/nullRepository: OpenZeppelin/openzeppelin-confidential-contracts
Length of output: 198
🏁 Script executed:
# Check the actual contract that's being deployed in tests to understand the inheritance
grep -r "ERC7984Mock" contracts/mocks/ | head -5Repository: OpenZeppelin/openzeppelin-confidential-contracts
Length of output: 676
🏁 Script executed:
# Look at what ERC7984Mock actually implements
cat contracts/mocks/token/ERC7984Mock.solRepository: OpenZeppelin/openzeppelin-confidential-contracts
Length of output: 3259
🏁 Script executed:
# Verify the test scenario more carefully - what happens when you pass a non-ERC165 contract?
# Search for how the test validates the error
grep -B 20 "should reject invalid fromToken" test/finance/BatcherConfidential.test.ts | tail -30Repository: OpenZeppelin/openzeppelin-confidential-contracts
Length of output: 626
🏁 Script executed:
# Check if there are any alternative implementations or workarounds for ERC165Checker in this project
find contracts -name "*.sol" | xargs grep -l "staticcall\|try.*supportsInterface" 2>/dev/nullRepository: OpenZeppelin/openzeppelin-confidential-contracts
Length of output: 150
🏁 Script executed:
# Let's verify the actual behavior: check if the test is expecting the revert to be due to supportsInterface call
# or due to the interface check returning false
cat -n test/finance/BatcherConfidential.test.ts | grep -B 30 "should reject invalid fromToken" | tail -40Repository: OpenZeppelin/openzeppelin-confidential-contracts
Length of output: 1016
🏁 Script executed:
# Understand what happens when an ERC7984Mock is passed - check if it implements IERC7984ERC20Wrapper interface
cat contracts/mocks/token/ERC7984ERC20WrapperMock.sol | head -30Repository: OpenZeppelin/openzeppelin-confidential-contracts
Length of output: 1264
🏁 Script executed:
# Check if toToken_ also needs validation
grep -n "toToken_" contracts/finance/BatcherConfidential.sol | head -10Repository: OpenZeppelin/openzeppelin-confidential-contracts
Length of output: 208
🏁 Script executed:
# Verify the exact error scenario: try to understand if passing a plain ERC20 would fail at the supportsInterface call
# Let's check if there are any tests with plain ERC20Mock
grep -n "ERC20Mock\|ZeroAddress" test/finance/BatcherConfidential.test.ts | head -10Repository: OpenZeppelin/openzeppelin-confidential-contracts
Length of output: 617
🏁 Script executed:
# Let's verify the actual behavior by checking the IERC7984ERC20Wrapper interface
# and understanding what supportsInterface should return for ERC7984Mock
grep -A 5 "type(IERC7984ERC20Wrapper).interfaceId" contracts/Repository: OpenZeppelin/openzeppelin-confidential-contracts
Length of output: 124
🏁 Script executed:
# Check what interface ID IERC7984ERC20Wrapper declares
cat contracts/interfaces/IERC7984ERC20Wrapper.solRepository: OpenZeppelin/openzeppelin-confidential-contracts
Length of output: 1220
🏁 Script executed:
# Verify the inheritance chain and supportsInterface implementation in ERC7984
cat -n contracts/token/ERC7984/ERC7984.sol | sed -n '1,80p'Repository: OpenZeppelin/openzeppelin-confidential-contracts
Length of output: 4040
🏁 Script executed:
# Check if toToken_ should also have the same validation
grep -C 10 "toToken_" contracts/finance/BatcherConfidential.sol | head -30Repository: OpenZeppelin/openzeppelin-confidential-contracts
Length of output: 1217
🏁 Script executed:
# Final check: search for any web resources about ERC165Checker in OpenZeppelin
# Try a web search capabilityRepository: OpenZeppelin/openzeppelin-confidential-contracts
Length of output: 74
🌐 Web query:
ERC165Checker OpenZeppelin Contracts documentation
💡 Result:
ERC165Checker is an OpenZeppelin Contracts library for querying whether an address (contract) supports an interface via ERC-165 (IERC165). It returns true/false (it does not revert if unsupported). [1]
Docs (v5.x API – Utilities → Introspection / ERC165Checker): [1]
Import:
import "@openzeppelin/contracts/utils/introspection/ERC165Checker.sol";[1]
Key functions:
supportsERC165(address account) -> boolsupportsInterface(address account, bytes4 interfaceId) -> boolgetSupportedInterfaces(address account, bytes4[] interfaceIds) -> bool[]supportsAllInterfaces(address account, bytes4[] interfaceIds) -> boolsupportsERC165InterfaceUnchecked(address account, bytes4 interfaceId) -> bool
[1]
Use an ERC165-safe probe for fromToken_ and consider toToken_ validation.
Line 104 calls supportsInterface directly on an arbitrary constructor argument. If fromToken_ is a plain ERC20 or non-ERC165 contract, that call reverts before require can emit InvalidFromToken(), leaving a gap in error handling.
Suggested fix
+import {ERC165Checker} from "@openzeppelin/contracts/utils/introspection/ERC165Checker.sol";
import {ERC7984ERC20Wrapper, IERC7984ERC20Wrapper} from "./../token/ERC7984/extensions/ERC7984ERC20Wrapper.sol";
constructor(ERC7984ERC20Wrapper fromToken_, ERC7984ERC20Wrapper toToken_) {
- require(fromToken_.supportsInterface(type(IERC7984ERC20Wrapper).interfaceId), InvalidFromToken());
+ require(
+ ERC165Checker.supportsInterface(address(fromToken_), type(IERC7984ERC20Wrapper).interfaceId),
+ InvalidFromToken()
+ );
_fromToken = fromToken_;
_toToken = toToken_;Tests currently only validate rejection of ERC7984Mock (which supports ERC165). Add regression tests with plain $ERC20Mock and ethers.ZeroAddress to ensure safe handling of non-ERC165 inputs. Also verify whether toToken_ requires similar validation.
📝 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.
| constructor(ERC7984ERC20Wrapper fromToken_, ERC7984ERC20Wrapper toToken_) { | |
| require(fromToken_.supportsInterface(type(IERC7984ERC20Wrapper).interfaceId), InvalidFromToken()); | |
| import {ERC165Checker} from "@openzeppelin/contracts/utils/introspection/ERC165Checker.sol"; | |
| import {ERC7984ERC20Wrapper, IERC7984ERC20Wrapper} from "./../token/ERC7984/extensions/ERC7984ERC20Wrapper.sol"; | |
| constructor(ERC7984ERC20Wrapper fromToken_, ERC7984ERC20Wrapper toToken_) { | |
| require( | |
| ERC165Checker.supportsInterface(address(fromToken_), type(IERC7984ERC20Wrapper).interfaceId), | |
| InvalidFromToken() | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/finance/BatcherConfidential.sol` around lines 103 - 105, The
constructor currently calls fromToken_.supportsInterface(...) directly which can
revert for non-ERC165 contracts; update the constructor in BatcherConfidential
to probe ERC165 safely (e.g. use ERC165Checker.supportsInterface or perform a
low-level/staticcall to supportsInterface and check the returned bool) before
requiring IERC7984ERC20Wrapper support and emit InvalidFromToken() on failure;
apply analogous validation for toToken_ if it should also implement
IERC7984ERC20Wrapper; add regression tests that pass a plain ERC20Mock and
address(0) to the constructor to assert safe rejections (and retain the existing
ERC7984Mock rejection test).
|
Should we validate to token as well? To token could be old versions of the wrapper (since we don't unwrap it ever) |
arr00
changed the title
fromToken on constructionfromToken and toToken on construction
* Start release candidate * Fix local docs generation (#320) * remove properties symlink * add properties.js * lint fix * Remove backwards compatibility from Batcher (#321) * Remove backwards compatibility from Batcher * Apply suggestion from @arr00 * Validate `fromToken` and `toToken` on construction (#322) * Validate `fromToken` on construction * use `ERC165Checker` * check toToken as well * directly validate to and from token * Add additional functions to `IERC7984ERC20Wrapper` (#323) * remove incorrect docs on `unwrap` function * add finalize unwrap and rate to wrapper interface * update batcher to use unwrap request ids instead of unwrap amounts * add `unwrapAmount` to wrapper interface for unwrap request introspection * additional docs * call getter for `unwrapAmount` in `finalizeUnwrap` * update events and errors appropriately * remove `ERC7984ERC20Wrapper` import * fix import order * Move events to interface from `ERC7984ERC20Wrapper` * add note on request id for `unwrap` * change mapping to be of `bytes32` identifiers * add changeset * Allow claiming on behalf of another account in batcher (#324) * Allow claiming on behalf of another account in batcher * revert on claim if users not part of the batch * add error on quit too * Add additional warnings to the batcher (#325) * Return a bytes32 unwrap id from the `_unwrap` function in `BatcherConfidential` (#326) * Return a bytes32 unwrap id from the `_unwrap` function in `BatcherConfidential` * modify warning * Document possible rounding down of user deposits (#327) * Document possible rounding down of user deposits * add contract level comment as well * up * Release v0.4.0 (rc) (#318) * Release v0.4.0 (rc) * update changelog * update changelog * Apply suggestion from @james-toussaint Co-authored-by: James Toussaint <33313130+james-toussaint@users.noreply.github.com> --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Arr00 <13561405+arr00@users.noreply.github.com> Co-authored-by: James Toussaint <33313130+james-toussaint@users.noreply.github.com> * Update node version to 24.x in CI (#328) * Exit release candidate * Release v0.4.0 (#335) * Release v0.4.0 * update changelog * remove changelog duplicate --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Arr00 <13561405+arr00@users.noreply.github.com> --------- Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Arr00 <13561405+arr00@users.noreply.github.com> Co-authored-by: James Toussaint <33313130+james-toussaint@users.noreply.github.com>
2 participants
Summary by CodeRabbit
Bug Fixes
Tests