UniV2LiquidityAmo addLiquidity can be DoS'd

[code423n4]

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/amo/UniV2LiquidityAmo.sol#L189-L250

Vulnerability details

Impact

The safeApprove() calls in addLiquidity() will revert if there is an existing non-zero allowance. This can happen if a previous addLiquidity() hasn't used up all token amounts.

Description

In addLiquidity() both tokens are approved with safeApprove() which reverts if there is a non-zero allowance.

The ratio of the added tokens in a Uniswap V2 LP-in operation is almost never guaranteed to be exactly as requested due to the ratio changing with every swap / lp action. If addLiquidity() leaves dust in a token, then there will be an outstanding non-zero approval for the token. Subseqquent calls to addLiqudity() will fail.

Proof of Concept

The following Foundry test illustrates this vulnerability.

• Paste the code in a new file PoC.t.sol under /test
• Run the test with forge test --match-test test_poc_amo -vvv

// File: PoC.t.sol
// SPDX-License-Identifier: UNLICENSED
pragma solidity 0.8.19;

import "forge-std/Test.sol";
import "forge-std/console.sol";

import {IERC20WithBurn} from "contracts/interfaces/IERC20WithBurn.sol";
import {MockToken} from "contracts/mocks/MockToken.sol";
import {MockRdpxEthPriceOracle} from "contracts/mocks/MockRdpxEthPriceOracle.sol";
import {UniV2LiquidityAMO} from "contracts/amo/UniV2LiquidityAmo.sol";
import {IUniswapV2Factory} from "contracts/uniswap_V2/IUniswapV2Factory.sol";
import {IUniswapV2Router} from "contracts/uniswap_V2/IUniswapV2Router.sol";
import {IUniswapV2Pair} from "contracts/uniswap_V2/IUniswapV2Pair.sol";

contract MockRdpxV2Core {
    function setUpApprovals(address token1, address token2, address spender) external {
        IERC20WithBurn(token1).approve(spender, type(uint256).max);
        IERC20WithBurn(token2).approve(spender, type(uint256).max);
    }
}

contract PoCAmo is Test {
    string internal constant ARBITRUM_RPC_URL =
        "https://arbitrum-mainnet.infura.io/v3/c088bb4e4cc643d5a0d3bb668a400685";
    uint256 internal constant BLOCK_NUM = 24023149; // 2022/09/13

    MockToken weth;
    MockToken rdpx;
    MockRdpxV2Core core;
    MockRdpxEthPriceOracle oracle;

    IUniswapV2Factory factory;
    IUniswapV2Router router;
    IUniswapV2Pair pair;
    UniV2LiquidityAMO amo;

    function setUp() public {
        uint256 forkId = vm.createFork(ARBITRUM_RPC_URL, BLOCK_NUM);
        vm.selectFork(forkId);

        weth = new MockToken("WETH", "weth");
        rdpx = new MockToken("RDPX", "rdpx");
        oracle = new MockRdpxEthPriceOracle();
        core = new MockRdpxV2Core();

        factory =
            IUniswapV2Factory(0xc35DADB65012eC5796536bD9864eD8773aBc74C4);
        router =
            IUniswapV2Router(0x1b02dA8Cb0d097eB8D57A175b88c7D8b47997506);

        pair = IUniswapV2Pair(
            factory.createPair(address(rdpx), address(weth))
        );

        amo = new UniV2LiquidityAMO();
        amo.setAddresses(
            address(weth),
            address(rdpx),
            address(pair),
            address(core),
            address(oracle),
            address(factory),
            address(router)
        );

        core.setUpApprovals(address(weth), address(rdpx), address(amo));
        weth.mint(address(core), 1000e18);
        rdpx.mint(address(core), 1000e18);
    }

    function test_poc_amo() public {
        // Setup: Add liquidity to initialize the pair at 1:1 ratio
        amo.addLiquidity(1 ether, 1 ether, 0, 0);

        // Add liquidity off-ratio - this leaves 0.01 of tokenA unspent, so a remaining non-zero approval
        amo.addLiquidity(0.99 ether, 1 ether, 0, 0);
        
        // Any subsequent add liquidity operation will fail because of the safeApprove call
        amo.addLiquidity(1 ether, 1 ether, 0, 0);
    }
}

// Output
Test result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 1.01s
Ran 1 test suites: 0 tests passed, 1 failed, 0 skipped (1 total tests)

Failing tests:
Encountered 1 failing test in tests/PoCAmo.t.sol:PoCAmo
[FAIL. Reason: SafeERC20: approve from non-zero to non-zero allowance] test_poc_amo() (gas: 608176)

Encountered a total of 1 failing tests, 0 tests succeeded

Tools Used

Manual review, Foundry

Recommended Mitigation Steps

Use safeIncreaseAllowance instead of safeApprove.
SafeERC20.safeApprove() Has unnecessary and unsecure added behavior #2219

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 duplicate of #1782

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory:
Out of scope