Calling setAddresses every subsequent time doesn't revoke the max token approval for the old addresses

[code423n4]

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L304-L350 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/reLP/ReLPContract.sol#L115-L164 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVault.sol#L181-L212 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/amo/UniV2LiquidityAmo.sol#L74-L102

Vulnerability details

Impact

Old contracts will still be able to transfer the funds of the protocol despite not being in use anymore.

Proof of Concept

The protocol allows its admin to change out contracts in-use by calling setAddresses(). The issue arises due to the function not revoking the token allowances of the old ones. This is an issue for the protocol as contracts are likely to only be swapped in the case of a security breach, which means that this will indirectly endanger the funds held in the protocol's other contract

    function setAddresses(
    address _dopexAMMRouter,
    address _dpxEthCurvePool,
    address _rdpxDecayingBonds,
    address _perpetualAtlanticVault,
    address _perpetualAtlanticVaultLP,
    address _rdpxReserve,
    address _rdpxV2ReceiptToken,
    address _feeDistributor,
    address _reLPContract,
    address _receiptTokenBonds
  ) external onlyRole(DEFAULT_ADMIN_ROLE) {

      ...

        // @audit-issue not revoking the allowances of old contracts if this is a subsequent call to setAdresses
    IERC20WithBurn(weth).approve(
      addresses.perpetualAtlanticVault,
      type(uint256).max
    );

    IERC20WithBurn(weth).approve(addresses.dopexAMMRouter, type(uint256).max);
    IERC20WithBurn(weth).approve(addresses.dpxEthCurvePool, type(uint256).max);
    IERC20WithBurn(weth).approve(
      addresses.rdpxV2ReceiptToken,
      type(uint256).max
    );
    emit LogSetAddresses(addresses);
  }

Tools Used

Manual review

Recommended Mitigation Steps

Consider revoking the allowances of old contracts upon changing them with newly-deployed ones.

    function setAddresses(
    address _dopexAMMRouter,
    address _dpxEthCurvePool,
    address _rdpxDecayingBonds,
    address _perpetualAtlanticVault,
    address _perpetualAtlanticVaultLP,
    address _rdpxReserve,
    address _rdpxV2ReceiptToken,
    address _feeDistributor,
    address _reLPContract,
    address _receiptTokenBonds
  ) external onlyRole(DEFAULT_ADMIN_ROLE) {

      ...
        IERC20WithBurn(weth).approve(oldAddresses.perpetualAtlanticVault, 0);
    IERC20WithBurn(weth).approve(oldAddresses.dopexAMMRouter, 0);
    IERC20WithBurn(weth).approve(oldAddresses.dpxEthCurvePool, 0);
    IERC20WithBurn(weth).approve(oldAddresses.rdpxV2ReceiptToken, 0);

        IERC20WithBurn(weth).approve(addresses.perpetualAtlanticVault, type(uint256).max);
    IERC20WithBurn(weth).approve(addresses.dopexAMMRouter, type(uint256).max);
    IERC20WithBurn(weth).approve(addresses.dpxEthCurvePool, type(uint256).max);
    IERC20WithBurn(weth).approve(addresses.rdpxV2ReceiptToken, type(uint256).max);
    emit LogSetAddresses(addresses);
  }

Assessed type

Other

[c4-pre-sort]

bytes032 marked the issue as duplicate of #1346

[c4-judge]

GalloDaSballo changed the severity to QA (Quality Assurance)