hansfriese - Protocol can become useless by malicious attackers through front-running

[sherlock-admin]

hansfriese

high

Protocol can become useless by malicious attackers through front-running

Summary

A malicious attacker can front-run createStream() and deploy the same stream implementation before the protocol and this prevents the stream's initialization, which makes the protocol useless.

Vulnerability Detail

The createStream() function at StreamFactory.sol #L184 clones the stream implementation with data and salt based on the input arguments. The problem is the stream initialization is called after calling cloneDeterministic().

function createStream(
    address payer,
    address recipient,
    uint256 tokenAmount,
    address tokenAddress,
    uint256 startTime,
    uint256 stopTime,
    uint8 nonce
) public returns (address stream) {
    // These input checks are here rather than in Stream because these parameters are written
    // using clone-with-immutable-args, meaning they are already set when Stream is created and can't be
    // verified there. The main benefit of this approach is significant gas savings.
    if (payer == address(0)) revert PayerIsAddressZero();
    if (recipient == address(0)) revert RecipientIsAddressZero();
    if (tokenAmount == 0) revert TokenAmountIsZero();
    if (stopTime <= startTime) revert DurationMustBePositive();
    if (tokenAmount < stopTime - startTime) revert TokenAmountLessThanDuration();

    stream = streamImplementation.cloneDeterministic(
        encodeData(payer, recipient, tokenAmount, tokenAddress, startTime, stopTime),
        salt(
            msg.sender, payer, recipient, tokenAmount, tokenAddress, startTime, stopTime, nonce
        )
    );
    IStream(stream).initialize();

    emit StreamCreated(
        msg.sender, payer, recipient, tokenAmount, tokenAddress, startTime, stopTime, stream
        );
}

Because create2() reverts if it is called with the same arguments, an attacker can front-run and deploys the exact same implementation before the protocol to make the call to cloneDeterministic() revert. As a result, stream initialization will not be called and remainingBalance() will stay zero. This affects the whole streaming logic and the protocol becomes useless. Furthermore, the streams are likely to be created based on DAO proposals which will go through some time period. So this attack can make the whole effort useless and selectively prevent creating an unfavorable stream.

Impact

The protocol becomes useless because its core functionalities will not work. Although I could not find any lock of funds from this exploit (because funds can be claimed via cancel() anyway), I set the impact level to High because it makes the whole protocol useless.

Code Snippet

https://github.com/sherlock-audit/2022-11-nounsdao/blob/main/src/StreamFactory.sol#L202

Tool used

Manual Review

Recommendation

It is not desirable doing initialization things out of contract from the factory level. I recommend adding an initialized flag in the stream contract and to check that before any user interactions. Of course, it should be ensured that initialization is done at most once.

[davidbrai]

The factory uses msg.sender as part of the salt, so front running doesn't work

[hansfriese]

Escalate for 1 USDC

Although msg.sender might be not visible directly the result of salt will still be observable in the public mempool and an attacker (or a possible business rival) can effectively make the protocol useless.

It is often regarded that front-running is not a problem because the same contract will be deployed all the time as mentioned in StackOverflow. But the problem in the current implementation is that the initialization is done after that which prevents the initialization of the contract.

[sherlock-admin]

Escalate for 1 USDC

Although msg.sender might be not visible directly the result of salt will still be observable in the public mempool and an attacker (or a possible business rival) can effectively make the protocol useless.

It is often regarded that front-running is not a problem because the same contract will be deployed all the time as mentioned in StackOverflow. But the problem in the current implementation is that the initialization is done after that which prevents the initialization of the contract.

You've created a valid escalation for 1 USDC!

To remove the escalation from consideration: Delete your comment. To change the amount you've staked on this escalation: Edit your comment (do not create a new comment).

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[hrishibhat]

Escalation rejected

It is not possible to front-run a valid stream contract deployment as it can be created through StreamFactory only. Any other stream created even with the same salt and other parameters gets a different address. Hence this attack would not be possible.

There is also a test case for the same.

https://github.com/sherlock-audit/2022-11-nounsdao/blob/5def6ce65aeae7c55c66bbeb0e5f92f2ad169211/test/StreamFactory.t.sol#L125

[sherlock-admin]

Escalation rejected

It is not possible to front-run a valid stream contract deployment as it can be created through StreamFactory only. Any other stream created even with the same salt and other parameters gets a different address. Hence this attack would not be possible.

There is also a test case for the same.

https://github.com/sherlock-audit/2022-11-nounsdao/blob/5def6ce65aeae7c55c66bbeb0e5f92f2ad169211/test/StreamFactory.t.sol#L125

This issue's escalations have been rejected!

Watsons who escalated this issue will have their escalation amount deducted from their next payout.