Usage of safeApprove will cause DoS in UniV2LiquidityAMO::addLiquidity and UniV2LiquidityAMO::swap

[code423n4]

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/amo/UniV2LiquidityAmo.sol#L200-L207
https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/amo/UniV2LiquidityAmo.sol#L328

Vulnerability details

OpenZeppelin's SafeERC20::safeApprove function contains the following code:

function safeApprove(IERC20 token, address spender, uint256 value) internal {
require(
    (value == 0) || (token.allowance(address(this), spender) == 0),
    "SafeERC20: approve from non-zero to non-zero allowance"
);
_callOptionalReturn(token, abi.encodeWithSelector(token.approve.selector, spender, value));

So, as can be seen, it will revert if value is not 0 and token.allowance is not 0.
This function is used in UniV2LiquidityAMO::addLiquidity and in UniV2LiquidityAMO::swap:

  function addLiquidity(
    uint256 tokenAAmount,
    uint256 tokenBAmount,
    uint256 tokenAAmountMin,
    uint256 tokenBAmountMin
  )
    external
    onlyRole(DEFAULT_ADMIN_ROLE)
    returns (uint256 tokenAUsed, uint256 tokenBUsed, uint256 lpReceived)
  {
    // approve the AMM Router
    IERC20WithBurn(addresses.tokenA).safeApprove(
      addresses.ammRouter,
      tokenAAmount
    );
    IERC20WithBurn(addresses.tokenB).safeApprove(
      addresses.ammRouter,
      tokenBAmount
    );

    // add Liquidity
    (tokenAUsed, tokenBUsed, lpReceived) = IUniswapV2Router(addresses.ammRouter)
      .addLiquidity(
        addresses.tokenA,
        addresses.tokenB,
        tokenAAmount,
        tokenBAmount,
        tokenAAmountMin,
        tokenBAmountMin,
        address(this),
        block.timestamp + 1
      );
    [...]

  function swap(
    uint256 token1Amount,
    uint256 token2AmountOutMin,
    bool swapTokenAForTokenB
  ) external onlyRole(DEFAULT_ADMIN_ROLE) returns (uint256 token2Amount) {
    [...]

    // approve the AMM Router
    IERC20WithBurn(token1).safeApprove(addresses.ammRouter, token1Amount);

    address[] memory path;
    path = new address[](2);
    path[0] = token1;
    path[1] = token2;

    // swap token A for token B
    token2Amount = IUniswapV2Router(addresses.ammRouter)
      .swapExactTokensForTokens(
        token1Amount,
        token2AmountOutMin,
        path,
        address(this),
        block.timestamp + 1
      )[path.length - 1];

It's important to notice that UniswapV2Router::addLiquidity (called by UniV2LiquidityAMO::addLiquidity does not guarantee that it will spend entire tokenAAmount and tokenBAmount - in fact it will almost always not spend full amounts - full amounts will only be spent if tokenAAmount / tokenBAmount precisely equals the token ratio in the pool (even 10 Wei difference in provided amounts will cause a situation where less than tokenAAmount or tokenBAmount was spent.

If that (almost certain) scenario happens, then the allowance for Uniswap will be nonzero for tokenA or tokenB - assume that it is the case for tokenA (which is rdpx). It means that safeApprove will fail for that token unless it is zeroed out. However, it is impossible to zero out that allowance:

• it's impossible to do this by approveContractToSpend, since there is require(_amount > 0) there, so _amount passed cannot be 0
• it's impossible to do this by addLiquidity with tokenAAmount == 0 since UniswapV2Library will revert on the quote function (https://github.com/Uniswap/v2-periphery/blob/0335e8f7e1bd1e8d8329fd300aea2ef2f36dd19f/contracts/libraries/UniswapV2Library.sol#L37; see PoC)
• finally, it's impossible to do this by swap with token1Amount == 0, since UniswapV2Library will revert from the getAmountOut function (https://github.com/Uniswap/v2-periphery/blob/0335e8f7e1bd1e8d8329fd300aea2ef2f36dd19f/contracts/libraries/UniswapV2Library.sol#L44; see PoC)
• no other function calls approve or safeApprove on rdpx, hence zeroing out rdpx allowance for Uniswap is impossible

So, both UniV2LiquidityAMO::addLiquidity and UniV2LiquidityAMO::swap will always fail, so the UniV2LiquidityAMO contract will become virtually useless.

Impact

UniV2LiquidityAMO contract will become virtually useless even after the very first call to addLiquidity - it won't be possible to call addLiquidity or swap anymore.

It will happen either after second liquidity provision (assuming that no liquidity was added by anyone earlier on to WETH-RDPX pair on Uniswap) or even after the first one (if there is already some liquidity on WETH-RDPX pair - anyone can add it before using any WETH / RDPX ratio, so it is even possible to frontrun the first call to UniV2LiquidityAMO::addLiquidity to make sure that it will not spend entire rdpx allowance).

It should be noted, that although UniV2LiquidityAMO will be impossible to use, all ERC20 tokens (including LP tokens) can still be recovered by calling emergencyWithdraw, so that is why I'm submitting this issue as Medium.

Proof of Concept

Please execute the following test (in Periphery.t.sol):

function testAddUniswapV2LiquidityTwice() public {
    uniV2LiquidityAMO = new UniV2LiquidityAMO();

    // set addresses
    uniV2LiquidityAMO.setAddresses(
      address(rdpx),
      address(weth),
      address(pair),
      address(rdpxV2Core),
      address(rdpxPriceOracle),
      address(factory),
      address(router)
    );

    // give amo premission to access rdpxV2Core reserve tokens

    rdpxV2Core.addAMOAddress(address(uniV2LiquidityAMO));

    rdpxV2Core.approveContractToSpend(
      address(rdpx),
      address(uniV2LiquidityAMO),
      type(uint256).max
    );

    rdpxV2Core.approveContractToSpend(
      address(weth),
      address(uniV2LiquidityAMO),
      type(uint256).max
    );

    rdpx.transfer(address(rdpxV2Core), 50e18);
    weth.transfer(address(rdpxV2Core), 11e18);

    // add liquidity
    uniV2LiquidityAMO.addLiquidity(1e18, 2e17, 0, 0);
    // even slight difference in `token1Amount / token2Amount` will cause the DoS
    uniV2LiquidityAMO.addLiquidity(1e18 + 10, 2e17, 0, 0);

    vm.expectRevert("SafeERC20: approve from non-zero to non-zero allowance");
    uniV2LiquidityAMO.swap(1e18, 0, true);

    // it will neither be possible to change allowance to `0` by adding `0` liquidity
    vm.expectRevert("UniswapV2Library: INSUFFICIENT_AMOUNT");
    uniV2LiquidityAMO.addLiquidity(0, 10, 0, 1);

    // nor it will be possible to zero it out through a `swap`
    vm.expectRevert("UniswapV2Library: INSUFFICIENT_INPUT_AMOUNT]");
    uniV2LiquidityAMO.swap(0, 0, true);
  }

Tools Used

VS Code

Recommended Mitigation Steps

Substitute all calls to safeApprove with calls to approve to avoid reverting when the current allowance is non-zero.

Assessed type

DoS

[bytes032]

Finding is OOS, found by bot.

1. https://github.com/code-423n4/2023-08-dopex-bot-findings/blob/main/data/IllIllI-bot-report.md#l05-approvesafeapprove-may-revert-if-the-current-approval-is-not-zero
2. https://github.com/code-423n4/2023-08-dopex-bot-findings/blob/main/data/IllIllI-bot-report.md#l04-safeapprove-is-deprecated

[c4-pre-sort]

bytes032 marked the issue as primary issue

[c4-pre-sort]

bytes032 marked the issue as low quality report

[bytes032]

Note: It might be more severe than L from the bot race, because of the impact.

[c4-pre-sort]

bytes032 marked the issue as duplicate of #1782

[c4-pre-sort]

bytes032 marked the issue as sufficient quality report

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory:
Out of scope