feat(contracts-rfq): zap router #3431
Conversation
WalkthroughThis pull request introduces a new interface Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (8)
packages/contracts-rfq/contracts/router/ZapRouterV1.sol (2)
82-88: Avoid reassigningamountInparameter within the functionReassigning the
amountInparameter within the loop can be confusing and may lead to unexpected behavior. Consider using a local variable, such aszapAmount, to store the amount for each Zap within the loop.
102-103: Ensure correct amount is compared for last Zap in verificationAfter the loop,
amountInholds the amount from the last Zap. To improve clarity and prevent potential bugs, consider explicitly storing the amount used in the last Zap in a separate variable (e.g.,lastZapAmount) and use it for the comparison withminLastZapAmountIn.packages/contracts-rfq/test/router/ZapRouterV1.t.sol (1)
229-806: Consider refactoring test functions to reduce code duplicationMany of the test functions share similar code structures and patterns. Consider refactoring these tests by using parameterized test functions or helper methods to reduce code duplication and improve maintainability.
packages/contracts-rfq/test/mocks/WETHMock.sol (2)
21-25: Add NatSpec documentation for the mint functionThe implementation correctly maintains the 1:1 ratio between ETH and WETH tokens, but would benefit from NatSpec documentation explaining the parameters and behavior.
+ /// @notice Mints WETH tokens to the specified address while maintaining the 1:1 ETH backing + /// @param to The address to mint tokens to + /// @param amount The amount of tokens to mint function mint(address to, uint256 amount) external {
11-11: Enhance security warning in contract documentationWhile the contract includes a "DO NOT USE IN PRODUCTION" warning, consider adding more explicit documentation about its test-only nature and potential security risks.
-/// @notice WETH mock for testing purposes. DO NOT USE IN PRODUCTION. +/// @notice WETH mock for testing purposes only. +/// @dev WARNING: This contract is strictly for testing and contains privileged functions like `mint`. +/// DO NOT USE IN PRODUCTION as it lacks proper access controls and security measures. contract WETHMock is ERC20, CommonBase {packages/contracts-rfq/test/mocks/PoolMock.sol (3)
30-43: Add safety checks to swap functionWhile this is a mock contract, consider adding basic safety checks to better simulate real-world conditions:
- Add slippage protection via minimum output amount
- Check for potential overflow in calculations
- Add zero amount validation
- function swap(uint256 amountIn, address tokenIn) external returns (uint256 amountOut) { + function swap( + uint256 amountIn, + address tokenIn, + uint256 minAmountOut + ) external returns (uint256 amountOut) { + if (amountIn == 0) revert("PoolMock: zero input"); address tokenOut; if (tokenIn == token0) { tokenOut = token1; amountOut = amountIn * ratioWei / 1e18; } else if (tokenIn == token1) { tokenOut = token0; amountOut = amountIn * 1e18 / ratioWei; } else { revert PoolMock__TokenNotSupported(); } + if (amountOut < minAmountOut) revert("PoolMock: insufficient output"); IERC20(tokenIn).safeTransferFrom(msg.sender, address(this), amountIn); IERC20(tokenOut).safeTransfer(msg.sender, amountOut); }
26-28: Add basic validations to setRatioWeiConsider adding basic validations to better simulate realistic scenarios:
- Validate that ratio is not zero
- Consider adding access control to simulate admin-only changes
+ error PoolMock__InvalidRatio(); + function setRatioWei(uint256 ratioWei_) external { + if (ratioWei_ == 0) revert PoolMock__InvalidRatio(); ratioWei = ratioWei_; }
16-16: Standardize error handling approachConsider defining custom errors for all error conditions to maintain consistency and improve gas efficiency:
error PoolMock__TokenNotSupported(); + error PoolMock__ZeroInput(); + error PoolMock__InsufficientOutput(uint256 actual, uint256 minimum);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
packages/contracts-rfq/contracts/interfaces/IZapRouterV1.sol(1 hunks)packages/contracts-rfq/contracts/interfaces/IZapRouterV1Errors.sol(1 hunks)packages/contracts-rfq/contracts/router/ZapRouterV1.sol(1 hunks)packages/contracts-rfq/test/mocks/PoolMock.sol(1 hunks)packages/contracts-rfq/test/mocks/WETHMock.sol(2 hunks)packages/contracts-rfq/test/router/ZapRouterV1.BalanceChecks.t.sol(1 hunks)packages/contracts-rfq/test/router/ZapRouterV1.t.sol(1 hunks)
🔇 Additional comments (7)
packages/contracts-rfq/contracts/interfaces/IZapRouterV1.sol (1)
1-58: Code and documentation look good
The interface is well-defined with appropriate documentation and function signatures.
packages/contracts-rfq/contracts/router/ZapRouterV1.sol (1)
117-119: Ensure token address is a valid contract
The check token.code.length == 0 in _transferZappedAsset ensures that the token address is a contract. This is a good practice to prevent transferring tokens to non-contract addresses.
packages/contracts-rfq/test/router/ZapRouterV1.BalanceChecks.t.sol (1)
1-187: Test cases are comprehensive and well-structured
The test suite covers various scenarios for unspent funds and balance checks, ensuring robust validation of the performZapsWithBalanceChecks function.
packages/contracts-rfq/test/router/ZapRouterV1.t.sol (1)
33-63: Initialization in setUp function is thorough
The setUp function properly initializes contracts, mints tokens, and sets up approvals, providing a solid foundation for the tests.
packages/contracts-rfq/contracts/interfaces/IZapRouterV1Errors.sol (1)
1-13: Error definitions are clear and appropriate
The custom error definitions are well-named and facilitate precise error handling.
packages/contracts-rfq/test/mocks/WETHMock.sol (1)
7-11: LGTM: Appropriate test utilities inheritance
The addition of CommonBase inheritance provides necessary testing utilities like vm which are used in the mock implementation.
packages/contracts-rfq/test/mocks/PoolMock.sol (1)
8-14: LGTM: Well-structured state variables
Good implementation choices:
- Immutable token addresses for gas optimization
- SafeERC20 usage for secure token operations
- Sensible default ratio of 1e18
| /// - ZapRouter does not perform any checks on the Zap Data, nor the ZapRecipient balance after the Zaps are | ||
| /// performed. |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clarify documentation regarding balance checks
In the performZapsWithBalanceChecks function, the documentation states that "ZapRouter does not perform any checks on the Zap Data, nor the ZapRecipient balance after the Zaps are performed." However, this function is intended to verify that the ZapRecipient balance has not increased after the last Zap. Please update the documentation to accurately reflect the functionality.
| /// @inheritdoc IZapRouterV1 | ||
| function performZapsWithBalanceChecks( | ||
| address zapRecipient, | ||
| uint256 amountIn, | ||
| uint256 minLastZapAmountIn, | ||
| uint256 deadline, | ||
| ZapParams[] calldata zapParams | ||
| ) | ||
| public | ||
| payable | ||
| { | ||
| // Record the initial balances of ZapRecipient for each token. | ||
| uint256 length = zapParams.length; | ||
| uint256[] memory initialBalances = new uint256[](length); | ||
| for (uint256 i = 0; i < length; i++) { | ||
| address token = zapParams[i].token; | ||
| initialBalances[i] = | ||
| token == NATIVE_GAS_TOKEN ? zapRecipient.balance : IERC20(token).balanceOf(zapRecipient); | ||
| } | ||
|
|
||
| // Perform the Zaps as usual. | ||
| performZaps(zapRecipient, amountIn, minLastZapAmountIn, deadline, zapParams); | ||
|
|
||
| // Verify that the ZapRecipient balance for each token has not increased. | ||
| for (uint256 i = 0; i < length; i++) { | ||
| address token = zapParams[i].token; | ||
| uint256 newBalance = | ||
| token == NATIVE_GAS_TOKEN ? zapRecipient.balance : IERC20(token).balanceOf(zapRecipient); | ||
| if (newBalance > initialBalances[i]) revert ZapRouterV1__ZapUnspentFunds(); | ||
| } | ||
| } | ||
|
|
||
| /// @inheritdoc IZapRouterV1 | ||
| function performZaps( | ||
| address zapRecipient, | ||
| uint256 amountIn, | ||
| uint256 minLastZapAmountIn, | ||
| uint256 deadline, | ||
| ZapParams[] calldata zapParams | ||
| ) | ||
| public | ||
| payable | ||
| { | ||
| // Validate the input parameters before proceeding. | ||
| uint256 length = zapParams.length; | ||
| if (block.timestamp > deadline) revert ZapRouterV1__DeadlineExceeded(); | ||
| if (length == 0) revert ZapRouterV1__NoZapsProvided(); | ||
|
|
||
| // Transfer the zapped asset from the user to ZapRecipient. `zapParams[0]` exists as per check above. | ||
| _transferZappedAsset(zapRecipient, zapParams[0].token, amountIn); | ||
|
|
||
| // Perform the Zaps, using predetermined amounts or the full balance of ZapRecipient, if instructed. | ||
| uint256 totalUsedMsgValue = 0; | ||
| for (uint256 i = 0; i < length; i++) { | ||
| address token = zapParams[i].token; | ||
| uint256 msgValue = zapParams[i].msgValue; | ||
|
|
||
| // Adjust amount to be the full balance, if needed. | ||
| amountIn = zapParams[i].amount; | ||
| if (amountIn == FULL_BALANCE) { | ||
| amountIn = token == NATIVE_GAS_TOKEN | ||
| // Existing native balance + msg.value that will be forwarded | ||
| ? zapRecipient.balance + msgValue | ||
| : IERC20(token).balanceOf(zapRecipient); | ||
| } | ||
|
|
||
| _performZap({ | ||
| zapRecipient: zapRecipient, | ||
| msgValue: msgValue, | ||
| zapRecipientCallData: abi.encodeCall(IZapRecipient.zap, (token, amountIn, zapParams[i].zapData)) | ||
| }); | ||
| unchecked { | ||
| // Can do unchecked addition here since we're guaranteed that the sum of all msg.value | ||
| // used for the Zaps won't overflow. | ||
| totalUsedMsgValue += msgValue; | ||
| } | ||
| } | ||
|
|
||
| // Verify amountIn used for the last Zap, and that we fully spent `msg.value`. | ||
| if (amountIn < minLastZapAmountIn) revert ZapRouterV1__AmountInsufficient(); | ||
| if (totalUsedMsgValue < msg.value) revert ZapRouterV1__MsgValueIncorrect(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding reentrancy guards to public functions
The performZapsWithBalanceChecks and performZaps functions make external calls to the zapRecipient. To prevent potential reentrancy attacks, consider adding a nonReentrant modifier (e.g., from OpenZeppelin's ReentrancyGuard) to these functions.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| function performZapsWithBalanceChecks( | ||
| address zapRecipient, | ||
| uint256 amountIn, | ||
| uint256 minLastZapAmountIn, | ||
| uint256 deadline, | ||
| ZapParams[] calldata zapParams | ||
| ) | ||
| public | ||
| payable | ||
| { | ||
| // Record the initial balances of ZapRecipient for each token. | ||
| uint256 length = zapParams.length; | ||
| uint256[] memory initialBalances = new uint256[](length); | ||
| for (uint256 i = 0; i < length; i++) { | ||
| address token = zapParams[i].token; | ||
| initialBalances[i] = | ||
| token == NATIVE_GAS_TOKEN ? zapRecipient.balance : IERC20(token).balanceOf(zapRecipient); | ||
| } | ||
|
|
||
| // Perform the Zaps as usual. | ||
| performZaps(zapRecipient, amountIn, minLastZapAmountIn, deadline, zapParams); | ||
|
|
||
| // Verify that the ZapRecipient balance for each token has not increased. | ||
| for (uint256 i = 0; i < length; i++) { | ||
| address token = zapParams[i].token; | ||
| uint256 newBalance = | ||
| token == NATIVE_GAS_TOKEN ? zapRecipient.balance : IERC20(token).balanceOf(zapRecipient); | ||
| if (newBalance > initialBalances[i]) revert ZapRouterV1__ZapUnspentFunds(); | ||
| } | ||
| } |
Check notice
Code scanning / Slither
Calls inside a loop Low
| function performZapsWithBalanceChecks( | ||
| address zapRecipient, | ||
| uint256 amountIn, | ||
| uint256 minLastZapAmountIn, | ||
| uint256 deadline, | ||
| ZapParams[] calldata zapParams | ||
| ) | ||
| public | ||
| payable | ||
| { | ||
| // Record the initial balances of ZapRecipient for each token. | ||
| uint256 length = zapParams.length; | ||
| uint256[] memory initialBalances = new uint256[](length); | ||
| for (uint256 i = 0; i < length; i++) { | ||
| address token = zapParams[i].token; | ||
| initialBalances[i] = | ||
| token == NATIVE_GAS_TOKEN ? zapRecipient.balance : IERC20(token).balanceOf(zapRecipient); | ||
| } | ||
|
|
||
| // Perform the Zaps as usual. | ||
| performZaps(zapRecipient, amountIn, minLastZapAmountIn, deadline, zapParams); | ||
|
|
||
| // Verify that the ZapRecipient balance for each token has not increased. | ||
| for (uint256 i = 0; i < length; i++) { | ||
| address token = zapParams[i].token; | ||
| uint256 newBalance = | ||
| token == NATIVE_GAS_TOKEN ? zapRecipient.balance : IERC20(token).balanceOf(zapRecipient); | ||
| if (newBalance > initialBalances[i]) revert ZapRouterV1__ZapUnspentFunds(); | ||
| } | ||
| } |
Check notice
Code scanning / Slither
Calls inside a loop Low
| function performZaps( | ||
| address zapRecipient, | ||
| uint256 amountIn, | ||
| uint256 minLastZapAmountIn, | ||
| uint256 deadline, | ||
| ZapParams[] calldata zapParams | ||
| ) | ||
| public | ||
| payable | ||
| { | ||
| // Validate the input parameters before proceeding. | ||
| uint256 length = zapParams.length; | ||
| if (block.timestamp > deadline) revert ZapRouterV1__DeadlineExceeded(); | ||
| if (length == 0) revert ZapRouterV1__NoZapsProvided(); | ||
|
|
||
| // Transfer the zapped asset from the user to ZapRecipient. `zapParams[0]` exists as per check above. | ||
| _transferZappedAsset(zapRecipient, zapParams[0].token, amountIn); | ||
|
|
||
| // Perform the Zaps, using predetermined amounts or the full balance of ZapRecipient, if instructed. | ||
| uint256 totalUsedMsgValue = 0; | ||
| for (uint256 i = 0; i < length; i++) { | ||
| address token = zapParams[i].token; | ||
| uint256 msgValue = zapParams[i].msgValue; | ||
|
|
||
| // Adjust amount to be the full balance, if needed. | ||
| amountIn = zapParams[i].amount; | ||
| if (amountIn == FULL_BALANCE) { | ||
| amountIn = token == NATIVE_GAS_TOKEN | ||
| // Existing native balance + msg.value that will be forwarded | ||
| ? zapRecipient.balance + msgValue | ||
| : IERC20(token).balanceOf(zapRecipient); | ||
| } | ||
|
|
||
| _performZap({ | ||
| zapRecipient: zapRecipient, | ||
| msgValue: msgValue, | ||
| zapRecipientCallData: abi.encodeCall(IZapRecipient.zap, (token, amountIn, zapParams[i].zapData)) | ||
| }); | ||
| unchecked { | ||
| // Can do unchecked addition here since we're guaranteed that the sum of all msg.value | ||
| // used for the Zaps won't overflow. | ||
| totalUsedMsgValue += msgValue; | ||
| } | ||
| } | ||
|
|
||
| // Verify amountIn used for the last Zap, and that we fully spent `msg.value`. | ||
| if (amountIn < minLastZapAmountIn) revert ZapRouterV1__AmountInsufficient(); | ||
| if (totalUsedMsgValue < msg.value) revert ZapRouterV1__MsgValueIncorrect(); | ||
| } |
Check notice
Code scanning / Slither
Calls inside a loop Low
| function performZaps( | ||
| address zapRecipient, | ||
| uint256 amountIn, | ||
| uint256 minLastZapAmountIn, | ||
| uint256 deadline, | ||
| ZapParams[] calldata zapParams | ||
| ) | ||
| public | ||
| payable | ||
| { | ||
| // Validate the input parameters before proceeding. | ||
| uint256 length = zapParams.length; | ||
| if (block.timestamp > deadline) revert ZapRouterV1__DeadlineExceeded(); | ||
| if (length == 0) revert ZapRouterV1__NoZapsProvided(); | ||
|
|
||
| // Transfer the zapped asset from the user to ZapRecipient. `zapParams[0]` exists as per check above. | ||
| _transferZappedAsset(zapRecipient, zapParams[0].token, amountIn); | ||
|
|
||
| // Perform the Zaps, using predetermined amounts or the full balance of ZapRecipient, if instructed. | ||
| uint256 totalUsedMsgValue = 0; | ||
| for (uint256 i = 0; i < length; i++) { | ||
| address token = zapParams[i].token; | ||
| uint256 msgValue = zapParams[i].msgValue; | ||
|
|
||
| // Adjust amount to be the full balance, if needed. | ||
| amountIn = zapParams[i].amount; | ||
| if (amountIn == FULL_BALANCE) { | ||
| amountIn = token == NATIVE_GAS_TOKEN | ||
| // Existing native balance + msg.value that will be forwarded | ||
| ? zapRecipient.balance + msgValue | ||
| : IERC20(token).balanceOf(zapRecipient); | ||
| } | ||
|
|
||
| _performZap({ | ||
| zapRecipient: zapRecipient, | ||
| msgValue: msgValue, | ||
| zapRecipientCallData: abi.encodeCall(IZapRecipient.zap, (token, amountIn, zapParams[i].zapData)) | ||
| }); | ||
| unchecked { | ||
| // Can do unchecked addition here since we're guaranteed that the sum of all msg.value | ||
| // used for the Zaps won't overflow. | ||
| totalUsedMsgValue += msgValue; | ||
| } | ||
| } | ||
|
|
||
| // Verify amountIn used for the last Zap, and that we fully spent `msg.value`. | ||
| if (amountIn < minLastZapAmountIn) revert ZapRouterV1__AmountInsufficient(); | ||
| if (totalUsedMsgValue < msg.value) revert ZapRouterV1__MsgValueIncorrect(); | ||
| } |
Check notice
Code scanning / Slither
Block timestamp Low
| function _performZap(address zapRecipient, uint256 msgValue, bytes memory zapRecipientCallData) internal { | ||
| // Perform the low-level call to ZapRecipient, bubbling up any revert reason. | ||
| bytes memory returnData = | ||
| Address.functionCallWithValue({target: zapRecipient, data: zapRecipientCallData, value: msgValue}); | ||
|
|
||
| // Explicit revert if no return data at all. | ||
| if (returnData.length == 0) revert ZapRouterV1__ZapNoReturnValue(); | ||
| // Check that exactly a single return value was returned. | ||
| if (returnData.length != 32) revert ZapRouterV1__ZapIncorrectReturnValue(); | ||
| // Return value should be abi-encoded hook function selector. | ||
| if (bytes32(returnData) != bytes32(IZapRecipient.zap.selector)) { | ||
| revert ZapRouterV1__ZapIncorrectReturnValue(); | ||
| } | ||
| } |
Check warning
Code scanning / Slither
Dangerous strict equalities Medium
Deploying sanguine-fe with
|
| Latest commit: |
c13c320
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://80d555a7.sanguine-fe.pages.dev |
| Branch Preview URL: | https://feat-zap-router.sanguine-fe.pages.dev |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3431 +/- ##
============================================
Coverage ? 98.31224%
============================================
Files ? 10
Lines ? 474
Branches ? 0
============================================
Hits ? 466
Misses ? 8
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Description
A clear and concise description of the features you're adding in this pull request.
Additional context
Add any other context about the problem you're solving.
Metadata
Summary by CodeRabbit
New Features
IZapRouterV1interface for executing complex token operations.performZapsWithBalanceChecksandperformZapsfunctions for enhanced token management.Bug Fixes
Tests