code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/strategy/Pool2SingleAssetCompounder.sol#L62

Vulnerability details

Impact

Trade in Uniswap V2 can be very sub-optimal

Proof of Concept

In the current implementation of the Pool2SingleAssetCompounder.sol, the harvest function wants to swap and trade the reward token via Uniswap V2 Router.

  /// @notice claim all token rewards and trade them for the underlying asset
  function harvest() public override {
    address router = abi.decode(IAdapter(address(this)).strategyConfig(), (address));
    address asset = IAdapter(address(this)).asset();
    address[] memory rewardTokens = IWithRewards(address(this)).rewardTokens();

    IWithRewards(address(this)).claim(); // hook to accrue/pull in rewards, if needed

    address[] memory tradePath = new address[](2);
    tradePath[1] = asset;

    uint256 len = rewardTokens.length;
    // send all tokens to destination
    for (uint256 i = 0; i < len; i++) {
      uint256 amount = ERC20(rewardTokens[i]).balanceOf(address(this));

      if (amount > 0) {
        tradePath[0] = rewardTokens[i];

        IUniswapRouterV2(router).swapExactTokensForTokens(amount, 0, tradePath, address(this), block.timestamp);
      }
    }
    IAdapter(address(this)).strategyDeposit(ERC20(asset).balanceOf(address(this)), 0);
  }

However, before Uniswap Trade, the code does not check if the trading pair exists.

If the trading pair does not exists, the transaction will revert.

OR

A malicious user can front-run the harvest function if he realize that the trading pair is not created. he can create a trading pair, control the liquidity balance, when liquidity is thin, the swapped output amount can be very sub-optimal.

Also, the trading pair may not exist in Uniswap V2, or the liqudiity for trading pair is very thin in Uniswap V2. Maybe the best pool to perform the trade is on Uniswap V3 because the trading pool has the most concentralized liquidity.

Or the best pool that has the most liquidity is in balancer, or DODO Swap, the lack of integration to these protocol means that the best trade that get the most token cannot be performed.

By only hardcode and restrict trade and integrate with Uniswap V2 or Uniswap V2 forks, the trade performed can be very sub-optimal.

Tools Used

Manual Review

Recommended Mitigation Steps

We recommend the protocol create the trading pair first before calling harvest and also validate that trading pair exists on Uniswap V2 and integrate with more AMM protocol to make sure the trade performed is optimal. In fact, the aggregator 1inch can be a good choice.

c4-sponsor commented 1 year ago

RedVeil marked the issue as sponsor disputed

c4-judge commented 1 year ago

dmvt marked the issue as unsatisfactory: Out of scope

code-423n4 / 2023-01-popcorn-findings