Impossible to change addresses twice

[code423n4]

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/reLP/ReLPContract.sol#L115-L164

Vulnerability details

Impact

If oracle address will be changed, it will be impossible to update it for ReLPContract.

Proof of Concept

See ReLPContract and function setAddresses.

  function setAddresses(
    address _tokenA,
    address _tokenB,
    address _pair,
    address _rdpxV2Core,
    address _tokenAReserve,
    address _amo,
    address _rdpxOracle,
    address _ammFactory,
    address _ammRouter
  ) external onlyRole(DEFAULT_ADMIN_ROLE) {
    require(
      _tokenA != address(0) &&
        _tokenB != address(0) &&
        _pair != address(0) &&
        _rdpxV2Core != address(0) &&
        _tokenAReserve != address(0) &&
        _amo != address(0) &&
        _rdpxOracle != address(0) &&
        _ammFactory != address(0) &&
        _ammRouter != address(0),
      "reLPContract: address cannot be 0"
    );
    addresses = Addresses({
      tokenA: _tokenA,
      tokenB: _tokenB,
      pair: _pair,
      rdpxV2Core: _rdpxV2Core,
      tokenAReserve: _tokenAReserve,
      amo: _amo,
      rdpxOracle: _rdpxOracle,
      ammFactory: _ammFactory,
      ammRouter: _ammRouter
    });

    IERC20WithBurn(addresses.pair).safeApprove(
      addresses.ammRouter,
      type(uint256).max
    );

    IERC20WithBurn(addresses.tokenA).safeApprove(
      addresses.ammRouter,
      type(uint256).max
    );

    IERC20WithBurn(addresses.tokenB).safeApprove(
      addresses.ammRouter,
      type(uint256).max
    );
  }

as you can see, this function updates new addresses and set approves.
Now lets dive into safeApprove.

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

This function can either set approve to 0 or change approve from 0 to any other value.

Now lets consider next case:

• When contract filled with new addresses approves sets for pair, tokenA and tokenB
• Next oracle broke, and you decided to replace it. Of course changing pair, tokenA and tokenB impossible.
• So safeApprove will fail because of (value == 0) || (token.allowance(address(this), spender) == 0)
• So you ether need to replace ammRouter to new one every time, or your transaction will fail.

Tools Used

Manual review

Recommended Mitigation Steps

Either split parameter modification by different functions, or add possibility to set approval to 0.

Better separate setAddresses by multiple atomic setters like:

setOptionPricing
setAssetPriceOracle
...

Assessed type

ERC20

[c4-pre-sort]

bytes032 marked the issue as duplicate of #928

[c4-pre-sort]

bytes032 marked the issue as low quality report

[c4-pre-sort]

bytes032 marked the issue as not a duplicate

[c4-pre-sort]

bytes032 marked the issue as primary issue

[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-judge]

GalloDaSballo marked the issue as unsatisfactory:
Out of scope