keccak123 - Missing address validation causes issues

[sherlock-admin]

keccak123

medium

Missing address validation causes issues

Summary

There is no protection for payer == receiver. If payer == receiver, the stream will act strangely because it is not designed to handle this case.

Another unusual case that is not protected against is when two stream clones are each others receiver. This is possible because payer can be any address.

Vulnerability Detail

When payer == receiver, balanceOf will only return _recipientBalance even when the payer calls it. This means the payer cannot get a valid answer from balanceOf. When payer == receiver, rescueERC20 can be called by the receiver even though the onlyPayer modifier is on this function because payer == receiver.

Another unusual case is when two stream clones pay each other. This is possible only because payer is not necessarily msg.sender and it creates an infinite loop. Because there is no on-chain storage where existing stream clones are stored (unlike Sablier, which maintains a mapping that can be queried), it is not possible to prevent this case with the existing contracts unless past events are checked to compile a list of existing streams.

Impact

Unexpected stream values can result from the case of payer == receiver and when stream clones pay each other.

Code Snippet

There is no code preventing payer == receiver https://github.com/sherlock-audit/2022-11-nounsdao/blob/main/src/StreamFactory.sol#L193-L200

payer == receiver will cause parts of the stream to act strangely. For example, balanceOf will only return _recipientBalance when the payer calls it https://github.com/sherlock-audit/2022-11-nounsdao/blob/main/src/Stream.sol#L285-L297

Compare to Sablier which has this validation https://github.com/sablierhq/sablier/blob/476bdd250a4f7ab7fcd79b9d740797da652d0690/packages/protocol/contracts/Sablier.sol#L185

Tool used

Manual Review

Recommendation

Prevent payer == receiver when a new stream is created. This should be added to the existing logic.

[davidbrai]

I don't think this can cause loss of funds, therefore disagree with severity