095.md

Ripe Macaroon Jellyfish

Medium

Precision loss was not accounted for in StreamEscrow::cancelStream function causing stream creator to receive lesser refunds

Summary

When streams are created with an uneven division of msg.value by streamLengthInTicks, the leftover remainder is sent to the treasury in createStream, but this remainder is not accounted for when calculating refunds in cancelStream. This leads to lesser refund amount being sent to the stream creator.

In createStream function, ethPerTick was calculated thus: uint128 ethPerTick = toUint128(msg.value / streamLengthInTicks); The possible precision loss above was accounted for thus:

   uint256 remainder = msg.value % streamLengthInTicks;
   sendETHToTreasury(remainder);

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);
    }

The bug occurs in the cancelStream function.

When a stream creator calls cancelStream, the amount to refund to them is calculated without accounting for the possible remainder that occured when creating stream. uint256 amountToRefund = stream.ethPerTick * ticksLeft;

Root Cause

In createStream:

ETH is divided evenly over the streamLengthInTicks:

    uint128 ethPerTick = toUint128(msg.value / streamLengthInTicks);

The remainder was sent to the treasury:

   uint256 remainder = msg.value % streamLengthInTicks;
   sendETHToTreasury(remainder);

However, in cancelStream: The refund calculation does not account for the remainder that was sent to the treasury:

    uint256 amountToRefund = stream.ethPerTick * ticksLeft;

This calculation assumes the entire msg.value was distributed across ticks, leading to an under-refund.

Internal pre-conditions

No response

External pre-conditions

No response

Attack Path

No response

Impact

Financial Loss: Stream owners may receive less ETH than expected when canceling a stream, due to the untracked remainder sent to the treasury.

PoC

Assume stream creator called createStream with 15 ETH and streamLengthInTicks is 6.

ethPerTick = 15 / 6 = 2 remainder = 3

The creator calls cancelStream after 3 ticks, refund is calculated as: uint256 amountToRefund = stream.ethPerTick * ticksLeft; amountToRefund = 2 * 3 = 6 ETH

Calculation without precision loss ethPerTick = (15 * 10 ** 18) / 6 = 25e17

Amount to Refund after 3 ticks: amountToRefund = 25e17 * 3 = 75e17 ie 7.5 ETH

In this case the stream creator loses 1.5 ETH when they cancel their stream.

Mitigation

Consider refactoring the ethPerTick calculation:

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);
+       uint128 ethPerTick = toUint128((msg.value * 10 ** 18) / 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);
    }