-
Notifications
You must be signed in to change notification settings - Fork 55
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: limit msg and payload size #376
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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 size of payload + revertOptions revert message. | ||||||
uint256 public constant MAX_PAYLOAD_SIZE = 1024; | ||||||
|
||||||
/// @custom:oz-upgrades-unsafe-allow constructor | ||||||
constructor() { | ||||||
|
@@ -294,6 +296,7 @@ contract GatewayEVM is | |||||
{ | ||||||
if (msg.value == 0) revert InsufficientETHAmount(); | ||||||
if (receiver == address(0)) revert ZeroAddress(); | ||||||
if (payload.length + revertOptions.revertMessage.length >= MAX_PAYLOAD_SIZE) revert PayloadSizeExceeded(); | ||||||
|
||||||
(bool deposited,) = tssAddress.call{ value: msg.value }(""); | ||||||
|
||||||
|
@@ -321,6 +324,7 @@ contract GatewayEVM is | |||||
{ | ||||||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Off-by-one error in payload size check Similar to the previous issue, the condition 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
Suggested change
|
||||||
|
||||||
transferFromToAssetHandler(msg.sender, asset, amount); | ||||||
|
||||||
|
@@ -341,6 +345,8 @@ contract GatewayEVM is | |||||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Off-by-one error in payload size check Consistent with earlier observations, updating the comparison operator ensures that payloads of exactly 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
Suggested change
|
||||||
|
||||||
emit Called(msg.sender, receiver, payload, revertOptions); | ||||||
} | ||||||
|
||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -84,11 +84,11 @@ interface IGatewayZEVMErrors { | |
/// @notice Error indicating that only WZETA or the protocol address can call the function. | ||
error OnlyWZETAOrProtocol(); | ||
|
||
/// @notice Error indicating call method received empty message as argument. | ||
error EmptyMessage(); | ||
lumtis marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
/// @notice Error indicating an insufficient gas limit. | ||
error InsufficientGasLimit(); | ||
|
||
/// @notice Error indicating message size exceeded in external functions. | ||
error MessageSizeExceeded(); | ||
Comment on lines
+89
to
+91
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Consider adding size information to the error. While the 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. |
||
} | ||
|
||
/// @title IGatewayZEVM | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ 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:
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 exactlyMAX_PAYLOAD_SIZE
. If the intention is to allow payloads up to and includingMAX_PAYLOAD_SIZE
bytes, the comparison should use>
instead of>=
.Apply this diff to correct the comparison operator:
📝 Committable suggestion