hyh - ethRecipient can cancel and create streams with lagged currentTick, can brick StreamEscrow

[sherlock-admin4]

hyh

Medium

ethRecipient can cancel and create streams with lagged currentTick, can brick StreamEscrow

Summary

1. ethRecipient turning malicious can reenter forwardAll() when ethStreamedPerTick for the current tick was already paid, but the tick counter isn't advanced yet. This allows it receiving extra tick of streaming

2. Also, it can reenter createStream() multiple times, pushing ethStreamEndingAtTick[currentTick + streamLengthInTicks] arbitrary high and bricking StreamEscrow in streamLengthInTicks ticks

Root Cause

Initially ethRecipient is set to DAO address, but later can be updated to be any independent contract or EOA. The first part, immediately adding extra tick to the streaming of the all DAO owned nouns with streams, is beneficial to the DAO as it is, the second part, driving ethStreamEndingAtTick[currentTick + streamLengthInTicks] high, doesn't look to have any monetary benefits, but can be used as some type of griefing.

1. The first part is possible as in forwardAll() outbound transfer happens before state update:

StreamEscrow.sol#L136-L149

>>  function forwardAll() public {
        // silently fail if at least a day hasn't passed. this is in order not to revert auction house.
        if (block.timestamp < lastForwardTimestamp + minimumTickDuration) {
            return;
        }

        lastForwardTimestamp = toUint48(block.timestamp);

>>      sendETHToTreasury(ethStreamedPerTick);

>>      (uint32 newTick, uint128 ethPerTickEnded) = increaseTicksAndFinishStreams();

        emit StreamsForwarded(newTick, ethPerTickEnded, ethStreamedPerTick, lastForwardTimestamp);
    }

forwardAll() is public and caller can manipulate the amount of gas being forwarded to ethRecipient:

StreamEscrow.sol#L303-L309

    function sendETHToTreasury(uint256 amount) internal {
        if (amount > 0) {
>>          (bool sent, ) = ethRecipient.call{ value: amount }('');
            require(sent, 'failed to send eth');
            emit ETHStreamedToDAO(amount);
        }
    }

This way ethRecipient can call forwardAll() with an arbitrary high amount of gas, providing the way to perform any operations desired.

E.g. all the streams for the Nouns with active streams ethRecipient owns (or controls via other accounts) can be repeated for the current tick via cancelStream() as it can be called from the forwardAll(), so ticksLeft there be bloated by 1:

StreamEscrow.sol#L167-L182

    function cancelStream(uint256 nounId) public {
        require(isStreamActive(nounId), 'stream not active');

        // transfer noun to treasury
        nounsToken.transferFrom(msg.sender, nounsRecipient, nounId);

        // cancel stream
        streams[nounId].canceled = true;
        Stream memory stream = streams[nounId];
        ethStreamedPerTick -= stream.ethPerTick;
        ethStreamEndingAtTick[stream.lastTick] -= stream.ethPerTick;

        // calculate how much needs to be refunded
>>      uint256 ticksLeft = stream.lastTick - currentTick;
        uint256 amountToRefund = stream.ethPerTick * ticksLeft;
        (bool sent, ) = msg.sender.call{ value: amountToRefund }('');

This way ethRecipient receives extra tick for all the nouns they owned and cancelled streams for: cancelStream() is done once for each noun, but ticksLeft is greater than its correct value by 1

2. Also, if ethRecipient be able to create streams (added to allowedToCreateStream in addition to the auction house contract) at some point then createStream() can repeatedly reenter itself, arbitrary inflating ethStreamEndingAtTick, essentially breaking StreamEscrow logic:

StreamEscrow.sol#L112-L130

    function createStream(uint256 nounId, uint16 streamLengthInTicks) public payable {
>>      require(allowedToCreateStream[msg.sender], 'not allowed');
>>      require(isApprovedOrOwner(msg.sender, nounId), 'only noun owner or approved');
>>      require(!isStreamActive(nounId), 'stream active');

        // register new stream
        uint128 ethPerTick = toUint128(msg.value / streamLengthInTicks);
        uint32 streamLastTick = currentTick + streamLengthInTicks;
>>      ethStreamEndingAtTick[streamLastTick] += ethPerTick;

        // the remainder is immediately streamed to the DAO
        uint256 remainder = msg.value % streamLengthInTicks;
>>      sendETHToTreasury(remainder);

        uint128 newEthStreamedPerTick = ethStreamedPerTick + ethPerTick;
        ethStreamedPerTick = newEthStreamedPerTick;
        streams[nounId] = Stream({ ethPerTick: ethPerTick, canceled: false, lastTick: streamLastTick });
        emit StreamCreated(nounId, msg.value, streamLengthInTicks, ethPerTick, newEthStreamedPerTick, streamLastTick);
    }

After streamLengthInTicks - 1 ticks the tick cannot be advanced to the tick number currentTick + streamLengthInTicks as ethStreamEndingAtTick[currentTick + streamLengthInTicks] > ethStreamedPerTick:

StreamEscrow.sol#L311-L317

    function increaseTicksAndFinishStreams() internal returns (uint32 newTick, uint128 ethPerTickEnding) {
        newTick = ++currentTick;
        ethPerTickEnding = ethStreamEndingAtTick[newTick];
        if (ethPerTickEnding > 0) {
>>          ethStreamedPerTick -= ethPerTickEnding;
        }
    }

This is not monetary beneficial for DAO, but can be used as a part of wider griefing attack

Internal pre-conditions

1. DAO majority driven ethRecipient is set to game streaming and receive delayed revenue early

2. ethRecipient is manipulated as a part of some griefing attack, seeking to damage existing infrastructure

External pre-conditions

None, the access rights are provided by default

Attack Path

1. Call forwardAll(), reenter with cancelStream() / cancelStreams() on receiving ether once

2. Call createStream(), reenter with createStream() on receiving ether multiple times, needed to drive ethStreamEndingAtTick[currentTick + streamLengthInTicks] high enough

Impact

1. ethRecipient receives ethStreamedPerTick * nounsWithStreamsOwned extra ETH

2. tick will not be able to be advanced to currentTick + streamLengthInTicks, so streaming and auctioning (due to integration) are stopped there

PoC

1. ethRecipient updates to enter cancelStreams() once on receiving ether, calls forwardAll() with enough gas

2. ethRecipient updates to enter createStream() several times in total, high enough to exceed future ethStreamedPerTick, on receiving ether, calls createStream() with enough gas

Mitigation

Consider making sendETHToTreasury() the last operation in forwardAll() and createStream(), e.g.:

StreamEscrow.sol#L136-L149

    function forwardAll() public {
        // silently fail if at least a day hasn't passed. this is in order not to revert auction house.
        if (block.timestamp < lastForwardTimestamp + minimumTickDuration) {
            return;
        }

        lastForwardTimestamp = toUint48(block.timestamp);

-       sendETHToTreasury(ethStreamedPerTick);

        (uint32 newTick, uint128 ethPerTickEnded) = increaseTicksAndFinishStreams();

+       sendETHToTreasury(ethStreamedPerTick);

        emit StreamsForwarded(newTick, ethPerTickEnded, ethStreamedPerTick, lastForwardTimestamp);
    }

Also, since cancelStream() and createStream() are infrequent operations, both can be made nonReentrant

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits:
nounsDAO/nouns-monorepo#872