setDrips may distribute the drip too fast if the time hints are not good enough

[code423n4]

Lines of code

https://github.com/code-423n4/2023-01-drips/blob/main/src/DripsHub.sol#L510 https://github.com/code-423n4/2023-01-drips/blob/main/src/Drips.sol#L721-L722 https://github.com/code-423n4/2023-01-drips/blob/main/src/Drips.sol#L649 https://github.com/code-423n4/2023-01-drips/blob/main/src/Drips.sol#L648 https://github.com/code-423n4/2023-01-drips/blob/main/src/Drips.sol#L607-L608

Vulnerability details

Impact

The setDrips() function is used to configure a drip. It can either be withdrawing it, adding a new one, or even managing an existing one by updating the configuration. Internally, it account for the drips that are yet to be distributed to refund them to the dripper, or request additional funds in order to cover each receiver dripping time range. To do the latter, it uses a binary search. This algorithm is often used to find x for which f(x) = y by narrowing the range by two at every iteration, until finding a range that is small enough and satisfying. In this case, it's a bit different: we are trying to find the first timestamp end for which there is enough balance in order to pay for the drip. This is fundamentally different.

Proof of Concept

The issue lies in the fact that this kind of binary search tries to find the very first timestamp that satisfies the expression _isBalanceEnough(balance, configs, configsLen, end) to return the timestamp, and will not try to guess any better choice that would be closer to notEnoughEnd which is the right side of the binary search. That is why if the hints given are not good enough, or not given, then it is going to calculate for some extreme cases a low resolution end timestamp for which the provided balance is enough to drip for this whole period.

Because of this issue, if the end timestamp is way too devaluated compared to the balance that was planned to drip the receivers, then the drip balance is going to be too big compared to the maxEnd. Resulting in a shorter period than expected.

Raising it as medium because of the assumptions written in the comments:

/// @param maxEndHint2 An optional parameter allowing gas optimization, pass `0` to ignore it.
/// The second hint for finding the maximum end time, see `maxEndHint1` docs for more details.

which imply that the optional hint parameters can be used and will only alter the gas usage while the consequences could be worse.

Tools Used

Manual inspection

Recommended Mitigation Steps

It seems reasonable to run the binary search using the hints to find the closest amount of tokens on the lower bound. We can constrain it to not have a too-far higher bound by using a range under which the tolerance is okay and that returns the maxEnd for this value. Define an acceptable range under the enoughEnd value so that _isBalanceEnough() is still satisfied in the binary search while loop without returning degraded results.

[xmxanuel]

I can't follow the described problem completely here but I reviewed the related PR carefully.

We are not trying to find the first timestamp where isBalanceEnough is true. We are trying to find the turning point where it is true but not for block.timstamp+1 anymore.

The hints can be incorrect and they will not break anything. It is not required that the necessary represent a range. (obviously, the caller should provide a range).

They could be just two hints for the lower bound or the higher bound. The order also doesn't matter.

[CodeSandwich]

[dispute validity] What Manuel said. If the bug is true, it should be easy to construct a PoC, can you do it?

[c4-sponsor]

CodeSandwich marked the issue as sponsor disputed

[GalloDaSballo]

@iFrostizz Please send us a POC for this one if you believe it's valid

[GalloDaSballo]

With the information I have, I believe that inputting an incorrect hint will simply cause further gas costs for operations

In the best case we could award this as Low Severity, however, due to lack of info, am closing for lack of Proof.

Am going to ask @iFrostizz to provide an instance of the issue if they wish to have this finding awarded

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory: Insufficient proof