Redacted Cartel contest - keccak123's results

Judge has assessed an item in Issue #316 as M risk. The relevant finding follows:

compound in AutoPxGmx can be called by anyone and can be sandwiched if a poorly chosen amountOutMinimum is used. The idea is to call the function often by adding an incentive to the caller. There is a problematic hidden incentive - the amountOutMinimum value is controlled by the caller. This means that MEV bots have incentive to call this function and take a cut of the rewards by setting amountOutMinimum to a lower value than could be received by the vault.

The function argument amountOutMinimum is on line 244. It is used in the Uniswap swap on line 275. It can be set to any value except zero, enabling the majority of the value from the swap to be lost.

Recommendation Disincentive sandwiching of this swap. There should be additional checks that the amountOutMinimum function argument is a reasonable value. One way to do this is using a TWAP or oracle to get the exchange rate and use gmxBaseRewardAmountIn to calculate what the minimum allowable amountOutMinimum is.

[QA-01]

compound in AutoPxGmx can be called by anyone and can be sandwiched if a poorly chosen amountOutMinimum is used. The idea is to call the function often by adding an incentive to the caller. There is a problematic hidden incentive - the amountOutMinimum value is controlled by the caller. This means that MEV bots have incentive to call this function and take a cut of the rewards by setting amountOutMinimum to a lower value than could be received by the vault.

The function argument amountOutMinimum is on line 244. It is used in the Uniswap swap on line 275. It can be set to any value except zero, enabling the majority of the value from the swap to be lost.

Recommendation

Disincentive sandwiching of this swap. There should be additional checks that the amountOutMinimum function argument is a reasonable value. One way to do this is using a TWAP or oracle to get the exchange rate and use gmxBaseRewardAmountIn to calculate what the minimum allowable amountOutMinimum is.

[QA-02]

_setupRole is deprecated. Instead use _grantRole. This also removes a function call because _setupRole calls _grantRole. _setupRole is found in PxERC20

OpenZeppelin clearly indicates this function is deprecated

Recommendation

Use _grantRole not _setupRole

[QA-03]

This comment about approving the Uniswap router is wrong. It was accidentally copied from another file.

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

Recommendation

Change the comment to Approve platform address to manage our base reward

[QA-04]

_platform is already of type address in this line. It does not need to be cast to address

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

Recommendation

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

[QA-05]

Identical functions in AutoPxGlp and AutoPxGmx have slightly different comments.

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L170-L171 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGlp.sol#L148-L149

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L196-L197 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGlp.sol#L174-L175

-         @param  assets  uint256  Assets
-         @return uint256  Shares
+         @param  assets   uint256  Assets amount
+         @return          uint256  Shares amount

Recommendation

Use the same comments in different contracts of a project when the code is the same.

[G-01] Replace require with errors

Solidity errors save gas compared to require

Three lines use require:

1. https://github.com/code-423n4/2022-11-redactedcartel/tree/main/src/vaults/PirexERC4626.sol#L68
2. https://github.com/code-423n4/2022-11-redactedcartel/tree/main/src/vaults/PirexERC4626.sol#L137
3. https://github.com/code-423n4/2022-11-redactedcartel/tree/main/src/vaults/AutoPxGmx.sol#L355

Recommendation

Use solidity errors to replace require for gas savings

[G-02] Change constant variables to private

Variables that are private or internal use less gas than public or external variables

Many variables can make use of this change if the contract does not need to make these variables public.

• https://github.com/code-423n4/2022-11-redactedcartel/tree/main/src/vaults/AutoPxGlp.sol#L18-L22
• https://github.com/code-423n4/2022-11-redactedcartel/tree/main/src/vaults/AutoPxGmx.sol#L18
• https://github.com/code-423n4/2022-11-redactedcartel/tree/main/src/vaults/AutoPxGmx.sol#L20-L23
• https://github.com/code-423n4/2022-11-redactedcartel/tree/main/src/PxERC20.sol#L9-L10
• https://github.com/code-423n4/2022-11-redactedcartel/tree/main/src/vaults/AutoPxGmx.sol#L36-L37
• https://github.com/code-423n4/2022-11-redactedcartel/tree/main/src/PirexGmx.sol#L50-L52
• https://github.com/code-423n4/2022-11-redactedcartel/tree/main/src/PirexGmx.sol#L55-L56

Recommendation

Make variables private or internal if they do not need to be public or external

[G-03] Make certain functions payable

A payable function saves gas compared to one that is not payable. If a function has an onlyOwner modifier or has similar access controls to prevent the function from receiving ETH, the function can be payable to save gas.

Many functions have the onlyOwner modifier and can be more efficient

• setWithdrawalPenalty
• setPlatformFee
• setCompoundIncentive
• setPlatform
• setPoolFee
• setWithdrawalPenalty
• setPlatformFee
• setCompoundIncentive
• setPlatform

Recommendation

Make all functions with onlyOwner payable

[G-04] Unchecked saves gas

Use unchecked when possible for gas savings. Line 403: https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexRewards.sol#L403

- uint256 amount = (rewardState * userRewards) / globalRewards;
+ unchecked { uint256 amount = (rewardState * userRewards) / globalRewards; }

Recommendation

Use unchecked for gas savings when there is no overflow or underflow risk