Uni v2 AMO unspent allowance may break functionality on addLiquidity

[code423n4]

Lines of code

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

Vulnerability details

Impact

Uni v2 AMO unspent allowance may break functionality on addLiquidity

Proof of Concept

The UniV2LiquidityAmo addLiquidity function contains several safeApprove functions on tokenA and tokenB for ammRouter with amount (tokenAAmount, tokenBAmount) provided.

The usage of this allowance is on addLiquidity function call on Uniswap v2 router, and by design it's possible that the usage of the allowance is not completely used, also the addLiquidity may only guarantee to use the amount based on the minimum parameter (the tokenAAmountMin and tokenBAmountMin).

File: UniV2LiquidityAmo.sol
199:     // approve the AMM Router
200:     IERC20WithBurn(addresses.tokenA).safeApprove(
201:       addresses.ammRouter,
202:       tokenAAmount
203:     );
204:     IERC20WithBurn(addresses.tokenB).safeApprove(
205:       addresses.ammRouter,
206:       tokenBAmount
207:     );
...
221:     // add Liquidity
222:     (tokenAUsed, tokenBUsed, lpReceived) = IUniswapV2Router(addresses.ammRouter)
223:       .addLiquidity(
224:         addresses.tokenA,
225:         addresses.tokenB,
226:         tokenAAmount,
227:         tokenBAmount,
228:         tokenAAmountMin,
229:         tokenBAmountMin,
230:         address(this),
231:         block.timestamp + 1
232:       );

When there is a difference between tokenAAmount and tokenAAmountMin (also tokenBAmount and tokenBAmountMin), it means there is some allowance left for the router which is not being used. (allowance is not 0).

Then later, if there is another addLiquidity function call, it will be reverted. This is due to the definition of safeApprove of Openzeppelin

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/v4.9.3/contracts/token/ERC20/utils/SafeERC20.sol#L45-L54

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

the above snippet shows that safeApprove need to make sure the existing allowance is 0. Thus, on our case, since the allowance is not 0 due to unspent allowance of previos addLiquidity, it will be reverted.

This issue is valid medium referring to code-423n4/2023-05-xeth-findings#29

Note: Even though the deprecated usage of safeApprove() is mentioned in the automated findings, this report demonstrates how this function can cause a serious vulnerability that may end up bricking the contract.

the custom TransferHelper.safeApprove doesn't effect of this issue. only the safeApprove based on OpenZeppelin.

Tools Used

Manual Analysis

Recommended Mitigation Steps

Consider to use approve(), or reset the allowance to zero after any allowance usage, or use safeIncreaseAllowance().

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