Funds can be drained from `DripsHub` in the event of a negative drip `amtPerCycle`

[code423n4]

Lines of code

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

Vulnerability details

Yet unreceived drips can be collected by a user via the DripsHub.receiveDrips function. It is only ever possible to receive drips beginning with the drip starting at nextReceivableCycle until either maxCycles or the last cycle.

The amount to be dripped is calculated by iterating over all those cycles and summing up the delta amounts (represented by the int128 values in amtDeltas[cycle].thisCycle and amtDeltas[cycle].nextCycle). This accumulator is assumed to always be positive. Otherwise, it would mean that a user loses token amounts instead of having tokens dripped to them.

However, in the (unlikely) event that a negative amtDeltas[cycle].thisCycle value is present in the cycles to be received, the cast from int128 to uint128 in L289 will result in a huge positive value which is returned to the DripsHub.receiveDrips function. This can be used to drain all funds from the DripsHub contract.

Impact

All funds from the DripsHub for a given asset can be drained.

Proof of Concept

DripsHub.sol#L244

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: }

Drips.sol#L242

233: function _receiveDrips(uint256 userId, uint256 assetId, uint32 maxCycles)
234:     internal
235:     returns (uint128 receivedAmt)
236: {
237:     uint32 receivableCycles;
238:     uint32 fromCycle;
239:     uint32 toCycle;
240:     int128 finalAmtPerCycle;
241:     (receivedAmt, receivableCycles, fromCycle, toCycle, finalAmtPerCycle) =
242:         _receiveDripsResult(userId, assetId, maxCycles);
...
243:     // [...]
257: }

Drips.sol#L289

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: {
281:     (fromCycle, toCycle) = _receivableDripsCyclesRange(userId, assetId);
282:     if (toCycle - fromCycle > maxCycles) {
283:         receivableCycles = toCycle - fromCycle - maxCycles;
284:         toCycle -= receivableCycles;
285:     }
286:     DripsState storage state = _dripsStorage().states[assetId][userId];
287:     for (uint32 cycle = fromCycle; cycle < toCycle; cycle++) {
288:         amtPerCycle += state.amtDeltas[cycle].thisCycle;
289:         receivedAmt += uint128(amtPerCycle); // @audit-info Potential unsafe cast from int128 to uint128
290:         amtPerCycle += state.amtDeltas[cycle].nextCycle;
291:     }
292: }

Consider the following attack scenario:

1. Attacker figured out an edge case that leads to a negative state.amtDeltas[cycle].thisCycle value within the cycles to be received
2. Attacker calls DripsHub.receiveDrips with an attacker controllable userId, erc20 asset to drain funds from and a maxCycles value which is appropriately large to iterate only over the cycle(s), which leads to the negative amtPerCycle value
3. Negative amtPerCycle value is unsafely cast to uint128 in L289 and receivedAmount is set to a huge positive value
4. Large positive receivedAmt value is returned to the DripsHub.receiveDrips function and is added to the splittable balance of the attacker-controlled userId
5. Configure split for userId to split the huge and likely too large token amount value into smaller chunks which can be collected and ultimately withdrawn (capped by the amount of escrowed ERC-20 tokens in the DripsHub contract)
6. Split
7. Collect
8. Profit!

Multiple factors contribute to the chosen severity of this issue:

1. All user funds can be drained from the DripsHub contract
2. Due to the complexity of the Drips logic, it is non-trivial to figure out all edge cases. An overlooked edge case can lead to a negative thisCycle value
3. The vulnerability can be easily mitigated by appropriate assertions

Tools Used

Manual review

Recommended mitigation steps

Consider adding additional security mechanisms to safeguard against overflows.

For example, add an assertion in Drips._receiveDripsResult to check that amtPerCycle is not negative:

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

[GalloDaSballo]

Would have liked a coded POC to show a scenario in which the execution does happen

[xmxanuel]

I agree that adding this require or assert check could prevent a potential attack scenario.

The Drips contracts are built on the assumption that the value will never be negative.

It would be some kind of sanity-check for an unknown edge case an attacker could exploit.

I think we should consider adding this check as a sanity check.

However, it is hard to judge the severity because it is built on the assumption of an unkown edge case which might also not exist.

[CodeSandwich]

[disagree with severity: QA] If a bug described here actually occurs, sanity checks probably won't help, it means that the state is corrupted and the protocol is completely broken. I'm not sure if checks like this are worth the gas, there are many places where things must fit perfectly and many edge cases that can't be covered with simple assertions. I'm not sure if the mitigation is worth adding.

[c4-sponsor]

CodeSandwich marked the issue as disagree with severity

[GalloDaSballo]

@berndartmueller can you come up with an instance of the invariant being broken?

[berndartmueller]

@GalloDaSballo Unfortunately, no. I submitted this finding to raise awareness that if there’s a yet unknown way to break the invariant, it’s catastrophic. But it can be prevented by adding appropriate assertions

[GalloDaSballo]

@GalloDaSballo Unfortunately, no. I submitted this finding to raise awareness that if there’s a yet unknown way to break the invariant, it’s catastrophic. But it can be prevented by adding appropriate assertions

Got it, pls lmk if you can find one

I think in lack of an instance the most appropriate severity is QA Low

[GalloDaSballo]

L

[c4-judge]

GalloDaSballo changed the severity to QA (Quality Assurance)

[c4-judge]

GalloDaSballo marked the issue as grade-a