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

Remediate Trevor's comments #2835

Merged
merged 6 commits into from
Oct 24, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
8 changes: 8 additions & 0 deletions solidity/contracts/hooks/MerkleTreeHook.sol
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import {MerkleLib} from "../libs/Merkle.sol";
import {Message} from "../libs/Message.sol";
import {MailboxClient} from "../client/MailboxClient.sol";
import {Indexed} from "../libs/Indexed.sol";
import {IPostDispatchHook} from "../interfaces/hooks/IPostDispatchHook.sol";
import {AbstractPostDispatchHook} from "./libs/AbstractPostDispatchHook.sol";
import {StandardHookMetadata} from "./libs/StandardHookMetadata.sol";

Expand Down Expand Up @@ -49,6 +50,13 @@ contract MerkleTreeHook is AbstractPostDispatchHook, MailboxClient, Indexed {
return (root(), count() - 1);
}

// ============ External Functions ============

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

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

/// @inheritdoc AbstractPostDispatchHook
Expand Down
9 changes: 3 additions & 6 deletions solidity/contracts/hooks/OPStackHook.sol
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ contract OPStackHook is AbstractMessageIdAuthHook {
constructor(
address _mailbox,
uint32 _destinationDomain,
address _ism,
bytes32 _ism,
address _l1Messenger
) AbstractMessageIdAuthHook(_mailbox, _destinationDomain, _ism) {
require(
Expand All @@ -58,8 +58,7 @@ contract OPStackHook is AbstractMessageIdAuthHook {
l1Messenger = ICrossDomainMessenger(_l1Messenger);
}

// ============ External functions ============

// ============ Internal functions ============
function _quoteDispatch(bytes calldata, bytes calldata)
internal
pure
Expand All @@ -69,8 +68,6 @@ contract OPStackHook is AbstractMessageIdAuthHook {
return 0; // gas subsidized by the L2
}

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

/// @inheritdoc AbstractMessageIdAuthHook
function _sendMessageId(bytes calldata metadata, bytes memory payload)
internal
Expand All @@ -81,7 +78,7 @@ contract OPStackHook is AbstractMessageIdAuthHook {
"OPStackHook: msgValue must be less than 2 ** 255"
);
l1Messenger.sendMessage{value: metadata.msgValue(0)}(
ism,
TypeCasts.bytes32ToAddress(ism),
payload,
GAS_LIMIT
);
Expand Down
8 changes: 8 additions & 0 deletions solidity/contracts/hooks/PausableHook.sol
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ pragma solidity >=0.8.0;
@@@@@@@@@ @@@@@@@@*/

import {StandardHookMetadata} from "../hooks/libs/StandardHookMetadata.sol";
import {IPostDispatchHook} from "../interfaces/hooks/IPostDispatchHook.sol";
import {AbstractPostDispatchHook} from "./libs/AbstractPostDispatchHook.sol";

import {Ownable} from "@openzeppelin/contracts/access/Ownable.sol";
Expand All @@ -32,6 +33,13 @@ contract PausableHook is AbstractPostDispatchHook, Ownable, Pausable {
_unpause();
}

// ============ External Functions ============

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

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

/// @inheritdoc AbstractPostDispatchHook
Expand Down
6 changes: 6 additions & 0 deletions solidity/contracts/hooks/StaticProtocolFee.sol
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ pragma solidity >=0.8.0;
import {Message} from "../libs/Message.sol";
import {StandardHookMetadata} from "./libs/StandardHookMetadata.sol";
import {AbstractPostDispatchHook} from "./libs/AbstractPostDispatchHook.sol";
import {IPostDispatchHook} from "../interfaces/hooks/IPostDispatchHook.sol";

// ============ External Imports ============
import {Address} from "@openzeppelin/contracts/utils/Address.sol";
Expand Down Expand Up @@ -59,6 +60,11 @@ contract StaticProtocolFee is AbstractPostDispatchHook, Ownable {

// ============ External Functions ============

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

/**
* @notice Sets the protocol fee.
* @param _protocolFee The new protocol fee.
Expand Down
12 changes: 9 additions & 3 deletions solidity/contracts/hooks/aggregation/ERC5164Hook.sol
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ contract ERC5164Hook is AbstractMessageIdAuthHook {
constructor(
address _mailbox,
uint32 _destinationDomain,
address _ism,
bytes32 _ism,
address _dispatcher
) AbstractMessageIdAuthHook(_mailbox, _destinationDomain, _ism) {
require(
Expand All @@ -43,13 +43,15 @@ contract ERC5164Hook is AbstractMessageIdAuthHook {
dispatcher = IMessageDispatcher(_dispatcher);
}

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

function _quoteDispatch(bytes calldata, bytes calldata)
internal
pure
override
returns (uint256)
{
revert("not implemented");
return 0; // EIP-5164 doesn't enforce a gas abstraction
}

function _sendMessageId(
Expand All @@ -58,6 +60,10 @@ contract ERC5164Hook is AbstractMessageIdAuthHook {
bytes memory payload
) internal override {
require(msg.value == 0, "ERC5164Hook: no value allowed");
dispatcher.dispatchMessage(destinationDomain, ism, payload);
dispatcher.dispatchMessage(
destinationDomain,
TypeCasts.bytes32ToAddress(ism),
payload
);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,11 @@ contract StaticAggregationHook is AbstractPostDispatchHook {

// ============ External functions ============

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

/// @inheritdoc AbstractPostDispatchHook
function _postDispatch(bytes calldata metadata, bytes calldata message)
internal
Expand Down
5 changes: 5 additions & 0 deletions solidity/contracts/hooks/igp/InterchainGasPaymaster.sol
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,11 @@ contract InterchainGasPaymaster is

// ============ External Functions ============

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

/**
* @param _owner The owner of the contract.
* @param _beneficiary The beneficiary.
Expand Down
14 changes: 10 additions & 4 deletions solidity/contracts/hooks/libs/AbstractMessageIdAuthHook.sol
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ pragma solidity >=0.8.0;
@@@@@@@@@ @@@@@@@@*/

// ============ Internal Imports ============
import {IPostDispatchHook} from "../../interfaces/hooks/IPostDispatchHook.sol";
import {AbstractPostDispatchHook} from "./AbstractPostDispatchHook.sol";
import {AbstractMessageIdAuthorizedIsm} from "../../isms/hook/AbstractMessageIdAuthorizedIsm.sol";
import {TypeCasts} from "../../libs/TypeCasts.sol";
Expand All @@ -35,8 +36,8 @@ abstract contract AbstractMessageIdAuthHook is

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

// address for ISM to verify messages
address public immutable ism;
// left-padded address for ISM to verify messages
bytes32 public immutable ism;
// Domain of chain on which the ISM is deployed
uint32 public immutable destinationDomain;

Expand All @@ -45,9 +46,9 @@ abstract contract AbstractMessageIdAuthHook is
constructor(
address _mailbox,
uint32 _destinationDomain,
address _ism
bytes32 _ism
) MailboxClient(_mailbox) {
require(_ism != address(0), "AbstractMessageIdAuthHook: invalid ISM");
require(_ism != bytes32(0), "AbstractMessageIdAuthHook: invalid ISM");
require(
_destinationDomain != 0,
"AbstractMessageIdAuthHook: invalid destination domain"
Expand All @@ -56,6 +57,11 @@ abstract contract AbstractMessageIdAuthHook is
destinationDomain = _destinationDomain;
}

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

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

/// @inheritdoc AbstractPostDispatchHook
Expand Down
11 changes: 9 additions & 2 deletions solidity/contracts/hooks/routing/DomainRoutingHook.sol
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,15 @@ contract DomainRoutingHook is AbstractPostDispatchHook, MailboxClient {
_transferOwnership(_owner);
}

function setHook(uint32 destination, address hook) public onlyOwner {
hooks[destination] = IPostDispatchHook(hook);
// ============ External Functions ============

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

function setHook(uint32 _destination, address _hook) public onlyOwner {
hooks[_destination] = IPostDispatchHook(_hook);
}

function setHooks(HookConfig[] calldata configs) external onlyOwner {
Expand Down
16 changes: 12 additions & 4 deletions solidity/contracts/hooks/routing/FallbackDomainRoutingHook.sol
Original file line number Diff line number Diff line change
Expand Up @@ -36,17 +36,25 @@ contract FallbackDomainRoutingHook is DomainRoutingHook {
fallbackHook = IPostDispatchHook(_fallback);
}

// ============ External Functions ============

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

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

function _getConfiguredHook(bytes calldata message)
internal
view
override
returns (IPostDispatchHook hook)
returns (IPostDispatchHook)
{
hook = hooks[message.destination()];
if (address(hook) == address(0)) {
hook = fallbackHook;
IPostDispatchHook _hook = hooks[message.destination()];
if (address(_hook) == address(0)) {
_hook = fallbackHook;
}
return _hook;
}
}
17 changes: 17 additions & 0 deletions solidity/contracts/interfaces/hooks/IPostDispatchHook.sol
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,23 @@ pragma solidity >=0.8.0;
@@@@@@@@@ @@@@@@@@*/

interface IPostDispatchHook {
enum Types {
UNUSED,
ROUTING,
AGGREGATION,
MERKLE_TREE,
INTERCHAIN_GAS_PAYMASTER,
FALLBACK_ROUTING,
ID_AUTH_ISM,
PAUSABLE,
PROTOCOL_FEE
}

/**
* @notice Returns an enum that represents the type of hook
*/
function hookType() external view returns (uint8);

/**
* @notice Returns whether the hook supports metadata
* @param metadata metadata
Expand Down
29 changes: 19 additions & 10 deletions solidity/contracts/isms/hook/AbstractMessageIdAuthorizedIsm.sol
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ pragma solidity >=0.8.0;
import {IInterchainSecurityModule} from "../../interfaces/IInterchainSecurityModule.sol";
import {LibBit} from "../../libs/LibBit.sol";
import {Message} from "../../libs/Message.sol";
import {TypeCasts} from "../../libs/TypeCasts.sol";

// ============ External Imports ============

Expand Down Expand Up @@ -46,9 +45,9 @@ abstract contract AbstractMessageIdAuthorizedIsm is
/// @dev the first bit is reserved for verification and the rest 255 bits are for the msg.value
mapping(bytes32 => uint256) public verifiedMessages;
/// @notice Index of verification bit in verifiedMessages
uint256 public constant MASK_INDEX = 255;
/// @notice Address for the authorized hook
address public authorizedHook;
uint256 public constant VERIFIED_MASK_INDEX = 255;
/// @notice address for the authorized hook
bytes32 public authorizedHook;

// ============ Events ============

Expand All @@ -57,9 +56,9 @@ abstract contract AbstractMessageIdAuthorizedIsm is

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

function setAuthorizedHook(address _hook) external initializer {
function setAuthorizedHook(bytes32 _hook) external initializer {
require(
_hook != address(0),
_hook != bytes32(0),
"AbstractMessageIdAuthorizedIsm: invalid authorized hook"
);
authorizedHook = _hook;
Expand All @@ -79,12 +78,18 @@ abstract contract AbstractMessageIdAuthorizedIsm is
bytes32 messageId = message.id();

// check for the first bit (used for verification)
bool verified = verifiedMessages[messageId].isBitSet(MASK_INDEX);
bool verified = verifiedMessages[messageId].isBitSet(
VERIFIED_MASK_INDEX
);
// rest 255 bits contains the msg.value passed from the hook
if (verified) {
payable(message.recipientAddress()).sendValue(
verifiedMessages[messageId].clearBit(MASK_INDEX)
uint256 _msgValue = verifiedMessages[messageId].clearBit(
VERIFIED_MASK_INDEX
);
yorhodes marked this conversation as resolved.
Show resolved Hide resolved
if (_msgValue > 0) {
verifiedMessages[messageId] -= _msgValue;
payable(message.recipientAddress()).sendValue(_msgValue);
}
}
return verified;
}
Expand All @@ -99,8 +104,12 @@ abstract contract AbstractMessageIdAuthorizedIsm is
_isAuthorized(),
"AbstractMessageIdAuthorizedIsm: sender is not the hook"
);
require(
msg.value < 2**VERIFIED_MASK_INDEX,
"AbstractMessageIdAuthorizedIsm: msg.value must be less than 2^255"
);

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

Expand Down
3 changes: 2 additions & 1 deletion solidity/contracts/isms/hook/OPStackIsm.sol
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ contract OPStackIsm is
* @notice Check if sender is authorized to message `verifyMessageId`.
*/
function _isAuthorized() internal view override returns (bool) {
return _crossChainSender() == authorizedHook;
return
_crossChainSender() == TypeCasts.bytes32ToAddress(authorizedHook);
yorhodes marked this conversation as resolved.
Show resolved Hide resolved
}
}
8 changes: 8 additions & 0 deletions solidity/contracts/test/TestPostDispatchHook.sol
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// SPDX-License-Identifier: MIT OR Apache-2.0
pragma solidity >=0.8.0;

import {IPostDispatchHook} from "../interfaces/hooks/IPostDispatchHook.sol";
import {AbstractPostDispatchHook} from "../hooks/libs/AbstractPostDispatchHook.sol";

contract TestPostDispatchHook is AbstractPostDispatchHook {
Expand All @@ -9,6 +10,13 @@ contract TestPostDispatchHook is AbstractPostDispatchHook {
// test fees for quoteDispatch
uint256 public fee = 0;

// ============ External Functions ============

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

function supportsMetadata(bytes calldata)
public
pure
Expand Down
5 changes: 5 additions & 0 deletions solidity/test/MerkleTreeHook.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import {Message} from "../contracts/libs/Message.sol";
import {TypeCasts} from "../contracts/libs/TypeCasts.sol";
import {TestMerkleTreeHook} from "../contracts/test/TestMerkleTreeHook.sol";
import {TestMailbox} from "../contracts/test/TestMailbox.sol";
import {IPostDispatchHook} from "../contracts/interfaces/hooks/IPostDispatchHook.sol";

contract MerkleTreeHookTest is Test {
using Message for bytes;
Expand Down Expand Up @@ -68,4 +69,8 @@ contract MerkleTreeHookTest is Test {
assertEq(hook.quoteDispatch("", currMessage), 0);
}
}

function testHookType() public {
assertEq(hook.hookType(), uint8(IPostDispatchHook.Types.MERKLE_TREE));
}
}
Loading