AllowedRelayers : anyone can become a relayer when relayersWhitelistEnabled is set false.

[hats-bug-reporter]

Github username: --
Twitter username: --
Submission hash (on-chain): 0x07a8c324cc7925fee2d1a4588ecf79c8dbc6468b57d4d9fa23a77a37250f6ccd
Severity: medium

Description:
Description

In the current implementation, owner has the power to set or update the relayer. This can be see in the abstract contract AllowedRelayers.

AllowedRelayers.sol#L27-L33

    function toggleRelayersWhitelistEnabled() public onlyOwner {
        relayersWhitelistEnabled = !relayersWhitelistEnabled;
    }

    function toggleRelayer(address _relayer) public onlyOwner {
        _toggleRelayer(_relayer);
    }

There is modifier implemented to check whether the caller is relayer or not.

onlyRelayer

    modifier onlyRelayer() {
        if (relayersWhitelistEnabled && !relayers[msg.sender]) {
            revert("NRL");
        }

        _;
    }

Please not the above check will work only if relayersWhitelistEnabled is enabled and then it check for relayer in the mapping.

when we see the function toggleRelayersWhitelistEnabled , this toggle can be update anytime by the owner. so , if it is set as false, the onlyRelayer will allow anyone act as relayer.

This can be problamatic in may cases where some of the criticial functions are depending on the relayer modifier.

For example, lets see the ackTransaction function in BitCoinProver contract.

Case 1:

BitcoinProver.sol#L263-L277

    function ackTransaction(
        TEERollup.FullComputationsProof memory txProof,
        IBitcoinDepositProcessingCallback callback,
        bytes memory _data
    ) public onlyRelayer {
        require(verifyComputations(txProof), "Invalid proof");

        (uint8 actionCode, IBitcoinDepositProcessingCallback.Transaction memory _tx) = abi.decode(
            txProof.partialProof.computationsResult,
            (uint8, IBitcoinDepositProcessingCallback.Transaction)
        );

        require(actionCode == uint8(ProvingAction.Transaction), "Invalid action code");
        callback.processDeposit(_tx, _data);
    }

At the end of the function, it makes the callback function with relayer provided callback address. malicioud relayer could misuse this callback function with its own callback structure.

case 2:

VaultBitcoinWallet.sol#L328-L344

    function finaliseOutgoingTxSerializing() public onlyRelayer {
        uint256 _index = outboundTransactionsCount++;
        TxSerializer _sr = _serializers[_index];

        (bytes memory txData, bytes32 txHash) = _sr.getRaw();

        emit SignedTxBroadcast(txHash, _index, txData);

        (uint256 _changeSystemIdx, uint64 _changeValue, uint256 _changeIdx) = _sr.getChangeInfo();
        _addOutboundTransaction(
            _index,
            txHash,
            _changeIdx,
            _changeSystemIdx,
            _changeValue
        );
    }

It can finalise any data without checking if the valid data or not inside the _serializers mapping.

Attack Scenario
Malicioud relayer can disrupt the deposit and withdrawal process completly.

Attachments

2. Revised Code File (Optional)

it would be better to remove the relayersWhitelistEnabled in the onlyRelayer modifier.

[aktech297]

@party-for-illuminati why this issue is invalid?

[party-for-illuminati]

@party-for-illuminati why this issue is invalid?

It is how it is by design.

"Malicioud relayer can disrupt the deposit and withdrawal process completly." please provide a PoC

[aktech297]

@party-for-illuminati why this issue is invalid?

It is how it is by design.

"Malicioud relayer can disrupt the deposit and withdrawal process completly." please provide a PoC

we are checking this in depth.