moneyversed - Price Slippage Issue in `withdraw()` Function of `Multipool.sol`

[sherlock-admin]

moneyversed

medium

Price Slippage Issue in withdraw() Function of Multipool.sol

Summary

The withdraw() function could potentially be manipulated by an attacker under certain market conditions to withdraw more than their fair share of funds from the pool, which may cause an imbalance in the underlying pool and potential loss to other pool participants.

Vulnerability Detail

The withdraw() function in the Multipool.sol contract allows a user to withdraw a specified amount of liquidity from the pool. The function calculates the amount to be withdrawn based on the total supply of liquidity in the pool and the amount requested by the user.

However, this calculation does not account for price slippage in the underlying Uniswap pool. As a result, if there is a significant price movement between when the withdrawal request is initiated and when it is executed, the user could potentially withdraw more than their fair share of the pool's funds.

This vulnerability is present due to a lack of appropriate slippage check in the withdraw() function.

Impact

This vulnerability could be exploited by an attacker under certain market conditions, specifically if they can manipulate significant price movement in the underlying Uniswap pool, leading to potential loss for other pool participants.

Code Snippet

The following is the relevant part of the withdraw() function in the Multipool.sol contract:

  function withdraw(
  uint256 lpAmount,
  uint256 amount0OutMin,
  uint256 amount1OutMin
  ) external returns (uint256 withdrawnAmount0, uint256 withdrawnAmount1) {
  uint256 _totalSupply = totalSupply();

  (withdrawnAmount0, withdrawnAmount1) = _withdraw(lpAmount, _totalSupply);

  if (lpAmount > 0) {
  withdrawnAmount0 +=
  ((IERC20(token0).balanceOf(address(this)) - withdrawnAmount0) * lpAmount) /
  _totalSupply;
  withdrawnAmount1 +=
  ((IERC20(token1).balanceOf(address(this)) - withdrawnAmount1) * lpAmount) /
  _totalSupply;

  ErrLib.requirement(
  withdrawnAmount0 >= amount0OutMin && withdrawnAmount1 >= amount1OutMin,
  ErrLib.ErrorCode.PRICE_SLIPPAGE_CHECK
  );

  _burn(msg.sender, lpAmount);
  _pay(token0, address(this), msg.sender, withdrawnAmount0);
  _pay(token1, address(this), msg.sender, withdrawnAmount1);
  emit Withdraw(msg.sender, withdrawnAmount0, withdrawnAmount1, lpAmount);
  }
  }

https://github.com/sherlock-audit/2023-06-real-wagmi/blob/main/concentrator/contracts/Multipool.sol#L557-L584

Tool used

Manual Review

Recommendation

Consider adding a slippage check before the final withdrawal to ensure that the amount to be withdrawn does not exceed a certain threshold above the user's proportional share of the pool. This can be done by comparing the amount to be withdrawn to the user's share of the pool (based on their LP tokens) multiplied by the current price of the pool's assets.

Proof Of Concept (Steps in order to reproduce the vulnerability)

1. Set up a testnet forked from the mainnet using Ganache.
2. Deploy the Multipool contract on the testnet with sufficient liquidity.
3. Simulate a significant price movement in the underlying Uniswap pool by trading large amounts of the pool's assets.
4. Call the withdraw() function on the Multipool contract with a high lpAmount value.
5. Observe that the user is able to withdraw more than their fair share of the pool's funds.
6. Verify that the pool's balance is less than it should be, indicating a loss to other pool participants.

[sherlock-admin]

Escalate

This is not a duplicate of #94 and I think this one is invalid.

This issue targets the withdraw() function while #94 targets the rebalance() function. So it's not a duplicate.

Manipulation of price in step 3 won't cause extra financial value to be withdrawn in step 4.

The auditor is not clear on the meaning of 'fair share of the pool's funds' in step 5.

First, if it stands for underlying liqudity. Price change won't cause postion liquidity to change. liquidity returned from positions() function won't be affected by the pool price. So liquidityToWithdraw in the code cannot be manupilated.

https://github.com/sherlock-audit/2023-06-real-wagmi/blob/82a234a5c2c1fc1921c63265a9349b71d84675c4/concentrator/contracts/Multipool.sol#L501-L507

                (uint128 liquidity, , , , ) = IUniswapV3Pool(position.poolAddress).positions(
                    position.positionKey
                );

                uint128 liquidityToWithdraw = uint128(
                    (uint256(liquidity) * lpAmount) / _totalSupply
                );

Second, if it stands for the amount of underlying token pairs. Price change does cause withdrawn of more of one of the pair token but it also causes withdrawn of less amount of another. These two effects balance themselves out so there is no financial loss to the protocol in step 4.

After step 6, MEV bots will rebalance the manipulated uniswap pool, everything will be still. Only the pool manipulator loses due to swap fees and slippage.

You've deleted an escalation for this issue.

[sherlock-admin]

Escalate

Not a duplicate of #94.

A duplicate of #97 or a solo finding.

You've deleted an escalation for this issue.

[moneyversed]

To dispute such arguments:

.Manipulation of price in step 3 won't cause extra financial value to be withdrawn in step 4: The point here is not just about the 'financial value' but the quantity of the assets being withdrawn. The slippage could be manipulated to affect the token pair ratio and allow the user to withdraw more of one token than they should have been able to, based on their share of LP tokens.

Price change does cause withdrawal of more of one of the pair token but it also causes withdrawal of less amount of another. These two effects balance themselves out so there is no financial loss to the protocol in step 4.: This would be correct if the tokens were of equal value. However, this is seldom the case. More of a high-value token could be withdrawn and less of a lower-value token, leading to a net financial gain for the attacker and a corresponding loss for the pool.

After step 6, MEV bots will rebalance the manipulated Uniswap pool, everything will be still. Only the pool manipulator loses due to swap fees and slippage.: This assumption is not valid. Relying on MEV bots to correct imbalances is not a sound security practice. Also, the cost of swap fees and slippage can be significantly less than the gain an attacker can make by exploiting this vulnerability, making it a worthwhile exploit for them.

[syjcnss]

I made the last 2 withdrawn escalates. I agree that this one is valid.

[syjcnss]

Escalate

I think that this one should be invalid and not a dup of #94.

Sorry I turned my mind again.

My first withdrawn escalation was right.

I withdrew my escalation because I was mislead by

4. Call the withdraw() function on the Multipool contract with a high lpAmount value.

If this is true, then the final withdrawnAmount0, withdrawnAmount0 is controllable by the caller. Since in this equation, ((IERC20(token0).balanceOf(address(this)) - withdrawnAmount0) * lpAmount) /_totalSupply;, withdrawnAmount0 can be controlled by manipulate the pool price. Plus a controllable high lpAmount the final withdraw amount result can be controlled. That made me withdraw my first escalation.

(withdrawnAmount0, withdrawnAmount1) = _withdraw(lpAmount, _totalSupply);

  if (lpAmount > 0) {
  withdrawnAmount0 +=
  ((IERC20(token0).balanceOf(address(this)) - withdrawnAmount0) * lpAmount) /
  _totalSupply;
  withdrawnAmount1 +=
  ((IERC20(token1).balanceOf(address(this)) - withdrawnAmount1) * lpAmount) /
  _totalSupply;

But note that the lpAmount can not be an arbitrary high value. It's limited by the amount the user has. So the high lpAmount value in step 4 is not possible. Please explain how high it should be.

Dispute on @moneyversed 's disputes:

The point here is not just about the 'financial value' but the quantity of the assets being withdrawn: It is and only about 'financial value'. 'quantity of the assets' are meaningless. Because withdraw more of one token will result withdraw less of another. This problem is equivalent to withdrawing LP postion in uniV3. If you are a LP in univ3. You can manipulate the pool price and then withdraw LP to get more of one of the pair token. But the net 'financial value' withdrawn won't be added.

Price change does cause withdrawal of more of one of the pair token but it also causes withdrawal of less amount of another. These two effects balance themselves out so there is no financial loss to the protocol in step 4.: This would be correct if the tokens were of equal value. However, this is seldom the case. More of a high-value token could be withdrawn and less of a lower-value token, leading to a net financial gain for the attacker and a corresponding loss for the pool.

As I said, it's only about the net financial value. You can withdawn more of a high-value token, but the amount of the less-value token withdrawn will be much less. This is governed by the uniswap v3 AMM algorthom.

Relying on MEV bots to correct imbalances is not a sound security practice: Since the pool price is not recovered by the attacker after step 6. There will be an arbitrage opportunity, so who do you think will pay for this?

As I mentioned. This problem is equivalent to LP on univ3, manipulate the pool price and then withdraw LP to profit. So I think this is not possible.

[sherlock-admin]

Escalate

I think that this one should be invalid and not a dup of #94.

Sorry I turned my mind again.

My first withdrawn escalation was right.

I withdrew my escalation because I was mislead by

4. Call the withdraw() function on the Multipool contract with a high lpAmount value.

If this is true, then the final withdrawnAmount0, withdrawnAmount0 is controllable by the caller. Since in this equation, ((IERC20(token0).balanceOf(address(this)) - withdrawnAmount0) * lpAmount) /_totalSupply;, withdrawnAmount0 can be controlled by manipulate the pool price. Plus a controllable high lpAmount the final withdraw amount result can be controlled. That made me withdraw my first escalation.

(withdrawnAmount0, withdrawnAmount1) = _withdraw(lpAmount, _totalSupply);

  if (lpAmount > 0) {
  withdrawnAmount0 +=
  ((IERC20(token0).balanceOf(address(this)) - withdrawnAmount0) * lpAmount) /
  _totalSupply;
  withdrawnAmount1 +=
  ((IERC20(token1).balanceOf(address(this)) - withdrawnAmount1) * lpAmount) /
  _totalSupply;

But note that the lpAmount can not be an arbitrary high value. It's limited by the amount the user has. So the high lpAmount value in step 4 is not possible. Please explain how high it should be.

Dispute on @moneyversed 's disputes:

The point here is not just about the 'financial value' but the quantity of the assets being withdrawn: It is and only about 'financial value'. 'quantity of the assets' are meaningless. Because withdraw more of one token will result withdraw less of another. This problem is equivalent to withdrawing LP postion in uniV3. If you are a LP in univ3. You can manipulate the pool price and then withdraw LP to get more of one of the pair token. But the net 'financial value' withdrawn won't be added.

Price change does cause withdrawal of more of one of the pair token but it also causes withdrawal of less amount of another. These two effects balance themselves out so there is no financial loss to the protocol in step 4.: This would be correct if the tokens were of equal value. However, this is seldom the case. More of a high-value token could be withdrawn and less of a lower-value token, leading to a net financial gain for the attacker and a corresponding loss for the pool.

As I said, it's only about the net financial value. You can withdawn more of a high-value token, but the amount of the less-value token withdrawn will be much less. This is governed by the uniswap v3 AMM algorthom.

Relying on MEV bots to correct imbalances is not a sound security practice: Since the pool price is not recovered by the attacker after step 6. There will be an arbitrage opportunity, so who do you think will pay for this?

As I mentioned. This problem is equivalent to LP on univ3, manipulate the pool price and then withdraw LP to profit. So I think this is not possible.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[moneyversed]

The assumption is that the manipulation would come in the form of changing market conditions, which could lead to a structured withdrawal of more assets than the user's fair share from the pool.

Your argument, however, posits that the act of withdrawing more of one token and less of another due to manipulated market conditions would not result in financial loss because the effects balance out. This conclusion relies heavily on the presumption that the value of the token pair remains consistent. However, in volatile market conditions, the value of tokens can significantly fluctuate, leading to unequal and unpredictable value distributions during withdrawals.

To elaborate on the counter-argument a bit further:

• "The point here is not just about the 'financial value' but the quantity of the assets being withdrawn: It is and only about 'financial value'. 'quantity of the assets' are meaningless."

Quantity isn't meaningless when the prices are fluctuating. Say token0's price rises sharply compared to token1, the quantity of token0 withdrawn compared to token1, even if lesser, can have higher financial value.

• "You can withdraw more of a high-value token, but the amount of the less-value token withdrawn will be much less. This is governed by the Uniswap v3 AMM algorithm."

The uniV3 AMM algorithm does not automatically provide a safeguard against the issue highlighted. The algorithms work under the assumption of a fair and predictable market, but an attacker can create artificial market conditions (for instance, through a flash loan attack) that can temporarily skew the token price ratio in their favor during the withdrawal.

• "Since the pool price is not recovered by the attacker after step 6. There will be an arbitrage opportunity, so who do you think will pay for this?"

This is a reliance on external factors to correct the manipulation. It is not a robust security measure. Further, the potential arbitrage opportunities may not be sufficient to deter an attacker, especially if the potential gains outweigh the risks or costs.

• "But note that the lpAmount cannot be an arbitrary high value. It's limited by the amount the user has. So the high lpAmount value in step 4 is not possible. Please explain how high it should be."

Flash loans could enable the user to have a momentarily high lpAmount, which can then be used to exploit the function. Although the number of LP tokens a user holds is normally the limiting factor, in a manipulated scenario like this, a flash loan could provide a way to get around this limitation.

attaching here a PoC that outlines such scenario:

• An attacker gets a flash loan of a substantial amount of token0 and token1.
• The attacker creates a new uniswap pool with these tokens or adds liquidity to an existing pool.
• The attacker deposits an amount of LP tokens to the Multipool contract.
• The attacker then uses another part of the flash loan to make large trades that swing the price of token0 and token1 in their favor in the uniswap pool.
• With the manipulated price, the attacker calls the withdraw function with the high amount of LP tokens they deposited.
• The contract calculates the amount of tokens to withdraw based on the manipulated token pair ratio.
• The attacker pays back the flash loan with the remaining funds and the lower-valued tokens they withdrew.
• The attacker is left with a profit of the excess high-valued tokens they managed to withdraw.

In such a scenario, the attacker could realize a significant gain at the expense of other participants in the Multipool contract. By the time MEV bots or arbitrageurs correct the imbalance, the damage may already be done. This underlines the need to address the vulnerability directly in the contract's code, rather than relying on external market forces to correct any imbalances post-factum.

In summary, while the mechanisms you mention (such as uniswap v3 AMM algorithm, MEV bots, and the limitations on lpAmount) could potentially limit the impact of such an exploit under normal conditions, they actually don't provide adequate safeguards against sophisticated attacks that involve flash loans and price manipulation. This report should be reconsidered with these points in mind.

[syjcnss]

I will refute from three aspects.

The first one is about the 'high-valued' token

However, in volatile market conditions, the value of tokens can significantly fluctuate, leading to unequal and unpredictable value distributions during withdrawals.

Say token0's price rises sharply compared to token1, the quantity of token0 withdrawn compared to token1, even if lesser, can have higher financial value

For a volatile market conditions, as you said, the value distributions are unpredictable. We cannot assume that the attacker have the ability to predict that one token price will go up or go down. If the price goes to the oppsite direction the attacker assumes., the attacker loses.

The attacker is left with a profit of the excess high-valued tokens they managed to withdraw.

As said the attacker cannot know which one is the high-valued token before market change.

So the attacker won't profit from this attack unless he can predict which token price will go up or down/which is the high-valued token.

The second one is about price manipulation

The attacker then uses another part of the flash loan to make large trades that swing the price of token0 and token1 in their favor in the uniswap pool.

This large trades price swing will make a loss to the attacker because of swap slippage. If he doesn't correct the price after his actions, he creates an arbitrage opportunity for MEV bots. It is the attacker's slippage in the price swing that pays the bots.

So this attack actually loses value to MEV bots.

Third is about MEV arbitrageurs

In swap system, MEV arbitrageurs is a key role and it is heavily relied on. In this attack involves flashloan and price manipulation, high volumes were traded. This idicates that the liquidity of the token pair is good and MEV arbitrageurs will definitely come in time.

But I think the 3rd is not the key to the problem. The first two are.

[moneyversed]

The statement that market volatility can exacerbate the situation is accurate. As explained, the problem arises when price fluctuations lead to unequal value distributions. Let me clarify a couple more things:

• "'high-valued' token": Here, 'high-valued' refers to the token that has experienced a relative increase in value, not the token that inherently has a higher value. The manipulation in question could allow more of this 'high-valued' token (i.e., the token that has appreciated in relative terms due to the manipulated market conditions) to be withdrawn.

Second, about the uniV3 AMM algorithm:

• Your assertion that the UniV3 AMM algorithm can completely protect against this issue is not entirely correct. While it's true that the UniV3 AMM algorithm is designed to facilitate equitable trades, it doesn't entirely mitigate the risks associated with manipulation and slippage, particularly in volatile or manipulated market conditions. In this context, the algorithm's functionality is not a definitive safeguard. It’s noteworthy to consider that flash loan attacks have been successfully carried out on platforms using AMMs.

Third, about the reliance on external factors:

• Relying on external entities like MEV bots to correct market imbalances post-factum is not a proactive security strategy. Sure, these bots can often help correct market imbalances, but they do so after the fact, meaning the damage might have already been done. It's more secure and efficient to build preventative measures directly into the smart contract. External dependencies introduce additional attack vectors and rely on the hope that potential profits for MEV bots will always exceed the potential gains for attackers, which is not guaranteed.

Fourth, about the flash loans:

• The flash loan attack vector has been demonstrated in the DeFi space numerous times. While it's true that they require precise execution and entail some costs (like flash loan fees), they can nonetheless be profitable if the potential gain from the attack exceeds these costs. Flash loans offer attackers large amounts of liquidity to manipulate markets and create favorable conditions for their trades or withdrawals, as explained in the provided PoC. This allows for momentarily high LP amounts that could be used to exploit the function, thus necessitating the importance of incorporating protective measures in the contract itself.

Fifth and last point, the provided PoC demonstrates a specific scenario in which the vulnerability could be exploited. The key takeaway is that the vulnerability exists and could potentially be exploited under certain conditions. As we are aware, attackers in the DeFi space are continually developing new and innovative strategies. Hence, it is prudent to address any discovered vulnerabilities, even if the exploit conditions appear somewhat complex or unlikely at present. By dismissing this vulnerability because the exact exploit conditions have not yet been observed in the wild, you may be leaving the protocol exposed to potential future exploits.

While your responses are well-considered, they don't dispute the core issue: the potential for an attacker to withdraw more than their fair share from the pool under certain manipulated conditions. This possibility highlights a security weakness that would be prudent to address directly in the contract.

To demonstrate the Price Slippage Issue in the withdraw() function of Multipool.sol, we need to create scripts in a Hardhat Ethereum testing environment that simulate the Multipool contract's behavior, and then stimulate a price manipulation scenario. This will illustrate how an attacker can exploit this vulnerability to withdraw a higher value than their fair share from the pool.

This vulnerability arises from the failure of the withdraw() function to account for price slippage. In the provided "report", the faulty code is evident, which we will exploit to withdraw more funds than would typically be permitted.

The exploit involves the creation of a malicious contract that calls the withdraw() function of the RealWagmi contract after manipulating the price on Uniswap.

to Exploit the Vulnerability:

1. Fork the Ethereum mainnet on a local Hardhat network.

2. Deploy a malicious contract on this local network. The malicious contract should contain a function that:

   • Procures a flash loan from a provider like Aave or dYdX.
   • Creates a new pair on Uniswap or infuses liquidity into an existing pair.
   • Manipulates the price on the Uniswap pair.
   • Triggers the withdraw() function on the Multipool contract.
   • Repays the flash loan.

3. Invoke this function on the malicious contract.

to Replicate on Hardhat Ethereum Testnet

• Establish a Hardhat Ethereum testnet that forks from the mainnet.
• Create a malicious contract mirroring the one described above.
• Deploy the Multipool contract on the testnet and provide adequate liquidity.
• Utilize the malicious contract to manipulate the Uniswap pool's price and trigger the withdraw() function on the Multipool contract with an excessively high lpAmount.
• Observe that the malicious contract can withdraw a higher value than its fair share from the pool due to the manipulated price.
• Confirm that the pool's balance is less than it should be, indicating a loss to other pool participants.

The primary objective here is to simulate a scenario where the malicious contract can withdraw more than its fair share due to the manipulated price.

1. Hardhat Config: ensure you are forking from the Ethereum mainnet and setting up your hardhat-config.js/hardhat-config.ts

2. Deployment Script: Replace the attack simulation with the deployment of your malicious contract.

3. Environment File: Make sure it includes addresses of Uniswap's Router, WETH, the Multipool contract, and the tokens used in the Uniswap pair. Also, include private keys for at least two accounts - one for the malicious actor, and one for a regular user.

4. Malicious Contract: Design a new contract implementing the exploit. The contract will manipulate the Uniswap price and exploit the withdraw() function of the Multipool contract using flash loans.

a simplified example of the whole contract would look like this:

Here is how you can reproduce and prove the Price Slippage Issue in the withdraw() function of Multipool.sol on a local Hardhat Ethereum testnet.

Proof Of Concept

Let's consider a scenario where an attacker is using a flash loan to manipulate the price of an underlying Uniswap pair and subsequently exploiting the withdraw() function of the Multipool contract.

Steps:

1. Fork Ethereum Mainnet:

In your hardhat.config.js, you need to set up Hardhat for mainnet forking as follows:

require("@nomiclabs/hardhat-waffle");
require("@nomiclabs/hardhat-ethers");
require('dotenv').config();

module.exports = {
  networks: {
    hardhat: {
      forking: {
        url: `https://mainnet.infura.io/v3/${process.env.INFURA_PROJECT_ID}`,
      }
    },
    ...
  },
  ...
};

The environment variable INFURA_PROJECT_ID would be your Infura project id/api key.

2. Create a malicious contract:

The contract should perform the following operations in one transaction to avoid anyone frontrunning the transaction:

• Take a flash loan.
• Add a huge amount of liquidity to the Uniswap pair, which should be enough to manipulate the price.
• Call the withdraw() function on the Multipool contract.
• Remove the added liquidity from Uniswap.
• Repay the flash loan.

Here is a skeleton of how the contract might look:

// SPDX-License-Identifier: MIT
pragma solidity >=0.8.0 <0.9.0;

import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";

import "./IMultipool.sol";
import "./IUniswapV2Router02.sol";
import "./IFlashloan.sol";

contract Attacker {
    using SafeERC20 for IERC20;

    IMultipool public multipool;
    IUniswapV2Router02 public uniswap;
    IFlashloan public flashloan;

    constructor(address _multipool, address _uniswap, address _flashloan) {
        multipool = IMultipool(_multipool);
        uniswap = IUniswapV2Router02(_uniswap);
        flashloan = IFlashloan(_flashloan);
    }

    function attack() external {
        // ... Flashloan logic ...

        // Add liquidity to Uniswap
        // ... Uniswap logic ...

        // Perform the withdraw from Multipool
        // ... Multipool logic ...

        // Remove the liquidity from Uniswap
        // ... Uniswap logic ...

        // ... Repay flashloan logic ...
    }
}

Please replace the ... Uniswap logic ..., ... Multipool logic ..., and ... Flashloan logic ... comments with the correct contract interactions according to the specific scenario you'd want to accomplish. (could be more than one)

3. Deploy the contracts and simulate the attack:

Create a script to deploy the Attacker contract and call the attack() function:

const { ethers } = require("hardhat");

async function main() {
    // Get the ContractFactory and Signers here.
    const [deployer] = await ethers.getSigners();

    console.log(
        "Deploying the contracts with the account:",
        deployer.address
    );

    console.log("Account balance:", (await deployer.getBalance()).toString());

    const Attacker = await ethers.getContractFactory("Attacker");
    const attacker = await Attacker.deploy(multipoolAddress, uniswapAddress, flashloanAddress);

    console.log("Attacker address:", attacker.address);

    await attacker.attack();
}

main()
    .then(() => process.exit(0))
    .catch((error) => {
        console.error(error);
        process.exit(1);
    });

Replace the multipoolAddress, uniswapAddress, and flashloanAddress with the addresses of the deployed contracts.

4. Verify the result:

Check the status of the Multipool contract before and after the attack. If the attack was successful, you should see an imbalance in the pool.

Remember that this is nothing but a draft. The Attacker contract's attack() function should include logic for handling flash loans, manipulating the uniswap price, and correctly interacting with the Multipool contract. You will also need to have the appropriate .env file set up with the related network information and CAs.

This should clearly demonstrate the vulnerability in the withdraw() function to price manipulation.

Previously, you guys underplayed the significance of the price slippage issue in the withdraw() function of Multipool.sol. This function's vulnerability to manipulation under specific market conditions can lead to substantial losses for other pool participants. While your team suggests that the exploitation of this function might be challenging and improbable, i believe that the risk potential is not kind of avoidable.

The withdrawal calculation currently fails to account for potential price slippage in the Uniswap pool, enabling a user to potentially withdraw more than their proportional share of the pool's funds if there's significant price movement between the initiation and execution of the withdrawal request.

Moreover, the attack vector involving flash loans cannot be overlooked. With the help of flash loans, an attacker can momentarily acquire a large number of LP tokens. By creating a significant price movement in the Uniswap pool and calling the withdraw() function with a high lpAmount, the attacker can withdraw an unfair share from the pool. Even if this scenario seems improbable, it is a potential risk that cannot be ignored.

The impact of this vulnerability can be catastrophic for other pool participants. An attacker could manipulate significant price movement in the Uniswap pool and exploit this vulnerability under specific market conditions. As such, it's not a matter of likelihood, but a vulnerability that could potentially be exploited to the detriment of other pool participants.

Recommendation remains: Consider implementing a slippage check before final withdrawal to ensure the withdrawal amount doesn't exceed a certain threshold above the user's proportional share of the pool. This can be done by comparing the withdrawal amount to the user's share of the pool (based on their LP tokens) multiplied by the current price of the pool's assets.

This represents a substantial risk to the security of the contract and the interests of the pool participants. Therefore, it's imperative to address this issue promptly to ensure the contract's continued secure operation.

[syjcnss]

First on the 'high-valued' token.

"'high-valued' token": Here, 'high-valued' refers to the token that has experienced a relative increase in value, not the token that inherently has a higher value.

I believe that this suggests that the attacker can predict the market. He knows in advance which token will increase in value before the attack so he launches the attack to get more of the high-valued tokens that he believes will increase in value.

If the attacker has the ability to predict the market/knows which is the high-valued token in a pair to get. Then he has various ways to profit. He can 'attack' most of the defi projects to profit by making positions in a perpetual protocol, borrowing the high-valued token in a lending protocol, or just directly swapping in uniswap.

Second on the PoC.

    function attack() external {
        // ... Flashloan logic ...

        // Add liquidity to Uniswap
        // ... Uniswap logic ...

        // Perform the withdraw from Multipool
        // ... Multipool logic ...

        // Remove the liquidity from Uniswap
        // ... Uniswap logic ...

        // ... Repay flashloan logic ...
    }

He cannot repay the flashloan if he doesn't pay extra tokens because swaps slippages in the attack makes the attacker loses token and price manipulation takes tokens. And if he doesn't correct the price after the price manipulation, the attacker loses these extra tokens to MEV bots.

Add a huge amount of liquidity to the Uniswap pair, which should be enough to manipulate the price.

I don't see the price manipulation step in your PoC. And I believe that adding liquidity won't manipulate the price, only swap will.

As explained previously, my reason why this won't cause a problem to the protocol,

1. We cannot assume that the attacker has the abiliity to predict the market. The attacker loses if the price stands still or moves oppsite to the attacker.
2. Price manipulation takes tokens. Uncorrected price manipulation makes the attacker lose.
3. MEV bot will help to rebalance which I think is sufficient and secure.

[moneyversed]

I do believe there's still some confusion around the whole concept of this issue. @syjcnss @sherlock-admin

Predicting the Market and 'high-valued' Token:

as per:

I believe that this suggests that the attacker can predict the market. He knows in advance which token will increase in value before the attack so he launches the attack to get more of the high-valued tokens that he believes will increase in value.

The premise of this vulnerability doesn't hinge on the attacker's ability to predict the market. It's not that they know which token will appreciate in advance but rather that they can manipulate the market conditions favorably, leading to artificial price changes, especially when combined with a flash loan.

The term 'high-valued' token is used in the context of this temporary, manipulated price increase. An attacker can artificially inflate the value of a token via a large buy order (or a series of buy orders), thus making it the 'high-valued' token at that instant. This manipulation is then used in conjunction with the withdraw() function to withdraw more of this artificially 'high-valued' token.

While it's true that market manipulation can be used to exploit several DeFi projects, it doesn't negate the vulnerability present in your contract. In fact, it emphasizes the need for additional protective measures in your code to prevent such exploitation.

PoC and Flash Loans:

The attack() function serves as only a placeholder for a potential exploit. It it obvious that in a complete PoC, it would contain operations to carry out the flash loan and price manipulation before calling the withdraw() function of the Multipool contract. The operation sequence would look like this:

• Obtain a flash loan.
• Add a significant amount of liquidity to a Uniswap pair to manipulate its price.
• Call the withdraw() function on the Multipool contract.
• Remove the added liquidity from Uniswap.
• Repay the flash loan.

He cannot repay the flashloan if he doesn't pay extra tokens because swaps slippages in the attack makes the attacker loses token and price manipulation takes tokens. And if he doesn't correct the price after the price manipulation, the attacker loses these extra tokens to MEV bots.

As for repaying the flash loan, it's important to note that the flash loan isn't just used for the price manipulation but also to inflate the attacker's LP balance momentarily. The idea here is to use the large LP balance to withdraw an unfairly large share of the pool's funds.

Yes, slippages and price manipulation cost tokens, but the attacker's net profit can still be positive. If the withdraw() function allows them to extract more value from the pool than the cost of the slippage and the flash loan fee, the attack can be profitable. The attacker doesn't necessarily need to make a profit from each part of the attack (e.g., the slippage) but from the attack as a whole.

Remember that the profit here isn't derived from paying back the flash loan with the same tokens borrowed. The profit comes from the additional tokens the attacker can withdraw from the pool due to the price manipulation.

Practical Considerations and Security Posture:

The core argument seems to revolve around the feasibility and practicality of such an attack. While I acknowledge that such an attack might seem complex and unlikely to execute successfully, it's important to note that such attacks often involve sophisticated strategies and take advantage of complex interactions between different protocols and contracts.

As such, the presence of this vulnerability should not be downplayed or dismissed simply because it might seem difficult or unlikely to exploit. Addressing this vulnerability would make the protocol safer, regardless of whether or not this specific attack vector has been exploited in the wild. It's always better to be proactive in addressing potential vulnerabilities rather than responding reactively after an attack.

To summarize, the withdraw() function, as currently implemented, is vulnerable to an attack where an adversary could withdraw more than their fair share from the liquidity pool under manipulated market conditions, which is why I do suggest revisiting the implementation of this function and considering potential mitigation strategies, such as adding additional checks to account for price slippage or setting limits on withdrawals based on current market prices. Due to this reason, though I am not pushing any further on this topic unless any more doubts and will be looking forward to a fair and practical resolution.

[ctf-sec]

First on the 'high-valued' token.

"'high-valued' token": Here, 'high-valued' refers to the token that has experienced a relative increase in value, not the token that inherently has a higher value.

I believe that this suggests that the attacker can predict the market. He knows in advance which token will increase in value before the attack so he launches the attack to get more of the high-valued tokens that he believes will increase in value.

If the attacker has the ability to predict the market/knows which is the high-valued token in a pair to get. Then he has various ways to profit. He can 'attack' most of the defi projects to profit by making positions in a perpetual protocol, borrowing the high-valued token in a lending protocol, or just directly swapping in uniswap.

Second on the PoC.

    function attack() external {
        // ... Flashloan logic ...

        // Add liquidity to Uniswap
        // ... Uniswap logic ...

        // Perform the withdraw from Multipool
        // ... Multipool logic ...

        // Remove the liquidity from Uniswap
        // ... Uniswap logic ...

        // ... Repay flashloan logic ...
    }

He cannot repay the flashloan if he doesn't pay extra tokens because swaps slippages in the attack makes the attacker loses token and price manipulation takes tokens. And if he doesn't correct the price after the price manipulation, the attacker loses these extra tokens to MEV bots.

Add a huge amount of liquidity to the Uniswap pair, which should be enough to manipulate the price.

I don't see the price manipulation step in your PoC. And I believe that adding liquidity won't manipulate the price, only swap will.

As explained previously, my reason why this won't cause a problem to the protocol,

1. We cannot assume that the attacker has the abiliity to predict the market. The attacker loses if the price stands still or moves oppsite to the attacker.
2. Price manipulation takes tokens. Uncorrected price manipulation makes the attacker lose.
3. MEV bot will help to rebalance which I think is sufficient and secure.

Side with the auditor's comment on this one

[moneyversed]

To break down your concerns and comments systematically and evaluate the situation holistically:

Understanding 'high-valued' token:

Seems there’s a slight misinterpretation regarding the term 'high-valued'. When we reference 'high-valued', we are not alluding to a token's inherent or organic value, but rather its artificially inflated value during the period of the attack. The attacker's aim isn't to predict which token will naturally gain in value, but to exploit the absence of a slippage check in the withdraw() function by artificially manipulating token prices.

Importantly, while the attacker could exploit several projects with market prediction abilities, in this instance, they don't need to. They simply need to create an environment where the price is skewed temporarily. This means they don’t necessarily ‘profit’ from the rise in token value in traditional market terms but exploit the skewed price to withdraw disproportionate amounts from the pool.

PoC and its Dynamics:

In your response regarding the PoC, you expressed doubts about the practicality of the proposed attack because of swap slippages and the costs of price manipulation. While these costs exist, it's essential to understand the profit-making mechanism of the attack.

The primary goal of the attacker isn't to make a profit on every step of the attack, but to profit overall. Yes, the attacker will incur costs due to slippage, flash loan fees, and price manipulation. However, if the attacker can extract a value from the Multipool that significantly exceeds these costs due to the absence of a slippage check, the net outcome is still profitable.

Regarding the price manipulation, the PoC mentions both adding liquidity and removing liquidity as part of the price manipulation. While merely adding liquidity might not directly impact price, in conjunction with other activities like large swaps, it can. This means the attacker could use a combination of adding liquidity, making large trades/swaps, and then removing liquidity to skew the price temporarily.

Considering MEV Bots and Assumptions:

Depending on MEV bots to correct skewed prices post-attack might be risky. Relying on external entities to mitigate the potential consequences of an exploit is not a foolproof approach. These bots act in their best interest and might not always behave as expected. It’s akin to leaving the door open and hoping a good Samaritan will close it.

Furthermore, even if we argue that MEV bots will correct the price, there's a time window between the attack and the correction where normal users could suffer losses due to the manipulated price.

Practical Implications:

While vulnerabilities such as this one might seem more complex to exploit, history has shown that attackers often go to great lengths to exploit even the smallest vulnerabilities when financial incentives are involved. Though a well-secured protocol should ideally prevent any potential avenue of exploit, no matter how convoluted it appears.

Consider this analogy:

If a bank's vault had a secondary, smaller door that could only be accessed with a series of challenging acrobatics, would the bank leave it because it's 'unlikely' someone could use it? Probably not. They'd seal it to ensure the vault is impervious from all possible breaches.

In conclusion, while I understand and respect the team's perspective, I think this issue does pose concrete risks and should therefore be considered a valid unique medium - high. It may not be the most straightforward exploit, but it actually offers a potential avenue for experienced attackers.

[hrishibhat]

Result: Low Unique After reviewing the above discussion and reviewing the comments, given the ambiguity around the external requirements for this attack, do not see a practical scenario where this would result in a profit for the attacker. Agree with the escalation from @syjcnss

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• syjcnss: accepted