code-423n4 / 2023-05-asymmetry-mitigation-findings

2 stars 2 forks source link

Mitigation of M-04: Mitigation error #45

Open code423n4 opened 1 year ago

code423n4 commented 1 year ago

MITIGATION IS NOT CONFIRMED

MITIGATION IS NOT CONFIRMED

Mitigation of M-04: Mitigation error

Link to Issue: https://github.com/code-423n4/2023-03-asymmetry-findings/issues/932

Comments

Even though the original issue is mitigated, as the exchange through Uniswap V3 has been completely removed in favor of using RocketSwapRouter, there's an assumption in the new implementation that may impact the user.

Technical Details

The new implementation of the deposit() function in the Reth derivative uses the RocketSwapRouter contract to acquire the rETH tokens. The main issue in this new change is that it has the portion parameters hardcoded to just use Balancer:

https://github.com/asymmetryfinance/smart-contracts/pull/228/files#diff-6abc8f2e4ad1647a12784e9fbf18e9c5f86c05668e3e89e2a51ab569992b214fR111-R116

RocketSwapRouterInterface(ROCKET_SWAP_ROUTER).swapTo{value: msg.value}(
    0,
    10,
    minOut,
    idealOut
);

This was probably chosen due to the fact the Balancer pool has more liquidity than Uniswap. However, this assumptions has two issues:

  1. This could not always be the case. It might be possible that eventually in the future liquidity is better in Uniswap than in Balancer.
  2. Large deposits may benefit from splitting a portion into Uniswap and a portion into Balancer, to minimize the price impact using both pools at the same time, and provide a more optimal outcome for the user.

Recommendation

Take the parameters to control each pool portion as input parameters in the function, so they can be fine tuned using off-chain mechanisms. The current parameters can be left as a default option.

elmutt commented 1 year ago

thanks

d3e4 commented 1 year ago

I don't agree that this is a mitigation error, but it is a good suggestion. And to that end, note that RocketSwapRouter has functions optimiseSwapFrom() and optimiseSwapTo() to calculate the best portions. But these are very gas inefficient and should be used offline.

romeroadrian commented 1 year ago

I agree that severity might be controversial. I aligned this with the "non ideal" situations described in M-09

c4-judge commented 1 year ago

Picodes marked the issue as satisfactory