UniswapSettlement does not use forceApprove, and may revert when quoteToken is USDT

[c4-bot-10]

Lines of code

https://github.com/code-423n4/2024-05-predy/blob/main/src/settlements/UniswapSettlement.sol#L47 https://github.com/code-423n4/2024-05-predy/blob/main/src/base/SettlementCallbackLib.sol#L160-L167

Vulnerability details

Impact

When using USDT as quoteToken, the UniswapSettlement.sol would fail during approve() due to zero allowance limit.

Bug Description

The contest README states that USDT is supported. An important factor of USDT is that when conducting approve(), the allowance must be zero. See here for more details.

When conducting a swap in SettlementCallbackLib, it uses the UniswapSettlement to swap tokens to/from uniswapV3. When we are buying baseToken, the code approves uniswapV3 router amountInMaximum amount of quoteTokens and use _swapRouter.exactOutput() for swapping out an exact amount of amountOut baseTokens.

The issue here is we may not use up all of amountInMaximum during the swap, and the allowance would be non-zero. This would brick the following approvals.

UniswapSettlement.sol

    function swapExactOut(
        address quoteToken,
        address,
        bytes memory data,
        uint256 amountOut,
        uint256 amountInMaximum,
        address recipient
    ) external override returns (uint256 amountIn) {
        ERC20(quoteToken).safeTransferFrom(msg.sender, address(this), amountInMaximum);
>       ERC20(quoteToken).approve(address(_swapRouter), amountInMaximum);

        amountIn = _swapRouter.exactOutput(
            ISwapRouter.ExactOutputParams(data, recipient, block.timestamp, amountOut, amountInMaximum)
        );

        if (amountInMaximum > amountIn) {
            ERC20(quoteToken).safeTransfer(msg.sender, amountInMaximum - amountIn);
        }
    }

SettlementCallbackLib.sol

    function buy(
        IPredyPool predyPool,
        address quoteToken,
        address baseToken,
        SettlementParams memory settlementParams,
        address sender,
        uint256 price,
        uint256 buyAmount
    ) internal {
        ...

        predyPool.take(true, address(this), settlementParams.maxQuoteAmount);
        ERC20(quoteToken).approve(address(settlementParams.contractAddress), settlementParams.maxQuoteAmount);

>       uint256 quoteAmountToUni = ISettlement(settlementParams.contractAddress).swapExactOut(
            quoteToken,
            baseToken,
            settlementParams.encodedData,
            buyAmount,
            settlementParams.maxQuoteAmount,
            address(predyPool)
        );
        ...
    }

Proof of Concept

N/A

Tools Used

Manual review

Recommended Mitigation Steps

Use forceApprove() from SafeERC20 in https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/utils/SafeERC20.sol#L76.

Assessed type

Token-Transfer

[alex-ppg]

The finding has been detected by the analyzer report (L-1) and thus is considered out-of-scope.

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Out of scope