Avoid the use of hard coded slippage

[code423n4]

Lines of code

https://github.com/code-423n4/2023-08-goodentry/blob/71c0c0eca8af957202ccdbf5ce2f2a514ffe2e24/contracts/PositionManager/OptionsPositionManager.sol#L477

Vulnerability details

Impact

In OptionsPositionManager.sol, swapExactTokensForTokens() has used the hardcoded slippage of 1% which is used in withdrawOptionAssets() and swapTokens() functions.

  function swapExactTokensForTokens(IUniswapV2Router01 ammRouter, IPriceOracle oracle, uint amount, address[] memory path) 
    internal returns (uint256 received)
  {
    if (amount > 0 && AmountsRouter(address(ammRouter)).getAmountsOut(amount, path)[1] > 0){
      checkSetAllowance(path[0], address(ammRouter), amount);
      uint[] memory amounts = ammRouter.swapExactTokensForTokens(
        amount, 
>>      getTargetAmountFromOracle(oracle, path[0], amount, path[1]) * 99 / 100, // allow 1% slippage 
        path, 
        address(this), 
        block.timestamp
      );
      received = amounts[1];
    }
  }

Issue here is that user can end up giving away the full 1% unconditionally to market situation because there may not be enough token available. Another one is that knowing that the conditions are bad or that there are not enough tokens available and willing to run the exchange with bigger slippage the user will not be able to as there are no means to control it and the functionality will end up unavailable.

Proof of Concept

https://github.com/code-423n4/2023-08-goodentry/blob/71c0c0eca8af957202ccdbf5ce2f2a514ffe2e24/contracts/PositionManager/OptionsPositionManager.sol#L477

Tools Used

Manual Review

Recommended Mitigation Steps

Consider adding the function argument with a default value of 1%, so the slippage can be tuned when it is needed.

Assessed type

Other

[c4-pre-sort]

141345 marked the issue as duplicate of #105

[c4-judge]

gzeon-c4 marked the issue as unsatisfactory: Overinflated severity