use_safe_approve is not correctly implemented and would lead to approveTarget() breaking if used while the bool is set to true

[code423n4]

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/amo/UniV3LiquidityAmo.sol#L139-L152

Vulnerability details

Impact

Break in contract's logic since approvals can't be set for tokens that require use_safe_approve to be true

Proof of Concept

It's pretty common that safeApprove() breaks logic in contracts, in the sense where there is any leftover allowance.

Sponsors know about this which is why these commented lines from the approveTarget() function state the below:

      // safeApprove needed for USDT and others for the first approval
      // You need to approve 0 every time beforehand for USDT: it resets

Problem is that the code does not follow the above, cause instead of calling safeApprove(_token, _target, 0 ) first then safeApprove(_token, _target, _amount) only the latter is called, which leads this to always revert

Click to see full referenced code

  function approveTarget(
    address _target,
    address _token,
    uint256 _amount,
    bool use_safe_approve
  ) public onlyRole(DEFAULT_ADMIN_ROLE) {
    if (use_safe_approve) {

      // safeApprove needed for USDT and others for the first approval
      // You need to approve 0 every time beforehand for USDT: it resets
      // @audit-issue
      TransferHelper.safeApprove(_token, _target, _amount);
    } else {
      IERC20WithBurn(_token).approve(_target, _amount);
    }
  }

Tool used

Manual Audit

Recommended Mitigation Steps

Set the allowance to 0 first before new value for use_safe_approve type of tokens

Assessed type

Error

[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 not a duplicate

[c4-pre-sort]

bytes032 marked the issue as duplicate of #1662

[c4-pre-sort]

bytes032 marked the issue as low quality report

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory:
Out of scope