Fee implementation is broken

[hats-bug-reporter]

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

Description:
Description

The feesetter can set or change satoshiPerByte, which is used as the unit of fee in the protocol. During withdrawal or deposit, the fee is calculated using satoshiPerByte. If transactions are instant, changing the fee per byte won't create any issue. However, if transactions aren't instant (e.g., a withdrawal goes into a queue), the fee per byte can be changed between the initiation and finalization of a withdrawal.

Proof of Concept (PoC) File

    function withdraw(bytes memory to, uint64 amount, uint64 minReceiveAmount, bytes32 idSeed) public {
        uint64 amountAfterNetworkFee = amount - (BYTES_PER_OUTGOING_TRANSFER * satoshiPerByte);
        require(amountAfterNetworkFee >= minWithdrawalLimit, "AFL");

        uint64 protocolFees = amountAfterNetworkFee * withdrawalFee / 1000;
        if (isExcludedFromFees[msg.sender]) {
            protocolFees = 0;
        }

        uint64 amountAfterFee = amountAfterNetworkFee - protocolFees;
        require(amountAfterFee >= minReceiveAmount, "FTH");

        btcToken.burn(msg.sender, amount);
        if (protocolFees > 0) {
            btcToken.mint(owner(), protocolFees);
        }

        bytes32 _transferId = keccak256(abi.encodePacked(idSeed, to, amount));
        queue.push(
            OutgoingQueue.OutgoingTransfer(
                BitcoinUtils.resolveLockingScript(to, _isTestnet(), workingScriptSet),
                amountAfterFee,
                _transferId
            )
        );
    }

The withdrawal deducts BYTES_PER_OUTGOING_TRANSFER * satoshiPerByte as network fee based on the current chain conditions and then checks for amountAfterNetworkFee >= minWithdrawalLimit, which is incorrect since the transaction is not instant. It goes to the OutgoingQueue and is executed later at a possibly different satoshiPerByte, which then breaks the initial checks like minWithdrawalLimit and fee calculations.

    function startOutgoingTxSerializing() public onlyRelayer {
        uint256 _index = outboundTransactionsCount;
        require(address(_serializers[_index]) == address(0), "AC");

        (OutgoingQueue.OutgoingTransfer[] memory _transfers,) = queue.popBufferedTransfersToBatch();
        require(_transfers.length > 0, "NT");

        TxSerializer _sr = serializerFactory.createSerializer(
            AbstractTxSerializer.FeeConfig({
                //@audit-issue satoshiPerByte can be changed 
                outgoingTransferCost: BYTES_PER_OUTGOING_TRANSFER * satoshiPerByte,
                incomingTransferCost: BYTES_PER_INCOMING_TRANSFER * satoshiPerByte
            }),
            _transfers
        );

        _serializers[_index] = _sr;
        _sr.toggleRelayer(msg.sender);
    }

We can see in startOutgoingTxSerializing it fetches the OutgoingQueue and updates the fee based on the current satoshiPerByte. Since a batch of transactions is executed together, it creates issues

Since a batch of tx executed together it creates issues

if updated satoshiPerByte is less than previous satoshiPerByte, User paid high inflated fees and left amount is not refunded to user.

If the updated satoshiPerByte is greater than the previous satoshiPerByte, it will inflate the fee, and the withdrawal which is now executed will cause
-
uint64 amountAfterNetworkFee = amount - (BYTES_PER_OUTGOING_TRANSFER * satoshiPerByte); require(amountAfterNetworkFee >= minWithdrawalLimit, "AFL");
updated satoshiPerByte can cause amountAfterNetworkFee < minWithdrawalLimit but txn will be executed which should revert due to transfer amount is less than withdrawal limit

-   The protocol will have to pay more network fees than taken from the user, since network fees is more now.

This is based on an attack vector in a staking protocol which charges fees based on some rate and doesn't cut fees before changing the rate. Hence, if the rate becomes higher, it will take more fees than actual fees, and if the rate goes lower, it will take less fees than actual fees.

Therefore, before changing the satoshiPerByte, process all the pending OutgoingQueue transactions

[Tri-pathi]

@party-for-illuminati why invalid?