fix(protocol): check no loops in multi-hop in Bridge (#16659)

taikoxyz · Apr 5, 2024 · 447cd52 · 447cd52
1 parent 8209a43
commit 447cd52
Show file tree

Hide file tree

Showing 2 changed files with 76 additions and 3 deletions.
diff --git a/packages/protocol/contracts/signal/SignalService.sol b/packages/protocol/contracts/signal/SignalService.sol
@@ -22,6 +22,7 @@ contract SignalService is EssentialContract, ISignalService {
     uint256[48] private __gap;
 
     error SS_EMPTY_PROOF();
+    error SS_INVALID_HOPS_WITH_LOOP();
     error SS_INVALID_SENDER();
     error SS_INVALID_LAST_HOP_CHAINID();
     error SS_INVALID_MID_HOP_CHAINID();
@@ -92,6 +93,11 @@ contract SignalService is EssentialContract, ISignalService {
     {
         HopProof[] memory hopProofs = abi.decode(_proof, (HopProof[]));
         if (hopProofs.length == 0) revert SS_EMPTY_PROOF();
+        uint256 lenLessOne;
+        unchecked {
+            lenLessOne = hopProofs.length - 1;
+        }
+        uint64[] memory trace = new uint64[](lenLessOne);
 
         uint64 chainId = _chainId;
         address app = _app;
@@ -103,13 +109,19 @@ contract SignalService is EssentialContract, ISignalService {
         for (uint256 i; i < hopProofs.length; ++i) {
             hop = hopProofs[i];
 
+            for (uint256 j; j < i; ++j) {
+                if (trace[j] == hop.chainId) revert SS_INVALID_HOPS_WITH_LOOP();
+            }
+
             bytes32 signalRoot = _verifyHopProof(chainId, app, signal, value, hop, signalService);
-            bool isLastHop = i == hopProofs.length - 1;
+            bool isLastHop = i == lenLessOne;
 
             if (isLastHop) {
                 if (hop.chainId != block.chainid) revert SS_INVALID_LAST_HOP_CHAINID();
                 signalService = address(this);
             } else {
+                trace[i] = hop.chainId;
+
                 if (hop.chainId == 0 || hop.chainId == block.chainid) {
                     revert SS_INVALID_MID_HOP_CHAINID();
                 }
@@ -148,23 +160,35 @@ contract SignalService is EssentialContract, ISignalService {
         HopProof[] memory hopProofs = abi.decode(_proof, (HopProof[]));
         if (hopProofs.length == 0) revert SS_EMPTY_PROOF();
 
+        uint256 lenLessOne;
+        unchecked {
+            lenLessOne = hopProofs.length - 1;
+        }
+        uint64[] memory trace = new uint64[](lenLessOne);
+
         uint64 chainId = _chainId;
         address app = _app;
         bytes32 signal = _signal;
         bytes32 value = _signal;
         address signalService = resolve(chainId, "signal_service", false);
 
         HopProof memory hop;
+
         for (uint256 i; i < hopProofs.length; ++i) {
             hop = hopProofs[i];
 
+            for (uint256 j; j < i; ++j) {
+                if (trace[j] == hop.chainId) revert SS_INVALID_HOPS_WITH_LOOP();
+            }
+
             _verifyHopProof(chainId, app, signal, value, hop, signalService);
-            bool isLastHop = i == hopProofs.length - 1;
 
-            if (isLastHop) {
+            if (i == lenLessOne) {
                 if (hop.chainId != block.chainid) revert SS_INVALID_LAST_HOP_CHAINID();
                 signalService = address(this);
             } else {
+                trace[i] = hop.chainId;
+
                 if (hop.chainId == 0 || hop.chainId == block.chainid) {
                     revert SS_INVALID_MID_HOP_CHAINID();
                 }

diff --git a/packages/protocol/test/signal/SignalService.t.sol b/packages/protocol/test/signal/SignalService.t.sol
@@ -455,6 +455,55 @@ contract TestSignalService is TaikoTest {
         });
     }
 
+    function test_SignalService_proveSignalReceived_revert_with_a_loop() public {
+        uint64 srcChainId = uint64(block.chainid + 1);
+
+        vm.prank(Alice);
+        addressManager.setAddress(srcChainId, "signal_service", randAddress());
+
+        SignalService.HopProof[] memory proofs = new SignalService.HopProof[](3);
+
+        // first hop with full merkle proof
+        proofs[0].chainId = uint64(block.chainid + 2);
+        proofs[0].blockId = 1;
+        proofs[0].rootHash = randBytes32();
+        proofs[0].accountProof = new bytes[](1);
+        proofs[0].storageProof = new bytes[](10);
+
+        // second hop with storage merkle proof
+        proofs[1].chainId = proofs[0].chainId; // same
+        proofs[1].blockId = 2;
+        proofs[1].rootHash = randBytes32();
+        proofs[1].accountProof = new bytes[](0);
+        proofs[1].storageProof = new bytes[](10);
+
+        // third/last hop with full merkle proof
+        proofs[2].chainId = uint64(block.chainid);
+        proofs[2].blockId = 3;
+        proofs[2].rootHash = randBytes32();
+        proofs[2].accountProof = new bytes[](1);
+        proofs[2].storageProof = new bytes[](10);
+
+        // Add two trusted hop relayers
+        vm.startPrank(Alice);
+        addressManager.setAddress(proofs[0].chainId, "signal_service", randAddress() /*relay1*/ );
+        addressManager.setAddress(proofs[1].chainId, "signal_service", randAddress() /*relay2*/ );
+        vm.stopPrank();
+
+        vm.prank(taiko);
+        signalService.syncChainData(
+            proofs[1].chainId, LibSignals.STATE_ROOT, proofs[2].blockId, proofs[2].rootHash
+        );
+
+        vm.expectRevert(SignalService.SS_INVALID_HOPS_WITH_LOOP.selector);
+        signalService.proveSignalReceived({
+            _chainId: srcChainId,
+            _app: randAddress(),
+            _signal: randBytes32(),
+            _proof: abi.encode(proofs)
+        });
+    }
+
     function test_SignalService_proveSignalReceived_multiple_hops_caching() public {
         uint64 srcChainId = uint64(block.chainid + 1);
         uint64 nextChainId = srcChainId + 100;