zimu - `RATE_DECIMALS_MULTIPLIER * tokenAmount() / duration` in unchecked block of `ratePerSecond()` can overflow

[sherlock-admin]

zimu

high

RATE_DECIMALS_MULTIPLIER * tokenAmount() / duration in unchecked block of ratePerSecond() can overflow

Summary

The ratePerSecond computation RATE_DECIMALS_MULTIPLIER * tokenAmount() / duration in ratePerSecond() can overflow. Since this computation is in a unchecked block and thus won't be reverted, wrong balance of recipient would be returned to _recipientBalance() and withdraw().

Vulnerability Detail

1. The related codes are shown as follow. RATE_DECIMALS_MULTIPLIER = 1e6, tokenAmount() can return a value as maximum as 2^256-1, and let duration = 1e5, then the computation result of RATE_DECIMALS_MULTIPLIER * tokenAmount() / duration would be larger than 2^256-1, and ratePerSecond() returns an overflowed uint256 value.

   uint256 public constant RATE_DECIMALS_MULTIPLIER = 1e6;
   
   function tokenAmount() public pure returns (uint256) {
       return _getArgUint256(60);
   }
   
   function ratePerSecond() public pure returns (uint256) {
       uint256 duration = stopTime() - startTime();
       unchecked {
           return RATE_DECIMALS_MULTIPLIER * tokenAmount() / duration;
       }
   }

2. Let balance' denote the overflowed balance, and balance represent the correct balance. We can have the following two cases:

   • case balance < amount < balance', the recipient can withdraw tokens at a faster rate, or in extreme case, only one-time withdraw is needed.

   • case balance' < amount < balance, the recipient would receive a smaller amount of tokens each time unless stop time reached.

     function withdraw(uint256 amount) external onlyPayerOrRecipient {
     if (amount == 0) revert CantWithdrawZero();
     address recipient_ = recipient();
     
     uint256 balance = balanceOf(recipient_);
     if (balance < amount) revert AmountExceedsBalance();
     
     // This is safe because it should always be the case that:
     // remainingBalance >= balance >= amount.
     unchecked {
         remainingBalance = remainingBalance - amount;
     }
     
     token().safeTransfer(recipient_, amount);
     emit TokensWithdrawn(msg.sender, recipient_, amount);
     }

Impact

The recipient would receive unexpected smaller or larger amount of tokens for each withdraw().

Code Snippet

https://github.com/sherlock-audit/2022-11-nounsdao/blob/main/src/Stream.sol#L125-L138 https://github.com/sherlock-audit/2022-11-nounsdao/blob/main/src/Stream.sol#L218

Tool used

Manual Review

Recommendation

Change from

    function ratePerSecond() public pure returns (uint256) {
        uint256 duration = stopTime() - startTime();
        unchecked {
            return RATE_DECIMALS_MULTIPLIER * tokenAmount() / duration;
        }
    }

to

    function ratePerSecond() public pure returns (uint256) {
        uint256 duration = stopTime() - startTime();
        return RATE_DECIMALS_MULTIPLIER * tokenAmount() / duration;
    }

Duplicate of #19