addLiquidity() safeApprove() the second execution may revert

[code423n4]

Lines of code

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

Vulnerability details

Impact

In UniV2LiquidityAMO.addLiquidity() use IERC20WithBurn(addresses.token).safeApprove()
The safeApprove() method must satisfy that all previous allowances must be used up or it will revert
But in IUniswapV2Router.addLiquidity(), not all of the tokenA/tokenB allowances will be used up.
This causes the second execution of addLiquidity() to revert when safeApprove() doesn't use up all the allowances.

Proof of Concept

safeApprove() It must be ensured that previous allowances must be used up

    function safeApprove(IERC20 token, address spender, uint256 value) internal {
        // safeApprove should only be called when setting an initial allowance,
        // or when resetting it to zero. To increase and decrease it, use
        // 'safeIncreaseAllowance' and 'safeDecreaseAllowance'
        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));
    }

but addLiquidity() It doesn't use up the entire allowance.

https://github.com/Uniswap/v2-periphery/blob/0335e8f7e1bd1e8d8329fd300aea2ef2f36dd19f/contracts/UniswapV2Router02.sol#L71

    function addLiquidity(
        address tokenA,
        address tokenB,
        uint amountADesired,
        uint amountBDesired,
        uint amountAMin,
        uint amountBMin,
        address to,
        uint deadline
    ) external virtual override ensure(deadline) returns (uint amountA, uint amountB, uint liquidity) {
@>      (amountA, amountB) = _addLiquidity(tokenA, tokenB, amountADesired, amountBDesired, amountAMin, amountBMin);
        address pair = UniswapV2Library.pairFor(factory, tokenA, tokenB);
        TransferHelper.safeTransferFrom(tokenA, msg.sender, pair, amountA);
        TransferHelper.safeTransferFrom(tokenB, msg.sender, pair, amountB);
        liquidity = IUniswapV2Pair(pair).mint(to);
    }

So on the second execution, UniV2LiquidityAMO.addLiquidity() will revert

test POC

The following code demonstrates that the second time addLiquidity() is revert with SafeERC20: approve from non-zero to non-zero allowance

add to Periphery.t.sol

  function testSafeApproveRevert() 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);

    uniV2LiquidityAMO.addLiquidity(5e18, 2e18, 0, 0);
    uniV2LiquidityAMO.addLiquidity(5e18, 2e18, 0, 0);
  }  

$ forge test --match-test testSafeApproveRevert

Running 1 test for tests/rdpxV2-core/Periphery.t.sol:Periphery
[FAIL. Reason: SafeERC20: approve from non-zero to non-zero allowance] testSafeApproveRevert() (gas: 2324150)
Test result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 1.98s
Ran 1 test suites: 0 tests passed, 1 failed, 0 skipped (1 total tests)

Failing tests:
Encountered 1 failing test in tests/rdpxV2-core/Periphery.t.sol:Periphery
[FAIL. Reason: SafeERC20: approve from non-zero to non-zero allowance] testSafeApproveRevert() (gas: 2324150)

Encountered a total of 1 failing tests, 0 tests succeeded

Tools Used

Recommended Mitigation Steps

use safeIncreaseAllowance() instead of safeApprove()

Assessed type

ERC20

[c4-pre-sort]

bytes032 marked the issue as duplicate of #928

[c4-pre-sort]

bytes032 marked the issue as low quality report

[c4-pre-sort]

bytes032 marked the issue as sufficient quality report

[c4-pre-sort]

bytes032 marked the issue as not a duplicate

[c4-pre-sort]

bytes032 marked the issue as duplicate of #1455

[c4-pre-sort]

bytes032 marked the issue as not a duplicate

[c4-pre-sort]

bytes032 marked the issue as primary issue

[bytes032]

This issue and its duplicates contain all the occurrences of:

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

Note: It's reported as L from the bot.

[c4-sponsor]

psytama (sponsor) confirmed

[GalloDaSballo]

The finding is OOS

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory:
Out of scope