search

code-423n4 / 2022-04-jpegd-findings

1 stars 1 forks source link

Uniswap V3 Swap is Sandwichable #199

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/yVault/strategies/StrategyPUSDConvex.sol#L341-L348

Vulnerability details

Impact

The Uniswap v3 swap in StrategyPUSDConvex has no amountOutMin set, making the swap sandwichable to a frontrunning attack which can cause loss of value during harvests. The root of the issue is that there is no check for the amount of want tokens left in the strategy to see if the minOutCurve value underestimated the want tokens received. Even if a private is used to hide these transactions, there is risk from an uncle bandit attack if the mining pool mines an uncle block https://medium.com/alchemy-api/unmasking-the-ethereum-uncle-bandit-a2b3eb694019

Proof of concept

The Uniswap v3 swap with no amountOutMin happens in the strategy harvest function https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/yVault/strategies/StrategyPUSDConvex.sol#L341-L348

An amountOutMin value of zero means that this Uniswap v3 check is useless https://github.com/Uniswap/v3-periphery/blob/eb0852f33c9135474ebdaf753994a6a01d8c9d36/contracts/SwapRouter.sol#L165

There is a comment stating that no amountOutMin is needed because a minOutCurve value is used elsewhere, but consider this scenario:

  1. Harvest number 1 happens. The minOutCurve value underestimates the want tokens that are sent to curve, but no sandwich attack happens
  2. Some want tokens remain in the strategy contract
  3. Harvest number 2 happens, and it is a large harvest. The minOutCurve value again underestimates the want tokens that are sent to curve, but this time a sandwich attack does happen
  4. The sandwich attack reduces the number of want tokens that the strategy receives, but this shortfall is compensated by the want tokens that were left in the contract from the last harvest
  5. The harvest does not revert and the strategist does not notice the loss of value until later

Tools Used

Manual analysis

Recommended Mitigation Steps

Setting an amountOutMin value in any swap is very important. Set a reasonable slippage value and use a price oracle to make sure a fair value is chosen for amountOutMin. A second possibility is to require the value of want tokens left in the contract after depositing minOutCurve tokens to Curve's stableswap pool to be less than a certain percentage of minOutCurve. For example, for 3 percent slippage one could use require((minOutCurve * 3) / 100 > usdc.balanceOf(address(this)));

If there is no strict requirement for using Uniswap, a MEV-resistant DEX like CowSwap could be a better option.

spaghettieth commented 2 years ago

Duplicate of #64

dmvt commented 2 years ago

Invalid, see #64