Token approval (max) not revoked for old addresses

[code423n4]

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L339-L348
https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVault.sol#L207

Vulnerability details

Impact

Old addresses keep approval after new addresses are set in setAddresses() function, which may lead to exploitation if old addresses are compromised.

Proof of Concept

• RdpxV2Core: link
• PerpetualAtlanticVault: link

The function allows the admin to set the new addresses for multiple contracts. However, the approval to spend weth for several old addresses are not revoked and remains at max for:

• perpetualAtlanticVault
• dopexAMMRouter
• dpxEthCurvePool
• rdpxV2ReceiptToken

In the event any of the old addresses are compromised this may lead to unwanted transfers of weth tokens.

Tools Used

Manual review

Recommended Mitigation Steps

Revoke the approval for old addresses before setting new ones. Suggestion to either set approval to 0 first for old addresses within the function:

RdpxV2Core.sol

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

PerpetualVault.sol

    collateralToken.safeApprove(
      addresses.perpetualAtlanticVaultLP,0);

Or, to implement an internal function call to set approval to 0 for all old addresses, place before the setting of the new addresses in the setAddresses() function; repeat for PerpetualVault with just the required address.

function revokeOldAddressApprovals() internal {
    // set all old addresses to 0 approval of weth
    IERC20WithBurn(weth).approve(
            addresses.perpetualAtlanticVault, 0);
    IERC20WithBurn(weth).approve(
            addresses.dopexAMMRouter, 0);
    IERC20WithBurn(weth).approve(
            addresses.dpxEthCurvePool, 0);
    IERC20WithBurn(weth).approve(
            addresses.rdpxV2ReceiptToken, 0);
}

Assessed type

Other

[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 low quality report

[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 #1662

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory:
Out of scope