unforgiven - attacker can deposit ERC20 and ERC721 tokens with wrong type and receive huge amount of entries

[sherlock-admin2]

unforgiven

high

attacker can deposit ERC20 and ERC721 tokens with wrong type and receive huge amount of entries

Summary

users can deposit whitelisted ERC20 and ERC721 tokens and receive entries based on deposited tokens value. the issue is that there is only one whitelist token for ERC20 and ERC721 and code trust the token type user defined and user can define ERC20 type for actual NFT token or vice versa. this would cause wrong price and entry calculation and user would receive tremendous amount of entry. of course attacker need to create a Uniswap pool for NFT token or Reservoir oracle to support ERC20 tokens.

Vulnerability Detail

This is part of _deposit() code:

            for (uint256 i; i < deposits.length; ++i) {
                DepositCalldata calldata singleDeposit = deposits[i];
                address tokenAddress = singleDeposit.tokenAddress;
                if (isCurrencyAllowed[tokenAddress] != 1) {
                    revert InvalidCollection();
                }
                uint256 price = prices[tokenAddress][roundId];
                if (singleDeposit.tokenType == YoloV2__TokenType.ERC721) {
               if (price == 0) {
                        price = _getReservoirPrice(singleDeposit);
                        prices[tokenAddress][roundId] = price;
                    }

                    uint256 entriesCount = price / round.valuePerEntry;
                ...................
                ..................
                } else if (singleDeposit.tokenType == YoloV2__TokenType.ERC20) {
                    if (price == 0) {
                        price = erc20Oracle.getTWAP(tokenAddress, uint32(TWAP_DURATION));
                        prices[tokenAddress][roundId] = price;
                    }

                    uint256[] memory amounts = singleDeposit.tokenIdsOrAmounts;
                    if (amounts.length != 1) {
                        revert InvalidLength();
                    }

                    uint256 amount = amounts[0];

                    uint256 entriesCount = ((price * amount) / (10 ** IERC20(tokenAddress).decimals())) /
                        round.valuePerEntry;

as you can see code uses the input value deposits[] to check the token type and trust the user input. after that code calculates tokens value based on the type of the token. attacker can use this and fool the system and set a ERC20 token as ERC721 or vice versa. this would result in wrong value calculation and wrong entries count. attacker can perform one of these two:

1. set ERC20 as ERC721, then if attacker deposits 10 wei tokens code would use oracle to get price of the token and it would return the price 10^18 tokens in ETH(PRICE1) but code would assume that that the price of the 1 NFT token and the total value user would gain would be 10*PRICE1.
2. set ERC721 as ERC20, then if attacker deposits a NFT token with ID=(1000 * 1e18) code would try to get price of the token from uniswap pool. the price is ETH amount for 10^18 token (PRICE1) and as result would calculate the whole deposits value as 1000 * PRICE1.

because code would use transferFrom(address, address, uint256) to transfer ERC20 and ERC721 so there won't be a problem in transferring tokens. the only problem is the oracle price, attacker can bypass it with:

exploiting #1 is possible if Reservoir oracle add support ERC20 tokens price . it's a logical expectation for Reservoir oracle to add a price for ERC20 tokens too in Reservior as they expand their products. As code only checks the token address and signature and timestamp the ERC20 signed by Reservoir would pass the code checks. it would be the price of the 1e18 (18 as decimal) tokens but code would assume it's a NFT and its the price of each NFT.

exploiting #2 is possible if attacker creates a Uniswap pool for the ERC721 token. it's possible to create pool for ERC721 token as pool's code would use transferFrom(address, address, amount) to transfer tokens and so attacker can deposit NFTs and ETH as liquidity to the target pool and create a valid price for the NFT token in uniswap pool.

Impact

attacker can set wrong token type and as result the calculations will be wrong and attacker would receive a lot of entry point and win the round with very high probability and steal others deposits.

Code Snippet

https://github.com/sherlock-audit/2024-01-looksrare/blob/7d76b96a58a6aee38f23bb38b8a5daa3bdc03f7c/contracts-yolo/contracts/YoloV2.sol#L1095-L1110 https://github.com/sherlock-audit/2024-01-looksrare/blob/7d76b96a58a6aee38f23bb38b8a5daa3bdc03f7c/contracts-yolo/contracts/YoloV2.sol#L1161-L1178

Tool used

Manual Review

Recommendation

code shouldn't trust the user input and there should be two different whitelist for ERC20 and ERC721 tokens.

[0xhiroshi]

1. Reservoir's oracle does not support ERC-20, saying it's going to is a big assumption. Even if it ends up doing it we will have plenty of time to react.
2. One cannot create a Uniswap pool with an NFT unless it's a fractionalized token, and it would have been a different address anyway

[jpopxfile]

3. Even if somehow, a user is able to create a Uniswap pool with an NFT, for ERC-20, the pool in the oracle has to have been whitelisted

   function getTWAP(address token, uint32 secondsAgo) external view returns (uint256 price) {
      address pool = oracles[token];
       if (pool == address(0)) {
           revert PoolNotAllowed();
       }

[jpopxfile]

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

[nevillehuang]

request poc

An additional poc is not required, but I want to facilitate discussions between sponsor @0xhiroshi @jpopxfile and the watson

[sherlock-admin]

PoC requested from @0xunforgiven

Requests remaining: 8

[0xunforgiven]

Hi, there is a execution path in deposit() code that doesn't call oracle's price and use the fixed price for that round. when the token address price is fixed for that round code just uses prices[tokenAddress][roundId] as price and don't call oracle. (getTWAP() or _getReservoirPrice() don't get executed)

so attacker can bypass oracle calls by doing this:

1. first deposit TOKEN1 (ERC20) as ERC20 to the ROUND1 and fix the price for that TOKEN1's address for ROUND1. code would set prices[TOKEN1][ROUND1] as price of TOKEN1 for ROUND1.
2. then attacker would deposit TOKEN1 as ERC721 and because prices[TOKEN1][ROUND1] is set so code won't check the oracle price again(ignore oracle) and attack will happen.

the same can be done for ERC721 tokens. (first deposit as ERC721 then deposit as ERC20)

there is coded POC for this in #150

[nevillehuang]

@0xhiroshi Can you take a look at #150 PoC and double check if your above comments apply? I believe a mock contract used for resevoir oracle could potentially influence the validity of the PoC, but I cannot confirm.

[0xhiroshi]

@nevillehuang

Not sure what you meant by a mock contract used for Reservoir oracle.

The reason I think this is invalid and #150 is valid is that the contestant did not come up with the correct reason for the bug to occur, whereas in #150 the cause is explained very clearly and the contestant also came up with a PoC that works.

In this issue, the contestant mentioned exploit 1 is possible if Reservoir oracle supports ERC-20 and exploit 2 is possible if the attacker creates a Uniswap pool for ERC-721 tokens. Both are not the actual reason that causes the vulnerability.

I think the contestant kind of got the problem right, but was not able to explain how to exploit it.

[nevillehuang]

Agree, I think #4, #150 and #56 is valid, and this is invalid.

[mstpr]

LooksRare/contracts-yolo#183

Fix LGTM!