int128 cast underflow in `_receiveDripsResult()`

[code423n4]

Lines of code

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

Vulnerability details

Impact

In _receiveDripsResult(), the type cast of uint128 could underflow, and result in wrong receivedAmt. The impacts could be:

• wrong amount being transferred to users and drain the protocol fund
• inaccurate transfer amount, some users lose fund and some receive undeserved fund

Proof of Concept

amtDelta.thisCycle and amtDelta.nextCycle can both be negative in function _addDelta(), if amtPerSec is less than 0:

1036:     function _addDelta(
1037:         mapping(uint32 => AmtDelta) storage amtDeltas,
1038:         uint256 timestamp,
1039:         int256 amtPerSec
1040:     ) private {
1041:         unchecked {

1044:             int256 amtPerSecMultiplier = int256(_AMT_PER_SEC_MULTIPLIER);
1045:             int256 fullCycle = (int256(uint256(_cycleSecs)) * amtPerSec) / amtPerSecMultiplier;

1047:             int256 nextCycle = (int256(timestamp % _cycleSecs) * amtPerSec) / amtPerSecMultiplier;
1048:             AmtDelta storage amtDelta = amtDeltas[_cycleOf(uint32(timestamp))];

1053:             amtDelta.thisCycle += int128(fullCycle - nextCycle);
1054:             amtDelta.nextCycle += int128(nextCycle);
1055:         }
1056:     }
1057: 

Imagine there is only 1 cycle to iterate in the for loop, and the thisCycle is -10,000e18, less than 0. Then in line 289, the reveiveAmt will be increased by uint128(-10,000e18) = 340282366920938453463374607431768211456.

File: src/Drips.sol
270:     function _receiveDripsResult(uint256 userId, uint256 assetId, uint32 maxCycles)
271:         internal
272:         view
273:         returns (
274:             uint128 receivedAmt,
275:             uint32 receivableCycles,
276:             uint32 fromCycle,
277:             uint32 toCycle,
278:             int128 amtPerCycle
279:         )
280:     {

287:         for (uint32 cycle = fromCycle; cycle < toCycle; cycle++) {
288:             amtPerCycle += state.amtDeltas[cycle].thisCycle;
289:             receivedAmt += uint128(amtPerCycle);
290:             amtPerCycle += state.amtDeltas[cycle].nextCycle;
291:         }

Then when receiveDrips() is called, the wrong amount of receivedAmt of 340282366920938453463374607431768211456 would be transferred, if the balance is enough, the fund in the protocol could be drained.

File: src/DripsHub.sol
238:     function receiveDrips(uint256 userId, IERC20 erc20, uint32 maxCycles)
239:         public
240:         whenNotPaused
241:         returns (uint128 receivedAmt)
242:     {
243:         uint256 assetId = _assetId(erc20);
244:         receivedAmt = Drips._receiveDrips(userId, assetId, maxCycles);
245:         if (receivedAmt > 0) {
246:             Splits._addSplittable(userId, assetId, receivedAmt);
247:         }
248:     }

File: src/Drips.sol
233:     function _receiveDrips(uint256 userId, uint256 assetId, uint32 maxCycles)
234:         internal
235:         returns (uint128 receivedAmt)
236:     {

241:         (receivedAmt, receivableCycles, fromCycle, toCycle, finalAmtPerCycle) =
242:             _receiveDripsResult(userId, assetId, maxCycles);

Or in other cases, the amount returned from _receiveDripsResult() will be inaccurate, the actual transferred amount will be wrong, some users could receive undeserved fund, and on the other hand, some users could loss fund due to the wrong amount.

Tools Used

Manual analysis.

Recommended Mitigation Steps

Calculate the sum of all the amtDeltas and add to receivedAmt only if the total is greater than 0:

File: src/Drips.sol
270:     function _receiveDripsResult(uint256 userId, uint256 assetId, uint32 maxCycles)
271:         internal
272:         view
273:         returns (
274:             uint128 receivedAmt,
275:             uint32 receivableCycles,
276:             uint32 fromCycle,
277:             uint32 toCycle,
278:             int128 amtPerCycle
279:         )
280:     {

+            int128 _recvAmt;
287:         for (uint32 cycle = fromCycle; cycle < toCycle; cycle++) {
288:             amtPerCycle += state.amtDeltas[cycle].thisCycle;
289:             _recvAmt += amtPerCycle;
290:             amtPerCycle += state.amtDeltas[cycle].nextCycle;
291:         }
+            if (_recvAmt > 0) {
+               receivedAmt += uint128(_recvAmt);
+            }

[c4-judge]

GalloDaSballo marked the issue as primary issue

[CodeSandwich]

[dispute validity] This is an expected and documented behavior which must be accounted for when using Drips contract in a protocol. It simplifies the code and reduces gas cost on overflow checks. See the docs of Drips:

/// @notice Drips can keep track of at most `type(int128).max`
/// which is `2 ^ 127 - 1` units of each asset.
/// It's up to the caller to guarantee that this limit is never exceeded,
/// failing to do so may result in a total protocol collapse.

The attack you've described relies on amtDelta.thisCycle + amtDelta.nextCycle overflowing int128, but it's not possible without putting more than type(int128).max tokens into the protocol. While it's reproduceable in the test environment by misusing the contract, it's impossible to pull of in the real world, because DripsHub tracks the total amount of each ERC-20 in the protocol:

[Drips.sol]
uint256 internal constant _MAX_TOTAL_DRIPS_BALANCE = uint128(type(int128).max);

[DripsHub.sol]
uint256 public constant MAX_TOTAL_BALANCE = _MAX_TOTAL_DRIPS_BALANCE;
...
require(totalBalances[erc20] + amt <= MAX_TOTAL_BALANCE, "Total balance too high");

[c4-sponsor]

CodeSandwich marked the issue as sponsor disputed

[GalloDaSballo]

Closing per the Sponsor's dispute

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory: Invalid