The splitting amount depends on the order of the receivers' userIDs, which could eventually lead to stealing splits

[code423n4]

Lines of code

https://github.com/code-423n4/2023-01-drips/blob/main/src/Splits.sol#L143-L170

Vulnerability details

Impact

In Splits._split(), the splitted amounts for each receivers are calculated based on their weights.

        for (uint256 i = 0; i < currReceivers.length; i++) {
            splitsWeight += currReceivers[i].weight;
            uint128 currSplitAmt =
                uint128((uint160(collectableAmt) * splitsWeight) / _TOTAL_SPLITS_WEIGHT - splitAmt);
            splitAmt += currSplitAmt;
            uint256 receiver = currReceivers[i].userId;
            _addSplittable(receiver, assetId, currSplitAmt);
            emit Split(userId, receiver, assetId, currSplitAmt);
        }

But if the collectableAmt * weight of a receiver is lower than _TOTAL_SPLITS_WEIGHT, then the splitted amount of tokens could be zero when collectableAmt * (splitsWeight - currReceivers[i].weight) / _TOTAL_SPLITS_WEIGHT == collectableAmt * splitsWeight / _TOTAL_SPLITS_WEIGHT. It can easily happen when the user is in the first place of the currReceivers list.

Since the calculation depends on the cumulative weights and the currReceivers list is ordered by userID, the splitted result depends on the order of userIDs. The 'less splitted' amount returns to the user in the next order, as the splitsWeight has been increased but splitAmt hasn't.

In addition, the DripsHub.split(), receiveDrips(), squeezeDrips() functions are callable by anyone.

    function split(uint256 userId, IERC20 erc20, SplitsReceiver[] memory currReceivers)
        public
        whenNotPaused
        returns (uint128 collectableAmt, uint128 splitAmt)

    function receiveDrips(uint256 userId, IERC20 erc20, uint32 maxCycles)
        public
        whenNotPaused
        returns (uint128 receivedAmt)

    function squeezeDrips(
      ...
    ) public whenNotPaused returns (uint128 amt) {

As a result, an attacker can easily steal splits from a small-portion splits receiver account by calling receiveDrips(), squeezeDrips() properly to fill modest amount of splittables, then calling split() to steal the 'less-splitted' amount.

Proof of Concept

Check that the splitted result is affect by the order of splitsReceivers

Splits.t.sol:
    function testSplitOrder1() public {
        uint32 totalWeight = Splits._TOTAL_SPLITS_WEIGHT;
        setSplits(
            user, splitsReceivers(receiver1, (totalWeight / 10), receiver2, (totalWeight / 10) * 9)
        ); // receiver1 gets 10%, receiver2 gets 90%
        addSplittable(user, 9);

        split(user, 0, 9);
        split(receiver1, 0, 0); // received nothing
        split(receiver2, 9, 0);
    }

    function testSplitOrder2() public {
        uint32 totalWeight = Splits._TOTAL_SPLITS_WEIGHT;
        setSplits(
            user, splitsReceivers(receiver1, (totalWeight / 10) * 9, receiver2, totalWeight / 10)
        );
        addSplittable(user, 9);

        split(user, 0, 9);
        split(receiver1, 8, 0);
        split(receiver2, 1, 0);
    }

Anyone can attack a low-weight account by calling receiveDrips(), squeezeDrips() and split() from DripsHub. If the attacker is the next receiver in the SplitsReceiver list, it would result in draining splitted amounts.

DripsHub.t.sol:
    function testAttack() public {
        uint32 totalWeight = dripsHub.TOTAL_SPLITS_WEIGHT();
        // Receiver1 gets 5 percent, Receiver2 gets the other 95%
        SplitsReceiver[] memory receivers = splitsReceivers(receiver1, totalWeight / 20, receiver2, (totalWeight / 20) * 19);
        // User1 gets 10 tokens dripped per cycle, then splits it
        setDrips(user2, 0, 100, dripsReceivers(user1, 1));
        setSplits(user1, receivers);

        skipToCycleEnd();
        // User1 gets 9 tokens in the first cycle
        dripsHub.receiveDrips(user1, erc20, type(uint32).max);
        assertSplittable(user1, 9);
        // Receiver1 gets nothing from the split
        dripsHub.split(user1, erc20, receivers);
        assertSplittable(receiver1, 0);
        assertSplittable(receiver2, 9);

        skipToCycleEnd();
        // User1 gets 10 tokens in the next cycle
        dripsHub.receiveDrips(user1, erc20, type(uint32).max);
        assertSplittable(user1, 10);
        // Receiver1 gets nothing from the split again
        dripsHub.split(user1, erc20, receivers);
        assertSplittable(receiver1, 0);
        assertSplittable(receiver2, 19);

        skipToCycleEnd();
        // User1 gets 10 tokens in the next cycle
        dripsHub.receiveDrips(user1, erc20, type(uint32).max);
        assertSplittable(user1, 10);
        // Receiver1 should have received at least a token, but nothing
        dripsHub.split(user1, erc20, receivers);
        assertSplittable(receiver1, 0);
        assertSplittable(receiver2, 29);
    }

Tools Used

Foundry

Recommended Mitigation Steps

You could add minSplitAmt as a splits configuration parameter, so that splits configurators can protect small-portion splits receivers by setting a lower bound on the splitted amount each time.

    struct SplitsState {
        /// @notice The user's splits configuration hash, see `hashSplits`.
        bytes32 splitsHash;
+       uint128 minSplitAmt;
        /// @notice The user's splits balance. The key is the asset ID.
        mapping(uint256 => SplitsBalance) balances;
    }

    function _split(uint256 userId, uint256 assetId, SplitsReceiver[] memory currReceivers)
        ...
        collectableAmt = balance.splittable;
-       if (collectableAmt == 0) {
+       if (collectableAmt < splitsStates[userId].minSplitAmt) {
            return (0, 0);
        }

[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

[GalloDaSballo]

Great report, but need more!