Skip to content

Latest commit

 

History

History
84 lines (60 loc) · 3.61 KB

055.md

File metadata and controls

84 lines (60 loc) · 3.61 KB

Future Honeysuckle Reindeer

Medium

Canceling and fast-forwarding a stream could be inaccurate, as forwardAll call is not enforced

Summary

Canceling or fast-forwarding via the functions in StreamEscrow.sol contract can be done by any noun owner. However, its possible that the currentTick may be in an outdated state which could lead them in receiving more "refund" or forwarding more ticks than they should.

Root Cause

forwardAll() function, which is responsible for moving the currentTick is not enforced in the fastForwardStream() and cancelStream() function calls.

Pre-conditions

  • 24hrs since last forwardAll() call has passed, but its not called, yet.

Flow Path

  1. Noun owner decides to cancel a stream. -> calls cancelStream() / This is supposed to return him all the ETH thats still not streamed to the treasury. /
  2. Noun owner gets amountToRefund = ethPerTick * ticksLeft, but as tickLeft = lastTick - currentTick and the currentTick not being accurate regarding the state, when the 24hrs since last forwardAll() has passed but forwardAll() is not called, we get to cancel 1 tick more, which would give more refund amount.
    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}("");
        require(sent, "failed to send eth");

        emit StreamCanceled(nounId, amountToRefund, ethStreamedPerTick);
    }

Let's also check the forwardAll() function itself to see that its supposed to update the currentTick

    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);
    }
    function increaseTicksAndFinishStreams() internal returns (uint32 newTick, uint128 ethPerTickEnding) {
@>>     newTick = ++currentTick; // this adds 1 to the current tick

        ethPerTickEnding = ethStreamEndingAtTick[newTick];
        if (ethPerTickEnding > 0) {
            ethStreamedPerTick -= ethPerTickEnding;
        }
    }

Impact

In the current setup where we have 1500 ticks (reference), the least impact would be 0.067% more ETH withdrawn from the noun owner or streamed to the treasury, which classifies as Medium severity issue.

1 tick difference is: 100% / 1500 = ~0,067%

This could happen in situations of frontrunning, or just normal usage.

Mitigation

Call forwardAll() in both fastForwardStream() and cancelStream() functions, to ensure state is accurate.