AbstractTxSerializer : `enrichOutgoingTransaction` updates the `inputsStorage.spendInput` even if it does not reach the `hasSufficientInputs`

[hats-bug-reporter[bot]]

Github username: -- Twitter username: -- Submission hash (on-chain): 0xf742018bfbd540bf64c86535341145b736e3d29e50ae9c5b15fbaf309752629b Severity: high

Description: Description\

when a user initiate withdrawal, after taking the withdraw data from qeue, there are multiple seralizeing is done.

The flow could be like,

outputRelayer --> queue : Read current size
outputRelayer -> vaultWallet ++ : startOutgoingTxSerializing()
vaultWallet -> serializer ** : deploys
vaultWallet -> serializer : Allow spendInput()

loop for N/M inputs
outputRelayer -> serializer ++ : enrichOutgoingTransaction(inputIds)
loop for each input
serializer -> vaultWallet : spendInput(inputId)

if we see the enrichOutgoingTransaction function, it does the following.

For each input in the provided list:

• Check if the input is allowed.
• Mark the input as spent in the storage.
• Add the input to the transaction skeleton.

After that it checks,

AbstractTxSerializer.sol#L153-L156

        if (_skeleton.totalValueImported >= _skeleton.totalTransfersValueWithoutChange + _netFee) {
            _addChangeOutput(_netFee);
            _skeleton.hasSufficientInputs = true;
        }

when we see the. always, it overestimate the tx.outputs.length by adding +1. this would be reasonable when the array lenght is zero. but if the array is already filled with value, this would lead to case where more fee computation.

_estimateFees

    function _estimateFees() internal virtual view returns (uint64) {
        return uint64(fees.outgoingTransferCost * (_skeleton.tx.outputs.length + 1))
            + uint64(fees.incomingTransferCost * _skeleton.tx.inputs.length);
    }

The issue here is, if the above condition does not met, the _skeleton.hasSufficientInputs never be updated. Since the _netFee could add more value with totalTransfersValueWithoutChange, the above if check will not be executed.

later, when relayer try to enrichOutgoingTransaction, the function revert, since the inputsStorage.spendInput( is already spent.

Impact\

Since theinputsToSpend are flagged as spent in the storage, these can not be retried again.

Attachments

2. Revised Code File (Optional)

This can be fixed with following suggestions.

1. revert if the if check violates.
2. later, when retry, use bool flag to skip this if check.

[party-for-illuminati]

This is a proper behaviour. Relayer will just call enrichOutgoingTransaction with more inputs until it has enough to cover the outputs total amount + fees