V3 Audit Remediation

Remediations to Trail of Bits Audit

Additional issues discovered

1. Routers have no way to unenroll a domain and the potential workaround of enrolling the zero address for that domain fails due to EnumerableMap.contains semantics
2. Mailbox recipientIsm view call reverts if a message recipient does not implement interchainSecurityModule() but has a non-compliant fallback function
3. Mailbox dispatch reverts with underflow if msg.value is not greater than required value in the Mailbox.dispatch() or possibly allows a user to circumvent required fees with preexisting address(mailbox).balance
4. AbstractMessageIdAuthorizedIsm (never previously deployed) msg.value could be double spent with consecutive calls to verify

Changes

• 1ecfc46 v3 Router changes (#2752)

  • Restructured the solidity/ subdirectory for better navigation and readability.
  • Internal _dispatch() function takes in argument _value to pass to the mailbox for MailboxClient and Router contracts.
  • Removed the duplicated local Openzeppelin's CrossChainEnabled library and replaced as a dependency. (Openzeppelin has removed the library from their repo (at release 5.0.0) but our dependency is pointing to the release ^4.8.0 before the removal)
  • Directly calling getAnnouncementDigest() in ValidatorAnnounce (now a MailboxClient) instead of the ValidatorAnnouncements library.
  • Removed the IInterchainAccountRouter and IInterchainQueryRouter interfaces which were previously not being used anywhere except for InterchainAccountRouter and InterchainQueryRouter contracts.
  • HypERC20Collateral, HypNative, and HypERC721Collateral and their subsequent dependents adopt the new constructor signature from TokenRouter and omit the initialize() function made redundant because of their inheritance of MailboxClient__initialize()
  • HypERC721 now correctly transfer ownership to the msg.sender instead of the mailbox.

• c91f589 Security Patch 1: Add explicit Router unenrollment (#2760)

  • Added a unenrollRemoteRouter() function to the Router contract to allow for removing a remote router from the local router's EnumerableMapExtended registry (useful for non-active routers). Similarily, the remove() has been added to DomainRoutingIsm to allow for removing a domain from the ISM's EnumerableMapExtended registry.
  • DefaultFallbackRoutingIsm also inherits from MailboxClient.

• bf3d3c4 Security Patch 2: Make recipient security module resilient to non-reverting empty fallback (#2767)

  • Resolve the case when the recipient contract doesn't specify their own ISM by omitting a interchainSecurityModule() function but including a fallback function. In this case, the Mailbox's recipientIsm function reverts by trying to decode empty bytes.

• 8e4f2bb Make factory implementations verifiable (#2761)

  • Renaming abstract static factories for more clarity and making the implementation() function public on the specific factory contracts for easier offchain querying.

• a60ec18 Unify overheardIgp and igp (#2766)

  • Consolidating InterchainGasPaymaster and OverheadIgp into a single InterchainGasPaymaster contract to remove the "inner IGP" design making it more accessible to deploy and interact with. DomainGasConfig struct now stores the gas oracle address and the gas overhead (accounting for remote chain's Mailbox.process() and ISM.verify() function calls in the gas calculation).

• d69d76a Security Patch 3: Underflow in Mailbox dispatch and error messages (#2769)

  • Adding a check on the msg.value to be greater than required value in the Mailbox.dispatch() to throw a more descriptive error in the required hook instead of an underflow in the mailbox. Otherwise, if the mailbox has sufficient msg.value prior to the dispatch and the user overrides the default hook, she can circument paying for the dispatch.

• e90ae5a Security Patch 4: Fix msg.value replay on AbstractMessageIdAuthorizedIsm.verify() calls

  • Prevents value on AbstractMessageIdAuthorizedIsm from being double-spent by any verify caller, independent of the Mailbox.process message lifecycle
  • An additional issue related to AbstractMessageIdAuthorizedIsm.verify() is captured in #2835.