imare - token can get stuck inside ``Stream`` contract

[sherlock-admin]

imare

medium

token can get stuck inside Stream contract

Summary

rescueERC20 function is used to rescue stucked tokens inside the Stream contract. It works for all tokens except for the token used for the stream payment.

When the stream has ended by reaching its stop time or by being canceled the surplus payment token sent after this "end" event gets stuck inside the contract.

Vulnerability Detail

Imagine the flowing two scenarios:

a) In the same block the stream gets canceled first then someone else (in the same block but a transaction after the cancellation) sends the payment token to the contract

or

b) The stream has already ended and someone else later sends the payment token to the contract.

In both scenarios if the sent token was not the payment one the payer user can save it by calling rescueERC20

Impact

Not all tokens can be saved from being stuck in the contract even if the stream has ended its founding and the recepient and payer got their fair share of tokens.

Code Snippet

https://github.com/sherlock-audit/2022-11-nounsdao/blob/main/src/Stream.sol#L268-L272

Tool used

Manual Review

Recommendation

Allow recovering the payment token after the stream has ended its founding like this:

    function rescueERC20(address tokenAddress, uint256 amount) external onlyPayer {
-      if (tokenAddress == address(token())) revert CannotRescueStreamToken();
+      if (tokenAddress == address(token()) && remainingBalance != 0) revert CannotRescueStreamToken();

        IERC20(tokenAddress).safeTransfer(msg.sender, amount);
    }

[hrishibhat]

cancel can be called again to retrieve the extra tokens sent.

[sherlock-admin]

Escalate for 15 USDC

This is not true calling cancel again will not work because the following line prevents the payer or recipient to get the right amount of token when the Stream is already canceled or ended.

https://github.com/sherlock-audit/2022-11-nounsdao/blob/5def6ce65aeae7c55c66bbeb0e5f92f2ad169211/src/Stream.sol#L347-L351

        uint256 remainingBalance_ = remainingBalance;

        // When this function is called after the stream has been cancelled, when balance is less than
        // tokenAmount, without this early exit, the withdrawal calculation below results in an underflow error.
        if (remainingBalance_ == 0) return 0;

balanceOf(payer or recepient) will after the Stream has been canceled/ended always return 0 because the line with the early exit no matter the real amount of token inside the contract.

After the first cancel remainingBalance is set to 0.

    function cancel() external onlyPayerOrRecipient {
        address payer_ = payer();
        address recipient_ = recipient();
        IERC20 token_ = token();

        uint256 recipientBalance = balanceOf(recipient_);

        // This zeroing is important because without it, it's possible for recipient to obtain additional funds
        // from this contract if anyone (e.g. payer) sends it tokens after cancellation.
        // Thanks to this state update, `balanceOf(recipient_)` will only return zero in future calls.
        remainingBalance = 0;

function balanceOf(address who) public view returns (uint256) {
        uint256 recipientBalance = _recipientBalance(); // THIS RETURNS ZERO

        if (who == recipient()) return recipientBalance; // ZERO
        if (who == payer()) {
            // This is safe because it should always be the case that:
            // remainingBalance >= recipientBalance.
            unchecked {
                return remainingBalance - recipientBalance; // THIS IS 0 - 0 == 0
            }
        }
        return 0;
    }

So once remainingBalance is set to 0 the extra rewards token are stuck inside the contract. Using the original rescueERC20 will not work without proper mitigations.

You've deleted an escalation for this issue.