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.

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

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

Merged
merged 59 commits into from
Oct 31, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
59 commits
Select commit Hold shift + click to select a range
1560f5c
add fix
aroralanuk Sep 5, 2024
44379e1
move to inside
aroralanuk Sep 13, 2024
d680e50
opl2tol1
aroralanuk Sep 16, 2024
ca26e88
arb
aroralanuk Sep 16, 2024
a5c4b62
rm revertWhen_invalidMetadata
aroralanuk Sep 16, 2024
54b4327
msg_value
aroralanuk Sep 16, 2024
66c3bb7
direct call
aroralanuk Sep 16, 2024
e98382d
invalid ism
aroralanuk Sep 16, 2024
56ea53c
no both branch test
aroralanuk Sep 16, 2024
30982d2
unauth hook
aroralanuk Sep 17, 2024
88cb33f
invalid messageId
aroralanuk Sep 17, 2024
baef8ff
5164
aroralanuk Sep 17, 2024
9684835
try catch
aroralanuk Sep 17, 2024
6c31fbf
formatting
aroralanuk Sep 17, 2024
f64611d
opstack
aroralanuk Sep 18, 2024
ac3613e
minor edits
aroralanuk Sep 18, 2024
4d293ea
minor fixes
aroralanuk Sep 18, 2024
fb66306
override inconsistent tests
aroralanuk Sep 18, 2024
13c61fd
Merge branch 'kunal/external-bridge-refactor' into kunal/HL2408-002-fix
aroralanuk Sep 20, 2024
fb089e2
opstack change quote
aroralanuk Sep 20, 2024
c70ac54
Merge branch 'kunal/external-bridge-refactor' into kunal/HL2408-002-fix
aroralanuk Sep 20, 2024
e6c5b10
fix tests
aroralanuk Sep 20, 2024
08dda88
remove reduntant arbitrary call
aroralanuk Sep 20, 2024
37ecb27
rm
aroralanuk Sep 20, 2024
1dd0fc3
changeset
aroralanuk Sep 20, 2024
c99eb24
add verifyMessageId
aroralanuk Sep 20, 2024
97b7bc8
add test
aroralanuk Sep 20, 2024
c6b5f76
Merge branch 'main' into kunal/verify-message-id
aroralanuk Sep 20, 2024
bef3470
add to current value
aroralanuk Sep 20, 2024
541be43
revert
aroralanuk Sep 20, 2024
4233c0a
magic
aroralanuk Sep 20, 2024
41a407d
5164
aroralanuk Sep 20, 2024
5610c33
changeset
aroralanuk Sep 20, 2024
ef780c6
check sufficient fees and return extra
aroralanuk Sep 23, 2024
a6731b0
changeset
aroralanuk Sep 23, 2024
47defbf
Merge branch 'main' into kunal/HL2408-002-fix
aroralanuk Sep 24, 2024
8510b6a
add childhook
aroralanuk Sep 24, 2024
a1a850e
add tests
aroralanuk Sep 24, 2024
7ba121d
inter
aroralanuk Sep 25, 2024
101fe72
Merge branch 'kunal/check-suff-refund-extra' into kunal/verify-messag…
aroralanuk Sep 25, 2024
cc69d2e
hook encode fix
aroralanuk Sep 25, 2024
a0aaf23
add msgvalue to quote
aroralanuk Sep 25, 2024
5d90cfe
Merge branch 'kunal/check-suff-refund-extra' into kunal/verify-messag…
aroralanuk Sep 25, 2024
43eba79
revert
aroralanuk Sep 25, 2024
b10361e
verify fixes
aroralanuk Sep 25, 2024
66bf788
Merge branch 'kunal/HL2408-002-fix' into kunal/verify-message-id
aroralanuk Sep 25, 2024
9851f10
test
aroralanuk Sep 26, 2024
750d09b
spelling
aroralanuk Sep 26, 2024
5991022
Merge branch 'main' into kunal/HL2408-002-fix
aroralanuk Sep 26, 2024
7028f14
Merge branch 'kunal/HL2408-002-fix' into kunal/verify-message-id
aroralanuk Sep 26, 2024
71d23f4
Merge branch 'kunal/HL2408-002-fix' into kunal/check-suff-refund-extra
aroralanuk Sep 26, 2024
8df9105
rm changeset
aroralanuk Sep 26, 2024
fd62639
Merge branch 'kunal/check-suff-refund-extra' into kunal/verify-messag…
aroralanuk Sep 26, 2024
c9e1fa7
spell
aroralanuk Sep 26, 2024
562fcd4
preverifyMessage
aroralanuk Oct 4, 2024
c604337
Merge branch 'main' into kunal/verify-message-id
aroralanuk Oct 31, 2024
89b001d
docs(changeset): Added msg.value to preverifyMessage to commit it as …
aroralanuk Oct 31, 2024
2150154
changeset
aroralanuk Oct 31, 2024
f4f638e
revert
aroralanuk Oct 31, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
revert
  • Loading branch information
aroralanuk committed Sep 25, 2024
commit 43eba79e8936629b91f5c79a0411e646daca0abb
5 changes: 0 additions & 5 deletions .changeset/metal-doors-float.md

This file was deleted.

2 changes: 1 addition & 1 deletion solidity/contracts/hooks/ArbL2ToL1Hook.sol
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@
// precompile contract on L2 for sending messages to L1
ArbSys public immutable arbSys;
// child hook to call first
AbstractPostDispatchHook public immutable childHook;
Fixed Show fixed Hide fixed

// ============ Constructor ============

Expand All @@ -52,7 +52,7 @@
uint32 _destinationDomain,
bytes32 _ism,
address _arbSys,
address _childHook
Dismissed Show dismissed Hide dismissed
) AbstractMessageIdAuthHook(_mailbox, _destinationDomain, _ism) {
arbSys = ArbSys(_arbSys);
childHook = AbstractPostDispatchHook(_childHook);
Expand Down Expand Up @@ -81,11 +81,11 @@
) internal override {
bytes memory payload = abi.encodeCall(
AbstractMessageIdAuthorizedIsm.verifyMessageId,
(message.id(), metadata.msgValue(0))
message.id()
);

childHook.postDispatch{
value: childHook.quoteDispatch(metadata, message)
Dismissed Show dismissed Hide dismissed
}(metadata, message);
arbSys.sendTxToL1{value: metadata.msgValue(0)}(
TypeCasts.bytes32ToAddress(ism),
Expand Down
2 changes: 1 addition & 1 deletion solidity/contracts/hooks/OPL2ToL1Hook.sol
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,9 @@
// precompile contract on L2 for sending messages to L1
ICrossDomainMessenger public immutable l2Messenger;
// child hook to call first
AbstractPostDispatchHook public immutable childHook;
Fixed Show fixed Hide fixed
// 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

// ============ Constructor ============

Expand All @@ -51,7 +51,7 @@
uint32 _destinationDomain,
bytes32 _ism,
address _l2Messenger,
address _childHook
Dismissed Show dismissed Hide dismissed
) AbstractMessageIdAuthHook(_mailbox, _destinationDomain, _ism) {
l2Messenger = ICrossDomainMessenger(_l2Messenger);
childHook = AbstractPostDispatchHook(_childHook);
Expand Down Expand Up @@ -80,11 +80,11 @@
) internal override {
bytes memory payload = abi.encodeCall(
AbstractMessageIdAuthorizedIsm.verifyMessageId,
(message.id(), metadata.msgValue(0))
message.id()
);

childHook.postDispatch{
value: childHook.quoteDispatch(metadata, message)
Dismissed Show dismissed Hide dismissed
}(metadata, message);
l2Messenger.sendMessage{value: metadata.msgValue(0)}(
TypeCasts.bytes32ToAddress(ism),
Expand Down
2 changes: 1 addition & 1 deletion solidity/contracts/hooks/OPStackHook.sol
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ contract OPStackHook is AbstractMessageIdAuthHook {
) internal override {
bytes memory payload = abi.encodeCall(
AbstractMessageIdAuthorizedIsm.verifyMessageId,
(message.id(), metadata.msgValue(0))
message.id()
);

l1Messenger.sendMessage{value: metadata.msgValue(0)}(
Expand Down
4 changes: 2 additions & 2 deletions solidity/contracts/hooks/PolygonPosHook.sol
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ contract PolygonPosHook is AbstractMessageIdAuthHook, FxBaseRootTunnel {

// ============ Internal functions ============
function _quoteDispatch(
bytes calldata,
bytes calldata metadata,
bytes calldata
) internal pure override returns (uint256) {
return metadata.msgValue(0);
Expand All @@ -77,7 +77,7 @@ contract PolygonPosHook is AbstractMessageIdAuthHook, FxBaseRootTunnel {

bytes memory payload = abi.encodeCall(
AbstractMessageIdAuthorizedIsm.verifyMessageId,
(message.id(), metadata.msgValue(0))
message.id()
);
_sendMessageToChild(payload);
}
Expand Down
2 changes: 1 addition & 1 deletion solidity/contracts/hooks/aggregation/ERC5164Hook.sol
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ contract ERC5164Hook is AbstractMessageIdAuthHook {

bytes memory payload = abi.encodeCall(
AbstractMessageIdAuthorizedIsm.verifyMessageId,
(message.id(), 0)
message.id()
);
dispatcher.dispatchMessage(
destinationDomain,
Expand Down
2 changes: 1 addition & 1 deletion solidity/contracts/hooks/layer-zero/LayerZeroV2Hook.sol
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ contract LayerZeroV2Hook is AbstractMessageIdAuthHook {
) internal override {
bytes memory payload = abi.encodeCall(
AbstractMessageIdAuthorizedIsm.verifyMessageId,
(message.id(), metadata.msgValue(0))
message.id()
);

bytes calldata lZMetadata = metadata.getCustomMetadata();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ abstract contract AbstractMessageIdAuthorizedIsm is
// ============ Events ============

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

// ============ Initializer ============

Expand Down Expand Up @@ -114,21 +114,18 @@ abstract contract AbstractMessageIdAuthorizedIsm is
* @dev Only callable by the authorized hook.
* @param messageId Hyperlane Id of the message.
*/
function verifyMessageId(
bytes32 messageId,
uint256 msgValue
) public payable virtual {
function verifyMessageId(bytes32 messageId) public payable virtual {
require(
_isAuthorized(),
"AbstractMessageIdAuthorizedIsm: sender is not the hook"
);
require(
msg.value < 2 ** VERIFIED_MASK_INDEX && msg.value == msgValue,
msg.value < 2 ** VERIFIED_MASK_INDEX,
"AbstractMessageIdAuthorizedIsm: msg.value must be less than 2^255"
);

verifiedMessages[messageId] = msg.value.setBit(VERIFIED_MASK_INDEX);
emit ReceivedMessage(messageId, msgValue);
emit ReceivedMessage(messageId);
}

// ============ Internal Functions ============
Expand Down
2 changes: 0 additions & 2 deletions solidity/test/isms/ERC5164ISM.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -150,8 +150,6 @@ contract ERC5164IsmTest is ExternalBridgeTest {

function test_verify_valueAlreadyClaimed(uint256) public override {}

function test_verify_override_msgValue() public override {}

function testFuzz_postDispatch_refundsExtraValue(uint256) public override {}

/* ============ helper functions ============ */
Expand Down
10 changes: 0 additions & 10 deletions solidity/test/isms/ExternalBridgeTest.sol
Original file line number Diff line number Diff line change
Expand Up @@ -250,16 +250,6 @@ abstract contract ExternalBridgeTest is Test {
assertEq(address(testRecipient).balance, _msgValue);
}

function test_verify_override_msgValue() public virtual {
bytes memory encodedHookData = _encodeHookData(messageId);

_externalBridgeDestinationCall(encodedHookData, MSG_VALUE);
_externalBridgeDestinationCall(encodedHookData, 0);

assertTrue(ism.verify(new bytes(0), encodedMessage));
assertEq(address(testRecipient).balance, MSG_VALUE);
}

/* ============ helper functions ============ */

function _encodeTestMessage() internal view returns (bytes memory) {
Expand Down
Loading