The message suspension mechanism can be circumvented if the message is with status RETRIABLE

[c4-bot-3]

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#L310-L337

Vulnerability details

Impact

Within Taiko's bridge there is a suspendMessages() function which serves to suspend or unsuspend invocation for a list of messages. This is a safety for the 2-step bridging, if the team notices "fantom/fake" transactions that can be proven (a bug in the Merkle tree) during the proof cooldown window, they can intervene to stop the message. By suspending the message or the list of messages, they will be setting the receivedAt timestamp at type(uint64).max so the current timestamp can't be greater than the receivedAt + invocationDelay so messages can't be processed or recalled. The vulnerability is that we don't have the needed checks that recallMessage() and processMessage() do for retryMessage().

If a fantom transaction failed to be processed (e.g. due to low gas) and is with status RETRIABLE and is noticed after the processing has failed, it can't be suspended as there are no such checks in retryMessage() allowing for anyone to retry and pass the message as a real one.

Proof of Concept

The suspendMessages() function can suspend/unsuspend a list of messages by suspending their invocation, meaning receivedAt will be set to type(uint64).max and the invocation checks block.timestamp > invocationDelay + receivedAt will always fail:

 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);
        }
    }

Invocation checks are present in both processMessage() and recallMessage():

if (block.timestamp >= invocationDelay + receivedAt)

The above-mentioned checks wouldn't hold true if the messages is "suspended" and receivedAt is set to type(uint64).max. If the above check fails, messages won't be invoked:

  if (_invokeMessageCall(_message, msgHash, gasLimit)) {
                    _updateMessageStatus(msgHash, Status.DONE);
                } else {
                    _updateMessageStatus(msgHash, Status.RETRIABLE);
                }

If a message with a status RETRIABLE is spotted to be a fantom one, it can't be suspended as no such checks are present within the retryMessage() function, allowing for fake/fantom messages to be invoked:

  function retryMessage(
        Message calldata _message,
        bool _isLastAttempt
    )
        external
        nonReentrant
        whenNotPaused
        sameChain(_message.destChainId)
    {
        // If the gasLimit is set to 0 or isLastAttempt is true, the caller must
        // be the message.destOwner.
        if (_message.gasLimit == 0 || _isLastAttempt) {
            if (msg.sender != _message.destOwner) revert B_PERMISSION_DENIED();
        }

        bytes32 msgHash = hashMessage(_message);
        if (messageStatus[msgHash] != Status.RETRIABLE) {
            revert B_NON_RETRIABLE();
        }

        // Attempt to invoke the messageCall.
        if (_invokeMessageCall(_message, msgHash, gasleft())) {
            _updateMessageStatus(msgHash, Status.DONE);
        } else if (_isLastAttempt) {
            _updateMessageStatus(msgHash, Status.FAILED);
        }
        emit MessageRetried(msgHash);
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Add the same checks present in recallMessage() and processMessage() for retryMessage() as well.

Assessed type

Invalid Validation

[c4-pre-sort]

minhquanym marked the issue as duplicate of #273

[c4-pre-sort]

minhquanym marked the issue as duplicate of #298

[c4-judge]

0xean changed the severity to QA (Quality Assurance)

[c4-judge]

0xean marked the issue as grade-c

[c4-judge]

This previously downgraded issue has been upgraded by 0xean

[c4-judge]

0xean marked the issue as satisfactory