Store hash of execution data (#103)

* Store hash of execution data

Reduces storage gas cost, but requires caller to pass in full execution
data.

Fixes #35.

* Rename and refactor to address comments

* Run forge fmt

* Nit fixes to address comment

src/MultiBridgeMessageReceiver.sol

@@ -21,6 +21,9 @@ import "./libraries/Message.sol";
 /// @dev The contract only accepts messages from trusted bridge receiver adapters, each of which implements the
 /// IMessageReceiverAdapter interface.
 contract MultiBridgeMessageReceiver is IMultiBridgeMessageReceiver, ExecutorAware {
+ using MessageLibrary for MessageLibrary.Message;
+ using MessageLibrary for MessageLibrary.MessageExecutionParams;
+
  /// @notice the id of the source chain that this contract can receive messages from
  uint256 public immutable srcChainId;
  /// @notice the global access control contract
@@ -42,8 +45,8 @@ contract MultiBridgeMessageReceiver is IMultiBridgeMessageReceiver, ExecutorAwar
  /// @notice count of bridge adapters that have delivered each message
  mapping(bytes32 msgId => uint256 deliveryCount) public msgDeliveryCount;
 
- /// @notice the data required for executing a message
- mapping(bytes32 msgId => ExecutionData execData) public msgExecData;
+ /// @notice the hash of the params required for executing a message
+ mapping(bytes32 msgId => bytes32 execParamsHash) public msgExecParamsHash;
 
  /// @notice whether a message has been sent to the governance timelock for execution
  mapping(bytes32 msgId => bool scheduled) public isExecutionScheduled;
@@ -114,7 +117,7 @@ contract MultiBridgeMessageReceiver is IMultiBridgeMessageReceiver, ExecutorAwar
 
  /// this msgId is totally different with each adapters' internal msgId(which is their internal nonce essentially)
  /// although each adapters' internal msgId is attached at the end of calldata, it's not useful to MultiBridgeMessageReceiver.sol.
- bytes32 msgId = MessageLibrary.computeMsgId(_message);
+ bytes32 msgId = _message.computeMsgId();
 
  if (msgDeliveries[msgId][msg.sender]) {
  revert Error.DUPLICATE_MESSAGE_DELIVERY_BY_ADAPTER();
@@ -127,29 +130,33 @@ contract MultiBridgeMessageReceiver is IMultiBridgeMessageReceiver, ExecutorAwar
 
  msgDeliveries[msgId][msg.sender] = true;
 
- /// increment quorum
+ /// increment vote count for a message
  ++msgDeliveryCount[msgId];
 
- /// stores the execution data required
- ExecutionData memory prevStored = msgExecData[msgId];
+ /// stores the hash of the execution params required
+ bytes32 prevStoredHash = msgExecParamsHash[msgId];
 
  /// stores the message if the amb is the first one delivering the message
- if (prevStored.target == address(0)) {
- msgExecData[msgId] = ExecutionData(
- _message.target, _message.callData, _message.nativeValue, _message.nonce, _message.expiration
- );
+ if (prevStoredHash == bytes32(0)) {
+ msgExecParamsHash[msgId] = _message.computeExecutionParamsHash();
  }
 
  string memory bridgeName = IMessageReceiverAdapter(msg.sender).name();
  emit BridgeMessageReceived(msgId, bridgeName, _message.nonce, msg.sender);
  }
 
  /// @inheritdoc IMultiBridgeMessageReceiver
- function scheduleMessageExecution(bytes32 _msgId) external override {
- ExecutionData memory _execData = msgExecData[_msgId];
+ function scheduleMessageExecution(bytes32 _msgId, MessageLibrary.MessageExecutionParams calldata _execParams)
+ external
+ override
+ {
+ bytes32 execParamsHash = msgExecParamsHash[_msgId];
+ if (_execParams.computeExecutionParamsHash() != execParamsHash) {
+ revert Error.EXEC_PARAMS_HASH_MISMATCH();
+ }
 
  /// @dev validates if msg execution is not beyond expiration
- if (block.timestamp > _execData.expiration) {
+ if (block.timestamp > _execParams.expiration) {
  revert Error.MSG_EXECUTION_PASSED_DEADLINE();
  }
 
@@ -167,10 +174,12 @@ contract MultiBridgeMessageReceiver is IMultiBridgeMessageReceiver, ExecutorAwar
 
  /// @dev queues the action on timelock for execution
  IGovernanceTimelock(governanceTimelock).scheduleTransaction(
- _execData.target, _execData.value, _execData.callData
+ _execParams.target, _execParams.value, _execParams.callData
  );
 
- emit MessageExecutionScheduled(_msgId, _execData.target, _execData.value, _execData.nonce, _execData.callData);
+ emit MessageExecutionScheduled(
+ _msgId, _execParams.target, _execParams.value, _execParams.nonce, _execParams.callData
+ );
  }
 
  /// @notice update the governance timelock contract.

src/MultiBridgeMessageSender.sol

@@ -17,6 +17,8 @@ import "./libraries/Error.sol";
 /// Both of these are configured in the Global Access Control contract. In the case of Uniswap, both the authorised caller
 /// and owner should be set to the Uniswap V2 Timelock contract on Ethereum.
 contract MultiBridgeMessageSender {
+ using MessageLibrary for MessageLibrary.Message;
+
  /*/////////////////////////////////////////////////////////////////
  STRUCTS
  ////////////////////////////////////////////////////////////////*/
@@ -268,7 +270,7 @@ contract MultiBridgeMessageSender {
  _args.nativeValue,
  block.timestamp + _args.expiration
  );
- bytes32 msgId = MessageLibrary.computeMsgId(message);
+ bytes32 msgId = message.computeMsgId();
  (bool[] memory adapterSuccess, uint256 successCount) =
  _dispatchMessages(adapters, mmaReceiver, _args.dstChainId, message, _args.fees);
 

src/interfaces/IMultiBridgeMessageReceiver.sol

@@ -5,20 +5,6 @@ import "../libraries/Message.sol";
 
 /// @notice interface for the multi-bridge message receiver
 interface IMultiBridgeMessageReceiver {
- /// @notice encapsulates data that is relevant to a message's intended transaction execution.
- struct ExecutionData {
- // target contract address on the destination chain
- address target;
- // data to pass to target by low-level call
- bytes callData;
- // value to pass to target by low-level call
- uint256 value;
- // nonce of the message
- uint256 nonce;
- // expiration timestamp for the message beyond which it cannot be executed
- uint256 expiration;
- }
-
  /// @notice emitted when a message has been received from a single bridge.
  /// @param msgId is the unique identifier of the message
  /// @param bridgeName is the name of the bridge from which the message was received
@@ -60,7 +46,9 @@ interface IMultiBridgeMessageReceiver {
 
  /// @notice Sends a message, that has achieved quorum and has not yet expired, to the governance timelock for eventual execution.
  /// @param _msgId is the unique identifier of the message
- function scheduleMessageExecution(bytes32 _msgId) external;
+ /// @param _execParams are the params for message execution
+ function scheduleMessageExecution(bytes32 _msgId, MessageLibrary.MessageExecutionParams calldata _execParams)
+ external;
 
  /// @notice adds or removes bridge receiver adapters.
  /// @param _receiverAdapters the list of receiver adapters to add or remove

src/libraries/Error.sol

@@ -66,6 +66,9 @@ library Error {
  /// @dev is thrown if refund address is zero (or) invalid
  error INVALID_REFUND_ADDRESS();
 
+ /// @dev is thrown if execution params do not match the stored hash
+ error EXEC_PARAMS_HASH_MISMATCH();
+
  /*/////////////////////////////////////////////////////////////////
  ADAPTER ERRORS
  ////////////////////////////////////////////////////////////////*/

src/libraries/Message.sol

@@ -22,6 +22,20 @@ library MessageLibrary {
  uint256 expiration;
  }
 
+ /// @notice encapsulates data that is relevant to a message's intended transaction execution.
+ struct MessageExecutionParams {
+ // target contract address on the destination chain
+ address target;
+ // data to pass to target by low-level call
+ bytes callData;
+ // value to pass to target by low-level call
+ uint256 value;
+ // nonce of the message
+ uint256 nonce;
+ // expiration timestamp for the message beyond which it cannot be executed
+ uint256 expiration;
+ }
+
  /// @notice computes the message id (32 byte hash of the encoded message parameters)
  /// @param _message is the cross-chain message
  function computeMsgId(Message memory _message) internal pure returns (bytes32) {
@@ -37,4 +51,24 @@ library MessageLibrary {
  )
  );
  }
+
+ function extractExecutionParams(Message memory _message) internal pure returns (MessageExecutionParams memory) {
+ return MessageExecutionParams({
+ target: _message.target,
+ callData: _message.callData,
+ value: _message.nativeValue,
+ nonce: _message.nonce,
+ expiration: _message.expiration
+ });
+ }
+
+ function computeExecutionParamsHash(MessageExecutionParams memory _params) internal pure returns (bytes32) {
+ return keccak256(
+ abi.encodePacked(_params.target, _params.callData, _params.value, _params.nonce, _params.expiration)
+ );
+ }
+
+ function computeExecutionParamsHash(Message memory _message) internal pure returns (bytes32) {
+ return computeExecutionParamsHash(extractExecutionParams(_message));
+ }
 }

test/integration-tests/GracePeriodExpiry.t.sol

@@ -11,6 +11,7 @@ import "test/contracts-mock/MockUniswapReceiver.sol";
 
 import {MultiBridgeMessageSender} from "src/MultiBridgeMessageSender.sol";
 import {MultiBridgeMessageReceiver} from "src/MultiBridgeMessageReceiver.sol";
+import "src/libraries/Message.sol";
 import {Error} from "src/libraries/Error.sol";
 import {GovernanceTimelock} from "src/controllers/GovernanceTimelock.sol";
 
@@ -40,11 +41,17 @@ contract GracePeriodExpiryTest is Setup {
  0.01 ether,
  wormholeFee
  );
- MultiBridgeMessageSender(contractAddress[SRC_CHAIN_ID][bytes("MMA_SENDER")]).remoteCall{value: 2 ether}(
+
+ bytes memory callData = abi.encode(MockUniswapReceiver.setValue.selector, "");
+ uint256 nativeValue = 0;
+ uint256 expiration = block.timestamp + EXPIRATION_CONSTANT;
+ MultiBridgeMessageSender sender = MultiBridgeMessageSender(contractAddress[SRC_CHAIN_ID][bytes("MMA_SENDER")]);
+ uint256 nonce = sender.nonce() + 1;
+ sender.remoteCall{value: 2 ether}(
  DST_CHAIN_ID,
  address(target),
- abi.encode(MockUniswapReceiver.setValue.selector, ""),
- 0,
+ callData,
+ nativeValue,
  EXPIRATION_CONSTANT,
  refundAddress,
  fees,
@@ -63,7 +70,16 @@ contract GracePeriodExpiryTest is Setup {
  vm.selectFork(fork[DST_CHAIN_ID]);
  vm.recordLogs();
  /// schedule the message for execution by moving it to governance timelock contract
- MultiBridgeMessageReceiver(contractAddress[DST_CHAIN_ID][bytes("MMA_RECEIVER")]).scheduleMessageExecution(msgId);
+ MultiBridgeMessageReceiver(contractAddress[DST_CHAIN_ID][bytes("MMA_RECEIVER")]).scheduleMessageExecution(
+ msgId,
+ MessageLibrary.MessageExecutionParams({
+ target: address(target),
+ callData: callData,
+ value: nativeValue,
+ nonce: nonce,
+ expiration: expiration
+ })
+ );
  (uint256 txId, address finalTarget, uint256 value, bytes memory data, uint256 eta) =
  _getExecParams(vm.getRecordedLogs());
 

test/integration-tests/MultiMessageAggregation.t.sol

@@ -11,6 +11,7 @@ import "test/contracts-mock/MockUniswapReceiver.sol";
 
 import {MultiBridgeMessageSender} from "src/MultiBridgeMessageSender.sol";
 import {MultiBridgeMessageReceiver} from "src/MultiBridgeMessageReceiver.sol";
+import "src/libraries/Message.sol";
 import {Error} from "src/libraries/Error.sol";
 import {GovernanceTimelock} from "src/controllers/GovernanceTimelock.sol";
 
@@ -40,11 +41,17 @@ contract MultiBridgeMessageAggregationTest is Setup {
  0.01 ether,
  wormholeFee
  );
- MultiBridgeMessageSender(contractAddress[SRC_CHAIN_ID][bytes("MMA_SENDER")]).remoteCall{value: 2 ether}(
+
+ bytes memory callData = abi.encode(MockUniswapReceiver.setValue.selector, "");
+ uint256 nativeValue = 0;
+ uint256 expiration = block.timestamp + EXPIRATION_CONSTANT;
+ MultiBridgeMessageSender sender = MultiBridgeMessageSender(contractAddress[SRC_CHAIN_ID][bytes("MMA_SENDER")]);
+ uint256 nonce = sender.nonce() + 1;
+ sender.remoteCall{value: 2 ether}(
  DST_CHAIN_ID,
  address(target),
- abi.encode(MockUniswapReceiver.setValue.selector, ""),
- 0,
+ callData,
+ nativeValue,
  EXPIRATION_CONSTANT,
  refundAddress,
  fees,
@@ -63,7 +70,16 @@ contract MultiBridgeMessageAggregationTest is Setup {
  vm.selectFork(fork[DST_CHAIN_ID]);
  vm.recordLogs();
  /// schedule the message for execution by moving it to governance timelock contract
- MultiBridgeMessageReceiver(contractAddress[DST_CHAIN_ID][bytes("MMA_RECEIVER")]).scheduleMessageExecution(msgId);
+ MultiBridgeMessageReceiver(contractAddress[DST_CHAIN_ID][bytes("MMA_RECEIVER")]).scheduleMessageExecution(
+ msgId,
+ MessageLibrary.MessageExecutionParams({
+ target: address(target),
+ callData: callData,
+ value: nativeValue,
+ nonce: nonce,
+ expiration: expiration
+ })
+ );
  (uint256 txId, address finalTarget, uint256 value, bytes memory data, uint256 eta) =
  _getExecParams(vm.getRecordedLogs());
 

test/integration-tests/RemoteAdapterAdd.t.sol

@@ -10,6 +10,7 @@ import "test/Setup.t.sol";
 
 import {MultiBridgeMessageSender} from "src/MultiBridgeMessageSender.sol";
 import {MultiBridgeMessageReceiver} from "src/MultiBridgeMessageReceiver.sol";
+import "src/libraries/Message.sol";
 import {Error} from "src/libraries/Error.sol";
 import {GovernanceTimelock} from "src/controllers/GovernanceTimelock.sol";
 
@@ -51,7 +52,7 @@ contract RemoteAdapterAdd is Setup {
  _adapterAdd(adaptersToAdd, operation);
  }
 
- function _adapterAdd(address[] memory adaptersToAdd, bool[] memory operation) internal {
+ function _adapterAdd(address[] memory adaptersToAdd, bool[] memory operation) private {
  vm.selectFork(fork[ETHEREUM_CHAIN_ID]);
  vm.startPrank(caller);
 
@@ -65,10 +66,28 @@ contract RemoteAdapterAdd is Setup {
  0.01 ether,
  wormholeFee
  );
- MultiBridgeMessageSender(contractAddress[SRC_CHAIN_ID][bytes("MMA_SENDER")]).remoteCall{value: 2 ether}(
+
+ _sendAndExecuteMessage(adaptersToAdd, operation, fees);
+
+ for (uint256 j; j < adaptersToAdd.length; ++j) {
+ bool isTrusted = MultiBridgeMessageReceiver(contractAddress[DST_CHAIN_ID][bytes("MMA_RECEIVER")])
+ .isTrustedExecutor(adaptersToAdd[j]);
+ assert(isTrusted);
+ }
+ }
+
+ function _sendAndExecuteMessage(address[] memory adaptersToAdd, bool[] memory operation, uint256[] memory fees)
+ private
+ {
+ MultiBridgeMessageSender sender = MultiBridgeMessageSender(contractAddress[SRC_CHAIN_ID][bytes("MMA_SENDER")]);
+ uint256 nonce = sender.nonce() + 1;
+ bytes memory callData =
+ abi.encodeWithSelector(MultiBridgeMessageReceiver.updateReceiverAdapters.selector, adaptersToAdd, operation);
+ uint256 expiration = block.timestamp + EXPIRATION_CONSTANT;
+ sender.remoteCall{value: 2 ether}(
  DST_CHAIN_ID,
- address(contractAddress[DST_CHAIN_ID][bytes("MMA_RECEIVER")]),
- abi.encodeWithSelector(MultiBridgeMessageReceiver.updateReceiverAdapters.selector, adaptersToAdd, operation),
+ contractAddress[DST_CHAIN_ID][bytes("MMA_RECEIVER")],
+ callData,
  0,
  EXPIRATION_CONSTANT,
  refundAddress,
@@ -88,7 +107,16 @@ contract RemoteAdapterAdd is Setup {
  vm.selectFork(fork[DST_CHAIN_ID]);
  vm.recordLogs();
  /// schedule the message for execution by moving it to governance timelock contract
- MultiBridgeMessageReceiver(contractAddress[DST_CHAIN_ID][bytes("MMA_RECEIVER")]).scheduleMessageExecution(msgId);
+ MultiBridgeMessageReceiver(contractAddress[DST_CHAIN_ID][bytes("MMA_RECEIVER")]).scheduleMessageExecution(
+ msgId,
+ MessageLibrary.MessageExecutionParams({
+ target: contractAddress[DST_CHAIN_ID][bytes("MMA_RECEIVER")],
+ callData: callData,
+ value: 0,
+ nonce: nonce,
+ expiration: expiration
+ })
+ );
  (uint256 txId, address finalTarget, uint256 value, bytes memory data, uint256 eta) =
  _getExecParams(vm.getRecordedLogs());
 
@@ -97,11 +125,5 @@ contract RemoteAdapterAdd is Setup {
  GovernanceTimelock(contractAddress[DST_CHAIN_ID][bytes("TIMELOCK")]).executeTransaction(
  txId, finalTarget, value, data, eta
  );
-
- for (uint256 j; j < adaptersToAdd.length; ++j) {
- bool isTrusted = MultiBridgeMessageReceiver(contractAddress[DST_CHAIN_ID][bytes("MMA_RECEIVER")])
- .isTrustedExecutor(adaptersToAdd[j]);
- assert(isTrusted);
- }
  }
 }