_squeezeDrips() passed the amount argument in place of amtPerSec for the _addDeltaRange, causing either underflow or the sender losing lots of fund!

[code423n4]

Lines of code

https://github.com/code-423n4/2023-01-drips/blob/9fd776b50f4be23ca038b1d0426e63a69c7a511d/src/Drips.sol#L366

Vulnerability details

Impact

Detailed description of the impact of this finding.

_squeezeDrips() passes the amount argument in place of amtPerSec for its callee _addDeltaRange in the following line https://github.com/code-423n4/2023-01-drips/blob/9fd776b50f4be23ca038b1d0426e63a69c7a511d/src/Drips.sol#L366-L1053

 _addDeltaRange(state, cycleStart, cycleStart + 1, -int256(amt * _AMT_PER_SEC_MULTIPLIER));

The last argument should be the dripping speed, not dripping amount. Because of this bug, it will cause either underflow, which reverts the _squeezeDrips() function, or much higher future dripping speed and thus the losing of fund for the sender.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

We show why using the wrong argument for _addDeltaRange will cause underflow/revert, or the losing of fund for the sender below:

1) Alice calls DripsHub.squeezeDrips(), which calls Drips._squeezeDrips().

2) Drips._squeezeDrips() calls _squeezeDripsResult() to calculate the squeezed amount, amt.

(amt, squeezedNum, squeezedRevIdxs, historyHashes, currCycleConfigs) =
            _squeezeDripsResult(userId, assetId, senderId, historyHash, dripsHistory);

3) Suppose amt = 3600, and cycleSecs_ = 3600, that is each cycle is 1 hour.

4) Drips._squeezeDrips() will call _addDeltaRange() to adjust the dripping speed at both ends: cycleStart and cycleStart+1. Note a negative speed is used here since we already squeezed the amount out, so a negative speed need to take effect for the current cycle.

     uint32 cycleStart = _currCycleStart();
        _addDeltaRange(state, cycleStart, cycleStart + 1, -int256(amt * _AMT_PER_SEC_MULTIPLIER));

5) Unfortunately, instead of using the dripping speed of amt/cycleSecs_ = 3600/3600 = 1/sec, it uses amt as the dripping speed (with a multiplication factor of _AMT_PER_SEC_MULTIPLIER). As a result, the dripping speed is mistakenly 3600X of the correct speed argument that should be passed to addDeltaRange() - passing (-3600*_AMT_PER_SEC_MULTIPLIER) instead of (-1*_AMT_PER_SEC_MULTIPLIER) for our example.

6) _addDeltaRange() calls _addDelta() at both ends to adjust the speed.

function _addDeltaRange(DripsState storage state, uint32 start, uint32 end, int256 amtPerSec)
        private
    {
        // slither-disable-next-line incorrect-equality,timestamp
        if (start == end) {
            return;
        }
        mapping(uint32 => AmtDelta) storage amtDeltas = state.amtDeltas;
        _addDelta(amtDeltas, start, amtPerSec);
        _addDelta(amtDeltas, end, -amtPerSec);
    }

7) The first _addDelta() will be called, since we had a negative speed, both fullCycle and nextCycle will take negative values below. Depending on which value is bigger,

amtDelta.thisCycle += int128(fullCycle - nextCycle);

might underflow, since we are mistakenly subtracting a much larger wrong number (3600X of it is supposed to be) from amtDelta.thisCycle (which might not be that big) instead of the original correct number. Similarly,

 amtDelta.nextCycle += int128(nextCycle);

might also underflow as well due to a much larger number of nextCycle (3600X of it is supposed to be). If these two underflows will not happen, then the second _addDelta() will result the losing of fund, which we show below.

function _addDelta(
        mapping(uint32 => AmtDelta) storage amtDeltas,
        uint256 timestamp,
        int256 amtPerSec
    ) private {
        unchecked {
            // In order to set a delta on a specific timestamp it must be introduced in two cycles.
            // These formulas follow the logic from `_drippedAmt`, see it for more details.
            int256 amtPerSecMultiplier = int256(_AMT_PER_SEC_MULTIPLIER);
            int256 fullCycle = (int256(uint256(_cycleSecs)) * amtPerSec) / amtPerSecMultiplier;
            // slither-disable-next-line weak-prng
            int256 nextCycle = (int256(timestamp % _cycleSecs) * amtPerSec) / amtPerSecMultiplier;
            AmtDelta storage amtDelta = amtDeltas[_cycleOf(uint32(timestamp))];
            // Any over- or under-flows are fine, they're guaranteed to be fixed by a matching
            // under- or over-flow from the other call to `_addDelta` made by `_addDeltaRange`.
            // This is because the total balance of `Drips` can never exceed `type(int128).max`,
            // so in the end no amtDelta can have delta higher than `type(int128).max`.
            amtDelta.thisCycle += int128(fullCycle - nextCycle);
            amtDelta.nextCycle += int128(nextCycle);
        }
    }

8) If there is no underflow, then the second _addDelta() will be executed: https://github.com/code-423n4/2023-01-drips/blob/9fd776b50f4be23ca038b1d0426e63a69c7a511d/src/Drips.sol#L1029:

       _addDelta(amtDeltas, end, -amtPerSec);

9) Here we are adjusting with a positive speed (since the original speed argument is negative). Since -amtPerSec is 3600X is what is supposed to be. _addDelta() will cause the increase of 3600X speed adjustments of it is supposed to be for both amtDelta.thisCycle and amtDelta.nextCycle.

10) As a result, the receiver will steal a lot of funding from the sender, the sender is losing fund!

Tools Used

Remix

Recommended Mitigation Steps

We need to pass the correct argument to as follows:

     uint32 cycleStart = _currCycleStart();
-        _addDeltaRange(state, cycleStart, cycleStart + 1, -int256(amt * _AMT_PER_SEC_MULTIPLIER));
+        _addDeltaRange(state, cycleStart, cycleStart + 1, -int256(amt * _AMT_PER_SEC_MULTIPLIER/_cycleSecs));

[CodeSandwich]

[dispute validity] The current behavior is correct. Calling _addDeltaRange(state, cycleStart, cycleStart + 1, -int256(amt * _AMT_PER_SEC_MULTIPLIER)); does apply dripping at a per-second rate of -amt, but it does so on a time range of cycleStart, cycleStart + 1,, which is 1 second long, so the "undripped" amount is amt. Applying your fix breaks 18 tests.

[c4-sponsor]

CodeSandwich marked the issue as sponsor disputed

[xmxanuel]

I agree with @CodeSandwich

[GalloDaSballo]

Looks to me like this is already accounted here: https://github.com/code-423n4/2023-01-drips/blob/9fd776b50f4be23ca038b1d0426e63a69c7a511d/src/Drips.sol#L1045-L1046

            int256 fullCycle = (int256(uint256(_cycleSecs)) * amtPerSec) / amtPerSecMultiplier;

Feel free to follow up with an instance of the issue @chaduke3730 but for now am closing as invalid

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory: Invalid