jasonxiale - `YoloV2.depositETHIntoMultipleRounds` lacks of checking if `amounts` containing **0 value** element

[sherlock-admin2]

jasonxiale

high

YoloV2.depositETHIntoMultipleRounds lacks of checking if amounts containing 0 value element

Summary

YoloV2.depositETHIntoMultipleRounds lacks of checking if amounts containing 0 value element

Vulnerability Detail

YoloV2.depositETHIntoMultipleRounds can be used to deposit ETH to multipile rounds, but the function lacks of checking if all elements in amounts are larger than 0. In YoloV2.sol#L313-L316, the function is used to make sure amounts has at least one element, and msg.sender is larger than 0.

In YoloV2.sol#L337-L340, the function only checks depositAmount % roundValuePerEntry != 0 but doesn't check (depositAmount/roundValuePerEntry > 0)

In YoloV2.sol#L341, _depositETH is used to deposit ETH into current round.

And in YoloV2._depositETH, there is also no such check, if depositAmount is 0 in YoloV2._depositETH, a Deposit can be pushed into round.deposits, and currentEntryIndex is the same as previous Deposit.

So to abuse this issue, Alice(the attacker) can call YoloV2.depositETHIntoMultipleRounds with a extremely large amount of elements in amounts, all of the elements are zero except one element, this way we can pass the check in YoloV2.sol#L1046-L1050.

Impact

By abusing this issue, attacker can:

1. pre-set global configuration like maximumNumberOfParticipantsPerRound, valuePerEntry, protocolFeeBp can be written by _writeDataToRound in YoloV2.sol#L331-L333 for lots of rounds-in-furture, which makes YoloV2.updateValuePerEntry, YoloV2.updateMaximumNumberOfParticipantsPerRound, and YoloV2.updateProtocolFeeBp useless
2. deposts on current round with 0 cost, and might be the winner of the round.

The follow POC can be used to prove the two impacts I mention above at the same time, copy the following code to Yolo.deposit.t.sol and run forge test --mc Yolo_Deposit_Test --mt test_depositETHIntoMultipleRounds -vv

+    function test_depositETHIntoMultipleRounds() public {
+        vm.deal(user2, 1 ether);
+        vm.deal(user3, 0.49 ether);
+
+        vm.prank(user2);
+        yolo.deposit{value: 1 ether}(1, _emptyDepositsCalldata());
+
+        uint256[] memory amounts = new uint256[](200); // we create an array contains 200 elements
+        amounts[199] = 1e18 / 100;                     // only the last elements has value
+
+        uint96 roundValuePerEntry;
+        (, , , , , , , roundValuePerEntry, , ) = yolo.getRound(100);
+        assertEq(roundValuePerEntry, 0);               // before calling depositETHIntoMultipleRounds, roundValuePerEntry for round 100 should be zero
+
+        vm.prank(user3);
+        yolo.depositETHIntoMultipleRounds{value: 0.01 ether}(amounts);
+
+        _drawRound();
+        uint256[] memory randomWords = new uint256[](1);
+        randomWords[0] = 99;
+
+        vm.prank(VRF_COORDINATOR);
+        VRFConsumerBaseV2(yolo).rawFulfillRandomWords(FULFILL_RANDOM_WORDS_REQUEST_ID, randomWords);
+        address winner = _getWinner(1);
+        assertEq(winner, user3);                         // the winner is user3, but user3 doesn't pay for current round
+        (, , , , , , , roundValuePerEntry, , ) = yolo.getRound(100);
+        assertEq(roundValuePerEntry, 10000000000000000); // after calling depositETHIntoMultipleRounds, roundValuePerEntry for round 100 is overwritten by current valuePerEntry, and now is 10000000000000000
+
+    }
+

Code Snippet

Tool used

Manual Review

Recommendation

diff --git a/contracts-yolo/contracts/YoloV2.sol b/contracts-yolo/contracts/YoloV2.sol
index b8afe96..f834eb8 100644
--- a/contracts-yolo/contracts/YoloV2.sol
+++ b/contracts-yolo/contracts/YoloV2.sol
@@ -338,6 +338,11 @@ contract YoloV2 is
             if (depositAmount % roundValuePerEntry != 0) {
                 revert InvalidValue();
             }
+
+            if (depositAmount / roundValuePerEntry == 0) {
+                revert InvalidValue();
+            }
+
             uint256 entriesCount = _depositETH(round, roundId, roundValuePerEntry, depositAmount);
             expectedValue += depositAmount;

Duplicate of #18