Drips Protocol contest - berndartmueller's results

Lines of code

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

Vulnerability details

Squeezing drips from a sender requires providing the sequence of drips configurations (see NatSpec description in L337-L338):

/// It can start at an arbitrary past configuration, but must describe all the configurations /// which have been used since then including the current one, in the chronological order.

The provided history entries are hashed and verified against the sender's dripsHistoryHash.

However, the sender can prevent a receiver from squeezing drips by front-running the squeeze transaction and adding a new configuration. Adding a new configuration updates the current dripsHistoryHash and invalidates the historyHash provided by the receiver when squeezing. The receiver will then fail the drips history verification in L461 and the squeeze will fail.

Impact

A sender can prevent its drip receivers from squeezing by front-running the squeeze transaction and adding a new configuration.

Proof of Concept

Drips.sol#L411

392: function _squeezeDripsResult(
393:     uint256 userId,
394:     uint256 assetId,
395:     uint256 senderId,
396:     bytes32 historyHash,
397:     DripsHistory[] memory dripsHistory
398: )
399:     internal
400:     view
401:     returns (
402:         uint128 amt,
403:         uint256 squeezedNum,
404:         uint256[] memory squeezedRevIdxs,
405:         bytes32[] memory historyHashes,
406:         uint256 currCycleConfigs
407:     )
408: {
409:     {
410:         DripsState storage sender = _dripsStorage().states[assetId][senderId];
411:         historyHashes = _verifyDripsHistory(historyHash, dripsHistory, sender.dripsHistoryHash);
412:         // If the last update was not in the current cycle,
413:         // there's only the single latest history entry to squeeze in the current cycle.
414:         currCycleConfigs = 1;
415:         // slither-disable-next-line timestamp
416:         if (sender.updateTime >= _currCycleStart()) currCycleConfigs = sender.currCycleConfigs;
417:     }
...      // [...]

Drips.sol#L461

444: function _verifyDripsHistory(
445:     bytes32 historyHash,
446:     DripsHistory[] memory dripsHistory,
447:     bytes32 finalHistoryHash
448: ) private pure returns (bytes32[] memory historyHashes) {
449:     historyHashes = new bytes32[](dripsHistory.length);
450:     for (uint256 i = 0; i < dripsHistory.length; i++) {
451:         DripsHistory memory drips = dripsHistory[i];
452:         bytes32 dripsHash = drips.dripsHash;
453:         if (drips.receivers.length != 0) {
454:             require(dripsHash == 0, "Drips history entry with hash and receivers");
455:             dripsHash = _hashDrips(drips.receivers);
456:         }
457:         historyHashes[i] = historyHash;
458:         historyHash = _hashDripsHistory(historyHash, dripsHash, drips.updateTime, drips.maxEnd);
459:     }
460:     // slither-disable-next-line incorrect-equality,timestamp
461:     require(historyHash == finalHistoryHash, "Invalid drips history");
462: }

Tools Used

Manual review

Recommended mitigation steps

Consider allowing a receiver to squeeze drips from a sender up until the current timestamp.

QA Report

Table of Contents

• Low-Risk Findings
  • [L-01] An authorized user can unauthorize other authorized users of the same sender
  • [L-02] Lack of reasonable boundaries for cycle secs
  • [L-03] Loading a drip configuration with assembly in _getConfig is unsafe
  • [L-04] Unused dripId of receiver config is used considered while sorting receivers
  • [L-05] Collecting funds should be usable while the DripsHub contract is paused
  • [L-06] An immutable split is unable to collect funds if it has itself set as a split receiver
  • [L-07] Precision issues caused by division before multiplication in Drips._drippedAmt
• Non-Critical Findings
  • [NC-01] The NatSpec comment of the DripsConfigImpl.lt function neglects to sorting of dripId
  • [NC-02] Unused amtPerCycle variable in Drips._drippedAmt calculation

Low-Risk Findings

[L-01] An authorized user can unauthorize other authorized users of the same sender

A user can grant authorization to another address to make calls on their behalf via the Caller.callAs function. The Caller.unauthorize function allows the user to revoke the authorization of another address.

An authorized user can revoke the authorization of another authorized user of the same sender. This is because the authorized user can call the Caller.unauthorize function on behalf of the sender.

Findings

Caller.sol#L114-L118

114: function unauthorize(address user) public {
115:     address sender = _msgSender();
116:     require(_authorized[sender].remove(user), "Address is not authorized");
117:     emit Unauthorized(sender, user);
118: }

Recommended mitigation steps

Consider preventing calls to the Caller contract address from within the _call function

[L-02] Lack of reasonable boundaries for cycle secs

If _cycleSecs is set too low, for example, to a value less than the Ethereum block time of 12 sec, it will not be possible to squeeze. If set too high, e.g., larger than the current block.timestamp, it will not be possible to receive drips - only squeezing will be possible.

Findings

Drips.sol#L221

219: constructor(uint32 cycleSecs, bytes32 dripsStorageSlot) {
220:     require(cycleSecs > 1, "Cycle length too low");
221:     _cycleSecs = cycleSecs;
222:     _dripsStorageSlot = dripsStorageSlot;
223: }

Recommended mitigation steps

Consider adding an appropriate upper limit for _cycleSecs.

[L-03] Loading a drip configuration with assembly in _getConfig is unsafe

The Drips._getConfig loads a drips configuration and returns the amtPerSec, start and end values. The configs parameter is a uint256[] array, containing the drips config.

As seen in Drips.create, the drip config has the dripId in the 32 most significant bits, followed by the amtPerSec in the next 160 bits.

Even though the dripId is not used in the contract and it's even stripped out (see Drips.sol#L805) when preprocessing the drips receiver in Drips._addConfig, it could cause serious trouble in the future if the dripId is used in the contract and not taken into account when loading with Drips._getConfig.

Findings

Drips.sol#L825

815: function _getConfig(uint256[] memory configs, uint256 idx)
816:     private
817:     pure
818:     returns (uint256 amtPerSec, uint256 start, uint256 end)
819: {
820:     uint256 val;
821:     // slither-disable-next-line assembly
822:     assembly ("memory-safe") {
823:         val := mload(add(32, add(configs, shl(5, idx))))
824:     }
825:     return (val >> 64, uint32(val >> 32), uint32(val));
826: }

Recommended mitigation steps

Consider truncating the uint256 value to uint160 before returning it.

[L-04] Unused dripId of receiver config is used considered while sorting receivers

The Drips._isOrdered function compares two given receivers. The receiver config can include the prefixed dripId in the 32 most significant bits. This allows using the dripId for changing the order of receivers when setting a new drips configuration without violating the sorting requirement, First compares their amtPerSecs, then their starts and then their durations (see Drips.sol#L93).

While no harmful effects are expected, it can be used in the Drips._setDrips function to control if delta amounts of receivers are first removed, added, or updated, are expected.

Findings

Drips.sol#L1069

1061: function _isOrdered(DripsReceiver memory prev, DripsReceiver memory next)
1062:     private
1063:     pure
1064:     returns (bool)
1065: {
1066:     if (prev.userId != next.userId) {
1067:         return prev.userId < next.userId;
1068:     }
1069:     return prev.config.lt(next.config);
1070: }

Recommended mitigation steps

Consider omitting the dripId from the receiver config when comparing the receivers.

[L-05] Collecting funds should be usable while the DripsHub contract is paused

The DripsHub.collect function is only callable when the DripsHub contract is not paused. This prevents a user from collecting accumulated funds. The admin of the DripsHub contract can potentially pause the contracts at any time, locking users out of their honestly earned funds.

Findings

DripsHub.sol#L388

386: function collect(uint256 userId, IERC20 erc20)
387:     public
388:     whenNotPaused
389:     onlyDriver(userId)
390:     returns (uint128 amt)
391: {
392:     amt = Splits._collect(userId, _assetId(erc20));
393:     _decreaseTotalBalance(erc20, amt);
394:     erc20.safeTransfer(msg.sender, amt);
395: }

Recommended mitigation steps

Consider removing the whenNotPaused modifier to allow collecting funds while the DripsHub contract is paused.

[L-06] An immutable split is unable to collect funds if it has itself set as a split receiver

An immutable split cannot collect funds if it has itself set as a split receiver. This is because the ImmutableSplitsDriver contract lacks the functionality to collect available funds.

Findings

ImmutableSplitsDriver.sol#L66

53: function createSplits(SplitsReceiver[] calldata receivers, UserMetadata[] calldata userMetadata)
54:     public
55:     whenNotPaused
56:     returns (uint256 userId)
57: {
58:     userId = nextUserId();
59:     StorageSlot.getUint256Slot(_counterSlot).value++;
60:     uint256 weightSum = 0;
61:     for (uint256 i = 0; i < receivers.length; i++) {
62:         weightSum += receivers[i].weight;
63:     }
64:     require(weightSum == totalSplitsWeight, "Invalid total receivers weight");
65:     emit CreatedSplits(userId, dripsHub.hashSplits(receivers));
66:     dripsHub.setSplits(userId, receivers);
67:     if (userMetadata.length > 0) dripsHub.emitUserMetadata(userId, userMetadata);
68: }

Recommended mitigation steps

Consider preventing setting one of the receivers to the user ID of the newly created immutable split.

[L-07] Precision issues caused by division before multiplication in Drips._drippedAmt

The Drips._drippedAmt function calculates the amount of dripped funds for a given time interval and amtPerSec.

However, by dividing the result of mul(cycleSecs, amtPerSec) by _AMT_PER_SEC_MULTIPLIER before multiplying it by endedCycles, the precision of the result is reduced. This can cause the amount of dripped funds to be lower if the multiplication would have been performed first.

Findings

Drips.sol#L1107

1093: function _drippedAmt(uint256 amtPerSec, uint256 start, uint256 end)
1094:     private
1095:     view
1096:     returns (uint256 amt)
1097: {
1098:     // This function is written in Yul because it can be called thousands of times
1099:     // per transaction and it needs to be optimized as much as possible.
1100:     // As of Solidity 0.8.13, rewriting it in unchecked Solidity triples its gas cost.
1101:     uint256 cycleSecs = _cycleSecs;
1102:     // slither-disable-next-line assembly
1103:     assembly {
1104:         let endedCycles := sub(div(end, cycleSecs), div(start, cycleSecs))
1105:         // slither-disable-next-line divide-before-multiply
1106:         let amtPerCycle := div(mul(cycleSecs, amtPerSec), _AMT_PER_SEC_MULTIPLIER) line
1107:         amt := mul(endedCycles, div(mul(cycleSecs, amtPerSec), _AMT_PER_SEC_MULTIPLIER)) // @audit-info Division before before multiplication
1108:         // slither-disable-next-line weak-prng
1109:         let amtEnd := div(mul(mod(end, cycleSecs), amtPerSec), _AMT_PER_SEC_MULTIPLIER)
1110:         amt := add(amt, amtEnd)
1111:         // slither-disable-next-line weak-prng
1112:         let amtStart := div(mul(mod(start, cycleSecs), amtPerSec), _AMT_PER_SEC_MULTIPLIER)
1113:         amt := sub(amt, amtStart)
1114:     }
1115: }

Recommended mitigation steps

Consider multiplying mul(cycleSecs, amtPerSec) by endedCycles before dividing by _AMT_PER_SEC_MULTIPLIER in line 1107 to avoid precision issues.

Non-Critical Findings

[NC-01] The NatSpec comment of the DripsConfigImpl.lt function neglects to sorting of dripId

According to the NatSpec comment of the DripsConfigImpl.lt function, the DripsConfig struct is sorted by amtPerSec, then start and then duration. However, the dripId is not mentioned in the comment:

/// First compares their amtPerSecs, then their starts and then their durations.

Findings

Drips.sol#L93

92: /// @notice Compares two `DripsConfig`s.
93: /// First compares their `amtPerSec`s, then their `start`s and then their `duration`s.
94: function lt(DripsConfig config, DripsConfig otherConfig) internal pure returns (bool) {
95:     return DripsConfig.unwrap(config) < DripsConfig.unwrap(otherConfig);
96: }

Drips.sol#L1069

1061: function _isOrdered(DripsReceiver memory prev, DripsReceiver memory next)
1062:     private
1063:     pure
1064:     returns (bool)
1065: {
1066:     if (prev.userId != next.userId) {
1067:         return prev.userId < next.userId;
1068:     }
1069:     return prev.config.lt(next.config); // @audit-info `dripId` of receiver config (The 32 most significant bits) is considered while sorting receivers
1070: }

Recommended mitigation steps

Consider updating the NatSpec comment to include the dripId in the sorting order.

[NC-02] Unused amtPerCycle variable in Drips._drippedAmt calculation

The Drips._drippedAmt function calculates the dripped amount. The amtPerCycle variable is not used in the calculation and can be removed.

Findings

Drips.sol#L1106

1093: function _drippedAmt(uint256 amtPerSec, uint256 start, uint256 end)
1094:     private
1095:     view
1096:     returns (uint256 amt)
1097: {
1098:     // This function is written in Yul because it can be called thousands of times
1099:     // per transaction and it needs to be optimized as much as possible.
1100:     // As of Solidity 0.8.13, rewriting it in unchecked Solidity triples its gas cost.
1101:     uint256 cycleSecs = _cycleSecs;
1102:     // slither-disable-next-line assembly
1103:     assembly {
1104:         let endedCycles := sub(div(end, cycleSecs), div(start, cycleSecs))
1105:         // slither-disable-next-line divide-before-multiply
1106:         let amtPerCycle := div(mul(cycleSecs, amtPerSec), _AMT_PER_SEC_MULTIPLIER) // @audit-info Unused variable
1107:         amt := mul(endedCycles, div(mul(cycleSecs, amtPerSec), _AMT_PER_SEC_MULTIPLIER))
1108:         // slither-disable-next-line weak-prng
1109:         let amtEnd := div(mul(mod(end, cycleSecs), amtPerSec), _AMT_PER_SEC_MULTIPLIER)
1110:         amt := add(amt, amtEnd)
1111:         // slither-disable-next-line weak-prng
1112:         let amtStart := div(mul(mod(start, cycleSecs), amtPerSec), _AMT_PER_SEC_MULTIPLIER)
1113:         amt := sub(amt, amtStart)
1114:     }
1115: }

Recommended mitigation steps

Consider removing the unused variable.