Redacted Cartel contest - aphak5010's results

Lines of code

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

Vulnerability details

Impact

The AutoPxGmx.compound (https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L242) function is used to compound the rewards from the pxGmx tokens.

The gmxBaseReward must therefore be swapped for gmx tokens.

This is done by calling the IV3SwapRouter.exactInputSingle function.

The AutoPxGmx.compound function takes a fee paramter that is then passed to the IV3SwapRouter (https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L272).

The fee parameter can be any uint24 number.

When the compound function is called internally, the fee is 3000 which the IV3SwapRouter interprets as 0.3%.

As you can see here, there can be different pools with different fees for a token pair. Also more fee levels can be introduced in the future.

Uniswap v3 introduces multiple pools for each token pair, each with a different swapping fee. Liquidity providers may initially create pools at three fee levels: 0.05%, 0.30%, and 1%. More fee levels may be added by UNI governance

This opens up the possibility of the compound function to be called with an unnecessarily high fee parameter, thereby causing a loss for aPxGmx token holders.

Proof of Concept

Just look at the compound function and see that the fee parameter is passed to the IV3SwapRouter (https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L272).

Tools Used

VSCode

Recommended Mitigation Steps

There exists a poolFee variable in the AutoPxGmx contract that can be adapted by the owner via the setPoolFee function. The poolFee variable is currently only used when the compound function is called internally.

The fee paramter could be removed from the compound function and instead the poolFee variable can be used.

Alternatively you might use a MAX_POOL_FEE variable and verify that the fee is not bigger than the MAX_POOL_FEE. Thereby the compound function might use 0.3% pools and 1% pools but there is no possibility for large fees if Uniswap introduces pools with higher fees.

Lines of code

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/PirexERC4626.sol#L225 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L14 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGlp.sol#L14 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L315 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L199 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L215-L216 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L332

Vulnerability details

Impact

The ERC-4626 Tokenized Vault Standard requires the maxWithdraw function to be implemented (https://ethereum.org/en/developers/docs/standards/tokens/erc-4626/#maxwithdraw).

This function is supposed to return "the maximum amount of underlying assets that can be withdrawn from the owner balance with a single withdraw call".

The PirexERC4626 contract implements this function (https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/PirexERC4626.sol#L225).
It is implemented correctly when PirexERC4626 is used on its own.

However in this project, the PirexERC4626 contract is not used on its own but inherited by AutoPxGmx (https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L14) and AutoPxGlp (https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGlp.sol#L14).

AutoPxGmx and AutoPxGlp implement a withdrawalPenalty i.e. a fee that is paid when a user wants to withdraw assets from the vault.

AutoPxGmx and AutoPxGlp do not override the maxWithdraw function.

This causes the maxWithdraw function to return an amount of assets that is too big to be withdrawn.

So when maxWithdraw is called and with the returned amount withdraw is called, the call to withdraw will revert.

This can cause issues in any upstream components that rely on AutoPxGmx and AutoPxGlp to correctly implement the ERC4626 standard.

For example an upstream wrapper might only allow withdrawals with the maximum amount and determine this maximum amount by calling the maxWithdraw function. As this function returns a value that is too big, no withdrawals will be possible.

Proof of Concept

1. The maxWithdraw function in a AutoPxGmx contract is called
2. Now the withdraw function is called with the value that was returned by the maxWithdraw function (https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L315)
3. The withdraw function in turn calls the previewWithdraw function (https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L199)
4. The previewWithdraw function will increase the amount of shares to include the withdrawalPenalty (https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L215-L216) which causes the amount of shares to burn to be too large and the call to burn will revert (https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L332)

Tools Used

VSCode

Recommended Mitigation Steps

In the AutoPxGmx and AutoPxGlp function implement the maxWithdraw function that overrides the function in PirexERC4626 and takes into account the withdrawalPenalty.

Potential fix:

function maxWithdraw(address owner) public view override returns (uint256) {
    uint256 shares = balanceOf(owner);

    // Calculate assets based on a user's % ownership of vault shares
    uint256 assets = convertToAssets(shares);

    uint256 _totalSupply = totalSupply;

    // Calculate a penalty - zero if user is the last to withdraw
    uint256 penalty = (_totalSupply == 0 || _totalSupply - shares == 0)
        ? 0
        : assets.mulDivDown(withdrawalPenalty, FEE_DENOMINATOR);

    // Redeemable amount is the post-penalty amount
    return assets - penalty;
}

Lines of code

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L97 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L370 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L242 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#L87

Vulnerability details

Impact

The AutoPxGmx contract calls in its constructor gmx.safeApprove(_platform, type(uint256).max); (https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L97).

This means that the platform can now transfer gmx token that is owned by AutoPxGmx.

This is needed in order for the AutoPxGmx.depositGmx (https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L370) and AutoPxGmx.compound (https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L242) functions to work since by calling any of them the platform will transfer the gmx tokens to be deposited from the AutoPxGmx contract.

The issue is this:
The AutoPxGmx contract has a function called setPlatform (https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L152) that allows to change the platform variable.
This function does NOT approve the new platform to transfer gmx tokens.
So effectively the platform cannot be changed since changing it makes the contract unable to function properly.
The owner of the AutoPxGmx contract would of course notice the issue and can change the platform to be the original again.
But the issue remains that the platform must stay the same in order for the contract to be fully operable.

The same issue exists for the AutoPxGlp contract and the gmxBaseReward token (https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGlp.sol#L87).
I won't go into this in detail since the issue is pretty much the same. You can see how to fix both issues in the "Recommend Mitigation Steps" section.

Proof of Concept

1. A AutoPxGmx contract is set up
2. The AutoPxGmx.setPlatform function is called in order to change the platform
3. Any call to AutoPxGmx.depositGmx or AutoPxGmx.compound is going to fail because the platform is missing approval to transfer gmx tokens owned by AutoPxGmx

Tools Used

VSCode

Recommended Mitigation Steps

Change AutoPxGmx.setPlatform function like this:

     function setPlatform(address _platform) external onlyOwner {
         if (_platform == address(0)) revert ZeroAddress();
 
+        gmx.safeApprove(platform, 0);
         platform = _platform;
+        gmx.safeApprove(_platform, type(uint256).max);
 
         emit PlatformUpdated(_platform);
     }

And change AutoPxGlp.setPlatform function like this:

     function setPlatform(address _platform) external onlyOwner {
         if (_platform == address(0)) revert ZeroAddress();
 
+        gmxBaseReward.safeApprove(address(platform), 0);
         platform = _platform;
+        gmxBaseReward.safeApprove(address(_platform), type(uint256).max);
 
         emit PlatformUpdated(_platform);
     }

Redacted Cartel Low Risk and Non-Critical Issues

Summary

[L-00] No zero address check and 2-Step-Transfer in solmate/auth/Owned.sol

4 contracts inherit the Owned contract in solmate/auth/Owned.sol (https://github.com/transmissions11/solmate/blob/main/src/auth/Owned.sol).

Inheriting contracts:

• https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexGmx.sol#L20
• https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexFees.sol#L8
• https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/PxGmxReward.sol#L11
• https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L14

This Owned contract is a very minimal implementation and allows the owner of the contract to assign any new owner (even the zero address) (https://github.com/transmissions11/solmate/blob/8d910d876f51c3b2585c9109409d601f600e68e1/src/auth/Owned.sol#L40).

It is best practice to add a zero-address check and make transferring ownership a 2-Step-Process.
The current owner sets a pending owner and the pending owner must then claim ownership.
This mitigates scenarios where ownership of the contract can be lost.

[L-01] Missing whenNotPaused modifier

The PirexGmx.claimUserReward (https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexGmx.sol#L824) and PirexGmx.claimRewards (https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexGmx.sol#L739) functions should include the whenNotPaused modifier meaning they should only be executed when the contract is not paused.

This is because in a paused state important gmx state variables can be configured with the PirexGmx.configureGmxState function (https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexGmx.sol#L272) and the contract can be migrated.

So one must assume that when the contract is paused, the gmx state variables might not be assigned correctly or the contract is undergoing migration.

However the claimUserReward and claimRewards functions rely on the state to be configured correctly. So they should only be callable when the contract is not paused.

In the worst case that the old gmx state behaves very badly, a loss of funds might occur by calling the claimRewards or claimUserReward function.

[L-02] Unsafe check that addres is a contract

The PirexRewards.setRewardRecipientPrivileged and PirexRewards.unsertRewardRecipientPrivileged functions check that the lpContract parameter is the address of a contract by checking the code.length property.

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

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

This is an unsafe way to check if the address is the address of a contract because a contract can temporarily sefldestruct and be recreated by using CREATE2.

This means that a contract can be set using the setRewardRecipientPrivileged function but cannot be unsert anymore using the unsetRewardRecipientPrivileged function if the contract uses selfdestruct.

In the function documentation it is mentioned that the functions will only be used to handle Pirex-GMX LP contracts (https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexRewards.sol#L424-L427). So there should hopefuilly be no issue as long as the functions are used as intended.

However I think this information might still be helpful to you.

[N-00] Event is missing indexed fields

Each event should use three indexed fields if it has three or more fields.

There are 34 instances of events that do not have 3 indexed fields.

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

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

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

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

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

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

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexGmx.sol#L95

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexGmx.sol#L96

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexGmx.sol#L97

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexGmx.sol#L125

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexGmx.sol#L133

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexGmx.sol#L140

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexGmx.sol#L141

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexGmx.sol#L142

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexGmx.sol#L143

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexFees.sol#L34

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexFees.sol#L35

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexFees.sol#L36

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

[N-01] Variable can be declared immutable

Variables that are only assigned in the constructor should be declared immutable.

There are 1 instances of this.

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

[N-02] Comment not according to logic

Instance 1 and 2

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

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

The above 2 comments state that the transfer / transferFrom function is overriden in order to allow a pre-transfer internal hook to run.

However when you look into the function code you will see that it is not a pre-transfer but post-transfer hook that runs.

Instance 3

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

It is not the "Uniswap V3 router" that is approved but the platform.

Redacted Cartel Gas Optimization Findings

Summary

The Gas savings are calculated using the scripts/forgeTest.sh --gas-report command.
The same tests were used that are provided in the contest repository.

[G-00] Claiming rewards from PirexRewards requires redundant calls to harvest function

A user can claim rewards from the PirexRewards contract with the claim function (https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexRewards.sol#L373).

The claim function calls the harvest function which claims rewards from the producer for multiple producerTokens (https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexRewards.sol#L346-L347).

So when a user wants to claim rewards from the PirexRewards contract for multiple producerTokens, he needs to call the claim function once for each producerToken which results in multiple of the same calls to producer.claimRewards.

One call to producer.claimRewards is enough.

This can be achieved by implementing a claimBatch function in PirexRewards that calls harvest and thereby producer.claimRewards once and then iterates over all provided producerTokens.

This results in substantial Gas savings since one call to producer.claimRewards requires on average 161614 Gas.

[G-01] Superfluous assert statement

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexGmx.sol#L225

The assert statement is redundant since it will always be true.

This is because the line above it calculates:

postFeeAmount = assets - feeAmount;

By applying basic algebra it follows that:

assets = postFeeAmount + feeAmount

This is exactly what the assert checks. But as you can see the check will always be true.

So the assert statement can just be removed.

Deployment Gas saved: 9216
Method call Gas saved: 95

[G-02] X = X + Y costs less Gas than X += Y

There is 1 instance of this:

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

Fix:

rewardState = rewardState + rewardAmount;

Deployment Gas saved: 1200
Method call Gas saved: 1714