Lines of code
<https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/vaults/AutoPxGlp.sol#L390-L400&gt;
<https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/vaults/AutoPxGmx.sol#L281-L283&gt;
<https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/vaults/AutoPxGmx.sol#L385-L388&gt;

Vulnerability details

Impact

After a migration of the platform (PirexGmx) contract, the approval of the AutoPxGlp from the new PirexGmx contract will be zero. The same issue is here for the AutoPxGmx contract.

Proof of Concept

Even though the approval of gmxBaseReward (formerly WETH for Ethereum and WAVAX for Avalanche) is granted in the constructor: <https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/vaults/AutoPxGlp.sol#L87&gt;

gmxBaseReward.safeApprove(address(_platform), type(uint256).max);

This one is not done anymore in the compound function at all: <https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/vaults/AutoPxGlp.sol#L238-L247&gt;

if (gmxBaseRewardAmountIn != 0) {
    // Deposit received rewards for pxGLP
    (, pxGlpAmountOut, ) = PirexGmx(platform).depositGlp(
        address(gmxBaseReward),
        gmxBaseRewardAmountIn,
        minUsdg,
        minGlp,
        address(this)
    );
}

And avoided in the depositGlp: <https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/vaults/AutoPxGlp.sol#L390-L400&gt;

if (erc20Token != gmxBaseReward) {
    erc20Token.safeApprove(platform, tokenAmount);
}

(, uint256 assets, ) = PirexGmx(platform).depositGlp(
    token,
    tokenAmount,
    minUsdg,
    minGlp,
    address(this)
);

After a migration of the platform, the allowance granted to it will be equal to zero with no way to grant it for this new platform. So this AutoPxGlp will be tied to the same PirexGmx contract during its whole lifetime.

Similarly for the AutoPxGmx contract, which is about the approval of GMX tokens:

<https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/vaults/AutoPxGmx.sol#L281-L283&gt;

(, pxGmxMintAmount, ) = PirexGmx(platform).depositGmx(
    gmx.balanceOf(address(this)),
    address(this)
);

<https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/vaults/AutoPxGmx.sol#L385-L388&gt;

(, uint256 assets, ) = PirexGmx(platform).depositGmx(
    amount,
    address(this)
);

Tools Used

Manual inspection

Recommended Mitigation Steps

Make sure to approve the exact amount of tokens to the PirexGmx before depositing any more tokens.

AutoPxGlp

if (gmxBaseRewardAmountIn != 0) {
+  gmxBaseReward.safeApprove(platform, gmxBaseRewardAmountIn);
    // Deposit received rewards for pxGLP
    (, pxGlpAmountOut, ) = PirexGmx(platform).depositGlp(
        address(gmxBaseReward),
        gmxBaseRewardAmountIn,
        minUsdg,
        minGlp,
        address(this)
    );
}


- if (erc20Token != gmxBaseReward) {
-    erc20Token.safeApprove(platform, tokenAmount);
- }

+ erc20Token.safeApprove(platform, tokenAmount);

(, uint256 assets, ) = PirexGmx(platform).depositGlp(
    token,
    tokenAmount,
    minUsdg,
    minGlp,
    address(this)
);

We can now safely remove it from the constructor as well.

gmxBaseReward = ERC20(_gmxBaseReward);
platform = _platform;
rewardsModule = _rewardsModule;

- // Approve the Uniswap V3 router to manage our base reward (inbound swap token)
- // gmxBaseReward.safeApprove(address(_platform), type(uint256).max);

AutoPxGmx

+ gmx.safeApprove(platform, amount);

(, uint256 assets, ) = PirexGmx(platform).depositGmx(
    amount,
    address(this)
);


+ gmx.safeApprove(platform, gmx.balanceOf(address(this)));

(, pxGmxMintAmount, ) = PirexGmx(platform).depositGmx(
    gmx.balanceOf(address(this)),
    address(this)
);  

---

The text was updated successfully, but these errors were encountered:

All reactions