`_updateReceiverStates` CAN WORK INCORRECTLY WHEN CURRENT RECEIVER AND NEW RECEIVER MATCHES WITH UPDATED DRIP TIMES

[code423n4]

Lines of code

https://github.com/code-423n4/2023-01-drips/blob/main/src/Drips.sol#L919-L920

Vulnerability details

Impact

States can be incorrectly updated.

Proof of Concept

In _updateReceiverStates method of Drips.sol, I noticed a different pattern of Using _addDeltaRange when current receiver and new receiver matches with updated drip times.

To Remove an existing drip, the code used was:

939:    _addDeltaRange(state, start, end, -amtPerSec);

Similarly to Create a new drip, Code used was:

947:    _addDeltaRange(state, start, end, amtPerSec);

So what Basically _addDeltaRange does is it takes Start time, End time and negative amtPerSec to remove the Drip and Positive to add new Drip.

But for one when receiver is just updating the time range, code used is:

919:    _addDeltaRange(state, currStart, newStart, -amtPerSec);
920:    _addDeltaRange(state, currEnd, newEnd, amtPerSec);

Here, We need to remove the old drip and add new drip. So code used should have been what written below where you pass the currStart and currEnd time with Negative amtPerSec to remove the old drip and newStart and newEnd Time with positive amtPerSec to add the new drip rather than currStart, newStart for removing and currEnd, newEnd for adding a drip.

919:    _addDeltaRange(state, currStart, currEnd, -amtPerSec);
920:    _addDeltaRange(state, newStart, newEnd, amtPerSec);

Tools Used

Manual

Recommended Mitigation Steps

Recommending to Update the code to:

File: Drips.sol

919:    _addDeltaRange(state, currStart, currEnd, -amtPerSec);
920:    _addDeltaRange(state, newStart, newEnd, amtPerSec);

Link to Code

[xmxanuel]

An update of a stream(drip) needs to consider the case that the drip has already started.

In this case, removing the old drip and adding the new drip is not possible. The old drip has to stream until block.timestamp and the new config should be applied starting from block. timestamp.

A clever way to achieve this was the existing addDeltaRange method.

Recommendation: Not an issue.

[CodeSandwich]

[dispute validity]

919:    _addDeltaRange(state, currStart, newStart, -amtPerSec);
920:    _addDeltaRange(state, currEnd, newEnd, amtPerSec);

and

919:    _addDeltaRange(state, currStart, currEnd, -amtPerSec);
920:    _addDeltaRange(state, newStart, newEnd, amtPerSec);

have exactly the same effects, see _addDeltaRange implementation. The current implementation has the benefit of being able to catch cases where the start time or the end time aren't moved and there's no need to touch the storage.

[c4-sponsor]

CodeSandwich marked the issue as sponsor disputed

[GalloDaSballo]

Agree with the sponsor, that the math is a linear translation

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory: Invalid