Koolex - The stream recipient can prevent the payer from cancelling the stream

[sherlock-admin]

Koolex

high

The stream recipient can prevent the payer from cancelling the stream

Summary

The stream recipient can prevent the payer from cancelling the stream.

Vulnerability Detail

For stream tokens that implement IERC777 (backward compatible with IERC-20), the recipient can prevent the payer from cancelling the stream. The receipent could be a contract which implements tokensReceived() hook which is called when transferring the tokens at the following line in cancel() function

 if (recipientBalance > 0) token_.safeTransfer(recipient_, recipientBalance);

The hook tokensReceived() can simply revert the transaction if the received amount is less than full fund. This way, the cancel() function will always revert till the stream ends.

Another scenario, the receipent can block cancel() function always (even after the stream ends) by reverting the transaction if the received amount doesn't equal tokenAmount-1. Then the receipent waits till the stream ends and use withdraw() function to withdraw tokenAmount-1. In this scenario, the payer can not withdraw what's left from the contract balance after the stream ends.

Note: tokenAmount refers to the amount that was set when the stream was created by the payer.

Impact

• The recipient guarantees receiving all the fund sent to the stream contract by the payer (of course up to the tokenAmount set when the stream was created).
• The payer can not withdraw what's left from the contract balance after the stream ends

Code Snippet

cancel() function

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

Tool used

Manual Review

Recommendation

Favor pull over push. The cancel() function used by two parties. When the function is called, it pulls for one and push for the other. this causes the vulnerability described above.

I suggest:

1. Let the cancel() function transfer the token to the payer only. An example:

   uint256 recipientBalance = balanceOf(recipient_);
   // set remainingBalance to recipientBalance (as it is the maximum at this time)
   remainingBalance = recipientBalance;
   // payerBalance after deducting recipientBalance
   uint256 payerBalance = tokenBalance()-recipientBalance;
   if (payerBalance > 0) {
           token_.safeTransfer(payer_, payerBalance);
   }

2. The recipient can still withdraw their fair share of the funds by calling withdraw() and passing their balance.

[davidbrai]

thank you for the finding, but ERC777 was not in scope for the audit: https://app.sherlock.xyz/audits/contests/27

[koolexcrypto]

Escalate for 10 USDC

• The contest at https://app.sherlock.xyz/audits/contests/27 provides the following:

  On-chain context:
  DEPLOYMENT: mainnet
  ERC20: USDC and WETH
  ERC721: none
  ERC777: none
  FEE-ON-TRANSFER: none
  REBASING TOKENS: none
  ADMIN: n/a

This gives a contextual info that none of the ERC777 tokens are used but not saying that it is out of scope. If that's the case, I would say that all ERC20 tokens are out of scope except USDC and WETH which is not the case as asking on Discord channel, It was confirmed that "any popular ERC20" is supported.

• ERC777 is backward compatible with ERC-20. That means every ERC777 is ERC20 so you can interact with these tokens as if they were ERC20. makes it in-scope since in practice it can be used as ERC20. Ref: https://eips.ethereum.org/EIPS/eip-777#simple-summary

[sherlock-admin]

Escalate for 10 USDC

• The contest at https://app.sherlock.xyz/audits/contests/27 provides the following:

  On-chain context:
  DEPLOYMENT: mainnet
  ERC20: USDC and WETH
  ERC721: none
  ERC777: none
  FEE-ON-TRANSFER: none
  REBASING TOKENS: none
  ADMIN: n/a

This gives a contextual info that none of the ERC777 tokens are used but not saying that it is out of scope. If that's the case, I would say that all ERC20 tokens are out of scope except USDC and WETH which is not the case as asking on Discord channel, It was confirmed that "any popular ERC20" is supported.

• ERC777 is backward compatible with ERC-20. That means every ERC777 is ERC20 so you can interact with these tokens as if they were ERC20. makes it in-scope since in practice it can be used as ERC20. Ref: https://eips.ethereum.org/EIPS/eip-777#simple-summary

You've created a valid escalation for 10 USDC!

To remove the escalation from consideration: Delete your comment. To change the amount you've staked on this escalation: Edit your comment (do not create a new comment).

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[hrishibhat]

Escalation rejected

Readme clearly says ERC777 to be out of scope.

[sherlock-admin]

Escalation rejected

Readme clearly says ERC777 to be out of scope.

This issue's escalations have been rejected!

Watsons who escalated this issue will have their escalation amount deducted from their next payout.