refactor(SwapHelperLib)!: replace system contract with uniswap router address #197
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces significant modifications to the Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
fadeev
changed the title
fadeev
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (28)
typechain-types/@openzeppelin/contracts/index.tsis excluded by!typechain-types/**typechain-types/@openzeppelin/contracts/interfaces/draft-IERC6093.sol/IERC1155Errors.tsis excluded by!typechain-types/**typechain-types/@openzeppelin/contracts/interfaces/draft-IERC6093.sol/IERC20Errors.tsis excluded by!typechain-types/**typechain-types/@openzeppelin/contracts/interfaces/draft-IERC6093.sol/IERC721Errors.tsis excluded by!typechain-types/**typechain-types/@openzeppelin/contracts/interfaces/draft-IERC6093.sol/index.tsis excluded by!typechain-types/**typechain-types/@openzeppelin/contracts/interfaces/index.tsis excluded by!typechain-types/**typechain-types/@openzeppelin/contracts/token/ERC20/ERC20.tsis excluded by!typechain-types/**typechain-types/@openzeppelin/contracts/token/ERC20/IERC20.tsis excluded by!typechain-types/**typechain-types/@openzeppelin/contracts/token/ERC20/extensions/IERC20Metadata.tsis excluded by!typechain-types/**typechain-types/contracts/EthZetaMock.sol/ZetaEthMock.tsis excluded by!typechain-types/**typechain-types/contracts/SwapHelperLib.tsis excluded by!typechain-types/**typechain-types/contracts/TestZRC20.tsis excluded by!typechain-types/**typechain-types/contracts/shared/MockZRC20.tsis excluded by!typechain-types/**typechain-types/factories/@openzeppelin/contracts/index.tsis excluded by!typechain-types/**typechain-types/factories/@openzeppelin/contracts/interfaces/draft-IERC6093.sol/IERC1155Errors__factory.tsis excluded by!typechain-types/**typechain-types/factories/@openzeppelin/contracts/interfaces/draft-IERC6093.sol/IERC20Errors__factory.tsis excluded by!typechain-types/**typechain-types/factories/@openzeppelin/contracts/interfaces/draft-IERC6093.sol/IERC721Errors__factory.tsis excluded by!typechain-types/**typechain-types/factories/@openzeppelin/contracts/interfaces/draft-IERC6093.sol/index.tsis excluded by!typechain-types/**typechain-types/factories/@openzeppelin/contracts/interfaces/index.tsis excluded by!typechain-types/**typechain-types/factories/@openzeppelin/contracts/token/ERC20/ERC20__factory.tsis excluded by!typechain-types/**typechain-types/factories/@openzeppelin/contracts/token/ERC20/IERC20__factory.tsis excluded by!typechain-types/**typechain-types/factories/@openzeppelin/contracts/token/ERC20/extensions/IERC20Metadata__factory.tsis excluded by!typechain-types/**typechain-types/factories/contracts/EthZetaMock.sol/ZetaEthMock__factory.tsis excluded by!typechain-types/**typechain-types/factories/contracts/SwapHelperLib__factory.tsis excluded by!typechain-types/**typechain-types/factories/contracts/TestZRC20__factory.tsis excluded by!typechain-types/**typechain-types/factories/contracts/shared/MockZRC20__factory.tsis excluded by!typechain-types/**typechain-types/hardhat.d.tsis excluded by!typechain-types/**typechain-types/index.tsis excluded by!typechain-types/**
📒 Files selected for processing (1)
contracts/SwapHelperLib.sol(6 hunks)
🔇 Additional comments (1)
contracts/SwapHelperLib.sol (1)
140-140:
Potential Path Error: wzeta May Not Be the Correct Intermediate Token
Setting path[1] = wzeta assumes that wzeta is the correct intermediate token for swaps involving zrc20 and targetZRC20. If wzeta does not represent the intended token, the swap path may be incorrect, leading to failed transactions or asset loss.
Ensure that wzeta is the correct intermediary token for the swap. If not, update the code to use the appropriate token:
- path[1] = wzeta;
+ path[1] = wZetaAddress; // Replace with the correct wZETA addressLikely invalid or redundant comment.
| uint256[] memory amounts = IUniswapV2Router01( | ||
| systemContract.uniswapv2Router02Address() | ||
| ).swapTokensForExactTokens( | ||
| IZRC20(zrc20).approve(router, amountInMax); |
There was a problem hiding this comment.
💡 Codebase verification
Approval Addresses Need Standardization
The verification reveals inconsistent approval addresses across the contract:
- Line 1: Approves
targetZRC20for gas fee handling - Line 2: Approves
address(router)explicitly cast - Line 3: Approves
wzetafor token swaps - Line 4: Approves
routerdirectly
While the specific line in question uses router, there's inconsistency in how router approvals are handled (address(router) vs router). Additionally, other approvals to different addresses (targetZRC20, wzeta) exist for specific purposes.
🔗 Analysis chain
Inconsistent Approval Address: Ensure Approval is Granted to router
In the swapTokensForExactTokens function, confirm that the approval is correctly set to the router address and not any other contract.
Run the following script to check for inconsistencies in approval addresses:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all approvals are granted to the `router` address.
# Search for approval statements in the contract
rg 'approve\(([^,]+),' contracts/SwapHelperLib.sol -A 2
# Expected result: All `approve` calls should target `router`.
Length of output: 661
contracts/SwapHelperLib.sol
Outdated
| uint256[] memory amounts = IUniswapV2Router01( | ||
| systemContract.uniswapv2Router02Address() | ||
| ).swapExactTokensForTokens( | ||
| IZRC20(zrc20).approve(wzeta, amount); |
There was a problem hiding this comment.
Incorrect Approval: Approving wzeta Instead of router
In the line IZRC20(zrc20).approve(wzeta, amount);, the contract approves wzeta to spend amount of zrc20 tokens. This seems incorrect, as typically, the approval should be granted to the router, not to wzeta.
Update the approval to target the router address:
- IZRC20(zrc20).approve(wzeta, amount);
+ IZRC20(zrc20).approve(router, amount);This aligns with the pattern used in other functions and ensures the router can execute the swap.
📝 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.
| IZRC20(zrc20).approve(wzeta, amount); | |
| IZRC20(zrc20).approve(router, amount); |
contracts/SwapHelperLib.sol
Outdated
| uint256[] memory amounts = IUniswapV2Router01( | ||
| systemContract.uniswapv2Router02Address() | ||
| ).swapExactTokensForTokens( | ||
| IZRC20(zrc20).approve(address(router), amount); |
There was a problem hiding this comment.
Security Concern: Approving Unverified router to Spend Tokens
Approving the router to spend amount of zrc20 tokens without ensuring the router is trusted poses a significant security risk. If the router address is malicious, it could drain tokens from the contract.
Before granting approval, validate the router address:
+ require(isTrustedRouter(router), "SwapHelperLib: Untrusted router address");
IZRC20(zrc20).approve(address(router), amount);Implement or update the isTrustedRouter function to perform proper validation.
Committable suggestion skipped: line range outside the PR's diff.
| address factory = IUniswapV2Router01(router).factory(); | ||
| address wzeta = IUniswapV2Router01(router).WETH(); | ||
|
|
There was a problem hiding this comment.
Repeat Validation: Ensure Consistent router Address Checks
Similar to earlier functions, the swapExactTokensForTokensDirectly function uses the router address to obtain factory and wzeta without validation.
Add validation for the router address before using it:
+ require(isTrustedRouter(router), "SwapHelperLib: Untrusted router address");
address factory = IUniswapV2Router01(router).factory();
address wzeta = IUniswapV2Router01(router).WETH();Ensure that this validation is consistently applied across all functions that use the router address.
📝 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.
| address factory = IUniswapV2Router01(router).factory(); | |
| address wzeta = IUniswapV2Router01(router).WETH(); | |
| require(isTrustedRouter(router), "SwapHelperLib: Untrusted router address"); | |
| address factory = IUniswapV2Router01(router).factory(); | |
| address wzeta = IUniswapV2Router01(router).WETH(); | |
| amount, | ||
| minAmountOut, | ||
| path | ||
| ); | ||
|
|
||
| bool isZETA = targetZRC20 == systemContract.wZetaContractAddress() || | ||
| zrc20 == systemContract.wZetaContractAddress(); | ||
| bool isZETA = targetZRC20 == wzeta || zrc20 == wzeta; |
There was a problem hiding this comment.
Incorrect Comparison: wzeta May Not Correspond to wZETA Token
The comparison targetZRC20 == wzeta || zrc20 == wzeta relies on wzeta being the correct wZETA token address. If wzeta is obtained from router.WETH() and does not represent the wZETA token on ZetaChain, this comparison may fail or produce unintended results.
Confirm that wzeta represents the correct wZETA token address. If necessary, adjust the code to retrieve the correct address or compare against the proper token:
- bool isZETA = targetZRC20 == wzeta || zrc20 == wzeta;
+ address wZetaAddress = /* obtain correct wZETA address */;
+ bool isZETA = targetZRC20 == wZetaAddress || zrc20 == wZetaAddress;Committable suggestion skipped: line range outside the PR's diff.
| address zrc20, | ||
| uint256 amount, | ||
| address targetZRC20, | ||
| uint256 minAmountOut | ||
| ) internal returns (uint256) { | ||
| address factory = IUniswapV2Router01(router).factory(); | ||
| address wzeta = IUniswapV2Router01(router).WETH(); |
There was a problem hiding this comment.
Potential Mismatch: WETH() May Not Return wZETA Address
The WETH() function in the IUniswapV2Router01 interface typically returns the address of the Wrapped Ether (WETH) token on Ethereum. In the context of ZetaChain, this may not correspond to the wZETA token as intended.
Verify that router.WETH() correctly returns the wZETA address in the ZetaChain environment. If not, consider modifying the contract to retrieve the correct wZETA address:
- address wzeta = IUniswapV2Router01(router).WETH();
+ address wzeta = IWZETA(wzetaContractAddress).address;Ensure that the correct interface or method is used to obtain the wZETA address.
Committable suggestion skipped: line range outside the PR's diff.
| address factory = IUniswapV2Router01(router).factory(); | ||
| address wzeta = IUniswapV2Router01(router).WETH(); | ||
|
|
There was a problem hiding this comment.
Validation Required: Unverified Use of router in View Function
The getMinOutAmount function uses the router to obtain factory and wzeta addresses without validation. Despite being a view function, it could still return incorrect values if router is untrusted.
Add a validation check for the router address:
+ require(isTrustedRouter(router), "SwapHelperLib: Untrusted router address");
address factory = IUniswapV2Router01(router).factory();
address wzeta = IUniswapV2Router01(router).WETH();Ensure that all functions interacting with external contracts validate input addresses.
Committable suggestion skipped: line range outside the PR's diff.
| address router, | ||
| address zrc20, | ||
| uint256 amount, | ||
| address targetZRC20, | ||
| uint256 minAmountOut |
There was a problem hiding this comment.
Critical Security Concern: Validate the router Address
The function now accepts an address router parameter. Since this address is used to interact with external contracts and is critical for the swapping functionality, it's imperative to ensure that the router address provided is a trusted Uniswap router contract. Passing an unverified router address could lead to interactions with malicious contracts, resulting in loss of funds or other security breaches.
Consider adding a validation mechanism to ensure the router address is trusted:
+ require(isTrustedRouter(router), "SwapHelperLib: Untrusted router address");Implement the isTrustedRouter function to check the router address against a whitelist of known, trusted router addresses.
Committable suggestion skipped: line range outside the PR's diff.
| address factory = IUniswapV2Router01(router).factory(); | ||
| address wzeta = IUniswapV2Router01(router).WETH(); |
There was a problem hiding this comment.
Security Risk: Relying on Unverified router for Critical Addresses
By obtaining the factory and wzeta addresses directly from the unvalidated router, there's a risk of retrieving incorrect or malicious addresses. This could compromise the integrity of the swapping operations and lead to potential security vulnerabilities.
Ensure that the router address is validated before using it to obtain critical addresses:
+ require(isTrustedRouter(router), "SwapHelperLib: Untrusted router address");
address factory = IUniswapV2Router01(router).factory();
address wzeta = IUniswapV2Router01(router).WETH();Alternatively, consider obtaining factory and wzeta from trusted sources or passing them as parameters after validation.
Committable suggestion skipped: line range outside the PR's diff.
System contract is being deprecated, so I've replaced it with Uniswap router.
Summary by CodeRabbit
New Features
IUniswapV2Router01interface.Bug Fixes