Skip to content

Commit

Permalink
preverifyMessage
Browse files Browse the repository at this point in the history
  • Loading branch information
aroralanuk committed Oct 4, 2024
1 parent c9e1fa7 commit 562fcd4
Show file tree
Hide file tree
Showing 18 changed files with 57 additions and 58 deletions.
2 changes: 1 addition & 1 deletion solidity/contracts/hooks/ArbL2ToL1Hook.sol
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ contract ArbL2ToL1Hook is AbstractMessageIdAuthHook {
bytes calldata message
) internal override {
bytes memory payload = abi.encodeCall(
AbstractMessageIdAuthorizedIsm.verifyMessageId,
AbstractMessageIdAuthorizedIsm.preVerifyMessage,
(message.id(), metadata.msgValue(0))
);

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 @@ -79,7 +79,7 @@ contract OPL2ToL1Hook is AbstractMessageIdAuthHook {
bytes calldata message
) internal override {
bytes memory payload = abi.encodeCall(
AbstractMessageIdAuthorizedIsm.verifyMessageId,
AbstractMessageIdAuthorizedIsm.preVerifyMessage,
(message.id(), metadata.msgValue(0))
);

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 @@ -74,7 +74,7 @@ contract OPStackHook is AbstractMessageIdAuthHook {
bytes calldata message
) internal override {
bytes memory payload = abi.encodeCall(
AbstractMessageIdAuthorizedIsm.verifyMessageId,
AbstractMessageIdAuthorizedIsm.preVerifyMessage,
(message.id(), metadata.msgValue(0))
);

Expand Down
2 changes: 1 addition & 1 deletion solidity/contracts/hooks/PolygonPosHook.sol
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ contract PolygonPosHook is AbstractMessageIdAuthHook, FxBaseRootTunnel {
require(msg.value == 0, "PolygonPosHook: does not support msgValue");

bytes memory payload = abi.encodeCall(
AbstractMessageIdAuthorizedIsm.verifyMessageId,
AbstractMessageIdAuthorizedIsm.preVerifyMessage,
(message.id(), metadata.msgValue(0))
);
_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 {
require(msg.value == 0, "ERC5164Hook: no value allowed");

bytes memory payload = abi.encodeCall(
AbstractMessageIdAuthorizedIsm.verifyMessageId,
AbstractMessageIdAuthorizedIsm.preVerifyMessage,
(message.id(), metadata.msgValue(0))
);
dispatcher.dispatchMessage(
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 @@ -59,7 +59,7 @@ contract LayerZeroV2Hook is AbstractMessageIdAuthHook {
bytes calldata message
) internal override {
bytes memory payload = abi.encodeCall(
AbstractMessageIdAuthorizedIsm.verifyMessageId,
AbstractMessageIdAuthorizedIsm.preVerifyMessage,
(message.id(), metadata.msgValue(0))
);

Expand Down
18 changes: 7 additions & 11 deletions solidity/contracts/isms/hook/AbstractMessageIdAuthorizedIsm.sol
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ abstract contract AbstractMessageIdAuthorizedIsm is
}

/**
* @notice Check if a message is verified through verifyMessageId first.
* @notice Check if a message is verified through preVerifyMessage first.
* @param message Message to check.
*/
function isVerified(bytes calldata message) public view returns (bool) {
Expand All @@ -114,7 +114,7 @@ abstract contract AbstractMessageIdAuthorizedIsm is
* @dev Only callable by the authorized hook.
* @param messageId Hyperlane Id of the message.
*/
function verifyMessageId(
function preVerifyMessage(
bytes32 messageId,
uint256 msgValue
) public payable virtual {
Expand All @@ -123,26 +123,22 @@ abstract contract AbstractMessageIdAuthorizedIsm is
"AbstractMessageIdAuthorizedIsm: sender is not the hook"
);
require(
msg.value < 2 ** VERIFIED_MASK_INDEX,
"AbstractMessageIdAuthorizedIsm: msg.value must be less than 2^255"
msg.value < 2 ** VERIFIED_MASK_INDEX && msg.value == msgValue,
"AbstractMessageIdAuthorizedIsm: invalid msg.value"
);
require(
msg.value == msgValue,
"AbstractMessageIdAuthorizedIsm: msg.value doesn't match"
verifiedMessages[messageId] == 0,
"AbstractMessageIdAuthorizedIsm: message already verified"
);

if (verifiedMessages[messageId] != 0) {
return;
}

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

// ============ Internal Functions ============

/**
* @notice Check if sender is authorized to message `verifyMessageId`.
* @notice Check if sender is authorized to message `preVerifyMessage`.
*/
function _isAuthorized() internal view virtual returns (bool);
}
13 changes: 10 additions & 3 deletions solidity/contracts/isms/hook/ArbL2ToL1Ism.sol
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,10 @@ contract ArbL2ToL1Ism is
// arbitrum nitro contract on L1 to forward verification
IOutbox public arbOutbox;

uint256 private constant DATA_LENGTH = 68;

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

uint256 private constant MESSAGE_ID_END = 36;

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 ============

constructor(address _bridge) CrossChainEnabledArbitrumL1(_bridge) {
Expand Down Expand Up @@ -110,13 +114,16 @@ contract ArbL2ToL1Ism is
l2Sender == TypeCasts.bytes32ToAddress(authorizedHook),
"ArbL2ToL1Ism: l2Sender != authorizedHook"
);
// this data is an abi encoded call of verifyMessageId(bytes32 messageId)
require(data.length == 68, "ArbL2ToL1Ism: invalid data length");
// this data is an abi encoded call of preVerifyMessage(bytes32 messageId)
require(
data.length == DATA_LENGTH,
"ArbL2ToL1Ism: invalid data length"
);
bytes32 messageId = message.id();
bytes32 convertedBytes;
assembly {
// data = 0x[4 bytes function signature][32 bytes messageId]
convertedBytes := mload(add(data, 36))
convertedBytes := mload(add(data, MESSAGE_ID_END))
}
// check if the parsed message id matches the message id of the message
require(
Expand Down
2 changes: 1 addition & 1 deletion solidity/contracts/isms/hook/ERC5164Ism.sol
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ contract ERC5164Ism is AbstractMessageIdAuthorizedIsm {
}

/**
* @notice Check if sender is authorized to message `verifyMessageId`.
* @notice Check if sender is authorized to message `preVerifyMessage`.
*/
function _isAuthorized() internal view override returns (bool) {
return msg.sender == executor;
Expand Down
2 changes: 1 addition & 1 deletion solidity/contracts/isms/hook/OPStackIsm.sol
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ contract OPStackIsm is
// ============ Internal function ============

/**
* @notice Check if sender is authorized to message `verifyMessageId`.
* @notice Check if sender is authorized to message `preVerifyMessage`.
*/
function _isAuthorized() internal view override returns (bool) {
return
Expand Down
2 changes: 1 addition & 1 deletion solidity/contracts/isms/hook/PolygonPosIsm.sol
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ contract PolygonPosIsm is
// ============ Internal function ============

/**
* @notice Check if sender is authorized to message `verifyMessageId`.
* @notice Check if sender is authorized to message `preVerifyMessage`.
*/
function _isAuthorized() internal view override returns (bool) {
return
Expand Down
10 changes: 5 additions & 5 deletions solidity/contracts/isms/hook/layer-zero/LayerZeroV2Ism.sol
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ contract LayerZeroV2Ism is AbstractMessageIdAuthorizedIsm {
/**
* @notice Entry point for receiving msg/packet from the LayerZero endpoint.
* @param _lzMessage The payload of the received message.
* @dev Authorization verification is done within verifyMessageId() -> _isAuthorized()
* @dev Authorization verification is done within preVerifyMessage() -> _isAuthorized()
*/
function lzReceive(
Origin calldata,
Expand All @@ -66,14 +66,14 @@ contract LayerZeroV2Ism is AbstractMessageIdAuthorizedIsm {
address,
bytes calldata
) external payable {
verifyMessageId(_messageId(_lzMessage), _msgValue(_lzMessage));
preVerifyMessage(_messageId(_lzMessage), _msgValue(_lzMessage));
}

// ============ Internal function ============

/**
* @notice Slices the messageId from the message delivered from LayerZeroV2Hook
* @dev message is created as abi.encodeCall(AbstractMessageIdAuthorizedIsm.verifyMessageId, id)
* @dev message is created as abi.encodeCall(AbstractMessageIdAuthorizedIsm.preVerifyMessage, id)
* @dev _message will be 36 bytes (4 bytes for function selector, and 32 bytes for messageId)
*/
function _messageId(
Expand All @@ -84,7 +84,7 @@ contract LayerZeroV2Ism is AbstractMessageIdAuthorizedIsm {

/**
* @notice Slices the msgValue from the message delivered from LayerZeroV2Hook
* @dev message is created as abi.encodeCall(AbstractMessageIdAuthorizedIsm.verifyMessageId, (id,msgValue))
* @dev message is created as abi.encodeCall(AbstractMessageIdAuthorizedIsm.preVerifyMessage, (id,msgValue))
* @dev _message will be 68 bytes (4 bytes for function selector, and 32 bytes for messageId, another 32 for msgValue)
*/
function _msgValue(
Expand All @@ -95,7 +95,7 @@ contract LayerZeroV2Ism is AbstractMessageIdAuthorizedIsm {

/**
* @notice Validates criteria to verify a message
* @dev this is called by AbstractMessageIdAuthorizedIsm.verifyMessageId
* @dev this is called by AbstractMessageIdAuthorizedIsm.preVerifyMessage
* @dev parses msg.value to get parameters from lzReceive()
*/
function _isAuthorized() internal view override returns (bool) {
Expand Down
4 changes: 2 additions & 2 deletions solidity/contracts/libs/OPL2ToL1Metadata.sol
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ library OPL2ToL1Metadata {
// LENGTH = 32 bytes
// } = 252 bytes
uint256 private constant FIXED_METADATA_LENGTH = 252;
// metadata here is double encoded call relayMessage(..., verifyMessageId)
// metadata here is double encoded call relayMessage(..., preVerifyMessage)
// Σ {
// _selector = 4 bytes
// _nonce = 32 bytes
Expand All @@ -31,7 +31,7 @@ library OPL2ToL1Metadata {
// _data
// OFFSET = 32 bytes
// LENGTH = 32 bytes
// PADDING + verifyMessageId = 96 bytes
// PADDING + preVerifyMessage = 96 bytes
// } = 324 bytes
uint256 private constant MESSENGER_CALLDATA_LENGTH = 324;

Expand Down
6 changes: 3 additions & 3 deletions solidity/test/isms/ERC5164ISM.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ contract ERC5164IsmTest is ExternalBridgeTest {
vm.expectRevert(
"AbstractMessageIdAuthorizedIsm: sender is not the hook"
);
ism.verifyMessageId(messageId, 0);
ism.preVerifyMessage(messageId, 0);
assertFalse(ism.isVerified(encodedMessage));
}

Expand Down Expand Up @@ -163,7 +163,7 @@ contract ERC5164IsmTest is ExternalBridgeTest {
uint256 _msgValue
) internal override {
vm.prank(address(executor));
ism.verifyMessageId(messageId, 0);
ism.preVerifyMessage(messageId, 0);
}

function _encodeExternalDestinationBridgeCall(
Expand All @@ -174,7 +174,7 @@ contract ERC5164IsmTest is ExternalBridgeTest {
) internal override returns (bytes memory) {
if (_from == address(hook)) {
vm.prank(address(executor));
ism.verifyMessageId{value: _msgValue}(messageId, 0);
ism.preVerifyMessage{value: _msgValue}(messageId, 0);
}
}
}
14 changes: 6 additions & 8 deletions solidity/test/isms/ExternalBridgeTest.sol
Original file line number Diff line number Diff line change
Expand Up @@ -123,16 +123,16 @@ abstract contract ExternalBridgeTest is Test {
hook.postDispatch{value: quote - 1}(testMetadata, encodedMessage);
}

/* ============ ISM.verifyMessageId ============ */
/* ============ ISM.preVerifyMessage ============ */

function test_verifyMessageId_asyncCall() public {
function test_preVerifyMessage_asyncCall() public {
bytes memory encodedHookData = _encodeHookData(messageId, 0);
_externalBridgeDestinationCall(encodedHookData, 0);

assertTrue(ism.isVerified(encodedMessage));
}

function test_verifyMessageId_externalBridgeCall() public virtual {
function test_preVerifyMessage_externalBridgeCall() public virtual {
bytes memory externalCalldata = _encodeExternalDestinationBridgeCall(
address(hook),
address(ism),
Expand Down Expand Up @@ -256,9 +256,7 @@ abstract contract ExternalBridgeTest is Test {

_externalBridgeDestinationCall(encodedHookData, MSG_VALUE);

vm.expectRevert(
"AbstractMessageIdAuthorizedIsm: msg.value doesn't match"
);
vm.expectRevert("AbstractMessageIdAuthorizedIsm: invalid msg.value");
_externalBridgeDestinationCall(encodedHookData, 0);

assertTrue(ism.verify(new bytes(0), encodedMessage));
Expand Down Expand Up @@ -295,7 +293,7 @@ abstract contract ExternalBridgeTest is Test {
) internal pure returns (bytes memory) {
return
abi.encodeCall(
AbstractMessageIdAuthorizedIsm.verifyMessageId,
AbstractMessageIdAuthorizedIsm.preVerifyMessage,
(_messageId, _msgValue)
);
}
Expand Down Expand Up @@ -331,5 +329,5 @@ abstract contract ExternalBridgeTest is Test {
receive() external payable {}

// meant to be mock an arbitrary successful call made by the external bridge
function verifyMessageId(bytes32 /*messageId*/) public payable {}
function preVerifyMessage(bytes32 /*messageId*/) public payable {}
}
12 changes: 5 additions & 7 deletions solidity/test/isms/OPStackIsm.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -145,20 +145,20 @@ contract OPStackIsmTest is ExternalBridgeTest {
}

// SKIP - no external bridge call
function test_verifyMessageId_externalBridgeCall() public override {}
function test_preVerifyMessage_externalBridgeCall() public override {}

function test_verify_msgValue_externalBridgeCall() public override {}

function test_verify_revertsWhen_invalidIsm() public override {}

function test_verify_false_arbitraryCall() public override {}

/* ============ ISM.verifyMessageId ============ */
/* ============ ISM.preVerifyMessage ============ */

function test_verify_revertsWhen_notAuthorizedHook() public override {
// needs to be called by the canonical messenger on Optimism
vm.expectRevert(NotCrossChainCall.selector);
ism.verifyMessageId(messageId, 0);
ism.preVerifyMessage(messageId, 0);

vm.startPrank(L2_MESSENGER_ADDRESS);
_setExternalOriginSender(address(this));
Expand All @@ -167,7 +167,7 @@ contract OPStackIsmTest is ExternalBridgeTest {
vm.expectRevert(
"AbstractMessageIdAuthorizedIsm: sender is not the hook"
);
ism.verifyMessageId(messageId, 0);
ism.preVerifyMessage(messageId, 0);
}

function _setExternalOriginSender(
Expand All @@ -182,9 +182,7 @@ contract OPStackIsmTest is ExternalBridgeTest {
function test_verify_tooMuchValue() public {
uint256 _msgValue = 2 ** 255 + 1;

vm.expectRevert(
"AbstractMessageIdAuthorizedIsm: msg.value must be less than 2^255"
);
vm.expectRevert("AbstractMessageIdAuthorizedIsm: invalid msg.value");
_externalBridgeDestinationCall(
_encodeHookData(messageId, _msgValue),
_msgValue
Expand Down
Loading

0 comments on commit 562fcd4

Please sign in to comment.