Sablier

Summary

An approved account can behave maliciously and can withdraw money to an address other than recipient's address.

Vulnerability Details

According to the docs the withdrawal address must be the recipient. The SablierV2Lockup:withdraw has a if block whick checks if msg.sender is either the stream's recipient or an approved third party. It uses && operator to ensure this, however there is a problem with it, when the to address is not recipients address and the caller is an approved account, it won't enter the if block then and won't throw an error. Because to enter if block both the coditions mentioned inside it should be true, but in the scenerio explained above when to != recipient && _isCallerStreamRecipientOrApproved(streamId), in this case it won't revert with an error, which it should as the to address is not recipient's address. Because the docs strictly mention it that the withdrawal address must be the recipient., which in this case, is not. And can withdraw money to address which is not equal to recipient's adddress Causing a user to lose all his funds.

Impact

An approved caller can act maliciously can withdraw all the money to an address other than recipient's address, it can either withdraw it for his own benifit (i.e. to his address) or it can just withdraw it to any address causing user griefing. An user can lose all his funds because of this minor mistake.

Tools Used

Manual Analysis

Recommendations

Use || operator instead of && inside the if block to check the conditions, upon using || instead of && when any of the condition is met annd is true, the if block executes and thus throw an error. whenever even an approved account will try to withraw money other than recipient's address, it will revert with an error revert Errors.SablierV2Lockup_WithdrawalAddressNotRecipient(streamId, msg.sender, to);, which is exactly the way we want it to behave.