Redacted Cartel contest - Diana's results

01 SAFEAPPROVE() IS DEPRECATED

Deprecated in favor of safeIncreaseAllowance() and safeDecreaseAllowance(). If only setting the initial allowance to the value that means infinite, safeIncreaseAllowance() can be used instead

There are 9 instances of this issue:

https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/PirexGmx.sol

File: src/PirexGmx.sol

292: gmx.safeApprove(address(stakedGmx), type(uint256).max);
348: gmx.safeApprove(address(stakedGmx), 0);
353: gmx.safeApprove(contractAddress, type(uint256).max);
507: t.safeApprove(glpManager, tokenAmount);

https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/vaults/AutoPxGlp.sol

File: src/vaults/AutoPxGlp.sol

87: gmxBaseReward.safeApprove(address(_platform), type(uint256).max);
347: stakedGlp.safeApprove(platform, amount);
391: erc20Token.safeApprove(platform, tokenAmount);

https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/vaults/AutoPxGmx.sol

File: src/vaults/AutoPxGmx.sol

96: gmxBaseReward.safeApprove(address(SWAP_ROUTER), type(uint256).max
97: gmx.safeApprove(_platform, type(uint256).max);

---

02 REQUIRE() SHOULD BE USED INSTEAD OF ASSERT()

There is 1 instance of this issue

https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/PirexGmx.sol

File: src/PirexGmx.sol

225: assert(feeAmount + postFeeAmount == assets);

---

03 EVENT IS MISSING INDEXED FIELDS

Each event should use three indexed fields if there are three or more fields

There are 34 instances of this issue

https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/PirexFees.sol

File: src/PirexFees.sol

34: event SetFeeRecipient(FeeRecipient f, address recipient);
35: event SetTreasuryFeePercent(uint8 _treasuryFeePercent);
36: event DistributeFees(

https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/PirexGmx.sol

File: src/PirexGmx.sol

86: event ConfigureGmxState(
95: event SetFee(Fees indexed f, uint256 fee);
96: event SetContract(Contracts indexed c, address contractAddress);
97: event DepositGmx(
125: event ClaimRewards(
133: event ClaimUserReward(
140: event InitiateMigration(address newContract)
141: event CompleteMigration(address oldContract);
142: event SetDelegationSpace(string delegationSpace, bool shouldClear);
143: event SetVoteDelegate(address voteDelegate);

https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/PirexRewards.sol

File: src/PirexRewards.sol

33: event SetProducer(address producer);
49: event RemoveRewardToken(ERC20 indexed producerToken, uint256 removalIndex);
50: event GlobalAccrue(
56: event UserAccrue(
63: event Harvest(

https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/vaults/AutoPxGlp.sol

File: src/vaults/AutoPxGlp.sol

35: event WithdrawalPenaltyUpdated(uint256 penalty);
36: event PlatformFeeUpdated(uint256 fee);
37: event CompoundIncentiveUpdated(uint256 incentive);
38: event PlatformUpdated(address _platform);
39: event Compounded(

https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/vaults/AutoPxGmx.sol

File: src/vaults/AutoPxGmx.sol

39: event PoolFeeUpdated(uint24 _poolFee);
40: event WithdrawalPenaltyUpdated(uint256 penalty);
41: event PlatformFeeUpdated(uint256 fee);
42: event CompoundIncentiveUpdated(uint256 incentive);
43: event PlatformUpdated(address _platform);
44: event Compounded(

https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/vaults/PirexERC4626.sol

File: src/vaults/PirexERC4626.sol

27: event Deposit(

https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/vaults/PxGmxReward.sol

File: src/vaults/PxGmxReward.sol

21: event GlobalAccrue(uint256 lastUpdate, uint256 lastSupply, uint256 rewards);
22: event UserAccrue(
28: event Harvest(uint256 rewardAmount);
29: event PxGmxClaimed(

---

04 USING BOTH NAMED RETURNS AND A RETURN STATEMENT ISN'T NECESSARY

Removing unused named returns variables can reduce gas usage (MSTOREs/MLOADs) and improve code clarity. To save gas and improve code quality: consider using only one of those.

There is 1 instance of this issue

https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/PirexRewards.sol

File: src/PirexRewards.sol

207: function getUserState(ERC20 producerToken, address user)

---

G-01 x += y COSTS MORE GAS THAN x = x + y FOR STATE VARIABLES (SAME APPLIES FOR x -= y , x = x - y)

There are 6 instances of this issue

https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/PirexRewards.sol

File: src/PirexRewards.sol

361: producerState.rewardStates[rewardTokens[i]] += r;

https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/PxERC20.sol

File: src/PxERC20.sol

85: balanceOf[msg.sender] -= amount;
90: balanceOf[to] += amount;
119: balanceOf[from] -= amount;
125: balanceOf[to] += amount;

https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/PxGmx.sol

File: src/PxGmx.sol

95: rewardState += rewardAmount;

---

G-02 INCREMENTS CAN BE UNCHECKED

In Solidity 0.8+, there’s a default overflow check on unsigned integers. It’s possible to uncheck this in for-loops and save some gas at each iteration, but at the cost of some code readability, as this uncheck cannot be made inline

Prior to Solidity 0.8.0, arithmetic operations would always wrap in case of under- or overflow leading to widespread use of libraries that introduce additional checks.

Since Solidity 0.8.0, all arithmetic operations revert on over- and underflow by default, thus making the use of these libraries unnecessary.

To obtain the previous behaviour, an unchecked block can be used

There are 3 instances of this issue

https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/PirexRewards.sol

File: src/PirexRewards.sol

163: for (uint256 i; i < len; ++i) {
351: for (uint256 i; i < pLen; ++i) {
396: for (uint256 i; i < rLen; ++i) {

---

G-03 USING BOTH NAMED RETURNS AND A RETURN STATEMENT ISN'T NECESSARY

Removing unused named returns variables can reduce gas usage (MSTOREs/MLOADs) and improve code clarity. To save gas and improve code quality: consider using only one of those.

There is 1 instance of this issue

https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/PirexRewards.sol

File: src/PirexRewards.sol

207: function getUserState(ERC20 producerToken, address user)

---

G-04 EXPRESSIONS FOR CONSTANT VALUES SUCH AS A CALL TO KECCAK256(), SHOULD USE IMMUTABLE RATHER THAN CONSTANT

There are 2 instances of this issue

https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/PxERC20.sol

File: src/PxERC20.sol

9: bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE");
10: bytes32 public constant BURNER_ROLE = keccak256("BURNER_ROLE");

---