feat(protocol): fix signal service multi-hop proof verification bugs (#…

…15680)

Co-authored-by: Keszey Dániel <keszeyd@MacBook-Pro.local>
Co-authored-by: Brecht Devos <Brechtp.Devos@gmail.com>

packages/protocol/contracts/signal/HopRelayRegistry.sol

@@ -0,0 +1,101 @@
+// SPDX-License-Identifier: MIT
+//  _____     _ _         _         _
+// |_   _|_ _(_) |_____  | |   __ _| |__ ___
+//   | |/ _` | | / / _ \ | |__/ _` | '_ (_-<
+//   |_|\__,_|_|_\_\___/ |____\__,_|_.__/__/
+//
+//   Email: security@taiko.xyz
+//   Website: https://taiko.xyz
+//   GitHub: https://github.com/taikoxyz
+//   Discord: https://discord.gg/taikoxyz
+//   Twitter: https://twitter.com/taikoxyz
+//   Blog: https://mirror.xyz/labs.taiko.eth
+//   Youtube: https://www.youtube.com/@taikoxyz
+
+pragma solidity 0.8.24;
+
+import "../common/EssentialContract.sol";
+import "./IHopRelayRegistry.sol";
+
+/// @title HopRelayRegistry
+contract HopRelayRegistry is EssentialContract, IHopRelayRegistry {
+    mapping(uint64 => mapping(uint64 => mapping(address => bool))) internal registry;
+    uint256[49] private __gap;
+
+    event RelayRegistered(
+        uint64 indexed srcChainId,
+        uint64 indexed hopChainId,
+        address indexed hopRelay,
+        bool registered
+    );
+
+    error MHG_INVALID_PARAMS();
+    error MHG_INVALID_STATE();
+
+    function init() external initializer {
+        __Essential_init();
+    }
+
+    /// @dev Register a trusted hop relay.
+    /// @param srcChainId The source chain ID where state roots correspond to.
+    /// @param hopChainId The hop relay's local chain ID.
+    /// @param hopRelay The address of the relay.
+    function registerRelay(
+        uint64 srcChainId,
+        uint64 hopChainId,
+        address hopRelay
+    )
+        external
+        onlyOwner
+    {
+        _registerRelay(srcChainId, hopChainId, hopRelay, true);
+    }
+
+    /// @dev Deregister a trusted hop relay.
+    /// @param srcChainId The source chain ID where state roots correspond to.
+    /// @param hopChainId The hop relay's local chain ID.
+    /// @param hopRelay The address of the relay.
+    function deregisterRelay(
+        uint64 srcChainId,
+        uint64 hopChainId,
+        address hopRelay
+    )
+        external
+        onlyOwner
+    {
+        _registerRelay(srcChainId, hopChainId, hopRelay, false);
+    }
+
+    /// @inheritdoc IHopRelayRegistry
+    function isRelayRegistered(
+        uint64 srcChainId,
+        uint64 hopChainId,
+        address hopRelay
+    )
+        public
+        view
+        returns (bool)
+    {
+        return registry[srcChainId][hopChainId][hopRelay];
+    }
+
+    function _registerRelay(
+        uint64 srcChainId,
+        uint64 hopChainId,
+        address hopRelay,
+        bool registered
+    )
+        private
+    {
+        if (
+            srcChainId == 0 || hopChainId == 0 || srcChainId == hopChainId || hopRelay == address(0)
+        ) {
+            revert MHG_INVALID_PARAMS();
+        }
+        if (registry[srcChainId][hopChainId][hopRelay] == registered) {
+            revert MHG_INVALID_STATE();
+        }
+        registry[srcChainId][hopChainId][hopRelay] = registered;
+        emit RelayRegistered(srcChainId, hopChainId, hopRelay, registered);
+    }
+}

packages/protocol/contracts/signal/IHopRelayRegistry.sol

@@ -0,0 +1,35 @@
+// SPDX-License-Identifier: MIT
+//  _____     _ _         _         _
+// |_   _|_ _(_) |_____  | |   __ _| |__ ___
+//   | |/ _` | | / / _ \ | |__/ _` | '_ (_-<
+//   |_|\__,_|_|_\_\___/ |____\__,_|_.__/__/
+//
+//   Email: security@taiko.xyz
+//   Website: https://taiko.xyz
+//   GitHub: https://github.com/taikoxyz
+//   Discord: https://discord.gg/taikoxyz
+//   Twitter: https://twitter.com/taikoxyz
+//   Blog: https://mirror.xyz/labs.taiko.eth
+//   Youtube: https://www.youtube.com/@taikoxyz
+
+pragma solidity 0.8.24;
+
+/// @title IHopRelayRegistry
+/// @notice A registry of hop relays for multi-hop bridging.
+//  A hop relay is a contract that relays a corresponding chain's state roots to its loal signal
+// service.
+interface IHopRelayRegistry {
+    /// @dev Returns if a relay is trusted.
+    /// @param srcChainId The source chain ID where state roots correspond to.
+    /// @param hopChainId The hop relay's local chain ID.
+    /// @param hopRelay The address of the relay.
+    /// @return trusted True if the relay is a trusted one.
+    function isRelayRegistered(
+        uint64 srcChainId,
+        uint64 hopChainId,
+        address hopRelay
+    )
+        external
+        view
+        returns (bool trusted);
+}

packages/protocol/contracts/signal/SignalService.sol

@@ -15,10 +15,11 @@
 pragma solidity 0.8.24;
 
 import "lib/openzeppelin-contracts/contracts/utils/math/SafeCast.sol";
-import "../common/AuthorizableContract.sol";
+import "../common/EssentialContract.sol";
 import "../common/ICrossChainSync.sol";
 import "../thirdparty/optimism/trie/SecureMerkleTrie.sol";
 import "../thirdparty/optimism/rlp/RLPReader.sol";
+import "./IHopRelayRegistry.sol";
 import "./ISignalService.sol";
 
 /// @title SignalService
@@ -31,38 +32,40 @@ import "./ISignalService.sol";
 /// Use the respective chain IDs as labels for authorization.
 /// Note: SignalService should not authorize Bridges or other Bridgable
 /// applications.
-contract SignalService is AuthorizableContract, ISignalService {
+contract SignalService is EssentialContract, ISignalService {
     using SafeCast for uint256;
 
     // merkleProof represents ABI-encoded tuple of (key, value, and proof)
     // returned from the eth_getProof() API.
     struct Hop {
+        uint64 chainId;
         address relay;
         bytes32 stateRoot;
         bytes merkleProof;
     }
 
     struct Proof {
-        address crossChainSync;
         uint64 height;
         bytes merkleProof;
+        // Ensure that hops are ordered such that those closer to the signal's source chain come
+        // before others.
         Hop[] hops;
     }
 
-    error SS_INVALID_FUNC_PARAMS();
-    error SS_INVALID_PROOF_PARAMS();
-    error SS_CROSS_CHAIN_SYNC_UNAUTHORIZED(uint256 chaindId);
-    error SS_CROSS_CHAIN_SYNC_ZERO_STATE_ROOT();
-    error SS_HOP_RELAYER_UNAUTHORIZED();
+    uint256[50] private __gap;
+
+    error SS_INVALID_PARAMS();
+    error SS_INVALID_PROOF();
     error SS_INVALID_APP();
-    error SS_INVALID_APP_PROOF();
-    error SS_INVALID_HOP_PROOF();
+    error SS_INVALID_RELAY();
     error SS_INVALID_SIGNAL();
+    error SS_INVALID_STATE_ROOT();
+    error SS_MULTIHOP_DISABLED();
     error SS_UNSUPPORTED();
 
     /// @dev Initializer to be called after being deployed behind a proxy.
-    function init() external initializer {
-        __OwnerUUPSUpgradable_init();
+    function init(address _addressManager) external initializer {
+        __Essential_init(_addressManager);
     }
 
     /// @inheritdoc ISignalService
@@ -100,49 +103,50 @@ contract SignalService is AuthorizableContract, ISignalService {
         returns (bool)
     {
         if (app == address(0) || signal == 0 || srcChainId == 0 || srcChainId == block.chainid) {
-            revert SS_INVALID_FUNC_PARAMS();
+            revert SS_INVALID_PARAMS();
         }
 
         Proof memory p = abi.decode(proof, (Proof));
-        if (p.crossChainSync == address(0) || p.merkleProof.length == 0) {
-            revert SS_INVALID_PROOF_PARAMS();
+        if (!isMultiHopEnabled() && p.hops.length > 0) {
+            revert SS_MULTIHOP_DISABLED();
         }
 
-        for (uint256 i; i < p.hops.length; ++i) {
-            if (p.hops[i].stateRoot == 0 || p.hops[i].merkleProof.length == 0) {
-                revert SS_INVALID_PROOF_PARAMS();
-            }
-        }
+        uint64 _srcChainId = srcChainId;
+        address _srcApp = app;
+        bytes32 _srcSignal = signal;
 
-        // p.crossChainSync is either a TaikoL1 contract or a TaikoL2 contract
-        if (!isAuthorizedAs(p.crossChainSync, bytes32(block.chainid))) {
-            revert SS_CROSS_CHAIN_SYNC_UNAUTHORIZED(block.chainid);
+        // Verify hop proofs
+        IHopRelayRegistry hrr;
+        if (p.hops.length > 0) {
+            hrr = IHopRelayRegistry(resolve("hop_relay_registry", false));
         }
 
-        bytes32 stateRoot = ICrossChainSync(p.crossChainSync).getSyncedSnippet(p.height).stateRoot;
-        if (stateRoot == 0) revert SS_CROSS_CHAIN_SYNC_ZERO_STATE_ROOT();
-
         // If a signal is sent from chainA -> chainB -> chainC (this chain), we verify the proofs in
         // the following order:
-        // 1. using chainC's latest parent's stateRoot to verify that chainB's TaikoL1/TaikoL2 contract has
+        // 1. using chainC's latest parent's stateRoot to verify that chainB's TaikoL1/TaikoL2
+        // contract has
         // sent a given hop stateRoot on chainB using its own signal service.
         // 2. using the verified hop stateRoot to verify that the source app on chainA has sent a
         // signal using its own signal service.
         // We always verify the proofs in the reversed order (top to bottom).
         for (uint256 i; i < p.hops.length; ++i) {
             Hop memory hop = p.hops[i];
-            if (hop.stateRoot == stateRoot) revert SS_INVALID_HOP_PROOF();
 
-            bytes32 label = authorizedAddresses[hop.relay];
-            if (label == 0) revert SS_HOP_RELAYER_UNAUTHORIZED();
+            if (!hrr.isRelayRegistered(_srcChainId, hop.chainId, hop.relay)) {
+                revert SS_INVALID_RELAY();
+            }
 
-            uint64 hopChainId = uint256(label).toUint64();
+            verifyMerkleProof(hop.stateRoot, _srcChainId, _srcApp, _srcSignal, hop.merkleProof);
 
-            verifyMerkleProof(stateRoot, hopChainId, hop.relay, hop.stateRoot, hop.merkleProof);
-            stateRoot = hop.stateRoot;
+            _srcChainId = hop.chainId;
+            _srcApp = hop.relay;
+            _srcSignal = hop.stateRoot;
         }
 
-        verifyMerkleProof(stateRoot, srcChainId, app, signal, p.merkleProof);
+        ICrossChainSync ccs = ICrossChainSync(resolve("taiko", false));
+        bytes32 stateRoot = ccs.getSyncedSnippet(p.height).stateRoot;
+
+        verifyMerkleProof(stateRoot, _srcChainId, _srcApp, _srcSignal, p.merkleProof);
         return true;
     }
 
@@ -157,7 +161,20 @@ contract SignalService is AuthorizableContract, ISignalService {
         view
         virtual
     {
+        if (stateRoot == 0) revert SS_INVALID_STATE_ROOT();
+        if (merkleProof.length == 0) revert SS_INVALID_PROOF();
+
+        bool verified;
+
         // TODO(dani): implement this please
+
+        if (!verified) revert SS_INVALID_PROOF();
+    }
+
+    /// @notice Checks if multi-hop is enabled.
+    /// @return Returns true if multi-hop bridging is enabled.
+    function isMultiHopEnabled() public view virtual returns (bool) {
+        return false;
     }
 
     /// @notice Get the storage slot of the signal.

packages/protocol/genesis/GenerateGenesis.g.sol

@@ -282,13 +282,6 @@ contract TestGenerateGenesis is Test, AddressResolver {
 
         signalServiceProxy.sendSignal(keccak256(abi.encodePacked(block.prevrandao)));
 
-        assertEq(
-            true,
-            signalServiceProxy.isAuthorizedAs(
-                getPredeployedContractAddress("TaikoL2"), bytes32((block.chainid))
-            )
-        );
-
         vm.startPrank(ownerSecurityCouncil);
 
         SignalService signalService =