code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-01-drips/blob/main/src/Drips.sol#L492 https://github.com/code-423n4/2023-01-drips/blob/main/src/Drips.sol#L429 https://github.com/code-423n4/2023-01-drips/blob/main/src/DripsHub.sol#L297-L306

Vulnerability details

Impact

In Drips.sol, the second for loop of _squeezedAmt() is going to break (on line 492 in the code block below) and exit the loop if receiver.userId != userId. As a result, the rest of the iterations that might fulfill receiver.userId == userId are not going to have amt separately added, i.e. the returned output, squeezedAmt is going to be lesser in value than expected. The impact could be very significant if this were to happen at the beginning part of the loop iterations.

File: Drips.sol#L470-L498

    function _squeezedAmt(
        uint256 userId,
        DripsHistory memory dripsHistory,
        uint32 squeezeStartCap,
        uint32 squeezeEndCap
    ) private view returns (uint128 squeezedAmt) {
        DripsReceiver[] memory receivers = dripsHistory.receivers;
        // Binary search for the `idx` of the first occurrence of `userId`
        uint256 idx = 0;
        for (uint256 idxCap = receivers.length; idx < idxCap;) {
            uint256 idxMid = (idx + idxCap) / 2;
            if (receivers[idxMid].userId < userId) {
                idx = idxMid + 1;
            } else {
                idxCap = idxMid;
            }
        }
        uint32 updateTime = dripsHistory.updateTime;
        uint32 maxEnd = dripsHistory.maxEnd;
        uint256 amt = 0;
        for (; idx < receivers.length; idx++) {
            DripsReceiver memory receiver = receivers[idx];
492:            if (receiver.userId != userId) break;
            (uint32 start, uint32 end) =
                _dripsRange(receiver, updateTime, maxEnd, squeezeStartCap, squeezeEndCap);
            amt += _drippedAmt(receiver.config.amtPerSec(), start, end);
        }
        return uint128(amt);
    }

Proof of Concept

This further affects the returned output, amt, of _squeezeDripsResult() since it is being privately linked to _squeezedAmt() on line 429.

File: Drips.sol#L392-L434

    function _squeezeDripsResult(
        uint256 userId,
        uint256 assetId,
        uint256 senderId,
        bytes32 historyHash,
        DripsHistory[] memory dripsHistory
    )
        internal
        view
        returns (
            uint128 amt,
            uint256 squeezedNum,
            uint256[] memory squeezedRevIdxs,
            bytes32[] memory historyHashes,
            uint256 currCycleConfigs
        )
    {
        {
            DripsState storage sender = _dripsStorage().states[assetId][senderId];
            historyHashes = _verifyDripsHistory(historyHash, dripsHistory, sender.dripsHistoryHash);
            // If the last update was not in the current cycle,
            // there's only the single latest history entry to squeeze in the current cycle.
            currCycleConfigs = 1;
            // slither-disable-next-line timestamp
            if (sender.updateTime >= _currCycleStart()) currCycleConfigs = sender.currCycleConfigs;
        }
        squeezedRevIdxs = new uint256[](dripsHistory.length);
        uint32[2 ** 32] storage nextSqueezed =
            _dripsStorage().states[assetId][userId].nextSqueezed[senderId];
        uint32 squeezeEndCap = _currTimestamp();
        for (uint256 i = 1; i <= dripsHistory.length && i <= currCycleConfigs; i++) {
            DripsHistory memory drips = dripsHistory[dripsHistory.length - i];
            if (drips.receivers.length != 0) {
                uint32 squeezeStartCap = nextSqueezed[currCycleConfigs - i];
                if (squeezeStartCap < _currCycleStart()) squeezeStartCap = _currCycleStart();
                if (squeezeStartCap < squeezeEndCap) {
                    squeezedRevIdxs[squeezedNum++] = i;
429:                    amt += _squeezedAmt(userId, drips, squeezeStartCap, squeezeEndCap);
                }
            }
            squeezeEndCap = drips.updateTime;
        }
    }

Additionally, squeezeDripsResult() is going to possibly return inaccurate result too.

File: DripsHub.sol#L297-L306

    function squeezeDripsResult(
        uint256 userId,
        IERC20 erc20,
        uint256 senderId,
        bytes32 historyHash,
        DripsHistory[] memory dripsHistory
    ) public view returns (uint128 amt) {
        (amt,,,,) =
            Drips._squeezeDripsResult(userId, _assetId(erc20), senderId, historyHash, dripsHistory);
    }

Tools Used

Manual inspection

Recommended Mitigation Steps

Consider replacing break with continue so that the for loop will encompass all valid iterations to accurately update amt.

c4-judge commented 1 year ago

GalloDaSballo marked the issue as duplicate of #71