feat: limit msg and payload size #376
Conversation
skosito
requested review from
andresaiello,
brewmaster012,
lumtis,
CharlieMc0,
fadeev and
fbac
as code owners
|
Caution Review failedThe pull request is closed. 📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces several modifications to the Changes
Possibly related PRs
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
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #376 +/- ##
==========================================
+ Coverage 84.19% 84.77% +0.57%
==========================================
Files 8 8
Lines 386 394 +8
Branches 126 134 +8
==========================================
+ Hits 325 334 +9
+ Misses 61 60 -1 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
🧹 Outside diff range and nitpick comments (10)
v2/contracts/evm/interfaces/IGatewayEVM.sol (3)
93-95: Approve new error with suggestions for improvementThe addition of the
PayloadSizeExceedederror is a good step towards implementing the payload size limitation. However, consider the following suggestions to enhance its usefulness:
- Update the error comment to include the specific size limit (512 characters) mentioned in the PR objectives.
- Consider adding parameters to the error to provide more information when it's thrown. For example:
error PayloadSizeExceeded(uint256 actualSize, uint256 maxSize);Also, note that this interface file doesn't show the implementation of the actual size check. Ensure that the size check is implemented in the contract that implements this interface.
Would you like me to provide an example of how to modify the error declaration with these suggestions?
Line range hint
1-1: Enhance documentation for size limitsTo fully implement the payload size limitation described in the PR objectives, consider the following suggestions:
Add documentation about the 512-character size limit to relevant methods that accept
bytesparameters (e.g.,payload,data) andRevertOptions. For example:/// @notice Deposits ETH to the TSS address and calls an omnichain smart contract. /// @param receiver Address of the receiver. /// @param payload Calldata to pass to the call. Must be less than 512 characters. /// @param revertOptions Revert options. The revertMessage must be less than 512 characters. function depositAndCall( address receiver, bytes calldata payload, RevertOptions calldata revertOptions ) external payable;Consider adding a constant for the size limit at the top of the file:
/// @notice Maximum allowed length for payloads and revert messages uint256 constant MAX_PAYLOAD_SIZE = 512;This constant can then be referenced in the documentation and used in the implementing contract.
Would you like me to provide examples of how to add this documentation to specific methods?
Line range hint
1-1: Summary and response to PR questionsThank you for implementing this payload size limitation. Here's a summary of the review and responses to your questions:
The new
PayloadSizeExceedederror is a good addition. Consider enhancing it with parameters and more specific documentation as suggested in the previous comments.To fully implement the size limit, add documentation about the 512-character limit to relevant methods and consider adding a constant for the limit.
Regarding your questions:
Is the limit of 512 characters too small?
- This depends on your specific use case. 512 characters should be sufficient for most scenarios, but you might want to analyze your typical payload sizes to ensure this limit doesn't unnecessarily restrict legitimate use cases.
Should the size limit be configurable by an administrator?
- Making the limit configurable adds flexibility but also complexity. If you anticipate needing different limits for different environments or expect the limit to change over time, then making it configurable could be beneficial. However, if you expect the limit to remain stable, a hard-coded constant might be simpler to maintain.
Consider discussing these points with your team to make a final decision on the size limit and its configurability.
Would you like suggestions on how to make the size limit configurable if you decide to go that route?
v2/contracts/zevm/interfaces/IGatewayZEVM.sol (1)
Line range hint
1-324: Summary: Message size limit implementation initiated, but further clarification needed.The addition of the
MessageSizeExceeded()error is a good start towards implementing the message and payload size limit as per the PR objectives. However, there are a few points that need to be addressed:
The current implementation doesn't specify the actual size limit of 512 characters mentioned in the PR description. This limit should be clearly defined and enforced in the contract implementation.
Regarding the questions posed in the PR description:
a. Whether the limit of 512 characters is too small: This depends on the specific requirements of your system. Consider analyzing typical message sizes in your current usage and future needs to determine if 512 characters is sufficient.
b. If the size limit should be configurable by an administrator: This could be a valuable feature for flexibility. Consider implementing a configurable limit using a setter function that can only be called by an admin role.The inconsistency between the AI summary and the visible changes (regarding the removal of
EmptyMessage()error) needs to be clarified. If this error was indeed removed, ensure that any code relying on it is updated accordingly.To fully implement the message size limit:
- Define a constant or state variable for the maximum message size.
- Implement checks in relevant functions (e.g.,
withdraw,withdrawAndCall,call) to ensure message size doesn't exceed the limit.- If making the limit configurable, implement a setter function with appropriate access control.
- Update the
MessageSizeExceedederror to include size information as suggested earlier.Please address these points to complete the implementation of the message size limit feature.
v2/contracts/zevm/GatewayZEVM.sol (2)
39-41: Consider making MAX_MESSAGE_SIZE configurableThe addition of
MAX_MESSAGE_SIZEconstant aligns with the PR objective. However, consider the following points:
- The value of 512 characters seems arbitrary. It would be helpful to provide a comment explaining the rationale behind this specific limit.
- As per the PR description, there's a question about whether this limit should be configurable by an administrator. Consider implementing this as an immutable variable set in the constructor or as a configurable value with appropriate access controls.
If you decide to make it configurable, you could implement it like this:
uint256 public immutable MAX_MESSAGE_SIZE; constructor(uint256 _maxMessageSize) { MAX_MESSAGE_SIZE = _maxMessageSize; }Or for runtime configurability (with appropriate access control):
uint256 public MAX_MESSAGE_SIZE; function setMaxMessageSize(uint256 _maxMessageSize) external onlyAdmin { MAX_MESSAGE_SIZE = _maxMessageSize; }
356-356: LGTM: Consistent message size check in non-transfer functionThe message size check is consistently implemented in the
callfunction, aligning with the checks in thewithdrawAndCallfunctions. This is a good practice as it ensures that all cross-chain messages, regardless of whether they involve asset transfer, are subject to the same size limitations.Consider adding a comment explaining why the size limit is also enforced for non-transfer calls. This could help future maintainers understand the rationale behind this decision.
v2/test/GatewayEVM.t.sol (3)
510-519: LGTM! Consider parameterizing the size limit.The test case correctly verifies that the
depositAndCallfunction fails when the payload size exceeds the limit. This aligns well with the PR objective of implementing size limitations for payloads and messages.Consider parameterizing the size limit (currently hardcoded to 256 bytes) to make the test more flexible and easier to maintain if the limit changes in the future. For example:
+ uint256 constant MAX_PAYLOAD_SIZE = 256; function testDepositERC20ToCustodyWithPayloadFailsIfPayloadSizeExceeded() public { uint256 amount = 100_000; - bytes memory payload = new bytes(256); - revertOptions.revertMessage = new bytes(256); + bytes memory payload = new bytes(MAX_PAYLOAD_SIZE); + revertOptions.revertMessage = new bytes(MAX_PAYLOAD_SIZE); token.approve(address(gateway), amount); vm.expectRevert(PayloadSizeExceeded.selector); gateway.depositAndCall(destination, amount, address(token), payload, revertOptions); }
572-579: LGTM! Consider parameterizing the size limit.This test case correctly verifies that the
depositAndCallfunction fails for ETH deposits when the payload size exceeds the limit. The implementation is consistent with the ERC20 test case, which is good for maintaining code uniformity.Similar to the previous suggestion, consider parameterizing the size limit to improve maintainability:
+ uint256 constant MAX_PAYLOAD_SIZE = 256; function testDepositEthToTssWithPayloadFailsIfPayloadSizeExceeded() public { uint256 amount = 100_000; - bytes memory payload = new bytes(256); - revertOptions.revertMessage = new bytes(256); + bytes memory payload = new bytes(MAX_PAYLOAD_SIZE); + revertOptions.revertMessage = new bytes(MAX_PAYLOAD_SIZE); vm.expectRevert(PayloadSizeExceeded.selector); gateway.depositAndCall{ value: amount }(destination, payload, revertOptions); }
Line range hint
510-611: Overall implementation looks good, with room for minor improvements.The new test cases effectively verify the payload size limitation feature, which aligns well with the PR objectives. The implementation is consistent across different functions (ERC20 deposits, ETH deposits, and calls), which is a good practice.
To further improve the implementation:
- Consider parameterizing the MAX_PAYLOAD_SIZE as suggested in individual comments.
- Run the provided verification scripts to gather data on typical payload sizes and existing configurable limits. This will help in making informed decisions about:
a. Whether the 512 character limit is appropriate.
b. If the size limit should be made configurable by an admin.After running the scripts and analyzing the results, you may want to:
- Adjust the MAX_PAYLOAD_SIZE if the current limit is too restrictive.
- Implement an admin-configurable limit if there's a need for flexibility or if other similar limits are configurable.
v2/contracts/evm/GatewayEVM.sol (1)
46-46: Re-evaluate the appropriateness of the 512-byte limitThe current
MAX_PAYLOAD_SIZEis set to 512 bytes. Depending on the complexity of the payloads and revert messages, this limit might be restrictive. Consider analyzing typical payload sizes to determine if a higher limit is necessary to accommodate all valid use cases.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (77)
v2/docs/src/contracts/Revert.sol/interface.Revertable.mdis excluded by!v2/docs/**v2/docs/src/contracts/Revert.sol/struct.RevertContext.mdis excluded by!v2/docs/**v2/docs/src/contracts/Revert.sol/struct.RevertOptions.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/ERC20Custody.sol/contract.ERC20Custody.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/GatewayEVM.sol/contract.GatewayEVM.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/ZetaConnectorBase.sol/abstract.ZetaConnectorBase.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/ZetaConnectorNative.sol/contract.ZetaConnectorNative.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/ZetaConnectorNonNative.sol/contract.ZetaConnectorNonNative.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IERC20Custody.sol/interface.IERC20Custody.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IERC20Custody.sol/interface.IERC20CustodyErrors.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IERC20Custody.sol/interface.IERC20CustodyEvents.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IGatewayEVM.sol/interface.Callable.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IGatewayEVM.sol/interface.IGatewayEVM.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IGatewayEVM.sol/interface.IGatewayEVMErrors.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IGatewayEVM.sol/interface.IGatewayEVMEvents.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IGatewayEVM.sol/struct.MessageContext.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IZetaConnector.sol/interface.IZetaConnectorEvents.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IZetaNonEthNew.sol/interface.IZetaNonEthNew.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/GatewayZEVM.sol/contract.GatewayZEVM.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/SystemContract.sol/contract.SystemContract.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/SystemContract.sol/interface.SystemContractErrors.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/ZRC20.sol/contract.ZRC20.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/ZRC20.sol/interface.ZRC20Errors.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IGatewayZEVM.sol/interface.IGatewayZEVM.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IGatewayZEVM.sol/interface.IGatewayZEVMErrors.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IGatewayZEVM.sol/interface.IGatewayZEVMEvents.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IGatewayZEVM.sol/struct.CallOptions.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/ISystem.sol/interface.ISystem.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IWZETA.sol/interface.IWETH9.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IZRC20.sol/enum.CoinType.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IZRC20.sol/interface.IZRC20.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IZRC20.sol/interface.IZRC20Metadata.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IZRC20.sol/interface.ZRC20Events.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/UniversalContract.sol/interface.UniversalContract.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/UniversalContract.sol/interface.zContract.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/UniversalContract.sol/struct.zContext.mdis excluded by!v2/docs/**v2/pkg/erc20custody.sol/erc20custody.gois excluded by!v2/pkg/**v2/pkg/erc20custody.t.sol/erc20custodytest.gois excluded by!v2/pkg/**v2/pkg/erc20custodyechidnatest.sol/erc20custodyechidnatest.gois excluded by!v2/pkg/**v2/pkg/gatewayevm.sol/gatewayevm.gois excluded by!v2/pkg/**v2/pkg/gatewayevm.t.sol/gatewayevminboundtest.gois excluded by!v2/pkg/**v2/pkg/gatewayevm.t.sol/gatewayevmtest.gois excluded by!v2/pkg/**v2/pkg/gatewayevmechidnatest.sol/gatewayevmechidnatest.gois excluded by!v2/pkg/**v2/pkg/gatewayevmupgrade.t.sol/gatewayevmuupsupgradetest.gois excluded by!v2/pkg/**v2/pkg/gatewayevmupgradetest.sol/gatewayevmupgradetest.gois excluded by!v2/pkg/**v2/pkg/gatewayevmzevm.t.sol/gatewayevmzevmtest.gois excluded by!v2/pkg/**v2/pkg/gatewayzevm.sol/gatewayzevm.gois excluded by!v2/pkg/**v2/pkg/gatewayzevm.t.sol/gatewayzevminboundtest.gois excluded by!v2/pkg/**v2/pkg/gatewayzevm.t.sol/gatewayzevmoutboundtest.gois excluded by!v2/pkg/**v2/pkg/igatewayevm.sol/igatewayevm.gois excluded by!v2/pkg/**v2/pkg/igatewayevm.sol/igatewayevmerrors.gois excluded by!v2/pkg/**v2/pkg/igatewayzevm.sol/igatewayzevm.gois excluded by!v2/pkg/**v2/pkg/igatewayzevm.sol/igatewayzevmerrors.gois excluded by!v2/pkg/**v2/pkg/receiverevm.sol/receiverevm.gois excluded by!v2/pkg/**v2/pkg/senderzevm.sol/senderzevm.gois excluded by!v2/pkg/**v2/pkg/zetaconnectornative.sol/zetaconnectornative.gois excluded by!v2/pkg/**v2/pkg/zetaconnectornative.t.sol/zetaconnectornativetest.gois excluded by!v2/pkg/**v2/pkg/zetaconnectornonnative.sol/zetaconnectornonnative.gois excluded by!v2/pkg/**v2/pkg/zetaconnectornonnative.t.sol/zetaconnectornonnativetest.gois excluded by!v2/pkg/**v2/pkg/zrc20.t.sol/zrc20test.gois excluded by!v2/pkg/**v2/types/GatewayEVM.tsis excluded by!v2/types/**v2/types/GatewayEVMEchidnaTest.tsis excluded by!v2/types/**v2/types/GatewayZEVM.tsis excluded by!v2/types/**v2/types/factories/ERC20CustodyEchidnaTest__factory.tsis excluded by!v2/types/**v2/types/factories/ERC20Custody__factory.tsis excluded by!v2/types/**v2/types/factories/GatewayEVMEchidnaTest__factory.tsis excluded by!v2/types/**v2/types/factories/GatewayEVMUpgradeTest__factory.tsis excluded by!v2/types/**v2/types/factories/GatewayEVM__factory.tsis excluded by!v2/types/**v2/types/factories/GatewayZEVM__factory.tsis excluded by!v2/types/**v2/types/factories/IGatewayEVM.sol/IGatewayEVMErrors__factory.tsis excluded by!v2/types/**v2/types/factories/IGatewayEVM.sol/IGatewayEVM__factory.tsis excluded by!v2/types/**v2/types/factories/IGatewayZEVM.sol/IGatewayZEVMErrors__factory.tsis excluded by!v2/types/**v2/types/factories/IGatewayZEVM.sol/IGatewayZEVM__factory.tsis excluded by!v2/types/**v2/types/factories/ReceiverEVM__factory.tsis excluded by!v2/types/**v2/types/factories/SenderZEVM__factory.tsis excluded by!v2/types/**v2/types/factories/ZetaConnectorNative__factory.tsis excluded by!v2/types/**v2/types/factories/ZetaConnectorNonNative__factory.tsis excluded by!v2/types/**
📒 Files selected for processing (6)
- v2/contracts/evm/GatewayEVM.sol (4 hunks)
- v2/contracts/evm/interfaces/IGatewayEVM.sol (1 hunks)
- v2/contracts/zevm/GatewayZEVM.sol (7 hunks)
- v2/contracts/zevm/interfaces/IGatewayZEVM.sol (1 hunks)
- v2/test/GatewayEVM.t.sol (3 hunks)
- v2/test/GatewayZEVM.t.sol (6 hunks)
🔇 Additional comments (6)
v2/contracts/zevm/interfaces/IGatewayZEVM.sol (1)
89-91: LGTM: New error for message size limit added.The addition of the
MessageSizeExceeded()error aligns well with the PR objective to limit payload and message size. This error will help in handling cases where the message size exceeds the allowed limit in external functions.v2/contracts/zevm/GatewayZEVM.sol (4)
223-223: LGTM: Consistent implementation of message size checkThe message size check here is consistent with the previous
withdrawAndCallfunction overload. This consistency is good for maintainability and reduces the chance of errors.
291-291: LGTM: Consistent message size check across overloadsThe message size check is consistently implemented across all overloads of
withdrawAndCall. This consistency is crucial for maintaining the integrity of the size limit across different function signatures.
Line range hint
1-556: Summary of changes and recommendationsThe implemented changes consistently add a message size check across multiple functions, aligning well with the PR objectives. Here's a summary of the key points and recommendations:
- The
MAX_MESSAGE_SIZEconstant is added, but consider making it configurable as per the PR description's question.- The message size checks are consistently implemented across all relevant functions, which is good for maintainability.
- Consider clarifying whether messages exactly 512 characters long should be allowed or not (>= vs > in the condition).
- Ensure that the
MessageSizeExceedederror is properly defined in the contract.- Consider adding a comment explaining the rationale for applying the size limit to non-transfer calls.
Overall, the changes appear to be well-implemented and achieve the desired objective of limiting message sizes.
184-184: Verify the intended behavior for MAX_MESSAGE_SIZE limitThe added check for message size aligns with the PR objective. However, there are a couple of points to consider:
The condition
>=will reject messages that are exactly 512 characters long. If the intention is to allow messages up to and including 512 characters, consider changing this to>.The error
MessageSizeExceededis not defined in the provided code snippet. Ensure it's properly defined elsewhere in the contract.To ensure consistency across the codebase, let's check for other occurrences of this error:
✅ Verification successful
MessageSizeExceeded is properly defined and used consistently across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
rg "error MessageSizeExceeded" --type solidityLength of output: 81
Script:
#!/bin/bash # Searching for the occurrence of 'MessageSizeExceeded' error in Solidity files rg "MessageSizeExceeded" -g "*.sol"Length of output: 2102
v2/test/GatewayEVM.t.sol (1)
605-611: LGTM! Consider parameterizing the size limit and addressing PR questions.This test case correctly verifies that the
callfunction fails when the payload size exceeds the limit. The implementation is consistent with the previous test cases, which is excellent for maintaining code uniformity.As suggested for the previous test cases, consider parameterizing the size limit:
+ uint256 constant MAX_PAYLOAD_SIZE = 256; function testCallWithPayloadFailsIfPayloadSizeExceeded() public { - bytes memory payload = new bytes(256); - revertOptions.revertMessage = new bytes(256); + bytes memory payload = new bytes(MAX_PAYLOAD_SIZE); + revertOptions.revertMessage = new bytes(MAX_PAYLOAD_SIZE); vm.expectRevert(PayloadSizeExceeded.selector); gateway.call(destination, payload, revertOptions); }Regarding the questions raised in the PR description:
Is the limit of 512 characters too small?
To verify this, we need to analyze the typical payload sizes in the current implementation:Should the size limit be configurable by an admin?
To determine if this is necessary, let's check if there are any existing configurable limits in the contract:These scripts will help gather information to make informed decisions about the payload size limit and its configurability.
|
|
||
| /// @notice Error indicating message size exceeded in external functions. | ||
| error MessageSizeExceeded(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding size information to the error.
While the MessageSizeExceeded() error is functional, it could be more informative. Consider modifying it to include the actual message size and the maximum allowed size. This would provide more context when the error is thrown.
Here's a suggested improvement:
/// @notice Error indicating message size exceeded in external functions.
/// @param actualSize The size of the message that exceeded the limit.
/// @param maxSize The maximum allowed size for messages.
error MessageSizeExceeded(uint256 actualSize, uint256 maxSize);This change would allow for more detailed error reporting and easier debugging.
v2/contracts/evm/GatewayEVM.sol
Outdated
| @@ -42,6 +42,8 @@ contract GatewayEVM is | |||
| bytes32 public constant ASSET_HANDLER_ROLE = keccak256("ASSET_HANDLER_ROLE"); | |||
| /// @notice New role identifier for pauser role. | |||
| bytes32 public constant PAUSER_ROLE = keccak256("PAUSER_ROLE"); | |||
| /// @notice Max payload size. | |||
| uint256 public constant MAX_PAYLOAD_SIZE = 512; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making MAX_PAYLOAD_SIZE configurable
To provide flexibility for future updates or varying use cases, consider allowing MAX_PAYLOAD_SIZE to be adjustable by an administrator through a setter function. This would enable changes to the payload size limit without requiring a contract redeployment.
| @@ -294,6 +296,7 @@ | |||
| { | |||
| if (msg.value == 0) revert InsufficientETHAmount(); | |||
| if (receiver == address(0)) revert ZeroAddress(); | |||
| if (payload.length + revertOptions.revertMessage.length >= MAX_PAYLOAD_SIZE) revert PayloadSizeExceeded(); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor repeated payload size checks into a private function
The payload size validation is repeated in multiple functions. Consider extracting this logic into a private helper function to enhance code maintainability and reduce duplication.
Example:
function _validatePayloadSize(bytes calldata payload, RevertOptions calldata revertOptions) private view {
if (payload.length + revertOptions.revertMessage.length > MAX_PAYLOAD_SIZE) revert PayloadSizeExceeded();
}Then, replace the checks in the functions with a call to this helper function.
Also applies to: 327-327, 348-348
Off-by-one error in payload size check
The condition >= MAX_PAYLOAD_SIZE will revert when the combined payload size is exactly MAX_PAYLOAD_SIZE. If the intention is to allow payloads up to and including MAX_PAYLOAD_SIZE bytes, the comparison should use > instead of >=.
Apply this diff to correct the comparison operator:
- if (payload.length + revertOptions.revertMessage.length >= MAX_PAYLOAD_SIZE) revert PayloadSizeExceeded();
+ if (payload.length + revertOptions.revertMessage.length > MAX_PAYLOAD_SIZE) revert PayloadSizeExceeded();📝 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.
| if (payload.length + revertOptions.revertMessage.length >= MAX_PAYLOAD_SIZE) revert PayloadSizeExceeded(); | |
| if (payload.length + revertOptions.revertMessage.length > MAX_PAYLOAD_SIZE) revert PayloadSizeExceeded(); |
| @@ -321,6 +324,7 @@ | |||
| { | |||
| if (amount == 0) revert InsufficientERC20Amount(); | |||
| if (receiver == address(0)) revert ZeroAddress(); | |||
| if (payload.length + revertOptions.revertMessage.length >= MAX_PAYLOAD_SIZE) revert PayloadSizeExceeded(); | |||
There was a problem hiding this comment.
Off-by-one error in payload size check
Similar to the previous issue, the condition >= MAX_PAYLOAD_SIZE may incorrectly revert valid payloads that are exactly at the maximum allowed size.
Apply this diff to correct the comparison operator:
- if (payload.length + revertOptions.revertMessage.length >= MAX_PAYLOAD_SIZE) revert PayloadSizeExceeded();
+ if (payload.length + revertOptions.revertMessage.length > MAX_PAYLOAD_SIZE) revert PayloadSizeExceeded();📝 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.
| if (payload.length + revertOptions.revertMessage.length >= MAX_PAYLOAD_SIZE) revert PayloadSizeExceeded(); | |
| if (payload.length + revertOptions.revertMessage.length > MAX_PAYLOAD_SIZE) revert PayloadSizeExceeded(); |
| @@ -341,6 +345,8 @@ | |||
| nonReentrant | |||
| { | |||
| if (receiver == address(0)) revert ZeroAddress(); | |||
| if (payload.length + revertOptions.revertMessage.length >= MAX_PAYLOAD_SIZE) revert PayloadSizeExceeded(); | |||
There was a problem hiding this comment.
Off-by-one error in payload size check
Consistent with earlier observations, updating the comparison operator ensures that payloads of exactly MAX_PAYLOAD_SIZE bytes are accepted as valid.
Apply this diff to correct the comparison operator:
- if (payload.length + revertOptions.revertMessage.length >= MAX_PAYLOAD_SIZE) revert PayloadSizeExceeded();
+ if (payload.length + revertOptions.revertMessage.length > MAX_PAYLOAD_SIZE) revert PayloadSizeExceeded();📝 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.
| if (payload.length + revertOptions.revertMessage.length >= MAX_PAYLOAD_SIZE) revert PayloadSizeExceeded(); | |
| if (payload.length + revertOptions.revertMessage.length > MAX_PAYLOAD_SIZE) revert PayloadSizeExceeded(); |
v2/test/GatewayZEVM.t.sol
Outdated
| function testWithdrawAndCallZRC20WithCallOptsFailsIfMessageSizeExceeded() public { | ||
| bytes memory message = new bytes(256); | ||
| revertOptions.revertMessage = new bytes(256); | ||
| vm.expectRevert(MessageSizeExceeded.selector); | ||
| gateway.withdrawAndCall(abi.encodePacked(addr1), 1, address(zrc20), message, callOptions, revertOptions); | ||
| } | ||
|
|
There was a problem hiding this comment.
Verify message size limit consistency across tests
Similar to a previous comment, ensure that the message size limit in this test aligns with the PR objectives.
🛠️ Refactor suggestion
Use a constant for the message size limit instead of hardcoding 256
In the test function testWithdrawAndCallZRC20WithCallOptsFailsIfMessageSizeExceeded, the message size limit is hardcoded. Defining a constant for the message size limit enhances maintainability.
Apply this diff:
+ uint256 constant MESSAGE_SIZE_LIMIT = 256;
+
function testWithdrawAndCallZRC20WithCallOptsFailsIfMessageSizeExceeded() public {
- bytes memory message = new bytes(256);
- revertOptions.revertMessage = new bytes(256);
+ bytes memory message = new bytes(MESSAGE_SIZE_LIMIT);
+ revertOptions.revertMessage = new bytes(MESSAGE_SIZE_LIMIT);📝 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.
| function testWithdrawAndCallZRC20WithCallOptsFailsIfMessageSizeExceeded() public { | |
| bytes memory message = new bytes(256); | |
| revertOptions.revertMessage = new bytes(256); | |
| vm.expectRevert(MessageSizeExceeded.selector); | |
| gateway.withdrawAndCall(abi.encodePacked(addr1), 1, address(zrc20), message, callOptions, revertOptions); | |
| } | |
| uint256 constant MESSAGE_SIZE_LIMIT = 256; | |
| function testWithdrawAndCallZRC20WithCallOptsFailsIfMessageSizeExceeded() public { | |
| bytes memory message = new bytes(MESSAGE_SIZE_LIMIT); | |
| revertOptions.revertMessage = new bytes(MESSAGE_SIZE_LIMIT); | |
| vm.expectRevert(MessageSizeExceeded.selector); | |
| gateway.withdrawAndCall(abi.encodePacked(addr1), 1, address(zrc20), message, callOptions, revertOptions); | |
| } |
v2/test/GatewayZEVM.t.sol
Outdated
| function testWithdrawAndCallZETAFailsIfMessageSizeExceeded() public { | ||
| bytes memory message = new bytes(256); | ||
| revertOptions.revertMessage = new bytes(256); | ||
| vm.expectRevert(MessageSizeExceeded.selector); | ||
| gateway.withdrawAndCall(abi.encodePacked(addr1), 1, 1, message, revertOptions); | ||
| } | ||
|
|
There was a problem hiding this comment.
Confirm message size limit matches PR objectives
Ensure the message size limit used here is consistent with the 512-character limit mentioned in the PR objectives.
🛠️ Refactor suggestion
Abstract message size limit into a constant
To avoid duplication and magic numbers, define a constant for the message size limit in testWithdrawAndCallZETAFailsIfMessageSizeExceeded.
+ uint256 constant MESSAGE_SIZE_LIMIT = 256;
+
function testWithdrawAndCallZETAFailsIfMessageSizeExceeded() public {
- bytes memory message = new bytes(256);
- revertOptions.revertMessage = new bytes(256);
+ bytes memory message = new bytes(MESSAGE_SIZE_LIMIT);
+ revertOptions.revertMessage = new bytes(MESSAGE_SIZE_LIMIT);📝 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.
| function testWithdrawAndCallZETAFailsIfMessageSizeExceeded() public { | |
| bytes memory message = new bytes(256); | |
| revertOptions.revertMessage = new bytes(256); | |
| vm.expectRevert(MessageSizeExceeded.selector); | |
| gateway.withdrawAndCall(abi.encodePacked(addr1), 1, 1, message, revertOptions); | |
| } | |
| uint256 constant MESSAGE_SIZE_LIMIT = 256; | |
| function testWithdrawAndCallZETAFailsIfMessageSizeExceeded() public { | |
| bytes memory message = new bytes(MESSAGE_SIZE_LIMIT); | |
| revertOptions.revertMessage = new bytes(MESSAGE_SIZE_LIMIT); | |
| vm.expectRevert(MessageSizeExceeded.selector); | |
| gateway.withdrawAndCall(abi.encodePacked(addr1), 1, 1, message, revertOptions); | |
| } |
v2/test/GatewayZEVM.t.sol
Outdated
| function testWithdrawAndCallZETAWithCallOptsFailsIfMessageSizeExceeded() public { | ||
| bytes memory message = new bytes(256); | ||
| revertOptions.revertMessage = new bytes(256); | ||
| vm.expectRevert(MessageSizeExceeded.selector); | ||
| gateway.withdrawAndCall(abi.encodePacked(addr1), 1, 1, message, callOptions, revertOptions); | ||
| } | ||
|
|
There was a problem hiding this comment.
Check for consistency with PR objectives on message size limit
Again, verify that the limit used here reflects the intention in the PR objectives.
🛠️ Refactor suggestion
Avoid hardcoding the message size limit
In testWithdrawAndCallZETAWithCallOptsFailsIfMessageSizeExceeded, consider using a constant for the message size limit.
+ uint256 constant MESSAGE_SIZE_LIMIT = 256;
+
function testWithdrawAndCallZETAWithCallOptsFailsIfMessageSizeExceeded() public {
- bytes memory message = new bytes(256);
- revertOptions.revertMessage = new bytes(256);
+ bytes memory message = new bytes(MESSAGE_SIZE_LIMIT);
+ revertOptions.revertMessage = new bytes(MESSAGE_SIZE_LIMIT);📝 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.
| function testWithdrawAndCallZETAWithCallOptsFailsIfMessageSizeExceeded() public { | |
| bytes memory message = new bytes(256); | |
| revertOptions.revertMessage = new bytes(256); | |
| vm.expectRevert(MessageSizeExceeded.selector); | |
| gateway.withdrawAndCall(abi.encodePacked(addr1), 1, 1, message, callOptions, revertOptions); | |
| } | |
| uint256 constant MESSAGE_SIZE_LIMIT = 256; | |
| function testWithdrawAndCallZETAWithCallOptsFailsIfMessageSizeExceeded() public { | |
| bytes memory message = new bytes(MESSAGE_SIZE_LIMIT); | |
| revertOptions.revertMessage = new bytes(MESSAGE_SIZE_LIMIT); | |
| vm.expectRevert(MessageSizeExceeded.selector); | |
| gateway.withdrawAndCall(abi.encodePacked(addr1), 1, 1, message, callOptions, revertOptions); | |
| } |
v2/test/GatewayZEVM.t.sol
Outdated
| function testCallFailsIfMessageSizeExceeded() public { | ||
| bytes memory message = new bytes(256); | ||
| revertOptions.revertMessage = new bytes(256); | ||
| vm.expectRevert(MessageSizeExceeded.selector); | ||
| gateway.call(abi.encodePacked(addr1), address(zrc20), message, 1, revertOptions); | ||
| } | ||
|
|
There was a problem hiding this comment.
Align message size limit with PR objectives
Ensure that the test reflects the intended message size limit of less than 512 characters.
🛠️ Refactor suggestion
Refactor to use a constant for message size limit
In testCallFailsIfMessageSizeExceeded, refactoring to use a constant enhances code clarity.
+ uint256 constant MESSAGE_SIZE_LIMIT = 256;
+
function testCallFailsIfMessageSizeExceeded() public {
- bytes memory message = new bytes(256);
- revertOptions.revertMessage = new bytes(256);
+ bytes memory message = new bytes(MESSAGE_SIZE_LIMIT);
+ revertOptions.revertMessage = new bytes(MESSAGE_SIZE_LIMIT);📝 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.
| function testCallFailsIfMessageSizeExceeded() public { | |
| bytes memory message = new bytes(256); | |
| revertOptions.revertMessage = new bytes(256); | |
| vm.expectRevert(MessageSizeExceeded.selector); | |
| gateway.call(abi.encodePacked(addr1), address(zrc20), message, 1, revertOptions); | |
| } | |
| uint256 constant MESSAGE_SIZE_LIMIT = 256; | |
| function testCallFailsIfMessageSizeExceeded() public { | |
| bytes memory message = new bytes(MESSAGE_SIZE_LIMIT); | |
| revertOptions.revertMessage = new bytes(MESSAGE_SIZE_LIMIT); | |
| vm.expectRevert(MessageSizeExceeded.selector); | |
| gateway.call(abi.encodePacked(addr1), address(zrc20), message, 1, revertOptions); | |
| } |
v2/test/GatewayZEVM.t.sol
Outdated
| function testCallWithCallOptsFailsIfMessageSizeExceeded() public { | ||
| bytes memory message = new bytes(256); | ||
| revertOptions.revertMessage = new bytes(256); | ||
| vm.expectRevert(MessageSizeExceeded.selector); | ||
| gateway.call(abi.encodePacked(addr1), address(zrc20), message, callOptions, revertOptions); | ||
| } | ||
|
|
There was a problem hiding this comment.
Verify consistency of message size limit with PR objectives
Double-check that the message size limit in this test corresponds with the 512-character limit discussed in the PR objectives.
🛠️ Refactor suggestion
Use a defined constant for message size limit
Consistently using a constant for the message size limit in testCallWithCallOptsFailsIfMessageSizeExceeded will improve maintainability.
+ uint256 constant MESSAGE_SIZE_LIMIT = 256;
+
function testCallWithCallOptsFailsIfMessageSizeExceeded() public {
- bytes memory message = new bytes(256);
- revertOptions.revertMessage = new bytes(256);
+ bytes memory message = new bytes(MESSAGE_SIZE_LIMIT);
+ revertOptions.revertMessage = new bytes(MESSAGE_SIZE_LIMIT);Committable suggestion was skipped due to low confidence.
Is this based on an issue raised by an audit? |
limits payload/message len + revertOptions.revertMessage len < 512
questions:
Summary by CodeRabbit
New Features
Bug Fixes
Tests