Overwrite of already finished tx serializations

[hats-bug-reporter[bot]]

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

Description: Anyone can overwrite an already finished tx serialization to any arbitrary input by directly calling the respective BitcoinUtils functions, since these inner functions don't have access control or any way to measure the state of the serialized transaction.

• serializeTransactionHeader()
• serializeTransactionInputs()
• serializeTransactionOutputsAndTail()

BitcoinUtils.sol - serializeTransactionOutputsAndTail()

    function serializeTransactionOutputsAndTail(
        StorageWritableBufferStream.WritableBufferStream storage _buffer,
        BitcoinTransaction storage _tx
    ) external {
        StorageWritableBufferStream.writeVarInt(_buffer, _tx.outputs.length);
        for (uint i = 0; i < _tx.outputs.length; i++) {
            BitcoinTransactionOutput storage _output = _tx.outputs[i];

            StorageWritableBufferStream.writeUint64(_buffer, Endian.reverse64(_output.value));

            StorageWritableBufferStream.writeVarInt(_buffer, _output.script.length);
            StorageWritableBufferStream.write(_buffer, _output.script);
        }

        StorageWritableBufferStream.write(_buffer, bytes.concat(bytes4(Endian.reverse32(uint32(_tx.lockTime)))));
    }

---

TxSerializerLib.sol - serializeTx()

    function serializeTx(
        TxSerializingProgress storage _progress,
        BitcoinUtils.BitcoinTransaction storage _tx,
        uint256 _count
    ) external {
        require(_progress.state != TxSerializingState.Finished, "AF");

        if (_progress.state == TxSerializingState.Headers) {
            BitcoinUtils.serializeTransactionHeader(_progress.stream, _tx);
            _progress.state = TxSerializingState.Inputs;
        }

        // Serialize inputs
        BitcoinUtils.serializeTransactionInputs(
            _progress.stream,
            _tx,
            _progress.progress,
            _progress.progress + _count
        );

        _progress.progress += _count;
        if (_progress.progress == _tx.inputs.length) {
            BitcoinUtils.serializeTransactionOutputsAndTail(_progress.stream, _tx);
            _progress.state = TxSerializingState.Finished;
        }
    }

Attack Scenario

1. Relayer starts tx serialization, soon TxSerializeLib.serializeTx() will be called
2. Attacker changes the serialized tx with directly calling the BitcoinUtils functions to change the finished serialized transaction for their own benefit (sends themselves tokens)
3. Relayer progresses with tx serialization, if they don't notice funds will be stolen by the attacker, if they notice the current tx is DoS-d

Recommendation

Consider to either make the BitcoinUtils functions internal instead of external and adjust contracts. Alternatively consider to add access control to the BitcoinUtils library.

[rotcivegaf]

BitcoinUtils is a library and the contract don't inherit it So the external functions of BitcoinUtils library can't be access externally