Malicious exploitation allowed via self-referential hops, undermining bridge contract security, facilitating attacks.

[c4-bot-1]

Lines of code

https://github.com/code-423n4/2024-03-taiko/blob/f58384f44dbf4c6535264a472322322705133b11/packages/protocol/contracts/signal/SignalService.sol#L104-L118

Vulnerability details

Description

proveSignalReceived function in the SignalService contract is vulnerable to self-referential hops, allowing a malicious user to potentially manipulate the multi-hop bridging process and exploit vulnerabilities in the underlying bridge contracts.

• The proveSignalReceived function is responsible for verifying the inclusion of a signal in the source chain's state during the multi-hop bridging process.
• It takes the following key inputs:
  • _chainId: The ID of the chain where the signal originates.
  • _app: The address of the application contract.
  • _signal: The signal being verified.
  • _proof: The merkle proof for the signal's inclusion in the source chain's state.
• The function iterates over the provided HopProof array, verifying each hop using the _verifyHopProof function.
• It checks the validity of the hops by ensuring that the last hop's chain ID matches the current chain ID and that the intermediate hops' chain IDs are not zero and do not match the current chain ID.

Vulnerability Details

• Lies in the lack of explicit checks for self-referential hops within the proveSignalReceived function.
• Currently, the function checks for potential loops and invalid hops by verifying the chain IDs at each hop. However, it does not prevent a hop's chain ID from matching one of the previous hops' chain IDs.
• This allows a malicious user to construct a proof with self-referential hops that bypass the loop detection mechanism.

File: SignalService.sol:L104-L117

for (uint256 i; i < hopProofs.length; ++i) {
    hop = hopProofs[i];


    bytes32 signalRoot = _verifyHopProof(chainId, app, signal, value, hop, signalService);
    bool isLastHop = i == hopProofs.length - 1;


    if (isLastHop) {
        if (hop.chainId != block.chainid) revert SS_INVALID_LAST_HOP_CHAINID();
        signalService = address(this);
    } else {
        if (hop.chainId == 0 || hop.chainId == block.chainid) {
            revert SS_INVALID_MID_HOP_CHAINID();
        }
        signalService = resolve(hop.chainId, "signal_service", false);
    }

This lines checks for invalid last hop and intermediate hop chain IDs but does not prevent self-referential hops.

Expected Behavior

• Under normal circumstances, the proveSignalReceived function should verify the inclusion of a signal in the source chain's state by iterating over the provided HopProof array and ensuring the validity of each hop.
• The function should only accept valid proofs where each hop's chain ID is unique and does not match any of the previous hops' chain IDs.

But the current Behavior

• Due to the vulnerability, the proveSignalReceived function allows proofs with self-referential hops to be accepted.
• A malicious user can create a proof with a sequence of hops that includes a self-referential hop, such as: Chain A -> Chain B -> Chain A -> Chain C.
• The function's current checks will pass because the last hop's chain ID (Chain C) does not match the current chain ID, and the intermediate hops' chain IDs are not zero and do not match the current chain ID.
• However, the presence of the self-referential hop (Chain A appearing twice) can potentially enable the malicious user to manipulate the bridging process or exploit vulnerabilities in the underlying bridge contracts.

Impact

Malicious users can construct proofs with self-referential hops to bypass the loop detection mechanism and potentially exploit vulnerabilities in the bridge contracts.

Recommended Mitigation Steps

The function should maintain a mapping or array to keep track of the visited chain IDs and ensure that each hop's chain ID is unique and has not been encountered in any previous hops.

• If a self-referential hop is detected, the function should revert with an appropriate error message.

     mapping(uint256 => bool) private visitedChainIds;
     
     // Inside the proveSignalReceived function
     for (uint256 i = 0; i < hopProofs.length; ++i) {
         // ...
         
         if (visitedChainIds[hop.chainId]) {
             revert("Self-referential hop detected");
         }
         visitedChainIds[hop.chainId] = true;
         
         // ...
     }

• By adding this check, the proveSignalReceived function can explicitly detect and prevent self-referential hops, ensuring the integrity of the multi-hop bridging process.

Assessed type

Reentrancy

[c4-pre-sort]

minhquanym marked the issue as primary issue

[minhquanym]

Insufficient proof

[c4-sponsor]

dantaik (sponsor) confirmed

[dantaik]

This PR mitigates the concerns in this issue , but I'm having a hard time to construct an exploiting transaction with a bridge loop that has actual security impact.

[c4-judge]

0xean marked the issue as satisfactory

[c4-judge]

0xean marked the issue as selected for report

[0xean]

Welcome more comment from wardens and sponsors here during PJQA but perhaps this is better as QA since there is not enough proof to show the attack.

[mcgrathcoutinho]

Hi @0xean, I believe this issue should be of QA-severity.

I do not see a security risk entailed on how this could be misused or could impact the protocol. Additionally, I agree with the sponsor's comment on the fix regarding this issue:
taikoxyz/taiko-mono#16659 (comment)

Please consider re-evaluating this issue.

Thank you!

[0xean]

After thinking about this more, marking down to QA, as I just don't think there is a sufficient proof to warrant M

[c4-judge]

0xean changed the severity to QA (Quality Assurance)

[c4-judge]

0xean marked the issue as grade-b

[c4-judge]

0xean marked the issue as not selected for report