adriro - `balanceOf` will return an incorrect amount if stream is unfunded or partially funded

[sherlock-admin]

adriro

medium

balanceOf will return an incorrect amount if stream is unfunded or partially funded

Summary

The function balanceOf present in the Stream will return a technically incorrect amount in case the stream hasn't been funded yet or is partially funded.

Vulnerability Detail

The balanceOf function is used to report the amount of tokens available to be withdrawn by each party (payer or recipient). For the recipient, this is the amount available at the current block timestamp (given the rate per second) minus the amount already withdrawn. For the payer, it's the remainingBalance minus the amount that corresponds to the recipient.

The main issue is that, for both cases, the calculation doesn't take into account how many tokens are actually in the contract, since the stream can be created without actually funding it, or can be created and then be partially funded too.

Impact

For the payer case, the impact isn't big since the balanceOf function isn't used within the contract (it's only used internally for the recipient address), and will be mostly informational to the outside.

In the recipient case, the balanceOf function is used in the withdraw function, but the safeTransfer will eventually fail to transfer the tokens (assuming a well behaved ERC20 implementation that won't silently fail) if those aren't available in the contract.

However other parts of the protocol (or other protocols) that integrate with this particular function can eventually receive an inaccurate value.

Since the comment attached to the function states the following, the implementation isn't technically correct:

@notice Returns the available funds to withdraw.

Code Snippet

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

function balanceOf(address who) public view returns (uint256) {
    uint256 recipientBalance = _recipientBalance();

    if (who == recipient()) return recipientBalance;
    if (who == payer()) {
        // This is safe because it should always be the case that:
        // remainingBalance >= recipientBalance.
        unchecked {
            return remainingBalance - recipientBalance;
        }
    }
    return 0;
}

Test

The following test demonstrates the issue in a simple scenario.

// SPDX-License-Identifier: GPL-3.0
pragma solidity ^0.8.17;

import "forge-std/Test.sol";
import { ERC20Mock } from "openzeppelin-contracts/mocks/ERC20Mock.sol";
import { StreamFactory } from "../src/StreamFactory.sol";
import { Stream } from "../src/Stream.sol";

contract AuditTest is Test {
    ERC20Mock token;
    Stream s;
    StreamFactory factory;

    function setUp() public virtual {
        token = new ERC20Mock("Mock Token", "MOCK", address(1), 0);
        factory = new StreamFactory(address(new Stream()));
    }

    function test_Stream_balanceOf_PartiallyFunded() public {
        address payer = makeAddr("payer");
        address recipient = makeAddr("recipient");
        uint256 amount = 1e6;

        token.mint(payer, amount);

        vm.startPrank(payer);
        s = Stream(
            factory.createStream(recipient, amount, address(token), block.timestamp, block.timestamp + 1 minutes)
        );
        // stream is partially funded
        token.transfer(address(s), 1);
        vm.stopPrank();

        // Stream has only 1 token while balanceOf(payer) reports 1e6
        assertEq(token.balanceOf(address(s)), 1);
        assertEq(s.balanceOf(payer), 1e6);
    }
}

Tool used

Manual Review

Recommendation

Take into account the amount of tokens that are actually available in the contract to cap the return amounts, favoring first the recipient (e.g. if both payer and recipient are currently entitled to 100 tokens and the contract has only 100 then recipient gets those 100 and payer 0).