MultiOffRamp - family-agnostic messages

[elatoskinas]

Motivation

The Multi-OffRamp can be converted to be family agnostic by using more generic Any2EVM structures and breaking the reliance on abi hashing

Solution

• Remove metadataHash and use an implicit metadata hash composed of (prefix, srcChainSelector, destChainSelector, onRampAddress)
• Convert EVM2EVMMessage to Any2EVMRampMessage, which uses a generic bytes sender
• Decouple messageId from hash(message)
• Out-of-scope: EVM2Any OnRamp changes

Sample on why hashes between Any2X and X2Any are no longer equivalent:

Solana:
  - messageId: hash(Sol2AnyMessage msg)
  - (Sol2Any message could contain some extra fields that are SOL-specific)

Off-chain:
  - Sol2AnyMessage -> Any2AnyMessage -> Any2EVMMessage
  - Route SOL -> Ethereum

Ethereum:
  - messageId: hash(Any2EVMMessage msg)
  - (Any2EVMMessage will contain some EVM-specific fields, but not the SOL specific fields)

hash(Sol2AnyMessage) != hash(Any2EVMMessage)

[elatoskinas]

Thoughts on this? Do we need a more complex validation that checks that at least 1 byte is non-zero? I'm thinking it would add much more complexity and contract size, maybe we could do this validation off-chain instead

[connorwstein]

yeah I think this is fine. If the configured onramp is junk then messages will be unexecutable until we configure the correct one

[elatoskinas]

This validation adds about ~6.4k gas to applySourceChainConfigUpdates in the median case, and a 0.07KB contract size increase.

  function validateNonZeroBytesAddress(bytes memory data) public pure {
      uint256 length = data.length;
      uint256 chunkCount = length / 32;

      // Iterate over the `bytes` array in chunks of 32 bytes
      for (uint256 i = 0; i < chunkCount; i++) {
          bytes32 chunk;
          assembly {
              chunk := mload(add(data, add(32, mul(i, 32))))
          }
          if (chunk != 0) {
              return;
          }
      }

      // Check any remaining bytes that do not fit into a `bytes32` chunk
      for (uint256 i = chunkCount * 32; i < length; i++) {
          if (data[i] != 0) {
              return;
          }
      }

      revert ZeroAddressNotAllowed();
  }

[elatoskinas]

until we configure the correct one

We are disallowing on ramp updates though (since we assume onramp change = offramp upgrade), so a misconfig would force us into a re-deployment

[connorwstein]

I don't mean code change, I mean just like we accidentally configure say 0xa as the onramp when the real one is 0x123. Messages just will fail to execute, then we adjust offramp to point to 0x123 and they will succeed

[elatoskinas]

then we adjust offramp to point to 0x123 and they will succeed

Yeah so this is not possible because of this check:

      if (currentOnRamp.length == 0) {
         // new chain selector
      } else if (
        keccak256(currentOnRamp) != keccak256(newOnRamp)
      ) {
        revert InvalidStaticConfig(sourceChainSelector);
      }

The offramp adjustment would be a complete redeployment of the offramp

[connorwstein]

ah and that check is there because we want to prevent minSeqNr overwrites?

[elatoskinas]

Yeah if we allowed overwriting the OnRamp, it would have some implications:

• chain => seqNumbers => executionStates would still be kept even though the OnRamp has changed (maybe not an issue since it's off-ramp specific)
• The same message could accidentally be included in the OffRamp and be replayed (since the _hash changes due to the different OnRamp, i.e. metadataHash)
• Committed roots are not cleared, so we would have different commit roots with different metadata hashes

I was always going under the assumption that the metadataHash must remain immutable for each OffRamp, to preserve the OnRamp <-> OffRamp dependency

[github-actions]

LCOV of commit eadb128 during Solidity Foundry #6226

Summary coverage rate:
  lines......: 98.6% (1816 of 1841 lines)
  functions..: 96.3% (337 of 350 functions)
  branches...: 90.5% (751 of 830 branches)

Files changed coverage rate: n/a

[asoliman92]

I think will be more efficient to check the newOnRamp first. Most probably the currentOnRamp will already have an address.

[elatoskinas]

Length checks are cheap so it wouldn't be substantial, but in this case this order makes sense because the onRamp can only be set if it was never set before (we treat it as immutable). newOnRamp.length == 0 checks are not required if the currentOnRamp is checked because we're checking equality directly