sherlock-audit / 2022-11-nounsdao-judging

4 stars 0 forks source link

hihen - A malicious recipient may cheat the payer and can ensure to withdraw all tokens #59

Closed sherlock-admin closed 1 year ago

sherlock-admin commented 1 year ago

hihen

high

A malicious recipient may cheat the payer and can ensure to withdraw all tokens

Summary

A malicious recipient can ensure that he gets all the tokens funded by preventing the payer from calling cancel().

Vulnerability Detail

The payer can call cancel() to cancel a Stream. The function cancle() will transfer the tokens that belongs to recipient currently first, and then transfer all remaining tokens to payer.

If the token of the Stream supports hooks (such as ERC777), the transfer to recipient may fail if the recipient is a contract.

A malicious user/builder could use this vulnerability to cheat the payer/DAO:

  1. User X deploy a custom contract R.
  2. The Stream is deployed with a ERC777 token T (imBTC for example) as the tokenAddress and R as the recipient.
  3. The Stream is sufficiently funded by the payer.
  4. X makes R to reject the receipt of token T(revert in R.tokensReceived()) after startTime.
  5. X makes R to accept the receipt of toimBTC after stopTime.
  6. X call Stream.withdraw() to take all tokens out of the Stream.

The payer won't be able to call cancle() successfully after step 4, because it will always revert when transfer token T to R.

Impact

The Steam contract does not protect payer's funds, and malicious users(recipients) can cheat to get all the funds.

Code Snippet

Stream.sol#cancel()

function cancel() external onlyPayerOrRecipient {
    ...
    ...
    if (recipientBalance > 0) token_.safeTransfer(recipient_, recipientBalance);
    ...
    ...
    uint256 payerBalance = tokenBalance();
    if (payerBalance > 0) {
        token_.safeTransfer(payer_, payerBalance);
    }
    ...
}

Tool used

Manual Review

Recommendation

There are multiple solutions:

  1. Do not revert when transferring token to recipient in cancel(), just ignore the call result.
  2. Do not transfer token to recipient in cancel(), record the amount first, and transfer it in another standalone function.

Duplicate of #38