Taiko - Fassi_Security's results

Lines of code

https://github.com/code-423n4/2024-03-taiko/blob/main/packages/protocol/contracts/bridge/Bridge.sol#L101-L112

Vulnerability details

Description

The owner or the bridge_watchdog can ban an address by calling Bridge.banAddress():

    function banAddress(
        address _addr,
        bool _ban
    )
        external
        onlyFromOwnerOrNamed("bridge_watchdog")
        nonReentrant
    {
        if (addressBanned[_addr] == _ban) revert B_INVALID_STATUS();
        addressBanned[_addr] = _ban;
        emit AddressBanned(_addr, _ban);
    }

In Bridge.processMessage, there is a check that checks if the address in the _message.to field is banned. If it is banned, the message status gets updated to DONE;

function processMessage() {
//.. code ommited
            if (
                _message.to == address(0) || _message.to == address(this)
                    || _message.to == signalService || addressBanned[_message.to]
            ) {
                refundAmount = _message.value;
                _updateMessageStatus(msgHash, Status.DONE);
            } 
//.. code ommited
}

After setting the message to status DONE, a refund will happen to either the msg.sender or the refundTo address alongside a small fee for the msg.sender.

function processMessage() {
//.. code ommited
            address refundTo =
                _message.refundTo == address(0) ? _message.destOwner : _message.refundTo;
            if (msg.sender == refundTo) {
                refundTo.sendEther(_message.fee + refundAmount);
            } else {
                msg.sender.sendEther(_message.fee);
                refundTo.sendEther(refundAmount);
            }
//.. code ommited

This means that it should not be possible to call _invokeMessageCall onto a banned message since this is reserved for the else branch after the if-branch where the checks happened for the banned addresses:

function processMessage() {
//.. code ommited
            if (
                _message.to == address(0) || _message.to == address(this)
                    || _message.to == signalService || addressBanned[_message.to]
            ) {
                refundAmount = _message.value;
                _updateMessageStatus(msgHash, Status.DONE);
->          } else {
                uint256 gasLimit = msg.sender == _message.destOwner ? gasleft() : _message.gasLimit;
                if (_invokeMessageCall(_message, msgHash, gasLimit)) {
                    _updateMessageStatus(msgHash, Status.DONE);
                } else {
                    _updateMessageStatus(msgHash, Status.RETRIABLE);
                }
            }
//.. code ommited

However, it is still possible to call _invokeMessageCall onto a banned address.

Proof of Concept

Consider the following:

• A message gets sent from src to destination
• Message arrives at destination
• Message becomes retriable

function processMessage() {
//.. code ommited
            } else {
                uint256 gasLimit = msg.sender == _message.destOwner ? gasleft() : _message.gasLimit;
                if (_invokeMessageCall(_message, msgHash, gasLimit)) {
                    _updateMessageStatus(msgHash, Status.DONE);
                } else {
->                  _updateMessageStatus(msgHash, Status.RETRIABLE);
                }
//.. code ommited

• The address that is specified as _message.to gets banned by the bridge_watchdog:

    function banAddress(
        address _addr,
        bool _ban
    )
        external
        onlyFromOwnerOrNamed("bridge_watchdog")
        nonReentrant
    {
        if (addressBanned[_addr] == _ban) revert B_INVALID_STATUS();
        addressBanned[_addr] = _ban;
        emit AddressBanned(_addr, _ban);
    }

• Since the _message.to got banned, it should not be possible to call _invokeMessageCall on _message.to.
• However, this message can still be retried and it will succesfully call _invokeMessageCall because there are no checks for banned messages in retryMessage:

    function retryMessage(
        Message calldata _message,
        bool _isLastAttempt
    )
        external
        nonReentrant
        whenNotPaused
        sameChain(_message.destChainId)
    {
        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();
        }
        if (_invokeMessageCall(_message, msgHash, gasleft())) {
            _updateMessageStatus(msgHash, Status.DONE);
        } else if (_isLastAttempt) {
            _updateMessageStatus(msgHash, Status.FAILED);
        }
        emit MessageRetried(msgHash);
    }

This results in banned addresses still being able to be interacted with, which should not be the case since _invokeMessageCall is strictly prohibited from being called on these banned addresses.

Tools Used

Manual Review

Recommended Mitigation Steps

Handle retryMessage calls to banned addresses in the same way they are handled inside processMessage. That means to send a refund.

Assessed type

Timing

Lines of code

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

Vulnerability details

Description

The owner or the bridge watchdog can suspend multiple messages by calling suspendMessages:

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

receivedAt gets set to type(uint64).max, which results in the following three functions to revert:

• recallMessage
• processMessage

In both these functions, when a message is suspended, this happens:

• This bool gets set to true because receivedAt is type(uint64).max

        bool isMessageProven = receivedAt != 0;

• Since isMessageProven is true, the next if-statement does not get executed:

        if (!isMessageProven) {

• Since receivedAt is set to type(uint64).max, this next if-statement does not get executed:

        if (block.timestamp >= invocationDelay + receivedAt) {

• Since isMessageProven is true, this else-if-statement does not get executed:

        } else if (!isMessageProven) {

• This will always lead us to this last else-statement, causing these functions to revert:

        } else {
            revert B_INVOCATION_TOO_EARLY();
        }

This demonstrates that it should not possible to interact with suspended messages.

However, this is not the case for suspended retriable messages. Consider the following:

• message(1) is RETRIABLE
• message(1) gets suspended by the bridge watchdog due to suspicious activity
• However, it is still possible to retryMessage since nothing checks if receivedAt is set to type(uint64).max:

    function retryMessage(
        Message calldata _message,
        bool _isLastAttempt
    )
        external
        nonReentrant
        whenNotPaused
        sameChain(_message.destChainId)
    {
        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();
        }
        if (_invokeMessageCall(_message, msgHash, gasleft())) {
            _updateMessageStatus(msgHash, Status.DONE);
        } else if (_isLastAttempt) {
            _updateMessageStatus(msgHash, Status.FAILED);
        }
        emit MessageRetried(msgHash);
    }

The impact can be big, if a bridge watchdog manages to catch a suspicious transaction in their RETRIABLE status and suspends it, then the malicious user can still retry it and execute it.

Tools Used

Manual Review

Recommended Mitigation Steps

Inside retryMessage, check if the receivedAt value is set to type(uint64).max. If this is true, revert.

Assessed type

Context

Lines of code

https://github.com/code-423n4/2024-03-taiko/blob/a30b5b6afd121e4de8ceff7165a2091e62194992/packages/protocol/contracts/automata-attestation/utils/RsaVerify.sol#L99-L110 https://github.com/code-423n4/2024-03-taiko/blob/a30b5b6afd121e4de8ceff7165a2091e62194992/packages/protocol/contracts/automata-attestation/utils/RsaVerify.sol#L247-L258

Vulnerability details

Description

In the library RsaVerify.sol, the function pkcs1Sha256 is used to verify a a PKCSv1.5 SHA256 signature:

    function pkcs1Sha256(
        bytes32 _sha256,
        bytes memory _s,
        bytes memory _e,
        bytes memory _m
    )
        internal
        view
        returns (bool)
    {
//.. code omitted
}

Another function in RsaVerify.sol, which verifies a PKCSv1.5 SHA1 signature, is the function pkcs1Sha1:

    function pkcs1Sha1(
        bytes20 _sha1,
        bytes memory _s,
        bytes memory _e,
        bytes memory _m
    )
        internal
        view
        returns (bool)
    {
//.. code omitted
}

Both these functions use the same assembly block to make a staticcall:

        assembly {
            pop(
                staticcall(
                    sub(gas(), 2000),
                    5,
                    add(input, 0x20),
                    inputlen,
                    add(decipher, 0x20),
                    decipherlen
                )
            )
        }

The gas used for the staticcall is calculated by sub(gas(), 2000). However, if there is not enough gas left to make the static call, the execution of pkcs1Sha256() or pkcs1Sha1() will continue because there is no check for the return value of the static call being 0.

Both pkcs1Sha256 andpkcs1Sha1 are either directly or indirectly called inside:

• SigVerifyLib.verifyRS256Signature
• SigVerifyLib.verifyRS1Signature
• SigVerifyLib.verifyCertificateSignature
• SigVerifyLib.verifyAttStmtSignature

This means that signatures will be either wrongfully validated or wrongfully invalidated, which impacts the system's core workings since signature validation is one of the key building blocks of a blockchain.

Tools Used

Manual Review

Recommended Mitigation Steps

Add a check that requires the return value of the staticcall to be true.

Assessed type

Context