Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix(contracts): add msg.Value to verifyMessageId #4541

Merged
merged 59 commits into from
Oct 31, 2024
Merged

fix(contracts): add msg.Value to verifyMessageId #4541

merged 59 commits into from
Oct 31, 2024

Conversation

aroralanuk
Copy link
Contributor

@aroralanuk aroralanuk commented Sep 20, 2024 •
edited
Loading

Description

  • Added to the msg.value to verifyMessageId to test if the msgValue passed in is the same as the msgValue used postDispatch

Drive-by changes

  • None

Related issues

Backward compatibility

Yes

Testing

Unit tests

@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Sep 20, 2024 •
edited
Loading

🦋 Changeset detected

Latest commit: f4f638e

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 9 packages
Name Type
@hyperlane-xyz/core Minor
@hyperlane-xyz/helloworld Patch
@hyperlane-xyz/sdk Patch
@hyperlane-xyz/infra Patch
@hyperlane-xyz/cli Patch
@hyperlane-xyz/widgets Patch
@hyperlane-xyz/ccip-server Patch
@hyperlane-xyz/github-proxy Patch
@hyperlane-xyz/utils Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@aroralanuk aroralanuk changed the base branch from main to kunal/external-bridge-refactor September 20, 2024 20:06
github-advanced-security[bot]
github-advanced-security bot found potential problems Sep 25, 2024
View reviewed changes
solidity/contracts/hooks/ArbL2ToL1Hook.sol Fixed Show fixed Hide fixed
solidity/contracts/hooks/ArbL2ToL1Hook.sol Dismissed Show dismissed Hide dismissed
solidity/contracts/hooks/ArbL2ToL1Hook.sol Dismissed Show dismissed Hide dismissed
solidity/contracts/hooks/OPL2ToL1Hook.sol Fixed Show fixed Hide fixed
solidity/contracts/hooks/OPL2ToL1Hook.sol Outdated
// child hook to call first
AbstractPostDispatchHook public immutable childHook;
// Minimum gas limit that the message can be executed with - OP specific
uint32 public constant MIN_GAS_LIMIT = 300_000;

Check warning

Code scanning / Olympix Integrated Security

Some state variables are not being fuzzed in test functions, potentially leaving vulnerabilities unexplored. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/unfuzzed-variables Medium

Some state variables are not being fuzzed in test functions, potentially leaving vulnerabilities unexplored. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/unfuzzed-variables
solidity/contracts/hooks/OPL2ToL1Hook.sol Dismissed Show dismissed Hide dismissed
solidity/contracts/hooks/OPL2ToL1Hook.sol Dismissed Show dismissed Hide dismissed
@aroralanuk aroralanuk changed the title fix(contracts): add msg.Value instead of update fix(contracts): add msg.Value to verifyMessageId Sep 26, 2024
@aroralanuk aroralanuk changed the base branch from main to kunal/HL2408-002-fix September 26, 2024 07:44
@aroralanuk aroralanuk changed the base branch from kunal/HL2408-002-fix to kunal/check-suff-refund-extra September 26, 2024 08:21
github-advanced-security[bot]
github-advanced-security bot found potential problems Oct 4, 2024
View reviewed changes
solidity/contracts/isms/hook/ArbL2ToL1Ism.sol
@@ -44,6 +44,10 @@
// arbitrum nitro contract on L1 to forward verification
IOutbox public arbOutbox;

uint256 private constant DATA_LENGTH = 68;

Check warning

Code scanning / Olympix Integrated Security

Some state variables are not being fuzzed in test functions, potentially leaving vulnerabilities unexplored. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/unfuzzed-variables Medium

Some state variables are not being fuzzed in test functions, potentially leaving vulnerabilities unexplored. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/unfuzzed-variables
solidity/contracts/isms/hook/ArbL2ToL1Ism.sol
@@ -44,6 +44,10 @@
// arbitrum nitro contract on L1 to forward verification
IOutbox public arbOutbox;

uint256 private constant DATA_LENGTH = 68;

uint256 private constant MESSAGE_ID_END = 36;

Check warning

Code scanning / Olympix Integrated Security

Some state variables are not being fuzzed in test functions, potentially leaving vulnerabilities unexplored. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/unfuzzed-variables Medium

Some state variables are not being fuzzed in test functions, potentially leaving vulnerabilities unexplored. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/unfuzzed-variables
Base automatically changed from kunal/check-suff-refund-extra to main October 31, 2024 01:37
github-advanced-security[bot]
github-advanced-security bot found potential problems Oct 31, 2024
View reviewed changes
solidity/contracts/isms/hook/AbstractMessageIdAuthorizedIsm.sol
@@ -53,7 +53,7 @@
// ============ Events ============

/// @notice Emitted when a message is received from the external bridge
event ReceivedMessage(bytes32 indexed messageId);
event ReceivedMessage(bytes32 indexed messageId, uint256 msgValue);

Check warning

Code scanning / Olympix Integrated Security

Test functions fail to assert the emission of expected events, potentially missing critical contract behaviors. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/missing-events-assertion Medium

Test functions fail to assert the emission of expected events, potentially missing critical contract behaviors. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/missing-events-assertion
solidity/contracts/isms/hook/AbstractMessageIdAuthorizedIsm.sol
msg.value < 2 ** VERIFIED_MASK_INDEX && msg.value == msgValue,
"AbstractMessageIdAuthorizedIsm: invalid msg.value"
);
require(

Check warning

Code scanning / Olympix Integrated Security

Test functions fail to verify specific revert reasons, potentially missing important contract behavior validation. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/missing-revert-reason-tests Medium

Test functions fail to verify specific revert reasons, potentially missing important contract behavior validation. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/missing-revert-reason-tests
solidity/contracts/isms/hook/ArbL2ToL1Ism.sol
// this data is an abi encoded call of verifyMessageId(bytes32 messageId)
require(data.length == 36, "ArbL2ToL1Ism: invalid data length");
// this data is an abi encoded call of preVerifyMessage(bytes32 messageId)
require(

Check warning

Code scanning / Olympix Integrated Security

Test functions fail to verify specific revert reasons, potentially missing important contract behavior validation. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/missing-revert-reason-tests Medium

Test functions fail to verify specific revert reasons, potentially missing important contract behavior validation. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/missing-revert-reason-tests
solidity/contracts/isms/hook/layer-zero/LayerZeroV2Ism.sol
* @dev _message will be 36 bytes (4 bytes for function selector, and 32 bytes for messageId)
*/
function _messageId(
bytes calldata _message
) internal pure returns (bytes32) {
return bytes32(_message[FUNC_SELECTOR_OFFSET:]);
return bytes32(_message[FUNC_SELECTOR_OFFSET:ORIGIN_SENDER_OFFSET]);

Check notice

Code scanning / Olympix Integrated Security

Performing a narrowing downcast may result in silent overflow due to bit truncation. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/unsafe-downcast Low

Performing a narrowing downcast may result in silent overflow due to bit truncation. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/unsafe-downcast
solidity/contracts/isms/hook/layer-zero/LayerZeroV2Ism.sol
function _msgValue(
bytes calldata _message
) internal pure returns (uint256) {
return uint256(bytes32(_message[ORIGIN_SENDER_OFFSET:]));

Check notice

Code scanning / Olympix Integrated Security

Performing a narrowing downcast may result in silent overflow due to bit truncation. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/unsafe-downcast Low

Performing a narrowing downcast may result in silent overflow due to bit truncation. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/unsafe-downcast
@aroralanuk aroralanuk enabled auto-merge October 31, 2024 02:16
@aroralanuk aroralanuk added this pull request to the merge queue Oct 31, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 31, 2024
@aroralanuk aroralanuk added this pull request to the merge queue Oct 31, 2024
Merged via the queue into main with commit f26453e Oct 31, 2024
35 of 36 checks passed
@aroralanuk aroralanuk deleted the kunal/verify-message-id branch October 31, 2024 03:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yorhodes yorhodes yorhodes approved these changes

@tkporter tkporter Awaiting requested review from tkporter tkporter is a code owner

@Skunkchain Skunkchain Awaiting requested review from Skunkchain

@avious00 avious00 Awaiting requested review from avious00

@ltyu ltyu Awaiting requested review from ltyu ltyu is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@aroralanuk @yorhodes