unforgiven - users can't specify round ID in depositETHIntoMultipleRounds and rollover function their deposits may put into different rounds and cause them loss

[sherlock-admin]

unforgiven

medium

users can't specify round ID in depositETHIntoMultipleRounds and rollover function their deposits may put into different rounds and cause them loss

Summary

when users deposits token with depositETHIntoMultipleRounds(), code deposits users token to current round. the issue is that user may want to deposit to current round but their tx stay in mempool for while and when their tx executed the current round has been changed and their funds get deposited into the wrong round. This is like slippage protection in the AMMs, the current behavior doesn't protect users from this and users may participate in a random round while they wanted to explicitly get into the specific round.

this issue exists for rollover function too.

Vulnerability Detail

This is depositETHIntoMultipleRounds() code, as you can see function doesn't have roundId and it start to deposit user tokens for current round roundsCount and users can't specify the round they want to deposit into:

    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);

        _setCutoffTimeIfNotSet(startingRound);

This makes users to become vulnerable for slippage scenarios. for example users see a good opportunity to deposit into the current round(ID1) and they want only to deposit into this ID1 round and they sign their transaction, but their transaction stuck in the mempool and when it's mined the current round is changed and it's not ID1 and user may lose their funds as this round is not according to their strategy.

it's true that the winner is random and it looks like there is no different between rounds but for these reasons the rounds can be different:

1. different round mean different price for ERC20 and ERC721 tokens and user may want to deposit his ETH into a round that have lower price for ERC20 or ERC721.
2. different rounds may have different parameters like duration or valuePerEntry.
3. user may have strategy to deposit to specific rounds.

also attacker can use front-run and fill the current rounds and cause user funds to be deposited into specific round that is not favorable for user. for example depositing huge ETH into a round that has low entry amount will result in loss(if a user deposit 100ETH and another user deposit 1ETH and the FEE is 10% then even so user1 will have higher chance of winning but the fee will be bigger than extra winning amount)

Impact

users funds will be deposited into random rounds that user didn't intended to do it. users can't perform their strategy and they may lose funds in the rounds that they didn't want to participate.

Code Snippet

https://github.com/sherlock-audit/2024-01-looksrare/blob/7d76b96a58a6aee38f23bb38b8a5daa3bdc03f7c/contracts-yolo/contracts/YoloV2.sol#L312-L321

Tool used

Manual Review

Recommendation

function should get roundID from the user and deposit ETH into those rounds.

[0xhiroshi]

https://github.com/LooksRare/contracts-yolo/pull/188

[Czar102]

From my understanding, it is users' responsibility to enforce a sufficient gas price and time window to deposit for the current round.

[0xunforgiven]

I want to add this to keep it as record.

issue https://github.com/sherlock-audit/2024-01-looksrare-judging/issues/131 is quite similar to the https://github.com/sherlock-audit/2024-01-looksrare-judging/issues/50. they are both in slippage protection category.

the reason you gave to invalidate #131 can be used against #50 too (or any other slippage protection):

From my understanding, it is users' responsibility to enforce a sufficient gas price and time window to deposit for the current round.

you assume user knows about this risk! user trust the project app's website that says deposit to current round. user don't know that it may be deposited into future rounds.

even if user set high gas price, there is no guarantee that when their tx will be included in the blockchain (attacker can create higher gas price txs, for cases that attack is more profitiable). this issue shows attack scenarios that cause loss for users. also forcing users to set high gas price(to avoid the issue) will cause loss of funds too, because users will pay avoidable high gas cost for their tx. (even so theire is no gurantee that it will work)

according to the comment round duration can be 2 to 5 min. this is very low duration in blockchain world and users can't make sure their tx will be mined in 2 minutes even if they knew this risk and try to mitigate it!

sponsor confirmed the issue(it shows code wasn't implemented based on their intention) and also sponsor fixed the issue.

furthermore the other deposit function in the code (for ERC721 and ERC20) only accept deposits to open round(only current round is open round) but still it requires user to send the roundID they want to deposit, it shows that developers clearly wanted users to set the round ID(and only allow deposit when roundID == current round) as they know not setting it would cause loss for users.

[0xunforgiven]

@0xhiroshi can you please give project point of view regarding Czar102's above comment(based on protocol's design):

From my understanding, it is users' responsibility to enforce a sufficient gas price and time window to deposit for the current round.

Project's view is very important because judge invalidated issue based on design choice.

[mstpr]

LooksRare/contracts-yolo#188

LGTM

[0xhiroshi]

@0xhiroshi can you please give project point of view regarding Czar102's above comment(based on protocol's design):

From my understanding, it is users' responsibility to enforce a sufficient gas price and time window to deposit for the current round.

Project's view is very important because judge invalidated issue based on design choice.

I think roundId should be specified so that players don't accidentally enter a round they don't intend to especially near deadline.