code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Splittable funds are split to previously configured receivers based on their weight. The unpermissioned and public callable DripsHub.split function internally calls the Splits._split function, which splits the available splittable amount all at once to its receivers.

The Splits._split function iterates all receivers of the split and accumulates the splitsWeight of all receivers up until the current receiver. Note that the order of a receiver within the currReceivers array is of much relevance (as demonstrated later).

The remaining available amount is then distributed to the originator of the split with the userId and is available for collection.

The unfair splitting is manifested in the following outcomes:

1. The split originator receives more funds than receivers with equal weight

The calculation of currSplitAmt in line 161 rounds down the result of the division for recipients, whereas the split originator just receives the remaining funds, including the rounding error.

2. Splitting funds to receivers with small weights can lead to precision loss and loss of funds

Receivers with small weights could receive 0 funds (due to rounding down) if the timing of the split is unfavorable for them.

Both outcomes are magnified if the ERC-20 token decimals are low (e.g. 2 decimals), the split amount is small and the split functionality is called frequently.

Impact

Some split receivers could receive fewer funds than they are entitled to, whereas the split originator receives the former's funds.

Proof of Concept

Splits.sol#L161

143: function _split(uint256 userId, uint256 assetId, SplitsReceiver[] memory currReceivers)
144:     internal
145:     returns (uint128 collectableAmt, uint128 splitAmt)
146: {
147:     _assertCurrSplits(userId, currReceivers);
148:     mapping(uint256 => SplitsState) storage splitsStates = _splitsStorage().splitsStates;
149:     SplitsBalance storage balance = splitsStates[userId].balances[assetId];
150:
151:     collectableAmt = balance.splittable;
152:     if (collectableAmt == 0) {
153:         return (0, 0);
154:     }
155:
156:     balance.splittable = 0;
157:     uint32 splitsWeight = 0;
158:     for (uint256 i = 0; i < currReceivers.length; i++) {
159:         splitsWeight += currReceivers[i].weight;
160:         uint128 currSplitAmt =
161:             uint128((uint160(collectableAmt) * splitsWeight) / _TOTAL_SPLITS_WEIGHT - splitAmt);
162:         splitAmt += currSplitAmt;
163:         uint256 receiver = currReceivers[i].userId;
164:         _addSplittable(receiver, assetId, currSplitAmt);
165:         emit Split(userId, receiver, assetId, currSplitAmt);
166:     }
167:     collectableAmt -= splitAmt;
168:     balance.collectable += collectableAmt;
169:     emit Collectable(userId, assetId, collectableAmt);
170:     emit SplitDone(currReceivers.length);
171: }

The already available test case "testSplittingSplitsAllFundsEvenWhenTheyDoNotDivideEvenly" in Splits.t.ts shows the presented rounding issue mentioned in the above 1st outcome.

Here's a simplified example:

Split receiver weight distribution (for simplification, weights have been scaled down):

Receiver	Weight
Receiver 1	3/10
Receiver 2	3/10
Receiver 3	1/10
Split originator	3/10 (remaining)

Split amount: 9

Calling the split function will yield the following results:

Receiver	Calculation	Amount received	Amount entitled to (based on weight)	Delta
Receiver 1	`(9 * 3) / 10 - 0`	2	2.7	-0.7
Receiver 2	`(9 * 6) / 10 - 2`	3	2.7	+0.3
Receiver 3	`(9 * 7) / 10 - 5`	1	0.9	+0.1
Split originator	`9 - 5` (remaining)	3	2.7	+0.3

As observed, the first receiver receives less than the later receivers and the split originator with equal weight.

Even though the rounding issue results in minuscule token amounts, the calculation is flawed by favoring the split originator and receivers positioned later in the currReceivers array.

Tools Used

Manual review

Recommended mitigation steps

Consider using a similar approach to OpenZeppelin's PaymentSplitter.

By using shares internally to keep track of each user's "weight" and instead of having a split function, which splits the available amount all at once to receivers, a receiver can collect their receivable amount individually at any time. A receiver can wait for the appropriate time to collect funds to avoid rounding issues (caused by receiving small split amounts).

This would also loosen the dependency on the _MAX_SPLITS_RECEIVERS = 200 constant, which is currently used to limit the number of receivers in a split.