TransferHelper.safeApprove(_token, _target, _amount); does not approve 0 first.

[code423n4]

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/libraries/TransferHelper.sol#L6-L15
https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/amo/UniV3LiquidityAmo.sol#L146-L148
https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/reLP/ReLPContract.sol#L150-L163

Vulnerability details

Impact

These are the functions that call TransferHelper.safeApprove and can be reverted due to SafeERC20.safeApprove. It makes impossible to change just one address for ReLPContract and PerpetualAtlanticVault.

• ReLPContract.setAddresses
• V3Amo.approveTarget
• PerpetualAtlanticVault.setAddress reverts if called twice and some arguments remain the same. This is shown below.

Proof of Concept

The comment says it approves 0 first, but actually TransferHelper.safeApprove does not approves 0 first.
https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/libraries/TransferHelper.sol#L6-L15

Add tests/perp-vault/POC.sol and run forge test --mt test_setAddresses.

// SPDX-License-Identifier: UNLICENSED
pragma solidity 0.8.19;

import { Test } from "forge-std/Test.sol";

import { ERC721Holder } from "@openzeppelin/contracts/token/ERC721/utils/ERC721Holder.sol";
import { Setup } from "./Setup.t.sol";
import { PerpetualAtlanticVault } from "contracts/perp-vault/PerpetualAtlanticVault.sol";
import { OptionPricingSimple } from "contracts/libraries/OptionPricingSimple.sol";
import { console } from "forge-std/console.sol";

contract POC is ERC721Holder, Setup {
    // ================================ CORE ================================ //
    function test_setAddresses() external {
        vm.expectRevert("SafeERC20: approve from non-zero to non-zero allowance");
        vault.setAddresses(
            address(1),
            address(priceOracle),
            address(volOracle),
            address(1),
            address(rdpx),
            address(vaultLp),
            address(this)
        );
    }
}

Tools Used

Manual, foundry

Recommended Mitigation Steps

1. Use SafeERC20.forceApprove. It approves 0 if necessary. https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/utils/SafeERC20.sol#L76
2. You can also use SafeERC20.increaseAllowance and decreaseAllowance instead.

Assessed type

Other

[c4-pre-sort]

bytes032 marked the issue as duplicate of #928

[c4-pre-sort]

bytes032 marked the issue as low quality report

[bytes032]

Mentions only OOS

[c4-pre-sort]

bytes032 marked the issue as not a duplicate

[c4-pre-sort]

bytes032 marked the issue as duplicate of #1662

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory:
Out of scope