Redacted Cartel contest - cryptonue's results

Lines of code

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

Vulnerability details

Impact

Potential PirexReward's producerTokens's rewardToken unsynced with PirexGmx rewardToken can miss calculate the actual reward for user

Proof of Concept

PirexReward initialization does not include rewardToken initialization for producerTokens. Meanwhile inside PirexGmx, it is already hardcoded, the claimRewards will return array of rewardToken contains 4 exact items.

In order for PirexReward to sync or know what are the rewardToken with its producerTokens, there are two functions related to this addRewardToken and removeRewardToken, (also this is prone to issues)

If at some point user do harvest() but the PirexReward only insert 2 rewardToken, or maybe insert 4 rewardToken but one of it is not the correct producerToken, this can raise an issue.

User'ss reward is not being correctly calculated since the rewardToken is missing, and the accounting is not running correctly (until the Owner/Admin correctly input the rewardToken synced with PirexGmx's)

File: PirexRewards.sol
338:     function harvest()
339:         public
340:         returns (
341:             ERC20[] memory _producerTokens,
342:             ERC20[] memory rewardTokens,
343:             uint256[] memory rewardAmounts
344:         )
345:     {
346:         (_producerTokens, rewardTokens, rewardAmounts) = producer
347:             .claimRewards();
348:         uint256 pLen = _producerTokens.length;
349: 
350:         // Iterate over the producer tokens and update reward state
351:         for (uint256 i; i < pLen; ++i) {
352:             ERC20 p = _producerTokens[i];
353:             uint256 r = rewardAmounts[i];
354: 
355:             // Update global reward accrual state and associate with the update of reward state
356:             ProducerToken storage producerState = producerTokens[p];
357: 
358:             _globalAccrue(producerState.globalState, p);
359: 
360:             if (r != 0) {
361:                 producerState.rewardStates[rewardTokens[i]] += r;
362:             }
363:         }
364: 
365:         emit Harvest(_producerTokens, rewardTokens, rewardAmounts);
366:     }
367: 

from this harvest() function the ProducerToken storage producerState = producerTokens[p]; is not being checked if it exist or not if the token is already being inputed by Admin/Owner.

If user assign to get reward of token X, meanwhile there is no rewardToken X set (or wrongly set) inside PirexRewards the harvest() will skip the user reward accounting, thus decrease the actual reward user should get. (because of delayed userAccrue/globalAccrue, for the token X)

Tools Used

Manual

Recommended Mitigation Steps

Include pirexReward initialization of rewardToken inside initialization function, or check if produceToken (and its rewardToken) inside pirexReward is equal to PirexGmx claimRewards returns data.

[G] USING PRIVATE RATHER THAN PUBLIC FOR CONSTANTS, SAVES GAS

If needed, the values can be read from the verified contract source code, or if there are multiple values there can be a single getter function that returns a tuple of the values of all currently-public constants. Saves 3406-3606 gas in deployment gas due to the compiler not having to create non-payable getter functions for deployment calldata, not having to store the bytes of the value outside of where it’s used, and not adding another entry to the method ID table

instances:

File: AutoPxGlp.sol
18:     uint256 public constant MAX_WITHDRAWAL_PENALTY = 500;
19:     uint256 public constant MAX_PLATFORM_FEE = 2000;
20:     uint256 public constant FEE_DENOMINATOR = 10000;
21:     uint256 public constant MAX_COMPOUND_INCENTIVE = 5000;
22:     uint256 public constant EXPANDED_DECIMALS = 1e30;

File: AutoPxGmx.sol
18:     IV3SwapRouter public constant SWAP_ROUTER =
19:         IV3SwapRouter(0x68b3465833fb72A70ecDF485E0e4C7bD8665Fc45);
20:     uint256 public constant MAX_WITHDRAWAL_PENALTY = 500;
21:     uint256 public constant MAX_PLATFORM_FEE = 2000;
22:     uint256 public constant FEE_DENOMINATOR = 10000;
23:     uint256 public constant MAX_COMPOUND_INCENTIVE = 5000;

File: PirexFees.sol
20:     uint8 public constant FEE_PERCENT_DENOMINATOR = 100;
21: 
22:     // Maximum treasury fee percent
23:     uint8 public constant MAX_TREASURY_FEE_PERCENT = 75;

File: PirexGmx.sol
43:     // Fee denominator
44:     uint256 public constant FEE_DENOMINATOR = 1_000_000;
45: 
46:     // Fee maximum (i.e. 20%)
47:     uint256 public constant FEE_MAX = 200_000;

File: PxERC20.sol
09:     bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE");
10:     bytes32 public constant BURNER_ROLE = keccak256("BURNER_ROLE");

[G] USAGE OF UINTS/INTS SMALLER THAN 32 BYTES (256 BITS) INCURS OVERHEAD

When using elements that are smaller than 32 bytes, your contract’s gas usage may be higher. This is because the EVM operates on 32 bytes at a time. Therefore, if the element is smaller than that, the EVM must use more operations in order to reduce the size of the element from 32 bytes to the desired size.

https://docs.soliditylang.org/en/v0.8.11/internals/layout_in_storage.html Each operation involving a uint8 costs an extra 22-28 gas (depending on whether the other operand is also a variable of type uint8) as compared to ones involving uint256, due to the compiler having to clear the higher bits of the memory word before operating on the uint8, as well as the associated stack operations of doing so. Use a larger size then downcast where needed

instances:

File: PirexFees.sol
20:     uint8 public constant FEE_PERCENT_DENOMINATOR = 100;
23:     uint8 public constant MAX_TREASURY_FEE_PERCENT = 75;
28:     uint8 public treasuryFeePercent = MAX_TREASURY_FEE_PERCENT;

[G] <X> += <Y> COSTS MORE GAS THAN <X> = <X> + <Y> FOR STATE VARIABLES

Using the addition operator instead of plus-equals saves 113 gas

instances:

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

File: PxGmxReward.sol
95:             rewardState += rewardAmount;

File: PxERC20.sol
90:             balanceOf[to] += amount;

[G] ++I/I++ SHOULD BE UNCHECKED{++I}/UNCHECKED{I++} WHEN IT IS NOT POSSIBLE FOR THEM TO OVERFLOW, AS IS THE CASE WHEN USED IN FOR- AND WHILE-LOOPS

The unchecked keyword is new in solidity version 0.8.0, so this only applies to that version or higher, which these instances are. This saves 30-40 gas per loop

instances:

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

[G] Emitting block.timestamp is not necessary

block.timestamp and block.number are added to event information by default so adding them manually wastes gas

instances:

File: PxGmxReward.sol
61:         emit GlobalAccrue(block.timestamp, totalSupply, rewards);

83:         emit UserAccrue(user, block.timestamp, balance, rewards);