retryMessage unable to handle edge cases.

[c4-bot-4]

Lines of code

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

Vulnerability details

Impact

The function retryMessage() is unable to handle some edge scenarios listed below.

1. Reverting or refunding a sender when the receiver is banned after the transaction is placed in a RETRIABLE state.
2. Message that is suspended after the transaction is placed in a RETRIABLE state.

Proof of Concept

A message is set to RETRIABLEin the processMessage when transfer fails or if the target address does not satisfy some conditions.

//IN PROCESSMESSAGE

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





//_invokeMessageCall() FUNCTION
function _invokeMessageCall(
        Message calldata _message,
        bytes32 _msgHash,
        uint256 _gasLimit
    )
        private
        returns (bool success_)
    {
        if (_gasLimit == 0) revert B_INVALID_GAS_LIMIT();
        assert(_message.from != address(this));

        _storeContext(_msgHash, _message.from, _message.srcChainId);

        if (
            _message.data.length >= 4 // msg can be empty
                && bytes4(_message.data) != IMessageInvocable.onMessageInvocation.selector
                && _message.to.isContract()
        ) {
            success_ = false;
        } else {
            (success_,) = ExcessivelySafeCall.excessivelySafeCall(
                _message.to,
                _gasLimit,
                _message.value,
                64, // return max 64 bytes
                _message.data
            );
        }

        // Must reset the context after the message call
        _resetContext();
    }

The issue here is, when the team attempts suspension of a message which is in a RETRIABLE state, the code does not block execution.
Same or a banned address.
This is because the proofReceipt[msgHash] which handles suspension is not checked. The addressBanned[_addr] is not checked too.

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

Tools Used

Manual Review, Josephdara

Recommended Mitigation Steps

Recheck necessary details to verify that the transaction is still good to go.
Check the proofReceipt[msgHash].receivedAt and the addressBanned[_addr]

Assessed type

Other

[c4-pre-sort]

minhquanym marked the issue as primary issue

[minhquanym]

Since this issue mentioned both edge cases, using it to group #26 and #273

[c4-pre-sort]

minhquanym marked the issue as sufficient quality report

[adaki2004]

I'd dispute this with an addition (see end sentence).

2 cases mentioned here:

1. Reverting or refunding a sender when the receiver is banned after the transaction is placed in a RETRIABLE state.

Intentional to NOT refund sender when he/she is banned. (Tho we might remove the "banAddress", because it confuses a lot of people. The original intention behind banning an address is: NOT be able to call another , very important contract (message.to) on behalf of the Bridge, like the SignalService.)

2. Message that is suspended after the transaction is placed in a RETRIABLE state.
   Not to refund suspended messages is a feature, it is a failsafe/security mechanism. In such case we need to use it, it would be a severe situation and we do not necessary want to refund (by default) the owner, since it might be a fake message on the destination chain. (That can be one reason - so makes no sense to refund).
   Also suspension would never happen after RETRIABLE. If we suspend message it is between NEW and (DONE or RETRIABLE).

So as we considering removing banning addresses (and not allow SignalService to be called) for avoid confusion, but not because it is an issue, but a simplification and to avoid confusion. taikoxyz/taiko-mono#16604

[c4-sponsor]

dantaik (sponsor) disputed

[c4-judge]

0xean changed the severity to QA (Quality Assurance)

[c4-judge]

0xean marked the issue as grade-b

[Josephdara]

I believe this bug report was understood.
The issue is not only about refund. I used the or statement but as I stated in the report:

The issue here is, when the team attempts suspension of a message which is in a RETRIABLE state, the code does not block execution.
Same or a banned address.
This is because the proofReceipt[msgHash] which handles suspension is not checked. The addressBanned[_addr] is not checked too.

Therefore if an address is banned or if a message is suspended after being it was placed in the RETRIABLE state. Invocation of the banned address can occur, and suspended messages will be fulfilled.

ALthough initially submitted as a Med, I believe this is a High severity bug. Even after excluding the banned address, suspended messages can immediately be fulfilled.

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

Also suspension would never happen after RETRIABLE. If we suspend message it is between NEW and (DONE or RETRIABLE).

This restriction is not in the code.

[mcgrathcoutinho]

Hi @0xean, adding on to the above comment.

I think the sponsor has misunderstood what this issue and the duplicates are entailing here. The sponsor seems to be talking about not refunding the banned addresses and suspended messages (especially not invoking invokeMessageCall on both of them). The issues here are talking about how this invoking is possible i.e. the executions validly go through due to the missing checks.

I think the sponsor's comments itself confirm the issue:

Banned addresses sponsor comments:
Intentional to NOT refund sender when he/she is banned
The original intention behind banning an address is: NOT be able to call another , very important contract (message.to) on behalf of the Bridge

Suspended messages sponsor comments:
Not to refund suspended messages is a feature

Regarding the timeline for banning and suspending:
Also suspension would never happen after RETRIABLE.

Please consider re-evaluating this issue and its duplicates.

Thank you!

[adaki2004]

@Josephdara @mcgrathcoutinho @0xean @dantaik @Brechtpd
As we decided not going forward with "bann" messages (since causing confusiong) - im OK reevaluating this finding and duplicates!
Regarding suspension: it should never happen after retriable, but indeed this code (under audit) was not enforcing that, tho still think it is a low severity bug in practice. (Please @dantaik and @Brechtpd give your .02)

[iamandreiski]

Thanks to everyone for their inputs, to add to the discussion regarding the suspension mechanism. Since monitoring for "fake messages" is not enforced in the Taiko contracts as the goal/purpose of the suspension mechanism is to is to manually intervene and prevent "fantom/fake" transactions that can be proven (a bug in the Merkle tree), it's logical to assume that noticing a fake message in a RETRIABLE status it's entirely possible.

Since monitoring and suspending are a manual action which isn't enforced/automatic contract logic, rather it's a third party mechanism/tool/effort, it is theoretically possible for a message with status RETRIABLE to be passed as a legitimate one since the suspension mechanism doesn't work for these messages.

If the recalling and processing of messages employ an invocation-check mechanism, it's only logical for the retrial mechanism to do so too as there is a theoretical possibility for the scenario to occur within the realms of the audit scope.

[c4-judge]

This previously downgraded issue has been upgraded by 0xean

[c4-judge]

0xean marked the issue as satisfactory

[mcgrathcoutinho]

I'm not sure how this is going to be grouped. But I believe there are two separate issues here:

1. Talking about banned addresses bypassing
2. Suspended messages bypassing

If (2) was not pointed out reported, only a fix for (1) would've been implemented and vice versa.

Combining them both will make the judge's life easier but basing it off root causes, I think they stem differently.

Will agree with the judge's final verdict though, since there is only a thin line separating them.

[c4-judge]

0xean marked the issue as selected for report

[0xean]

using this issue to aggregate all of the issues around ban list functionality that has since been removed from the sponsors code base.