s1ce - Zero entries in `depositETHIntoMultipleRounds` allows for malicious exploiter to profit

[sherlock-admin]

s1ce

high

Zero entries in depositETHIntoMultipleRounds allows for malicious exploiter to profit

Summary

Because of how the binary search works, zero entries, which should not be allowed but are, in depositETHIntoMultipleRounds will allow the attacker to gain probability share while depositing nothing

Vulnerability Detail

Here is what depositETHIntoMultipleRounds looks like:

    function depositETHIntoMultipleRounds(uint256[] calldata amounts) external payable nonReentrant whenNotPaused {
        uint256 numberOfRounds = amounts.length;
        if (msg.value == 0 || numberOfRounds == 0) {
            revert ZeroDeposits();
        }

        uint256 startingRoundId = roundsCount;
        Round storage startingRound = rounds[startingRoundId];
        _validateRoundIsOpen(startingRound);

        // this is wrong, there might not be a deposit
        _setCutoffTimeIfNotSet(startingRound);

        uint256 expectedValue;
        uint256[] memory entriesCounts = new uint256[](numberOfRounds);

        for (uint256 i; i < numberOfRounds; ++i) {
            uint256 roundId = _unsafeAdd(startingRoundId, i);
            Round storage round = rounds[roundId];
            uint256 roundValuePerEntry = round.valuePerEntry;
            if (roundValuePerEntry == 0) {
                (, , roundValuePerEntry) = _writeDataToRound({roundId: roundId, roundValue: 0});
            }

            _incrementUserDepositCount(roundId, round);

            uint256 depositAmount = amounts[i];
            if (depositAmount % roundValuePerEntry != 0) {
                revert InvalidValue();
            }
            uint256 entriesCount = _depositETH(round, roundId, roundValuePerEntry, depositAmount);
            expectedValue += depositAmount;

            entriesCounts[i] = entriesCount;
        }

        if (expectedValue != msg.value) {
            revert InvalidValue();
        }

        emit MultipleRoundsDeposited(msg.sender, startingRoundId, amounts, entriesCounts);

        if (
            _shouldDrawWinner(
                startingRound.numberOfParticipants,
                startingRound.maximumNumberOfParticipants,
                startingRound.deposits.length
            )
        ) {
            _drawWinner(startingRound, startingRoundId);
        }
    }

Notice that we check that msg.value = 0 at the beginning and revert if so, but this does not check that the amount deposited into each individual round is greater than 0 (so we can deposit 0 into one round and some non zero amount into another, and the checks would still pass).

Thus, by setting amounts[0] != 0 and amounts[1] = 0 for example, we have a way to call _depositETH with depositAmount = 0. This means that a user can deposit with 0 ETH and gain exactly 0 entries as a result of their deposit. This has detrimental implications because of how the findUpperBound function in fulfillRandomWords works:

round.winner = round.deposits[currentEntryIndexArray.findUpperBound(winningEntry)].depositor;

Reading the findUpperBound function, it clearly states: array is expected to be sorted in ascending order, and to contain no repeated elements. And there's a good reason for this! If the array contains repeated elements, the functionality will actually end up picking the last element that fits the criteria in the equality case. To be more concrete, if we have the array [1, 1, 1], if we call upper bound here, it will actually pick the last 1.

Now let's take an example where a user has deposited one roundValuePerEntry and gained one entry. So the array at the beginning would look like [1]. Now malicious exploiter deposits 0 and gets zero entry, so the array looks like [1, 1]. Now the draw happens, and the malicious exploiter will always win since 1 will be selected as the random number and the last 1 will be picked. Note that the malicious exploiter cannot always steal all the probability share from users; sometimes they can only steal a part of it (i.e. they only steal the equality case).

There are some other impacts of this like griefing the first depositor by filling up the rest of the deposits, forcing a draw, and making them pay the protocol fee for nothing.

Impact

Malicious attacker steals probability share from other users through use of zero entries

Code Snippet

https://github.com/sherlock-audit/2024-01-looksrare/blob/main/contracts-yolo/contracts/YoloV2.sol#L312-L362

Tool used

Manual Review

Recommendation

Disallow zero entries

Duplicate of #18