low value splits can be abused by receivers with higher shares

[code423n4]

Lines of code

https://github.com/code-423n4/2023-01-drips/blob/main/src/Splits.sol#L22 https://github.com/code-423n4/2023-01-drips/blob/main/src/Splits.sol#L160-L161

Vulnerability details

Description

Splits are used to divide drips from streams. Due to the nature of drips they will often be very low amounts, even more so combined with the squeeze functionality.

Due to the high value of _TOTAL_SPLITS_WEIGHT = 1_000_000 when a user with a low share is splitting a low value they might be left without a share:

File: Splits.sol

160:            uint128 currSplitAmt =
161:                uint128((uint160(collectableAmt) * splitsWeight) / _TOTAL_SPLITS_WEIGHT - splitAmt);

Impact

For low value splits the share splitting is unfair and can be abused by receivers with higher shares.

If the low value split is for a token with low decimals but high value, like wbtc the receiver might be tricked from quite a lot of money over time.

This can possibly be done for profit, given a favorable enough split configuration or just to grief the low share receivers.

With an ImmutableDrip there is no way to change this as well.

Proof of Concept

Imagine a scenario where someone wants to drip 1 wbtc over a year to a bunch of receivers (possibly for charity or promotion).

A couple of receivers has a lower share, like 1% while others have a larger share.

If the ones with larger shares calls squeeze and split very often they can then abuse the high _TOTAL_SPLITS_WEIGHT to steal the share of the lower share receivers:

Test in Split.t.sol:

    function testSplitLowAmountWithLowWeightReceiver() public {
        SplitsReceiver[] memory list = new SplitsReceiver[](2);
        uint32 weight = 10000; // 1%
        list[0] = SplitsReceiver(receiver1, weight);
        list[1] = SplitsReceiver(receiver2, Splits._TOTAL_SPLITS_WEIGHT/2 - weight);

        Splits._setSplits(user, list);

        Splits._addSplittable(user, asset, 80); // ~2 blocks of 1 wbtc being dripped over a year 

        (uint128 collectableAmt, uint128 splitAmt) = Splits._split(user, asset, list);

        // user got 40 (50%)
        console.log("collectableAmt",collectableAmt);
        console.log("splitAmt",splitAmt);

        // receiver1 got nothing
        console.log("receiver1",Splits._splittable(receiver1,asset));
        // receiver2 got 40 which includes receiver1's share
        console.log("receiver2",Splits._splittable(receiver2,asset));
    }

Tools Used

Manual audit, vs code, forge

Recommended Mitigation Steps

I can think of two different mitigations with different trade offs:

Either lower the _TOTAL_SPLITS_WEIGHT which would make this abuse less effective. That would be a trade off for lower precision when doing splits though.

or

Do a zero check in the split and revert if a receiver got no share. This would have the trade off that splits are not guaranteed to work.

[c4-judge]

GalloDaSballo marked the issue as duplicate of #182

[c4-judge]

GalloDaSballo marked the issue as not a duplicate

[c4-judge]

GalloDaSballo changed the severity to QA (Quality Assurance)

[GalloDaSballo]

L

[c4-judge]

GalloDaSballo marked the issue as grade-c