An attacker can steal other users' entries and dominate a round.

Summary

In YoloV2.sol#depositETHIntoMultipleRounds function, it does not check amounts[i] == 0 so an attacker can deposit 0 eth to a specific round. In this case, the attacker earns same entryIndex as previous deposit. Then, in YoloV2.sol#fulfillRandomWords() when previously deposited entry index is selected as winningEntry, the attacker becomes winner.

On the other hand, an attacker can deposit many times with amounts[i] == 0 so he can make a specific round drawing without increasing entryCount.

Vulnerability Detail

YoloV2.sol#depositETHIntoMultipleRounds() function which deposits eth to several rounds is as follows.

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

        ...

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

        for (uint256 i; i < numberOfRounds; ++i) {
            ...

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

            entriesCounts[i] = entriesCount;
        }

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

As you can see, it does not check amounts[i] == 0 and it only checks that total amount is zero. And _depositETH function on L341 is as follows.

    function _depositETH(
        Round storage round,
        uint256 roundId,
        uint256 roundValuePerEntry,
        uint256 depositAmount
    ) private returns (uint256 entriesCount) {
1423    entriesCount = depositAmount / roundValuePerEntry;
        uint256 roundDepositCount = round.deposits.length;

        _validateOnePlayerCannotFillUpTheWholeRound(_unsafeAdd(roundDepositCount, 1), round.numberOfParticipants);

1428    uint40 currentEntryIndex = _getCurrentEntryIndexWithoutAccrual(round, roundDepositCount, entriesCount);
        ...
        uint256 roundDepositsLengthSlot = _getRoundSlot(roundId) + ROUND__DEPOSITS_LENGTH_SLOT_OFFSET;
        uint256 depositDataSlotWithCountOffset = _getDepositDataSlotWithCountOffset(
            roundDepositsLengthSlot,
            roundDepositCount
        );
        // We don't have to write tokenType, tokenAddress, tokenId, and withdrawn because they are 0.
        _writeDepositorAndCurrentEntryIndexToDeposit(depositDataSlotWithCountOffset, currentEntryIndex);
        _writeDepositAmountToDeposit(depositDataSlotWithCountOffset, depositAmount);
        assembly {
            sstore(roundDepositsLengthSlot, add(roundDepositCount, 1))
        }
    }

Here if depositAmount == 0, on L1423 entriesCount == 0. And on L1428 it calculates entryIndex about entriesCount and _getCurrentEntryIndexWithoutAccrual() function is as follows.

    function _getCurrentEntryIndexWithoutAccrual(
        Round storage round,
        uint256 roundDepositCount,
        uint256 entriesCount
    ) private view returns (uint40 currentEntryIndex) {
        if (roundDepositCount == 0) {
            currentEntryIndex = uint40(entriesCount);
        } else {
            currentEntryIndex = uint40(
1660            round.deposits[_unsafeSubtract(roundDepositCount, 1)].currentEntryIndex + entriesCount
            );
        }
    }

As we can see above, here it does not check entriesCount == 0, too. If entriesCount == 0, on L1660 currentEntryIndex becomes round.deposits[_unsafeSubtract(roundDepositCount, 1)].currentEntryIndex which is the index deposited last.

Next, fulfillRandomWords() function which determines a winner is as follows.

    function fulfillRandomWords(uint256 requestId, uint256[] memory randomWords) internal override {
        if (randomnessRequests[requestId].exists) {
            uint256 roundId = randomnessRequests[requestId].roundId;
            Round storage round = rounds[roundId];

            if (round.status == RoundStatus.Drawing) {
                round.status = RoundStatus.Drawn;
                uint256 randomWord = randomWords[0];
                randomnessRequests[requestId].randomWord = randomWord;

                uint256 count = round.deposits.length;
                uint256[] memory currentEntryIndexArray = new uint256[](count);
                for (uint256 i; i < count; ++i) {
                    currentEntryIndexArray[i] = uint256(round.deposits[i].currentEntryIndex);
                }

                uint256 currentEntryIndex = currentEntryIndexArray[_unsafeSubtract(count, 1)];
                uint256 winningEntry = _unsafeAdd(randomWord % currentEntryIndex, 1);
1288            round.winner = round.deposits[currentEntryIndexArray.findUpperBound(winningEntry)].depositor;
                round.protocolFeeOwed = (round.valuePerEntry * currentEntryIndex * round.protocolFeeBp) / 10_000;

                emit RoundStatusUpdated(roundId, RoundStatus.Drawn);

                _startRound({_roundsCount: roundId});
            }
        }
    }

On L1288, it finds the index of deposits corresponding to winningEntry by using findUpperBound() function.

Arrays.sol#findUpperBound function is as follows.

    /**
     * @dev Searches a sorted `array` and returns the first index that contains
     * a value greater or equal to `element`. If no such index exists (i.e. all
     * values in the array are strictly less than `element`), the array length is
     * returned. Time complexity O(log n).
     *
     * `array` is expected to be sorted in ascending order, and to contain no
     * repeated elements.
     */
    function findUpperBound(uint256[] memory array, uint256 element) internal pure returns (uint256) {
        if (array.length == 0) {
            return 0;
        }

        uint256 low = 0;
        uint256 high = array.length;

        while (low < high) {
            uint256 mid = Math.average(low, high);

            // Note that mid will always be strictly less than high (i.e. it will be a valid array index)
            // because Math.average rounds down (it does integer division with truncation).
            if (array[mid] > element) {
                high = mid;
            } else {
                unchecked {
                    low = mid + 1;
                }
            }
        }

        // At this point `low` is the exclusive upper bound. We will return the inclusive upper bound.
        if (low > 0 && array[low - 1] == element) {
            unchecked {
                return low - 1;
            }
        } else {
            return low;
        }
    }

As you can see from doc, it assumes that elements of array are not repeated. But the elements of array can be repeated and in fact if the array is [0, 1, 1] and element == 1 it returns last index, 2. So if entry-1 is won, the attacker who deposited 0 eth becomes winner.

On the other hand, an attacker can deposit many times with amounts[i] == 0 and make roundDepositCount == MAXIMUM_NUMBER_OF_DEPOSITS_PER_ROUND so he can make a specific round drawing early without increasing entryCount.

    function _shouldDrawWinner(
        uint256 numberOfParticipants,
        uint256 maximumNumberOfParticipants,
        uint256 roundDepositCount
    ) private pure returns (bool shouldDraw) {
        shouldDraw =
            numberOfParticipants == maximumNumberOfParticipants ||
            (numberOfParticipants > 1 && roundDepositCount == MAXIMUM_NUMBER_OF_DEPOSITS_PER_ROUND);
    }

As we can see above, after a user deposits, an attacker can deposit 0 eth about a specific round and he can draw that round early on L360.

Impact

An attacker can steal entries which other users deposited and he can draw a round early without increasing entryCount.

Code Snippet

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

Tool used

Manual Review

Recommendation

YoloV2.sol#depositETHIntoMultipleRounds() function has to be modified as follows so that it has to check amounts[i] == 0.

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

        ...

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

        for (uint256 i; i < numberOfRounds; ++i) {
            ...

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

            entriesCounts[i] = entriesCount;
        }

        ...
    }

Duplicate of #18

sherlock-audit / 2024-01-looksrare-judging

dany.armstrong90 - An attacker can steal other users' entries and dominate a round. #129