Redacted Cartel contest - hansfriese's results

Lines of code

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L242

Vulnerability details

Impact

A malicious user might call AutoPxGmx.compound() with a higher fee than poolFee.

As a result, there would be a fund loss for the users because they paid more than expected for the swap router.

Proof of Concept

As we can see here, Uniswap V3 introduces multiple pools for each token pair.

In the AutoPxGmx.sol, all functions like withdraw(), redeem() and beforeDeposit() use the poolFee set by admin.

But compound() itself is a public function and users can call this function for the incentive.

Users can set the fee as a parameter and the below situation would be possible.

• Admin set poolFee = 0.3% and all functions mentioned above will use this fee percent.
• A malicious user calls compound() with fee = 1%.
• Then the pool of fee = 1% might exist here, the swap router will take 1% as a fee.
• As a result, the users get fewer gmx than expected.

Tools Used

Manual Review

Recommended Mitigation Steps

I think we should remove the fee param from compound() function and use poolFee by default.

Lines of code

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L152 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGlp.sol#L130

Vulnerability details

Impact

In AutoPxGmx.sol and AudotPxGlp.sol, it doesn't approve properly when platform is changed.

As a result, PirexGmx contract can't transfer gmx or gmxBaseReward from these contracts and the main logic won't work as expected.

Proof of Concept

As we can see here and here, it approves the platform in the constructor.

But in the AutoPxGmx.setPlatform() and AudotPxGlp.setPlatform(), it doesn't approve again for the updated platform.

As a result, AutoPxGmx.depositGmx() and AudotPxGlp.depositGlp() wouldn't work properly and always revert.

    // Convert sender GMX into pxGMX and get the post-fee amount (i.e. assets)
    (, uint256 assets, ) = PirexGmx(platform).depositGmx(
        amount,
        address(this)
    );

    // Approve as needed here since it can be a new whitelisted token (unless it's the baseReward)
    if (erc20Token != gmxBaseReward) {
        erc20Token.safeApprove(platform, tokenAmount);
    }

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

Tools Used

Manual Review

Recommended Mitigation Steps

We should approve the new platform again after it's changed.

[L-01] Wrong comment

• https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGlp.sol#L86

    // Approve the Uniswap V3 router to manage our base reward (inbound swap token)

It should be removed as this contract doesn't use the swap router.

[L-02] Wrong parameter for beforeDeposit()

• https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/PirexERC4626.sol#L65

    if (totalAssets() != 0) beforeDeposit(receiver, assets, shares);

    // Check for rounding error since we round down in previewDeposit.
    require((shares = previewDeposit(assets)) != 0, "ZERO_SHARES");

It calls beforeDeposit() before shares is calculated so shares will be 0 always.

Although beforeDeposit() doesn't use the parameters now, it's recommened to pass correct values. (Or we can just use 0 for all).

• https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/PirexERC4626.sol#L85

    if (totalAssets() != 0) beforeDeposit(receiver, assets, shares);

    assets = previewMint(shares); // No need to check for rounding error, previewMint rounds up.

It's almost the same as the above case and assets is used before calculated.

[N-01] Typo

• https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexRewards.sol#L336

    @return rewardAmounts    ERC20[]  Reward token amounts

Change ERC20[] to uint256[].