DepositTradeHelper.depositAndTrade will not work normally with ERC20 that need to clear allowance

[code423n4]

Lines of code

https://github.com/prepo-io/prepo-monorepo/blob/feat/2022-12-prepo/apps/smart-contracts/core/contracts/DepositTradeHelper.sol#L24

Vulnerability details

Impact

DepositTradeHelper.depositAndTrade will not work normally with ERC20 that need to clear allowance as it sets allowance to uint256.max and do not set it to 0 after. As result next time when user calls depositAndTrade function it will fail as allowance should be set to 0 before.

Proof of Concept

DepositTradeHelper.depositAndTrade function allows user to deposit base tokens into Collateral token and then swap it using Uniswap.
https://github.com/prepo-io/prepo-monorepo/blob/feat/2022-12-prepo/apps/smart-contracts/core/contracts/DepositTradeHelper.sol#L22-L34

  function depositAndTrade(uint256 baseTokenAmount, Permit calldata baseTokenPermit, Permit calldata collateralPermit, OffChainTradeParams calldata tradeParams) external override {
    if (baseTokenPermit.deadline != 0) {
      IERC20Permit(address(_baseToken)).permit(msg.sender, address(this), type(uint256).max, baseTokenPermit.deadline, baseTokenPermit.v, baseTokenPermit.r, baseTokenPermit.s);
    }
    _baseToken.transferFrom(msg.sender, address(this), baseTokenAmount);
    if (collateralPermit.deadline != 0) {
      _collateral.permit(msg.sender, address(this), type(uint256).max, collateralPermit.deadline, collateralPermit.v, collateralPermit.r, collateralPermit.s);
    }
    uint256 _collateralAmountMinted = _collateral.deposit(msg.sender, baseTokenAmount);
    _collateral.transferFrom(msg.sender, address(this), _collateralAmountMinted);
    ISwapRouter.ExactInputSingleParams memory exactInputSingleParams = ISwapRouter.ExactInputSingleParams(address(_collateral), tradeParams.tokenOut, POOL_FEE_TIER, msg.sender, tradeParams.deadline, _collateralAmountMinted, tradeParams.amountOutMinimum, tradeParams.sqrtPriceLimitX96);
    _swapRouter.exactInputSingle(exactInputSingleParams);
  }

It uses permit function to provide allowance to himself and it sets allowance to type(uint256).max. After the transfer it doesn't set allowance to 0.
And next time when user will call this function it will revert if _baseToken doesn't support providing allowance when current allowance is not set to 0(like USDT).

Tools Used

VsCode

Recommended Mitigation Steps

Do not set allowance to type(uint256).max, set it to baseTokenAmount value.

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)

[c4-judge]

Picodes marked the issue as grade-c

[c4-judge]

Picodes marked the issue as grade-b

[c4-judge]

Picodes marked the issue as grade-a