Double Subtraction of _outstandingTokenAmounts in claimPreviouslyUnclaimable

[hats-bug-reporter[bot]]

Github username: @0xShax2nk Twitter username: 0xShashanks_07 Submission hash (on-chain): 0x9aec5bf94d610c96126661486521c8d3124b77513f19a97df9ea0b9f5a6712dd Severity: high

Description: Description\

The PP_Streaming_v1 contract manages streaming payments and allows users to claim their payments. The contract interacts with the ERC20PaymentClientBase_v1 to handle token transfers and updates the _outstandingTokenAmounts variable to track outstanding token amounts. An issue arises when a token transfer fails, leading to a double subtraction of the _outstandingTokenAmounts variable.

In PP_Streaming_v1 Contract the state variable _outstandingTokenAmounts is getting decremented twice for the same stream under certain conditions. This occurs when a token transfer fails, the stream ends, and the user later claims the previously unclaimable tokens. The sequence of operations leads to _outstandingTokenAmounts being updated in both _removePaymentForSpecificStream() and _claimPreviouslyUnclaimable(), resulting in an incorrect state.

Impact

The incorrect state of _outstandingTokenAmounts can lead to financial discrepancies, causing overpayment or underpayment to users. Also the integrity of the payment processing logic is compromised, leading to potential errors in future transactions cause of this issue.

Double subtraction of _outstandingTokenAmounts leads to incorrect tracking of outstanding token amounts, which can cause:

• Inaccurate accounting of tokens.
• Potential underflow issues.
• Discrepancies in the contract's state, leading to potential financial losses or inconsistencies.

Attack Scenario\

1. Setup: A user has an active stream that is supposed to transfer tokens periodically.
2. Failure and Stream End: The token transfer fails due to insufficient balance or other reasons. If the stream has ended, _afterClaimCleanup() is triggered, which calls _removePaymentForSpecificStream() and updates _outstandingTokenAmounts.
3. Claim Unclaimable: The user later calls claimPreviouslyUnclaimable() to claim the previously unclaimable tokens, which again updates _outstandingTokenAmounts.
4. Double Subtraction: This results in _outstandingTokenAmounts being decremented twice for the same stream, leading to an incorrect state.

Attachments

https://github.com/hats-finance/Inverter-Network-0xe47e52c4fea05e555920f1dcdcc6fb8eca103eeb/blob/62892384fd7d0ce4d0e389c530200c69921473f7/src/modules/paymentProcessor/PP_Streaming_v1.sol#L132

https://github.com/hats-finance/Inverter-Network-0xe47e52c4fea05e555920f1dcdcc6fb8eca103eeb/blob/62892384fd7d0ce4d0e389c530200c69921473f7/src/modules/paymentProcessor/PP_Streaming_v1.sol#L691

https://github.com/hats-finance/Inverter-Network-0xe47e52c4fea05e555920f1dcdcc6fb8eca103eeb/blob/62892384fd7d0ce4d0e389c530200c69921473f7/src/modules/paymentProcessor/PP_Streaming_v1.sol#L713

https://github.com/hats-finance/Inverter-Network-0xe47e52c4fea05e555920f1dcdcc6fb8eca103eeb/blob/62892384fd7d0ce4d0e389c530200c69921473f7/src/modules/paymentProcessor/PP_Streaming_v1.sol#L416

https://github.com/hats-finance/Inverter-Network-0xe47e52c4fea05e555920f1dcdcc6fb8eca103eeb/blob/62892384fd7d0ce4d0e389c530200c69921473f7/src/modules/paymentProcessor/PP_Streaming_v1.sol#L549

https://github.com/hats-finance/Inverter-Network-0xe47e52c4fea05e555920f1dcdcc6fb8eca103eeb/blob/62892384fd7d0ce4d0e389c530200c69921473f7/src/modules/logicModule/abstracts/ERC20PaymentClientBase_v1.sol#L206

https://github.com/hats-finance/Inverter-Network-0xe47e52c4fea05e555920f1dcdcc6fb8eca103eeb/blob/62892384fd7d0ce4d0e389c530200c69921473f7/src/modules/paymentProcessor/PP_Streaming_v1.sol#L143

https://github.com/hats-finance/Inverter-Network-0xe47e52c4fea05e555920f1dcdcc6fb8eca103eeb/blob/62892384fd7d0ce4d0e389c530200c69921473f7/src/modules/paymentProcessor/PP_Streaming_v1.sol#L783

1. Proof of Concept (PoC) File

2. Revised Code File (Optional)

[PlamenTSV]

I see the issue here, however _afterClaimCleanup() is invoked even for a failed transfer, only if the stream has completely ended, due to this block:

if ( block.timestamp >= endForSpecificStream(client, paymentReceiver, streamId) ) {
            _afterClaimCleanup(client, paymentReceiver, streamId);
        }

Which makes it even more unlikely, since the conditions are:

1. Ended stream
2. Failed transfer Thus Medium, I will let the other judge and sponsor review this

[FHieser]

The call to adapt outstandingtoken amount in _removePaymentForSpecificStream is based on the remainingReleasable of the stream. Even if the transfer fails it counts it as _released in the stream struct. That way it isnt double counted. At least that is the idea.

If you think this doesnt account for it please submit a POC for me to check Thanks