feat: add validations libraries #593
Conversation
📝 WalkthroughWalkthroughCentralizes input validation by introducing GatewayEVMValidations and GatewayZEVMValidations libraries; updates GatewayEVM and GatewayZEVM to use them. Enhances error types with contextual parameters, adds revert gas limit constant and error, adjusts ZEVM public API and adds getters, and updates tests to new error signatures and validation paths. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant GatewayEVM
participant Validations as GatewayEVMValidations
Note over User,GatewayEVM: EVM deposit/depositAndCall
User->>GatewayEVM: deposit/depositAndCall(params, revertOptions)
GatewayEVM->>Validations: validateDeposit(…)/validateDepositAndCall(…)
Validations-->>GatewayEVM: ok or revert (InsufficientEVMAmount, PayloadSizeExceeded, RevertGasLimitExceeded, ZeroAddress)
alt validation ok
GatewayEVM->>GatewayEVM: proceed with custody/allowance
GatewayEVM-->>User: emit events / success
else validation failed
GatewayEVM-->>User: revert with parameterized error
end
sequenceDiagram
autonumber
actor User
participant GatewayZEVM
participant Valid as GatewayZEVMValidations
participant ZRC20 as IZRC20 Token
Note over User,GatewayZEVM: ZEVM withdraw/deposit/call
User->>GatewayZEVM: withdraw/deposit/execute(…)
GatewayZEVM->>Valid: validate*(receiver/amount/gas/message/revertOptions)
Valid-->>GatewayZEVM: ok or revert (EmptyAddress, InsufficientAmount, InvalidGasLimit, MessageSizeExceeded, RevertGasLimitExceeded)
alt withdraw path
GatewayZEVM->>ZRC20: _safeTransferFrom/_safeBurn
ZRC20-->>GatewayZEVM: success=false? (caught)
alt failure
GatewayZEVM-->>User: revert ZRC20TransferFailed/ZRC20BurnFailed(zrc20, …)
else success
GatewayZEVM-->>User: emit Withdrawn/… and return
end
else deposit/execute path
GatewayZEVM->>ZRC20: _safeDeposit / other
ZRC20-->>GatewayZEVM: success flag
alt failure
GatewayZEVM-->>User: revert ZRC20DepositFailed(zrc20, …)
else success
GatewayZEVM-->>User: emit events / success
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
contracts/zevm/GatewayZEVM.sol (4)
171-195: Validate zrc20 is non-zero in withdraw (ZRC20)Callers can pass address(0), leading to a low-level call failure when reading GAS_LIMIT(). Validate early to return EmptyAddress.
function withdraw( bytes memory receiver, uint256 amount, address zrc20, RevertOptions calldata revertOptions ) external whenNotPaused { - GatewayZEVMValidations.validateWithdrawalParams(receiver, amount, revertOptions); + GatewayZEVMValidations.validateNonZeroAddress(zrc20); + GatewayZEVMValidations.validateWithdrawalParams(receiver, amount, revertOptions);
205-231: Validate zrc20 is non-zero in withdraw (custom gas limit)Same as above; prevent ambiguous low-level failures.
{ - GatewayZEVMValidations.validateWithdrawalParams(receiver, amount, revertOptions); + GatewayZEVMValidations.validateNonZeroAddress(zrc20); + GatewayZEVMValidations.validateWithdrawalParams(receiver, amount, revertOptions); GatewayZEVMValidations.validateGasLimit(gasLimit);
240-266: Validate zrc20 is non-zero in withdrawAndCallEnsure explicit EmptyAddress error before IZRC20 calls.
{ - GatewayZEVMValidations.validateWithdrawalAndCallParams(receiver, amount, message, callOptions, revertOptions); + GatewayZEVMValidations.validateNonZeroAddress(zrc20); + GatewayZEVMValidations.validateWithdrawalAndCallParams(receiver, amount, message, callOptions, revertOptions);
347-359: Validate zrc20 is non-zero in callCall uses zrc20 to compute fees; add non-zero validation to avoid low-level revert on interfaces.
function call( bytes memory receiver, address zrc20, bytes calldata message, CallOptions calldata callOptions, RevertOptions calldata revertOptions ) external whenNotPaused { + GatewayZEVMValidations.validateNonZeroAddress(zrc20); GatewayZEVMValidations.validateCallAndRevertOptions(callOptions, revertOptions, message.length); _call(receiver, zrc20, message, callOptions, revertOptions); }
🧹 Nitpick comments (17)
contracts/Revert.sol (1)
4-11: Introduce MAX_REVERT_GAS_LIMIT and RevertGasLimitExceededGood addition. Consider exposing the max via a common validation utility or a getter to avoid divergence across modules consuming this constant in the future.
contracts/evm/GatewayEVM.sol (4)
181-199: Validate token address in executeWithERC20Add a non-zero (and ideally contract code) check for token to fail fast on bad inputs.
Apply this diff:
function executeWithERC20( MessageContext calldata messageContext, address token, address to, uint256 amount, bytes calldata data ) public nonReentrant onlyRole(ASSET_HANDLER_ROLE) whenNotPaused { - GatewayEVMValidations.validateAmount(amount); - GatewayEVMValidations.validateNonZeroAddress(to); + GatewayEVMValidations.validateAmount(amount); + GatewayEVMValidations.validateNonZeroAddress(to); + GatewayEVMValidations.validateNonZeroAddress(token); // Approve the target contract to spend the tokens if (!_resetApproval(token, to)) revert ApprovalFailed(token, to); IERC20(token).forceApprove(to, amount); ... }
227-234: Validate token address in revertWithERC20Mirror executeWithERC20: validate token is non-zero (and ideally a contract).
Apply this diff:
function revertWithERC20( address token, address to, uint256 amount, bytes calldata data, RevertContext calldata revertContext ) external nonReentrant onlyRole(ASSET_HANDLER_ROLE) whenNotPaused { - GatewayEVMValidations.validateAmount(amount); - GatewayEVMValidations.validateNonZeroAddress(to); + GatewayEVMValidations.validateAmount(amount); + GatewayEVMValidations.validateNonZeroAddress(to); + GatewayEVMValidations.validateNonZeroAddress(token); IERC20(token).safeTransfer(address(to), amount); Revertable(to).onRevert(revertContext); }
412-419: Use SafeERC20.forceApprove for zetaConnector approvalsDirect IERC20.approve can break on non-standard tokens; use SafeERC20.forceApprove for consistency.
Apply this diff:
- if (!IERC20(token).approve(zetaConnector, amount)) revert ApprovalFailed(token, zetaConnector); + IERC20(token).forceApprove(zetaConnector, amount);
239-247: Add nonReentrant to deposit/depositAndCall/call for parity and safetyMain flows forward ETH/tokens and emit events; adding nonReentrant matches the upgrade test and hardens against future changes.
Apply these diffs:
-function deposit(address receiver, RevertOptions calldata revertOptions) external payable whenNotPaused { +function deposit(address receiver, RevertOptions calldata revertOptions) external payable nonReentrant whenNotPaused {-function deposit( +function deposit( address receiver, uint256 amount, address asset, RevertOptions calldata revertOptions ) external - whenNotPaused + whenNotPaused + nonReentrant {-function depositAndCall( +function depositAndCall( address receiver, bytes calldata payload, RevertOptions calldata revertOptions ) external payable - whenNotPaused + whenNotPaused + nonReentrant {-function depositAndCall( +function depositAndCall( address receiver, uint256 amount, address asset, bytes calldata payload, RevertOptions calldata revertOptions ) external - whenNotPaused + whenNotPaused + nonReentrant {-function call( +function call( address receiver, bytes calldata payload, RevertOptions calldata revertOptions ) external - whenNotPaused + whenNotPaused + nonReentrant {Also applies to: 254-268, 274-291, 298-314, 320-330
test/utils/upgrades/GatewayEVMUpgradeTest.sol (3)
185-191: Validate token address in executeWithERC20 (upgrade test)Add a non-zero check for token similar to to.
Apply this diff:
- GatewayEVMValidations.validateAmount(amount); - GatewayEVMValidations.validateNonZeroAddress(to); + GatewayEVMValidations.validateAmount(amount); + GatewayEVMValidations.validateNonZeroAddress(to); + GatewayEVMValidations.validateNonZeroAddress(token);
231-233: Validate token address in revertWithERC20 (upgrade test)Mirror executeWithERC20; validate token not zero.
Apply this diff:
- GatewayEVMValidations.validateAmount(amount); - GatewayEVMValidations.validateNonZeroAddress(to); + GatewayEVMValidations.validateAmount(amount); + GatewayEVMValidations.validateNonZeroAddress(to); + GatewayEVMValidations.validateNonZeroAddress(token);
427-429: Use SafeERC20.forceApprove for zetaConnector approvals (upgrade test)Prefer SafeERC20 to handle non-standard tokens.
Apply this diff:
- if (!IERC20(token).approve(zetaConnector, amount)) revert ApprovalFailed(token, zetaConnector); + IERC20(token).forceApprove(zetaConnector, amount);contracts/zevm/libraries/GatewayZEVMValidations.sol (1)
72-90: DRY up revert gas limit check in validateCallAndRevertOptionsYou repeat the onRevertGasLimit check here. Prefer reusing validateRevertOptions to keep logic centralized and avoid drift.
Apply:
function validateCallAndRevertOptions( CallOptions calldata callOptions, RevertOptions calldata revertOptions, uint256 messageLength ) internal pure { validateGasLimit(callOptions.gasLimit); validateMessageSize(messageLength, revertOptions.revertMessage.length); - if (revertOptions.onRevertGasLimit > MAX_REVERT_GAS_LIMIT) { - revert RevertGasLimitExceeded(revertOptions.onRevertGasLimit, MAX_REVERT_GAS_LIMIT); - } + validateRevertOptions(revertOptions); }test/GatewayZEVM.t.sol (1)
182-188: Tests align with new error selectors and payload sizing — add edge coverageLGTM on updated selectors, abi-encoded reverts, and dynamic size checks. Please add:
- Upper-bound gas limit: expect InvalidGasLimit when gasLimit > gateway.getMaxGasLimit().
- Revert gas limit: expect RevertGasLimitExceeded when revertOptions.onRevertGasLimit > gateway.getMaxRevertGasLimit().
- ZRC20 address zero for user paths (withdraw/withdrawAndCall/call): expect EmptyAddress once contract adds that validation (see contract comment).
Also applies to: 193-199, 203-211, 255-265, 268-276, 276-286, 288-299, 301-311, 313-337, 339-350, 425-429, 431-441, 443-448, 450-454, 516-525, 547-557, 722-726, 728-736, 738-750
contracts/zevm/GatewayZEVM.sol (2)
115-126: Fix _safeDeposit doc commentsParam docs are mismatched; clarify for maintainability.
-// @notice Helper function to safely deposit -// @param zrc20 The ZRC20 token address -// @param amount The target address to receive the deposited tokens -// @param amount The amount to deposit -// @return True if the deposit was successful, false otherwise +/// @notice Helper function to safely deposit ZRC20 tokens +/// @param zrc20 The ZRC20 token address +/// @param target The address to receive the deposited tokens +/// @param amount The amount to deposit +/// @return True if the deposit was successful, false otherwise
30-31: Remove unused using directiveYou call the library with its name; using GatewayZEVMValidations for * is not utilized. Safe to remove.
contracts/evm/libraries/GatewayEVMValidations.sol (1)
27-35: Minor clarity: reuse totalSize variableSmall nit: use totalSize in the conditional for consistency.
function validatePayloadSize(uint256 payloadLength, uint256 revertMessageLength) internal pure { uint256 totalSize = payloadLength + revertMessageLength; - if (payloadLength + revertMessageLength > MAX_PAYLOAD_SIZE) { + if (totalSize > MAX_PAYLOAD_SIZE) { revert IGatewayEVMErrors.PayloadSizeExceeded(totalSize, MAX_PAYLOAD_SIZE); } }test/utils/upgrades/GatewayZEVMUpgradeTest.sol (4)
132-137: Fix NatSpec/comments for _safeDeposit parameters.Param names/descriptions are incorrect/duplicated; use proper order and phrasing.
-// @notice Helper function to safely deposit -// @param zrc20 The ZRC20 token address -// @param amount The target address to receive the deposited tokens -// @param amount The amount to deposit -// @return True if the deposit was successful, false otherwise +/// @notice Helper function to safely deposit ZRC20 tokens. +/// @param zrc20 The ZRC20 token address. +/// @param target The address to receive the deposited tokens. +/// @param amount The amount to deposit. +/// @return success True if the deposit was successful, false otherwise.
33-33: Remove unused using directive.using GatewayZEVMValidations for *; is unused (all calls are static). Drop it for clarity.
- using GatewayZEVMValidations for *;
189-212: Add nonReentrant to external flows performing token calls.withdraw/withdraw (custom gas)/withdrawAndCall (and call) perform external token interactions before completion. Align with other entry points (execute*, depositAnd*) and add nonReentrant.
function withdraw( @@ - external - whenNotPaused + external + nonReentrant + whenNotPaused ) {function withdraw( @@ - external - whenNotPaused + external + nonReentrant + whenNotPaused ) {function withdrawAndCall( @@ - external - whenNotPaused + external + nonReentrant + whenNotPaused ) {function call( @@ - external - whenNotPaused + external + nonReentrant + whenNotPaused ) {Also applies to: 223-248, 258-283, 365-376
255-255: Fix typo in NatSpec.Use “arbitrary”.
-/// @param callOptions Call options including gas limit and arbirtrary call flag. +/// @param callOptions Call options including gas limit and arbitrary call flag.Also applies to: 362-362
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (12)
contracts/Revert.sol(1 hunks)contracts/evm/GatewayEVM.sol(14 hunks)contracts/evm/interfaces/IGatewayEVM.sol(3 hunks)contracts/evm/libraries/GatewayEVMValidations.sol(1 hunks)contracts/zevm/GatewayZEVM.sol(17 hunks)contracts/zevm/interfaces/IGatewayZEVM.sol(2 hunks)contracts/zevm/libraries/GatewayZEVMValidations.sol(1 hunks)test/ERC20Custody.t.sol(3 hunks)test/GatewayEVM.t.sol(9 hunks)test/GatewayZEVM.t.sol(20 hunks)test/utils/upgrades/GatewayEVMUpgradeTest.sol(14 hunks)test/utils/upgrades/GatewayZEVMUpgradeTest.sol(17 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
contracts/**
⚙️ CodeRabbit configuration file
Review the Solidity contracts for security vulnerabilities and best practices.
Files:
contracts/evm/interfaces/IGatewayEVM.solcontracts/evm/libraries/GatewayEVMValidations.solcontracts/zevm/GatewayZEVM.solcontracts/zevm/libraries/GatewayZEVMValidations.solcontracts/zevm/interfaces/IGatewayZEVM.solcontracts/evm/GatewayEVM.solcontracts/Revert.sol
test/**
⚙️ CodeRabbit configuration file
Review the test files for proper coverage, edge cases, and best practices.
Files:
test/ERC20Custody.t.soltest/GatewayEVM.t.soltest/GatewayZEVM.t.soltest/utils/upgrades/GatewayZEVMUpgradeTest.soltest/utils/upgrades/GatewayEVMUpgradeTest.sol
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: generate
- GitHub Check: test
- GitHub Check: slither
🔇 Additional comments (12)
test/ERC20Custody.t.sol (1)
295-296: Selectors update to InsufficientEVMAmount looks correctUsing InsufficientEVMAmount.selector aligns with the new consolidated error in IGatewayEVMErrors.
Also applies to: 411-412, 560-561
contracts/evm/GatewayEVM.sol (1)
49-51: Ensure MAX_PAYLOAD_SIZE enforcement is single-sourcedTests read MAX_PAYLOAD_SIZE() from GatewayEVM; confirm GatewayEVMValidations uses the same constant to avoid drift.
Briefly confirm the library derives the max from GatewayEVM (or a common shared source) to prevent inconsistencies between validation and test expectations.
Also applies to: 283-291, 308-314, 327-330
contracts/evm/interfaces/IGatewayEVM.sol (1)
84-86: Interface error updates look consistent with validations
- InsufficientEVMAmount consolidates ETH/ERC20 variants.
- ApprovalFailed(token, spender), NotWhitelistedInCustody(token), PayloadSizeExceeded(provided, maximum) add needed context.
Ensure all emit/revert sites across contracts and tests pass the new parameters correctly, especially PayloadSizeExceeded encodings in deposit/call paths.
Also applies to: 91-94, 102-104, 112-115
test/GatewayEVM.t.sol (3)
537-540: NotWhitelistedInCustody now encoded with token addressCorrectly uses abi.encodeWithSelector(NotWhitelistedInCustody.selector, address(token)).
Also applies to: 632-635
547-551: PayloadSizeExceeded assertions now include provided and max sizesAccurate computation of payloadSize and maxSize; matches new error signature.
Also applies to: 608-612, 708-712, 744-748
576-577: InsufficientEVMAmount selector migrationConsolidation to InsufficientEVMAmount for zero-amount checks looks correct across ETH and ERC20 paths.
Also applies to: 601-602, 677-678, 720-721
contracts/zevm/GatewayZEVM.sol (1)
141-156: Token operations hardened via try/catch — LGTMSafe wrappers around transferFrom/burn reduce bubble-up reverts and enable precise custom errors.
contracts/zevm/interfaces/IGatewayZEVM.sol (1)
78-91: Error surface improvements look goodContext-rich errors (transfer/burn/deposit failures, message size, invalid gas) are consistent and testable. LGTM.
Also applies to: 92-104, 114-129
contracts/evm/libraries/GatewayEVMValidations.sol (1)
53-59: Call revert options and payload checks — LGTMForbidding callOnRevert and enforcing combined payload size matches intended public API guarantees.
Also applies to: 102-113
test/utils/upgrades/GatewayZEVMUpgradeTest.sol (3)
200-211: Confirm event choices and chainId.First withdraw emits WithdrawnV2 with chainId = 0; custom-gas withdraw emits Withdrawn. Is this divergence intentional for the upgrade test surface and do tests assert it? If not, standardize.
Also applies to: 236-247
402-407: Good: deposit safety wrappers and contextual errors._validateDepositParams plus _safeDeposit with ZRC20DepositFailed(zrc20, target, amount) improves safety and debuggability.
Also applies to: 449-453, 489-493
30-32: INotSupportedMethods is already imported via Errors.sol
The interface is declared in contracts/Errors.sol and that file is imported at the top of GatewayZEVMUpgradeTest.sol; no missing import exists.Likely an incorrect or invalid review comment.
| /// @dev Validates that amount is not zero. | ||
| /// @param amount The amount to validate. | ||
| function validateAmount(uint256 amount) internal pure { | ||
| if (amount == 0) revert IGatewayEVMErrors.InsufficientEVMAmount(); |
There was a problem hiding this comment.
Would say ZeroAmount rather than Insufficient
| /// @dev Validates that amount is not zero | ||
| /// @param amount The amount to validate | ||
| function validateAmount(uint256 amount) internal pure { | ||
| if (amount == 0) revert IGatewayZEVMErrors.InsufficientAmount(); |
| GatewayZEVMValidations.validateWithdrawalParams(receiver, amount, revertOptions); | ||
|
|
||
| uint256 gasFee = _withdrawZRC20(amount, zrc20); | ||
| uint256 gasFee = _withdrawZRC20WithGasLimit(amount, zrc20, IZRC20(zrc20).GAS_LIMIT()); |
There was a problem hiding this comment.
Context on this change? Seems out of scope of the PR?
| function _resetApproval(address token, address to) private returns (bool) { | ||
| return IERC20(token).approve(to, 0); | ||
| // Use low-level call to handle tokens that don't return boolean values | ||
| (bool success, bytes memory returnData) = token.call(abi.encodeWithSelector(IERC20.approve.selector, to, 0)); |
| vm.prank(protocolAddress); | ||
| gateway.depositAndCall(context, amount, address(gateway), message); | ||
| } | ||
| // function testDepositZETAAndCallUniversalContractFailsIfTargetIsZeroAddress() public { |
There was a problem hiding this comment.
Let's not keep uncommented code without context, we should provide comment why it's commented with eventual tracked issue or remove if no longer necessary
Implement validation libraries for common checks.
Summary by CodeRabbit
New Features
Refactor
Tests