lil.eth - costToHeal() in InfiltrationPeriphery.sol allow easy sandwich and user paying more than healing cost

[sherlock-admin2]

lil.eth

medium

costToHeal() in InfiltrationPeriphery.sol allow easy sandwich and user paying more than healing cost

Summary

In InfitrationPeriphery.sol any user can calculate how much he must sent eth to heal it's agents, allowing him tu use ETH instead of Looks.
To do this the easiest way and recommended way seems to be using InfiltrationPeriphery.sol#costToHeal() that returns the amount of ETH needed to heal some NFT agents.
However costToHealInLOOKS is first calculated by making a call to QuoterV2. This simulates the transaction directly on the target pool. This is the fundamental problem with this design. If the pool is sandwich attacked then the expected out will also fall.
Amount returned will be false and then when user will submit the false amount of ETH to effectively heal it's agent he will open door for another sandwich attack where surplus amount will be stolen.

Vulnerability Detail

Example: Assume Alice wants to heal its 10 agents using ETH :

• current price of ETH is $1500
• 1 looks = 1$
• price to heal one agent = 150$ so price to heal 1 agent in eth should be 0.1 ETH

There is no slippage added as only costToHealInLooks is added (if (sqrtPriceLimitX96 == 0) require(amountOutReceived == amountOut);).

This means there is no highest price Alice should get :

1. Assume that an attacker is observing the mempool. They see the transaction and sandwich attack it.
2. First the sell ETH into the pool lowering the price to $1300.
3. When Alice transaction executes, QuoterV2 will quote the price of ETH as $1300 and therefore price to heal in eth will be 0,115 ETH instead of 0,1 for one agent
4. Alice send 0,115 ETH * 5 = 1.15 as msg.value in InfiltrationPeriphery.sol#heal() and she gets sandwiched as again only amountOut is needed for this transactionsqrtPriceLimitX96: 0 //E if (sqrtPriceLimitX96 == 0) require(amountOutReceived == amountOut);
5. Alice paid 0.15 ETH more than what she should have.

Here is the related code :

    function heal(uint256[] calldata agentIds) external payable {
        uint256 costToHealInLOOKS = INFILTRATION.costToHeal(agentIds);
        IV3SwapRouter.ExactOutputSingleParams memory params = IV3SwapRouter.ExactOutputSingleParams({
            tokenIn: WETH,
            tokenOut: LOOKS,
            fee: POOL_FEE, //E 3_000
            recipient: address(this),
            amountOut: costToHealInLOOKS,
            amountInMaximum: msg.value,
            sqrtPriceLimitX96: 0 //E if (sqrtPriceLimitX96 == 0) require(amountOutReceived == amountOut);
        });

        uint256 amountIn = SWAP_ROUTER.exactOutputSingle{value: msg.value}(params);

        IERC20(LOOKS).approve(address(TRANSFER_MANAGER), costToHealInLOOKS);

        INFILTRATION.heal(agentIds);

        if (msg.value > amountIn) {
            SWAP_ROUTER.refundETH();
            unchecked {
                _transferETHAndWrapIfFailWithGasLimit(WETH, msg.sender, msg.value - amountIn, gasleft());
            }
        }
    }

    function costToHeal(uint256[] calldata agentIds) external returns (uint256 costToHealInETH) {
        uint256 costToHealInLOOKS = INFILTRATION.costToHeal(agentIds);

        IQuoterV2.QuoteExactOutputSingleParams memory params = IQuoterV2.QuoteExactOutputSingleParams({
            tokenIn: WETH,
            tokenOut: LOOKS,
            amount: costToHealInLOOKS,
            fee: POOL_FEE,
            sqrtPriceLimitX96: uint160(0) //E if (sqrtPriceLimitX96 == 0) amountOutCached = params.amount = costToHealInLOOKS
        });

        (costToHealInETH, , , ) = QUOTER.quoteExactOutputSingle(params);
    }

Impact

• Users healing payment can be sandwich attacked due to ineffective slippage controls

Code Snippet

• https://github.com/Uniswap/v3-periphery/blob/main/contracts/lens/QuoterV2.sol#L197
• https://github.com/Uniswap/v3-periphery/blob/main/contracts/interfaces/IQuoterV2.sol
• https://github.com/sherlock-audit/2023-10-looksrare/blob/main/contracts-infiltration/contracts/InfiltrationPeriphery.sol#L78

Tool used

Manual Review

Recommendation

Allow users to specify an min/max sqrtPriceLimitX96. If the pool ever goes above/below that value then revert the call

[0xhiroshi]

costToHeal is a view/callStatic RPC call that happens before the heal transaction is submitted to the mempool and that's the amountInMaximum. So before a transaction is submitted there is already slippage protection because we know what msg.value we are going to send - it is based on the returned value that cannot be sandwiched.

https://github.com/sherlock-audit/2023-10-looksrare/blob/main/contracts-infiltration/test/foundry/Infiltration.healWithETH.t.sol#L56

[nevillehuang]

https://github.com/sherlock-audit/2023-10-looksrare/blob/main/contracts-infiltration/test/foundry/Infiltration.healWithETH.t.sol#L56

Could you expand more about this? I am not exactly sure how a view/callStatic RPC call exactly prevents price manipulation within a pool when slippage is not present.

[0xhiroshi]

We provide the msg.value as the amountInMaximum based on what we get from costToHeal when the price isn't artificially inflated, and we get the exact output we want. If the price is inflated beyond msg.value then the transaction should revert, right?

[nevillehuang]

@0xhiroshi @0xBunta

Wanted to bring up this interesting point by LSW. See #30 and #53 for more detailed explanation. However, the impact seems to only be limited to excess ETH, which logically speaking for a user performing a heal should not be significant because they shouldn't swap with excessive amounts in the first place.

The sponsor is right, the user has control over what msg.value he sends, and cannot pay higher than that. What can happen is that the user can send a msg.value a bit higher than what is needed (relying on the fact that there is other logic in this function which refunds excess eth) and be sandwiched for the excess eth, because no price limit specified.

[0xhiroshi]

Our frontend might add a tiny bit of buffer to make sure it goes through but even if we do that it is going to be a very tight percentage.

[nevillehuang]

Our frontend might add a tiny bit of buffer to make sure it goes through but even if we do that it is going to be a very tight percentage.

Front-end solutions are not considered in Sherlock rules for determining issue validity, though I can see your view with the protocol being a game that users will primarily be interacting with the protocol on the front-end where the cost of healing will be predetermined.

This statement here in #53 convince me that this could be a medium. Any thoughts? @0xhiroshi

When considering transactions made in haste (which is a reasonable expectation given the intended size of the audience versus the relative scarcity of healing), this could lead to significant unintentional material losses for unsuspecting users.

[0xhiroshi]

Our frontend might add a tiny bit of buffer to make sure it goes through but even if we do that it is going to be a very tight percentage.

Front-end solutions are not considered in Sherlock rules for determining issue validity, though I can see your view with the protocol being a game that users will primarily be interacting with the protocol on the front-end where the cost of healing will be predetermined.

This statement here in #53 convince me that this could be a medium. Any thoughts? @0xhiroshi

When considering transactions made in haste (which is a reasonable expectation given the intended size of the audience versus the relative scarcity of healing), this could lead to significant unintentional material losses for unsuspecting users.

That is if the user manually changes the ETH value being sent to the contract, if we follow the recommendation in issue 53, it can also be said that the user can manually change sqrtPriceLimitX96 as it's a client side parameter.

Indeed normal users aren't going to know how to change a function call parameter (while it is easier to change the ETH value) if it is a transaction through Metamask or some web wallets. But what if the user is in so much haste that he decides to go to Etherscan instead of our UI to submit the transaction? There he can set the price limit to whatever he likes.

Also, from the smart contract's point of view, is a client side provided msg.value really different from a client side provided sqrtPriceLimitX96? There is nothing stopping sqrtPriceLimitX96 to be set as 0.

[nevillehuang]

Agree with sponsor. Additionally, impact is only limited to excess swaps so assigning as low severity.