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 35 commits
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
5 changes: 5 additions & 0 deletions .changeset/green-suits-dance.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@hyperlane-xyz/core': patch
---

Adding msg.value instead of updating in AuthorizedHookIsm
5 changes: 5 additions & 0 deletions .changeset/metal-doors-float.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@hyperlane-xyz/core': patch
---

Added check for msg.value > quote for AuthorizedHook
31 changes: 22 additions & 9 deletions solidity/contracts/hooks/ArbL2ToL1Hook.sol
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,10 @@
@@@@@@@@@ @@@@@@@@*/

// ============ Internal Imports ============
import {Message} from "../libs/Message.sol";
import {AbstractPostDispatchHook} from "./libs/AbstractMessageIdAuthHook.sol";
import {AbstractMessageIdAuthHook} from "./libs/AbstractMessageIdAuthHook.sol";
import {Mailbox} from "../Mailbox.sol";
import {AbstractMessageIdAuthorizedIsm} from "../isms/hook/AbstractMessageIdAuthorizedIsm.sol";
import {StandardHookMetadata} from "./libs/StandardHookMetadata.sol";
import {Message} from "../libs/Message.sol";
import {TypeCasts} from "../libs/TypeCasts.sol";
Expand All @@ -35,13 +36,14 @@
*/
contract ArbL2ToL1Hook is AbstractMessageIdAuthHook {
using StandardHookMetadata for bytes;
using Message for bytes;

// ============ Constants ============

// precompile contract on L2 for sending messages to L1
ArbSys public immutable arbSys;
// Immutable quote amount
uint256 public immutable GAS_QUOTE;
// child hook to call first
AbstractPostDispatchHook public immutable childHook;
Fixed Show fixed Hide fixed

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

Expand All @@ -50,30 +52,41 @@
uint32 _destinationDomain,
bytes32 _ism,
address _arbSys,
uint256 _gasQuote
address _childHook
Dismissed Show dismissed Hide dismissed
) AbstractMessageIdAuthHook(_mailbox, _destinationDomain, _ism) {
arbSys = ArbSys(_arbSys);
GAS_QUOTE = _gasQuote;
childHook = AbstractPostDispatchHook(_childHook);
}

/// @inheritdoc IPostDispatchHook
function hookType() external pure override returns (uint8) {
return uint8(IPostDispatchHook.Types.ARB_L2_TO_L1);
}

/// @inheritdoc AbstractPostDispatchHook
function _quoteDispatch(
bytes calldata,
bytes calldata
bytes calldata metadata,
bytes calldata message
) internal view override returns (uint256) {
return GAS_QUOTE;
return
metadata.msgValue(0) + childHook.quoteDispatch(metadata, message);
}

// ============ Internal functions ============

/// @inheritdoc AbstractMessageIdAuthHook
function _sendMessageId(
bytes calldata metadata,
bytes memory payload
bytes calldata message
) internal override {
bytes memory payload = abi.encodeCall(
AbstractMessageIdAuthorizedIsm.verifyMessageId,
(message.id(), metadata.msgValue(0))
aroralanuk marked this conversation as resolved.
Show resolved Hide resolved
);

childHook.postDispatch{
value: childHook.quoteDispatch(metadata, message)
Dismissed Show dismissed Hide dismissed
}(metadata, message);
arbSys.sendTxToL1{value: metadata.msgValue(0)}(
TypeCasts.bytes32ToAddress(ism),
payload
Expand Down
35 changes: 23 additions & 12 deletions solidity/contracts/hooks/OPL2ToL1Hook.sol
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,13 @@
@@@@@@@@@ @@@@@@@@*/

// ============ Internal Imports ============
import {Message} from "../libs/Message.sol";
import {AbstractPostDispatchHook, AbstractMessageIdAuthHook} from "./libs/AbstractMessageIdAuthHook.sol";
import {AbstractMessageIdAuthorizedIsm} from "../isms/hook/AbstractMessageIdAuthorizedIsm.sol";
import {StandardHookMetadata} from "./libs/StandardHookMetadata.sol";
import {TypeCasts} from "../libs/TypeCasts.sol";
import {IPostDispatchHook} from "../interfaces/hooks/IPostDispatchHook.sol";
import {InterchainGasPaymaster} from "./igp/InterchainGasPaymaster.sol";

// ============ External Imports ============
import {ICrossDomainMessenger} from "../interfaces/optimism/ICrossDomainMessenger.sol";
Expand All @@ -30,13 +33,16 @@
*/
contract OPL2ToL1Hook is AbstractMessageIdAuthHook {
using StandardHookMetadata for bytes;
using Message for bytes;

// ============ Constants ============

// precompile contract on L2 for sending messages to L1
ICrossDomainMessenger public immutable l2Messenger;
// Immutable quote amount
uint32 public immutable GAS_QUOTE;
// 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 notice

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 Low

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 @@ -45,10 +51,10 @@
uint32 _destinationDomain,
bytes32 _ism,
address _l2Messenger,
uint32 _gasQuote
address _childHook
Dismissed Show dismissed Hide dismissed
) AbstractMessageIdAuthHook(_mailbox, _destinationDomain, _ism) {
GAS_QUOTE = _gasQuote;
l2Messenger = ICrossDomainMessenger(_l2Messenger);
childHook = AbstractPostDispatchHook(_childHook);
}

/// @inheritdoc IPostDispatchHook
Expand All @@ -58,27 +64,32 @@

/// @inheritdoc AbstractPostDispatchHook
function _quoteDispatch(
bytes calldata,
bytes calldata
bytes calldata metadata,
bytes calldata message
) internal view override returns (uint256) {
return GAS_QUOTE;
return
metadata.msgValue(0) + childHook.quoteDispatch(metadata, message);
}

// ============ Internal functions ============

/// @inheritdoc AbstractMessageIdAuthHook
function _sendMessageId(
bytes calldata metadata,
bytes memory payload
bytes calldata message
) internal override {
require(
msg.value >= metadata.msgValue(0) + GAS_QUOTE,
"OPL2ToL1Hook: insufficient msg.value"
bytes memory payload = abi.encodeCall(
AbstractMessageIdAuthorizedIsm.verifyMessageId,
(message.id(), metadata.msgValue(0))
);

childHook.postDispatch{
value: childHook.quoteDispatch(metadata, message)
Dismissed Show dismissed Hide dismissed
}(metadata, message);
l2Messenger.sendMessage{value: metadata.msgValue(0)}(
TypeCasts.bytes32ToAddress(ism),
payload,
GAS_QUOTE
MIN_GAS_LIMIT
);
}
}
15 changes: 9 additions & 6 deletions solidity/contracts/hooks/OPStackHook.sol
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import {AbstractMessageIdAuthHook} from "./libs/AbstractMessageIdAuthHook.sol";
import {StandardHookMetadata} from "./libs/StandardHookMetadata.sol";
import {TypeCasts} from "../libs/TypeCasts.sol";
import {Message} from "../libs/Message.sol";
import {AbstractMessageIdAuthorizedIsm} from "../isms/hook/AbstractMessageIdAuthorizedIsm.sol";
import {IPostDispatchHook} from "../interfaces/hooks/IPostDispatchHook.sol";

// ============ External Imports ============
Expand All @@ -32,6 +33,7 @@ import {Address} from "@openzeppelin/contracts/utils/Address.sol";
*/
contract OPStackHook is AbstractMessageIdAuthHook {
using StandardHookMetadata for bytes;
using Message for bytes;

// ============ Constants ============

Expand Down Expand Up @@ -60,21 +62,22 @@ contract OPStackHook is AbstractMessageIdAuthHook {

// ============ Internal functions ============
function _quoteDispatch(
bytes calldata,
bytes calldata metadata,
bytes calldata
) internal pure override returns (uint256) {
return 0; // gas subsidized by the L2
return metadata.msgValue(0); // gas subsidized by the L2
}

/// @inheritdoc AbstractMessageIdAuthHook
function _sendMessageId(
bytes calldata metadata,
bytes memory payload
bytes calldata message
) internal override {
require(
metadata.msgValue(0) < 2 ** 255,
"OPStackHook: msgValue must be less than 2 ** 255"
bytes memory payload = abi.encodeCall(
AbstractMessageIdAuthorizedIsm.verifyMessageId,
(message.id(), metadata.msgValue(0))
);

l1Messenger.sendMessage{value: metadata.msgValue(0)}(
TypeCasts.bytes32ToAddress(ism),
payload,
Expand Down
13 changes: 10 additions & 3 deletions solidity/contracts/hooks/PolygonPosHook.sol
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import {StandardHookMetadata} from "./libs/StandardHookMetadata.sol";
import {TypeCasts} from "../libs/TypeCasts.sol";
import {Message} from "../libs/Message.sol";
import {IPostDispatchHook} from "../interfaces/hooks/IPostDispatchHook.sol";
import {AbstractMessageIdAuthorizedIsm} from "../isms/hook/AbstractMessageIdAuthorizedIsm.sol";

// ============ External Imports ============
import {FxBaseRootTunnel} from "fx-portal/contracts/tunnel/FxBaseRootTunnel.sol";
Expand All @@ -31,6 +32,7 @@ import {Address} from "@openzeppelin/contracts/utils/Address.sol";
*/
contract PolygonPosHook is AbstractMessageIdAuthHook, FxBaseRootTunnel {
using StandardHookMetadata for bytes;
using Message for bytes;

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

Expand All @@ -56,22 +58,27 @@ contract PolygonPosHook is AbstractMessageIdAuthHook, FxBaseRootTunnel {

// ============ Internal functions ============
function _quoteDispatch(
bytes calldata,
bytes calldata metadata,
bytes calldata
) internal pure override returns (uint256) {
return 0;
return metadata.msgValue(0);
}

/// @inheritdoc AbstractMessageIdAuthHook
function _sendMessageId(
bytes calldata metadata,
bytes memory payload
bytes calldata message
) internal override {
require(
metadata.msgValue(0) == 0,
"PolygonPosHook: does not support msgValue"
);
require(msg.value == 0, "PolygonPosHook: does not support msgValue");

bytes memory payload = abi.encodeCall(
AbstractMessageIdAuthorizedIsm.verifyMessageId,
(message.id(), metadata.msgValue(0))
);
_sendMessageToChild(payload);
}

Expand Down
11 changes: 10 additions & 1 deletion solidity/contracts/hooks/aggregation/ERC5164Hook.sol
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,11 @@ pragma solidity >=0.8.0;

// ============ Internal Imports ============
import {TypeCasts} from "../../libs/TypeCasts.sol";
import {Message} from "../../libs/Message.sol";
import {IPostDispatchHook} from "../../interfaces/hooks/IPostDispatchHook.sol";
import {IMessageDispatcher} from "../../interfaces/hooks/IMessageDispatcher.sol";
import {AbstractMessageIdAuthHook} from "../libs/AbstractMessageIdAuthHook.sol";
import {AbstractMessageIdAuthorizedIsm} from "../../isms/hook/AbstractMessageIdAuthorizedIsm.sol";

// ============ External Imports ============
import {Address} from "@openzeppelin/contracts/utils/Address.sol";
Expand All @@ -28,6 +30,8 @@ import {Address} from "@openzeppelin/contracts/utils/Address.sol";
* any of the 5164 adapters.
*/
contract ERC5164Hook is AbstractMessageIdAuthHook {
using Message for bytes;

IMessageDispatcher public immutable dispatcher;

constructor(
Expand Down Expand Up @@ -55,9 +59,14 @@ contract ERC5164Hook is AbstractMessageIdAuthHook {
function _sendMessageId(
bytes calldata,
/* metadata */
bytes memory payload
bytes calldata message
) internal override {
require(msg.value == 0, "ERC5164Hook: no value allowed");

bytes memory payload = abi.encodeCall(
AbstractMessageIdAuthorizedIsm.verifyMessageId,
(message.id(), 0)
);
dispatcher.dispatchMessage(
destinationDomain,
TypeCasts.bytes32ToAddress(ism),
Expand Down
10 changes: 8 additions & 2 deletions solidity/contracts/hooks/layer-zero/LayerZeroV2Hook.sol
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import {TypeCasts} from "../../libs/TypeCasts.sol";
import {Indexed} from "../../libs/Indexed.sol";
import {IPostDispatchHook} from "../../interfaces/hooks/IPostDispatchHook.sol";
import {AbstractMessageIdAuthHook} from "../libs/AbstractMessageIdAuthHook.sol";
import {AbstractMessageIdAuthorizedIsm} from "../../isms/hook/AbstractMessageIdAuthorizedIsm.sol";
import {StandardHookMetadata} from "../libs/StandardHookMetadata.sol";

struct LayerZeroV2Metadata {
Expand Down Expand Up @@ -55,8 +56,13 @@ contract LayerZeroV2Hook is AbstractMessageIdAuthHook {
/// @inheritdoc AbstractMessageIdAuthHook
function _sendMessageId(
bytes calldata metadata,
bytes memory payload
bytes calldata message
) internal override {
bytes memory payload = abi.encodeCall(
AbstractMessageIdAuthorizedIsm.verifyMessageId,
(message.id(), metadata.msgValue(0))
);

bytes calldata lZMetadata = metadata.getCustomMetadata();
(
uint32 eid,
Expand Down Expand Up @@ -96,7 +102,7 @@ contract LayerZeroV2Hook is AbstractMessageIdAuthHook {
message.senderAddress()
);

return msgFee.nativeFee;
return metadata.msgValue(0) + msgFee.nativeFee;
}

/**
Expand Down
27 changes: 19 additions & 8 deletions solidity/contracts/hooks/libs/AbstractMessageIdAuthHook.sol
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,9 @@ import {Message} from "../../libs/Message.sol";
import {StandardHookMetadata} from "./StandardHookMetadata.sol";
import {MailboxClient} from "../../client/MailboxClient.sol";

// ============ External Imports ============
import {Address} from "@openzeppelin/contracts/utils/Address.sol";

/**
* @title AbstractMessageIdAuthHook
* @notice Message hook to inform an Abstract Message ID ISM of messages published through
Expand All @@ -31,6 +34,7 @@ abstract contract AbstractMessageIdAuthHook is
AbstractPostDispatchHook,
MailboxClient
{
using Address for address payable;
using StandardHookMetadata for bytes;
using Message for bytes;

Expand Down Expand Up @@ -68,7 +72,7 @@ abstract contract AbstractMessageIdAuthHook is
function _postDispatch(
bytes calldata metadata,
bytes calldata message
) internal override {
) internal virtual override {
bytes32 id = message.id();
require(
_isLatestDispatched(id),
Expand All @@ -82,20 +86,27 @@ abstract contract AbstractMessageIdAuthHook is
metadata.msgValue(0) < 2 ** 255,
"AbstractMessageIdAuthHook: msgValue must be less than 2 ** 255"
);
bytes memory payload = abi.encodeCall(
AbstractMessageIdAuthorizedIsm.verifyMessageId,
id
);
_sendMessageId(metadata, payload);

_sendMessageId(metadata, message);

uint256 _overpayment = msg.value - _quoteDispatch(metadata, message);
if (_overpayment > 0) {
address _refundAddress = metadata.refundAddress(msg.sender);
require(
_refundAddress != address(0),
"AbstractPostDispatchHook: no refund address"
);
payable(_refundAddress).sendValue(_overpayment);
}
}

/**
* @notice Send a message to the ISM.
* @param metadata The metadata for the hook caller
* @param payload The payload for call to the ISM
* @param message The message to send to the ISM
*/
function _sendMessageId(
bytes calldata metadata,
bytes memory payload
bytes calldata message
) internal virtual;
}
Loading
Loading