Redacted Cartel contest - chaduke's results

GA1. https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexGmx.sol#L300 It is important to check the fee range to make sure it is no more than FEE_DONOMINATOR, otherwise line 222 will not make sense - one need to make sure it is no more than 100%. Also check f' value to make sure is within Fees`.

GA2. https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexGmx.sol#L367 it is better to check for the last case GlpManager as well and treat other c value as an error. That is the purpose of enum.

        if (c == Contracts.GlpManager) {
            gmxVault = IVault(contractAddress);
            return;
        }
 
       revert WrongContractParameter(c);
    

QA3: https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexGmx.sol#L71-L72 Maybe change the two variables to rewardTrackerStakedGlp and rewardTrackerStakedGmx.

QA4: https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexFees.sol#L20 Use FEE_PERCENT_DENOMINATOR = 10000 might give a better precision

QA5: PirexRewards.sol and PirexRewards.sol share four functions: globalAccure(), userAccruee(), harvet(), and claim, refactoring into a library or tinheriting from a base class will be helpful

QA6: https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexGmx.sol#L953 the documentation should be ``called by the owner at the new contract".

QA7: https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexGmx.sol#L234 The _calculateRewards`() function differentiate four cases of reward types by the two boolean variables isBasedReward and useGmx. It might better to introduce the following enum type

     enum RewardTypes {gmxBasedRewards, glpBasedRewards, gmxEsGmxRewards, glpEsGmxRewards}
     function _calculateRewards(RewardTypes rt){
     }

QA8: https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PxGmx.sol#L19 The PirexGmx contract has a burn() function that has an empty body, which overides the behavior of that of the base contract PxERC20. As a result, PxGmx cannot be burned. It this is the intention of the design, then it is better to document it and use a revert statement in the burn implemetation to make it explicit.

QA9: https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexGmx.sol#L292 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexGmx.sol#L921 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexGmx.sol#L940 The allowance set at line L292 for gmx to stakedGmx needs to be reset to zero when migrating to a new contract.

QA10. https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexGmx.sol#L792-L806 This part can be revised to the following to prevent the discrepancy, a concern raised in the documentation

require(baseRewards == gmxBasedRewards + glpBaseRewards);
require(esGmxRewards == gmxEsGmxRewards + glpEsGmxRewards);
rewardsAmounts[0] =  gmxBaseRewards;
rewardsAmounts[1] = glpBaseRewards;
rewardsAmounts[2] =  gmxEsGmxRewards;
rewardsAmounts[3] =  glpEsGmxRewards;

GA11: https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexGmx.sol#L20 Contract PirexGmx needs to inherit interface IProducer.

contract PirexGmx is IProducer, ReentrancyGuard, Owned, Pausable {

GA12: https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexRewards.sol#L27 Better to mention PirexGmx as the current producer, it takes a while to figure this out.

G1. https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexGmx.sol#L940 Cache state variable migratedTo since it is accessed twice in the function.

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

Cache block.timestamp.

G3. https://code4rena.com/contests/2022-11-redacted-cartel-contest Check whether it is necessary to change the state variables (instead of changing them nevertheless) can save gas:

       if(lastSupply != totalSupply){
            globalState.lastSupply = totalSupply.safeCastTo224();
       }

       if(blocktime != lastUpdate){
           globalState.lastUpdate = blocktime // blocktime is a cache of block.timestamp
           globalState.rewards = globalState.rewards +(blocktime - lastUpdate) *
                lastSupply;
       }

G4. https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexRewards.sol#L293-L295 Checking wether it is really necessary to change the state vairables can save gas.

       if(blocktime != lastUpdate){
                   u.lastUpdate = blocktime; // blocktime caches block.timestamp
                   u.rewards = u.rewards +  lastBlance *   (blocktime - lastUpdate);
       }

       if(lastBalance != balance){
                  u.lastBalance = balance.safeCastTo224();
       }
 ``

G5. https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexGmx.sol#L863
Change the argument ``string memory _delegationSpace`` to ``bytes32`` can save gas since no transformation is needed.

function setDelegationSpace( bytes32 _delegationSpace, bool shouldClear ) external onlyOwner { if (shouldClear) { // Clear the delegation for the current delegation space clearVoteDelegate(); }
delegationSpace = _delegationSpace

    emit SetDelegationSpace(_delegationSpace, shouldClear);
}