Unspent allowance may break addLiquidity functionality in UniV2LiquidityAMO

[code423n4]

Lines of code

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

Vulnerability details

The addLiquidity function within the UniV2LiquidityAMO contract can be called either by reLPContract or directly as per the sponsors to add liquidity to RPDX/WETH pool. The function approves the AMM Router to spend a specific amount of tokenA and tokenB on behalf of the contract using the safeApprove method. However, it does not reset the previous approval to 0 before setting a new approval amount.
If the actual amount used by the AMM Router's addLiquidity function is less than what was approved (because of the specified minimum amounts tokenAAmountMin/tokenBAmountMin), there will be leftover approval. As a result, the next call to addLiquidity in the contract that attempts to approve a new amount will fail unless the previous approval is manually reset to 0.

Impact

Due to the way the ERC20 approval mechanism works, if you try to change an existing non-zero approval without first setting it to 0, it will result in a revert. This means that if addLiquidity function of the AMM Router does not use all of the approved tokens, subsequent calls to the addLiquidity function in the provided contract will fail until someone manually resets the approval. This can result in a denial-of-service (DoS) condition for the contract, preventing liquidity from being added.

Proof of Concept

1. Assume the function addLiquidity is called with tokenAAmount = 100 and tokenBAmount = 50.
2. The AMM Router uses, let's say, 90 of tokenA and 45 of tokenB because of price fluctuation due to malicious manipulation, leaving an approval of 10 for tokenA and 5 for tokenB unutilized.
3. A subsequent call to the contract's addLiquidity function tries to approve a different amount. But it will fail because of the previous leftover approval.

Tools Used

Manual review

Recommended Mitigation Steps

1. Consider using the approve() function from the ERC20 standard. This approach directly sets the allowance and avoids the potential pitfalls of managing incremental allowances.
2. If using safeApprove is essential for some safety reasons, always reset the allowance to zero before setting a new one.

IERC20WithBurn(addresses.tokenA).safeApprove(addresses.ammRouter, 0);
IERC20WithBurn(addresses.tokenA).safeApprove(addresses.ammRouter, tokenAAmount);

Assessed type

DoS

[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