The ability of the Uniswap V2 AMO to add liquidity can be bricked indefinitely

[code423n4]

Lines of code

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

Vulnerability details

Impact

An important function of the Uniswap V2 AMO is adding and managing liquidity for select pools. There is however a logic flaw in the addLiquidity function of the UniV2LiquidityAmo contract with the way that tokens are approved for the AMM router, which can result in effectively permanent DOS of the ability to add liquidity for that pool (e.g. a tokenA,TokenB pair). This issue is due to unused allowance when safeApprove is called, wherein for one of the tokens in the pair, not all of the allowance will be used. This then means, given the logic of safeApprove which requires existing approvals to be 0, any future call which references this same token will revert. Since the AMO only handles one pool with two unique tokens (tokenA and tokenB are fixed) this means the entire functionality of the contract will be bricked.

Proof of Concept

The addLiquidity function of the Uniswap V2 AMO is defined as follows:

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
  );

  // transfer token A and B from the rdpxV2Core to this contract
  IERC20WithBurn(addresses.tokenA).safeTransferFrom(
    addresses.rdpxV2Core,
    address(this),
    tokenAAmount
  );
  IERC20WithBurn(addresses.tokenB).safeTransferFrom(
    addresses.rdpxV2Core,
    address(this),
    tokenBAmount
  );

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

Notice that in the call to addLiquidity, the admin will specify a tokenAAmount and tokenBAmount in which they intend to add as liquidity for a given Uniswap V2 pool. Both of these tokens are approved for the router through the call of safeApprove. This safeApprove function requires that the existing approval for these tokens to be 0 prior to approving the new amounts (else reverts), which is where the issue arises.

Generally when the admin calls addLiquidity, they will specify amounts of tokenA and tokenB such that they equal the current ratio of the tokens in the pool. However, due to market movements prior to this tx being mined, it's possible that the actual ratio has changed (note: tokenAAmountMin and tokenBAmountMin are set to allow for this). Due to price movements, its possible that the Uniswap V2 router does not actually use all of the tokenAAmount or tokenBAmount allowance. This then means the allowance will not be 0 for the next call, and so all future calls will revert. This all happens because the Uniswap V2 router calculates the optimal amount of both tokens, which means that the actual tokens transferred can be less than the allowance set.

Note: it's also not possible to recover from this situation (from calling this function with tokenAAmount=0 and tokenBAmount=0), because this will result in attempting to mint 0 liquidity, which reverts in the UniswapV2Pair contract.

Tools Used

Manual review

Recommended Mitigation Steps

Consider approving 0 prior to approving the actual tokenAAmount and tokenBAmount.

Assessed type

Uniswap

[c4-pre-sort]

bytes032 marked the issue as duplicate of #928

[c4-pre-sort]

bytes032 marked the issue as not a duplicate

[c4-pre-sort]

bytes032 marked the issue as duplicate of #928

[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 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