Redacted Cartel contest - 0xSmartContract's results

Lines of code

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

Vulnerability details

Impact

EIP4626 Rounding Specification (https://eips.ethereum.org/EIPS/eip-4626) Thus, the result of the previewMint and previewWithdraw should be rounded up

Finally, ERC-4626 Vault implementers should be aware of the need for specific, 
opposing rounding directions across the different mutable and view methods, as it is 
considered most secure to favor the Vault itself during calculations over its users:

  If (1) it’s calculating how many shares to issue to a user for a certain amount of the 
underlying tokens they provide or (2) it’s determining the amount of the underlying tokens 
to transfer to them for returning a certain amount of shares, it should round down.
  If (1) it’s calculating the amount of shares a user has to supply to receive a given 
amount of the underlying tokens or (2) it’s calculating the amount of underlying 
tokens a user has to provide to receive a certain amount of shares, it should round up.

The current implementation of convertToShares function will round down the number of shares returned due to how solidity handles Integer Division. ERC4626 expects the returned value of convertToShares to be rounded down. Thus, this function behaves as expected:

src/vaults/PirexERC4626.sol:
  155  
  156:     function convertToShares(uint256 assets)
  157:         public
  158:         view
  159:         virtual
  160:         returns (uint256)
  161:     {
  162:         uint256 supply = totalSupply; // Saves an extra SLOAD if totalSupply is non-zero.
  163: 
  164:         return supply == 0 ? assets : assets.mulDivDown(supply, totalAssets());
  165:     }

ERC 4626 expects the result returned from previewWithdraw function to be rounded up. However, within the previewWithdraw function, it calls the convertToShares function

Recall earlier that the convertToShares function returned a rounded down value, thus previewWithdraw will return a rounded down value instead of round up value. Thus, this function does not behave as expected

src/vaults/AutoPxGmx.sol:
  200       */
  201:     function previewWithdraw(uint256 assets)
  202:         public
  203:         view
  204:         override
  205:         returns (uint256)
  206:     {
  207:         // Calculate shares based on the specified assets' proportion of the pool
  208:         uint256 shares = convertToShares(assets);
  209: 
  210:         // Save 1 SLOAD
  211:         uint256 _totalSupply = totalSupply;
  212: 
  213:         // Factor in additional shares to fulfill withdrawal if user is not the last to withdraw
  214:         return
  215:             (_totalSupply == 0 || _totalSupply - shares == 0)
  216:                 ? shares
  217:                 : (shares * FEE_DENOMINATOR) /
  218:                     (FEE_DENOMINATOR - withdrawalPenalty);
  219:     }

Tools Used

Manual Code Review

Recommended Mitigation Steps

Ensure that the rounding of vault's functions behave as expected. Following are the expected rounding direction for each vault function:

previewMint(uint256 shares) - Round Up ⬆

previewWithdraw(uint256 assets) - Round Up ⬆

previewRedeem(uint256 shares) - Round Down ⬇

previewDeposit(uint256 assets) - Round Down ⬇

convertToAssets(uint256 shares) - Round Down ⬇

convertToShares(uint256 assets) - Round Down ⬇

previewMint returns the amount of assets that would be deposited to mint specific amount of shares. Thus, the amount of assets must be rounded up, so that the vault won't be shortchanged.

previewWithdraw returns the amount of shares that would be burned to withdraw specific amount of asset. Thus, the amount of shares must to be rounded up, so that the vault won't be shortchanged.

Alternatively, if such alignment of rounding could not be achieved due to technical limitation, at the minimum, document this limitation in the comment so that the developer performing the integration is aware of this.

Lines of code

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

Vulnerability details

Impact

A well known attack vector for almost all shares based liquidity pool contracts, where an early user can manipulate the price per share and profit from late users' deposits because of the precision loss caused by the rather large value of price per share.

depositGMX() function has 0 shares protect. But it hasn't few shares protect. For example; It's like getting 1 share after deposit with high token;

 if (
            (shares = supply == 0
                ? assets
                : assets.mulDivDown(supply, totalAssets() - assets)) == 0
        ) revert ZeroShares();

Proof Of Concept

1 - A malicious early user can depositGMX() with 1 wei of asset token as the first depositor of the LToken, and get 1 wei of shares

2 - Then the attacker can send 10000e18 - 1 of asset tokens and inflate the price per share from 1.0000 to an extreme value of 1.0000e22 ( from (1 + 10000e18 - 1) / 1)

3 - As a result, the future user who deposits 19999e18 will only receive 1 wei (from 19999e18 * 1 / 10000e18) of shares token

4 - They will immediately lose 9999e18 or half of their deposits if they redeem() right after the depositGMX()

The attacker can profit from future users' deposits. While the late users will lose part of their funds to the attacker.

Recommended Mitigation Steps

Consider requiring a minimal amount of share tokens to be minted for the first minter, and send a port of the initial mints as a reserve to the Redacted MultiSig so that the pricePerShare can be more resistant to manipulation

function depositGmx(uint256 amount, address receiver)
        external
        nonReentrant
        returns (uint256 shares)
    {
        if (amount == 0) revert ZeroAmount();
        if (receiver == address(0)) revert ZeroAddress();

        // Handle compounding of rewards before deposit (arguments are not used by `beforeDeposit` hook)
        if (totalAssets() != 0) beforeDeposit(address(0), 0, 0);

        // Intake sender GMX
        gmx.safeTransferFrom(msg.sender, address(this), amount);

        // Convert sender GMX into pxGMX and get the post-fee amount (i.e. assets)
        (, uint256 assets, ) = PirexGmx(platform).depositGmx(
            amount,
            address(this)
        );

        // NOTE: Modified `convertToShares` logic to consider assets already being in the vault
        // and handle it by deducting the recently-deposited assets from the total
        uint256 supply = totalSupply;

        if (
            (shares = supply == 0
                ? assets
                : assets.mulDivDown(supply, totalAssets() - assets)) == 0
        ) revert ZeroShares();


+	/** for the first mint, we require the mint amount > (10 ** decimals) / 100 and send (10 ** decimals) /
+	1_000_000 of the initial supply as a reserve to Redacted MultiSig
+               */
+ if (totalSupply == 0 && decimals >= 6) {
+       require(shares > 10 ** (decimals - 2));
+      uint256 reserveShares = 10 ** (decimals - 6);
+     _mint(Redacted_MultiSig, reserveShares);
+       shares -= reserveShares;
+     }

        _mint(receiver, shares);

        emit Deposit(msg.sender, receiver, assets, shares);
    }

Summary

Low Risk Issues List

Total 11 issues

Non-Critical Issues List

Total 19 issues

Suggestions

Total 2 suggestions

[L-01] PirexERC4626's implmentation is not fully up to EIP-4626's specification

Must return the maximum amount of shares mint would allow to be deposited to receiver and not cause a revert, which must not be higher than the actual maximum that would be accepted (it should underestimate if necessary).

This assumes that the user has infinite assets, i.e. must not rely on balanceOf of asset.

src/vaults/PirexERC4626.sol:
  216  
  217:     function maxDeposit(address) public view virtual returns (uint256) {
  218:         return type(uint256).max;
  219:     }
  220: 
  221:     function maxMint(address) public view virtual returns (uint256) {
  222:         return type(uint256).max;
  223:     }

Could cause unexpected behavior in the future due to non-compliance with EIP-4626 standard.

Similar problem for Sentimentxyz ; https://github.com/sentimentxyz/protocol/pull/235/files

Recommended Mitigation Steps

maxMint() and maxDeposit() should reflect the limitation of maxSupply.

Consider changing maxMint() and maxDeposit() to:

function maxMint(address) public view virtual returns (uint256) {
    if (totalSupply >= maxSupply) {
        return 0;
    }
    return maxSupply - totalSupply;
}
function maxDeposit(address) public view virtual returns (uint256) {
    return convertToAssets(maxMint(address(0)));
}

[L-02] initialize() function can be called by anybody

initialize() function can be called anybody when the contract is not initialized.

More importantly, if someone else runs this function, they will have full authority because of the __Ownable_init() function.

Here is a definition of initialize() function.

src/PirexRewards.sol:
  84  
  85:     function initialize() public initializer {
  86:         __Ownable_init();
  87:     }

Recommended Mitigation Steps

Add a control that makes initialize() only call the Deployer Contract or EOA;

if (msg.sender != DEPLOYER_ADDRESS) {
            revert NotDeployer();
        }

[L-03] Solmate's SafeTransferLib doesn't check whether the ERC20 contract exists

Solmate's SafeTransferLib, which is often used to interact with non-compliant/unsafe ERC20 tokens, does not check whether the ERC20 contract exists. The following code will not revert in case the token doesn't exist (yet).

This is stated in the Solmate library: https://github.com/transmissions11/solmate/blob/main/src/utils/SafeTransferLib.sol#L9

19 results 

src/PirexFees.sol:
    5: import {SafeTransferLib} from "solmate/utils/SafeTransferLib.sol";

  114:         token.safeTransfer(treasury, treasuryDistribution);
  115:         token.safeTransfer(contributors, contributorsDistribution);

src/PirexGmx.sol:
    7: import {SafeTransferLib} from "solmate/utils/SafeTransferLib.sol";

  392:         gmx.safeTransferFrom(msg.sender, address(this), amount);

  506:             t.safeTransferFrom(msg.sender, address(this), tokenAmount);

  643:             ERC20(pxGlp).safeTransferFrom(

  844:             gmxBaseReward.safeTransfer(receiver, postFeeAmount);
 
  847:                 gmxBaseReward.safeTransfer(address(pirexFees), feeAmount);

  946:         gmxBaseReward.safeTransfer(



src/vaults/AutoPxGlp.sol:
    6: import {SafeTransferLib} from "solmate/utils/SafeTransferLib.sol";


  258:                 asset.safeTransfer(msg.sender, pxGlpIncentive);
  259  
  260:             asset.safeTransfer(owner, totalPxGlpFee - pxGlpIncentive);

  274:                 pxGmx.safeTransfer(msg.sender, pxGmxIncentive);
  275  
  276:             pxGmx.safeTransfer(owner, totalPxGmxFee - pxGmxIncentive);

  344:         stakedGlp.safeTransferFrom(msg.sender, address(this), amount);

  387:         erc20Token.safeTransferFrom(msg.sender, address(this), tokenAmount);
  388  

src/vaults/AutoPxGmx.sol:
    7: import {SafeTransferLib} from "solmate/utils/SafeTransferLib.sol";
	
  297:             if (incentive != 0) asset.safeTransfer(msg.sender, incentive);
  298  
  299:             asset.safeTransfer(owner, totalFee - incentive);

  336:         asset.safeTransfer(receiver, assets);

  361:         asset.safeTransfer(receiver, assets);

  382:         gmx.safeTransferFrom(msg.sender, address(this), amount);

Recommended Mitigation Steps

Add a contract exist control in functions;

pragma solidity >=0.8.0;

function isContract(address _addr) private returns (bool isContract) {
    isContract = _addr.code.length > 0;
}

[L-04] Use Ownable2StepUpgradeable instead of OwnableUpgradeable contract

Context: PirexRewards.sol#L4

Description: transferOwnership function is used to change Ownership from OwnableUpgradeable.sol.

There is another Openzeppelin Ownable contract (Ownable2StepUpgradeable.sol) has transferOwnership function , use it is more secure due to 2-stage ownership transfer.

Ownable2StepUpgradeable.sol

[L-05] Owner can renounce Ownership

Context: PirexRewards.sol#L4

Description: Typically, the contract’s owner is the account that deploys the contract. As a result, the owner is able to perform certain privileged activities.

The Openzeppelin’s Ownable used in this project contract implements renounceOwnership. This can represent a certain risk if the ownership is renounced for any other reason than by design. Renouncing ownership will leave the contract without an owner, thereby removing any functionality that is only available to the owner.

onlyOwner functions;

src/PirexRewards.sol:
  93:     function setProducer(address _producer) external onlyOwner {
151:     function addRewardToken(ERC20 producerToken, ERC20 rewardToken)  external onlyOwner

Recommendation: We recommend to either reimplement the function to disable it or to clearly specify if it is part of the contract design.

[L-06] Critical Address Changes Should Use Two-step Procedure

The critical procedures should be two step process.

src/PirexFees.sol:
  63:     function setFeeRecipient(FeeRecipient f, address recipient)

src/PirexGmx.sol:
  313:     function setContract(Contracts c, address contractAddress)
  884:     function setVoteDelegate(address voteDelegate) external onlyOwner {

src/external/DelegateRegistry.sol:
  18:     function setDelegate(bytes32 id, address delegate) public {

Recommended Mitigation Steps Lack of two-step procedure for critical operations leaves them error-prone. Consider adding two step procedure on the critical functions.

[L-07] DepositGlp Event arguments names are confusing

The uint256 amount argument of the depositFsGlp function in the PirexGmx.sol contract is named deposited in the event DepositGlp parameter. In emit DepositGlp this causes confusion in terms of user, code readability and web pages.

src/PirexGmx.sol:
  421       */
  422:     function depositFsGlp(uint256 amount, address receiver)
  423:         external
  424:         whenNotPaused
  425:         nonReentrant
  426:         returns (
  427:             uint256,
  428:             uint256,
  429:             uint256
  430:         )

 452:         emit DepositGlp(
  453:             msg.sender,          	// caller
  454:             receiver,          		// receiver
  455:             address(stakedGlp),  	// token
  456:             0,                   		// tokenAmount
  457:             0,                  		 // minUsdg
  458:             0,                   		// minGlp
  459:             amount,          		 // deposited
  460:             postFeeAmount,      	 // postFeeAmount
  461:             feeAmount          	 // feeAmount
  462:         );

Recommended Mitigation Steps Event-Emit parameter names must match the function arguments or the argument they take value from.

[L-08] Loss of precision due to rounding

src/PirexGmx.sol:
  221      {
  222:         feeAmount = (assets * fees[f]) / FEE_DENOMINATOR;
  223          postFeeAmount = assets - feeAmount;;

[L-09] First value check of argument of type enum in setFee function is missing

src/PirexGmx.sol:
  298          @param  fee  uint256  Fee amount
  299:      */
  300:     function setFee(Fees f, uint256 fee) external onlyOwner {
  301:         if (fee > FEE_MAX) revert InvalidFee();
  302: 
  303:         fees[f] = fee;
  304: 
  305:         emit SetFee(f, fee);
  306:     }

Leaving the enum type argument Fees in the setFee function empty and returning the value 0 gives the same result as returning the value 0, this is a property of the enum type and therefore error-prone

Recommended Mitigation Step;

Use struck instead of enum

[L-10] Hardcode the address causes no future updates

src/vaults/AutoPxGmx.sol:
  18      IV3SwapRouter public constant SWAP_ROUTER =
  19:         IV3SwapRouter(0x68b3465833fb72A70ecDF485E0e4C7bD8665Fc45);

Router etc. In case the addresses change due to reasons such as updating their versions in the future, addresses coded as constants cannot be updated, so it is recommended to add the update option with the onlyOwner modifier.

[L-11] Lack of Input Validation

For defence-in-depth purpose, it is recommended to perform additional validation against the amount that the user is attempting to deposit, mint, withdraw and redeem to ensure that the submitted amount is valid.

OpenZeppelinTokenizedVault.sol#L9

src/PirexGmx.sol:
  429       */
  430:     function depositGmx(uint256 amount, address receiver)
  431:         external
  432:         whenNotPaused
  433:         nonReentrant
  434:         returns (
  435:             uint256,
  436:             uint256,
  437:             uint256
  438:         )
  439:     {
+	         require(amount <= maxDeposit(receiver), "deposit more than max");

src/vaults/PirexERC4626.sol:
  79  
  80:     function mint(uint256 shares, address receiver)
  81:         public
  82:         virtual
  83:         returns (uint256 assets)
  84:     {
+ 	       require(shares <= maxMint(receiver), "mint more than max");

src/vaults/AutoPxGlp.sol:
  438:     function withdraw(
  439:         uint256 assets,
  440:         address receiver,
  441:         address owner
  442:     ) public override returns (uint256 shares) {
+                require(assets <= maxWithdraw(owner), "withdraw more than max");

src/vaults/AutoPxGmx.sol:
  317:     function withdraw(
  318:         uint256 assets,
  319:         address receiver,
  320:         address owner
  321:     ) public override returns (uint256 shares) {
+               require(assets <= maxWithdraw(owner), "withdraw more than max");

src/vaults/AutoPxGlp.sol:
  450       */
  451:     function redeem(
  452:         uint256 shares,
  453:         address receiver,
  454:         address owner
  455:     ) public override returns (uint256 assets) {
+               require(shares <= maxRedeem(owner), "redeem more than max");

src/vaults/AutoPxGmx.sol:
  340  
  341:     function redeem(
  342:         uint256 shares,
  343:         address receiver,
  344:         address owner
  345:     ) public override returns (uint256 assets) {
+                require(shares <= maxRedeem(owner), "redeem more than max");

[NC-01] Insufficient coverage

Description: Testing all functions is best practice in terms of security criteria.

| File                              | % Lines           | % Statements      | % Branches       | % Funcs         |
|-----------------------------------|-------------------|-------------------|------------------|-----------------|
| src/PirexRewards.sol              | 98.98% (97/98)    | 99.24% (131/132)  | 94.44% (51/54)   | 94.12% (16/17)  |
| src/PxGmx.sol                     | 100.00% (0/0)     | 100.00% (0/0)     | 100.00% (0/0)    | 0.00% (0/1)     |
| src/vaults/AutoPxGlp.sol          | 91.11% (82/90)    | 93.39% (113/121)  | 87.04% (47/54)   | 94.44% (17/18)  |
| src/vaults/AutoPxGmx.sol          | 89.39% (59/66)    | 91.46% (75/82)    | 71.05% (27/38)   | 92.31% (12/13)  |
| src/vaults/PirexERC4626.sol       | 75.00% (39/52)    | 76.79% (43/56)    | 37.50% (6/16)    | 47.62% (10/21)  |
| src/vaults/PxGmxReward.sol        | 78.12% (25/32)    | 79.49% (31/39)    | 50.00% (5/10)    | 75.00% (3/4)    |
| Total                             | 56.72% (523/922)  | 58.93% (660/1120) | 59.95% (241/402) | 47.87% (90/188) |

Due to its capacity, test coverage is expected to be 100%

[NC-02] Not using the named return variables anywhere in the function is confusing

 function getUserState(ERC20 producerToken, address user)
        external
        view
        returns (
            uint256 lastUpdate,
            uint256 lastBalance,
            uint256 rewards
        )
    {
        UserState memory userState = producerTokens[producerToken].userStates[
            user
        ];

        return (userState.lastUpdate, userState.lastBalance, userState.rewards);
    }

Recommendation: Consider adopting a consistent approach to return values throughout the codebase by removing all named return variables, explicitly declaring them as local variables, and adding the necessary return statements where appropriate. This would improve both the explicitness and readability of the code, and it may also help reduce regressions during future code refactors.

[NC-03] Same Constant redefined elsewhere

Keeping the same constants in different files may cause some problems or errors, reading constants from a single file is preferable. This should also be preferred in gas optimization

src/vaults/AutoPxGlp.sol:
  17  
  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;

src/vaults/AutoPxGmx.sol:

  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;

[NC-04] Omissions in Events

Throughout the codebase, events are generally emitted when sensitive changes are made to the contracts. However, some events are missing important parameters

The events should include the new value and old value where possible:

Events with no old value;

8 results 

src/PirexFees.sol:
  63:     function setFeeRecipient(FeeRecipient f, address recipient)

  83:     function setTreasuryFeePercent(uint8 _treasuryFeePercent)

src/PirexGmx.sol:
  300:     function setFee(Fees f, uint256 fee) external onlyOwner {
  301          if (fee > FEE_MAX) revert InvalidFee();

  313:     function setContract(Contracts c, address contractAddress)

  862:     function setDelegationSpace(
  863          string memory _delegationSpace,

  884:     function setVoteDelegate(address voteDelegate) external onlyOwner {
  885          if (voteDelegate == address(0)) revert ZeroAddress();

  909:     function setPauseState(bool state) external onlyOwner {
  910          if (state) {

src/PirexRewards.sol:
   93:     function setProducer(address _producer) external onlyOwner {
   94          if (_producer == address(0)) revert ZeroAddress();

[NC-05] Add parameter to Event-Emit

Some event-emit description hasn’t parameter. Add to parameter for front-end website or client app , they can has that something has happened on the blockchain.

Events with no old value;

src/PirexGmx.sol:
  894       */
  895:     function clearVoteDelegate() public onlyOwner {
  896:         emit ClearVoteDelegate();

[NC-06] NatSpec is missing

Description: NatSpec is missing for the following functions , constructor and modifier:

Context:

src/vaults/PxGmxReward.sol:
  15:     ERC20 public pxGmx;
  17:     GlobalState public globalState;
  18:     uint256 public rewardState;
  19:     mapping(address => UserState) public userRewardStates;

[NC-07] Use require instead of assert

src/PirexGmx.sol:
  224  
  225:         assert(feeAmount + postFeeAmount == assets);
  226      }

Description: Assert should not be used except for tests, require should be used

Prior to Solidity 0.8.0, pressing a confirm consumes the remainder of the process's available gas instead of returning it, as request()/revert() did.

assert() and ruqire(); The big difference between the two is that the assert()function when false, uses up all the remaining gas and reverts all the changes made. Meanwhile, a require() function when false, also reverts back all the changes made to the contract but does refund all the remaining gas fees we offered to pay.This is the most common Solidity function used by developers for debugging and error handling.

Assertion() should be avoided even after solidity version 0.8.0, because its documentation states "The Assert function generates an error of type Panic(uint256). Code that works properly should never Panic, even on invalid external input. If this happens, you need to fix it in your contract. there's a mistake".

[NC-08] Implement some type of version counter that will be incremented automatically for contract upgrades

PirexRewards.sol#L85

I suggest implementing some kind of version counter that will be incremented automatically when you upgrade the contract.

Recommendation:

uint256 public authorizeUpgradeCounter;

 function upgradeTo(address _newImplementation) external onlyOwner {
        _setPendingImplementation(_newImplementation);
       authorizeUpgradeCounter+=1;

    }

[NC-09] Constant values such as a call to keccak256(), should used to immutable rather than constant

There is a difference between constant variables and immutable variables, and they should each be used in their appropriate contexts.

While it doesn't save any gas because the compiler knows that developers often make this mistake, it's still best to use the right tool for the task at hand.

Constants should be used for literal values written into the code, and immutable variables should be used for expressions, or values calculated in, or passed into the constructor.

2 results 

src/PxERC20.sol:
    9:     bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE");
  10:     bytes32 public constant BURNER_ROLE = keccak256("BURNER_ROLE");

[N-10 ] For functions, follow Solidity standard naming conventions

Context: AutoPxGlp.sol#L487 AutoPxGlp.sol#L501 AutoPxGlp.sol#L474 AutoPxGlp.sol#L462

Description: The above codes don't follow Solidity's standard naming convention,

internal and private functions : the mixedCase format starting with an underscore (_mixedCase starting with an underscore)

[NC-11] Mark visibility of initialize(...) functions as external

L1GraphTokenGateway.sol#L99

Description: If someone wants to extend via inheritance, it might make more sense that the overridden initialize(...) function calls the internal {...}_init function, not the parent public initialize(...) function.

External instead of public would give more the sense of the initialize(...) functions to behave like a constructor (only called on deployment, so should only be called externally)

Security point of view, it might be safer so that it cannot be called internally by accident in the child contract

It might cost a bit less gas to use external over public

It is possible to override a function from external to public (= "opening it up") ✅ but it is not possible to override a function from public to external (= "narrow it down"). ❌

For above reasons you can change initialize(...) to external

https://github.com/OpenZeppelin/openzeppelin-contracts/issues/3750

[NC-12] No same value input control

src/PirexFees.sol:
  63:     function setFeeRecipient(FeeRecipient f, address recipient)

  83:     function setTreasuryFeePercent(uint8 _treasuryFeePercent)

src/PirexGmx.sol:

  313:     function setContract(Contracts c, address contractAddress)

  884:     function setVoteDelegate(address voteDelegate) external onlyOwner {

  909:     function setPauseState(bool state) external onlyOwner {

src/PirexRewards.sol:
   93:     function setProducer(address _producer) external onlyOwner {

  107:     function setRewardRecipient(

  432:     function setRewardRecipientPrivileged(

Recommendation: Add code like this; if (oracle == _oracle revert ADDRESS_SAME();

[NC-13] Include return parameters in NatSpec comments

Context: All contracts

Description: It is recommended that Solidity contracts are fully annotated using NatSpec for all public interfaces (everything in the ABI). It is clearly stated in the Solidity official documentation. In complex projects such as Defi, the interpretation of all functions and their arguments and returns is important for code readability and auditability.

https://docs.soliditylang.org/en/v0.8.15/natspec-format.html

If Return parameters are declared, you must prefix them with "/// @return".

Some code analysis programs do analysis by reading NatSpec details, if they can't see the "@return" tag, they do incomplete analysis.

Recommendation: Include return parameters in NatSpec comments

Recommendation Code Style:

   /// @notice information about what a function does
   /// @param pageId The id of the page to get the URI for.
   /// @return Returns a page's URI if it has been minted 
   function tokenURI(uint256 pageId) public view virtual override returns (string memory) {
       if (pageId == 0 || pageId > currentId) revert("NOT_MINTED");

       return string.concat(BASE_URI, pageId.toString());
   }

[NC-14] 0 address check for asset

Context:

src/vaults/PirexERC4626.sol:
  47  
  48:     constructor(
  49:         ERC20 _asset,
  50:         string memory _name,
  51:         string memory _symbol
  52:     ) ERC20(_name, _symbol, _asset.decimals()) {
  53:         asset = _asset;
  54:     }

Description: Also check of the address to protect the code from 0x0 address problem just in case. This is best practice or instead of suggesting that they verify address != 0x0, you could add some good NatSpec comments explaining what is valid and what is invalid and what are the implications of accidentally using an invalid address.

Recommendation: like this; if (_asset == address(0)) revert ADDRESS_ZERO();

[NC-15] Use a single file for all system-wide constants

There are many addresses and constants used in the system. It is recommended to put the most used ones in one file (for example constants.sol, use inheritance to access these values)

This will help with readability and easier maintenance for future changes. This also helps with any issues, as some of these hard-coded values are admin addresses.

constants.left Use and import this file in contracts that require access to these values. This is just a suggestion, in some use cases this may result in higher gas usage in the distribution.

21 results - 6 files

src/PirexFees.sol:
  20:     uint8 public constant FEE_PERCENT_DENOMINATOR = 100;
  23:     uint8 public constant MAX_TREASURY_FEE_PERCENT = 75;

src/PirexGmx.sol:
  44:     uint256 public constant FEE_DENOMINATOR = 1_000_000;
  47:     uint256 public constant FEE_MAX = 200_000;

src/PxERC20.sol:
   9:     bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE");
  10:     bytes32 public constant BURNER_ROLE = keccak256("BURNER_ROLE");

src/external/RewardTracker.sol:
  772:     uint256 public constant BASIS_POINTS_DIVISOR = 10000;
  773:     uint256 public constant PRECISION = 1e30;
  775:     uint8 public constant decimals = 18;

src/vaults/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;
  23  

src/vaults/AutoPxGmx.sol:
  18:     IV3SwapRouter public constant SWAP_ROUTER =
  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;

[NC-16] Function writing that does not comply with the Solidity Style Guide

Context: All Contracts

Description: Order of Functions; ordering helps readers identify which functions they can call and to find the constructor and fallback definitions easier. But there are contracts in the project that do not comply with this.

https://docs.soliditylang.org/en/v0.8.17/style-guide.html

Functions should be grouped according to their visibility and ordered:

constructor receive function (if exists) fallback function (if exists) external public internal private within a grouping, place the view and pure functions last

[NC-17] Missing Upgrade Path for PirexRewards Implementation

src/PirexRewards.sol:
   4: import {OwnableUpgradeable} from "openzeppelin-contracts-upgradeable/contracts/access/OwnableUpgradeable.sol";
  85:     function initialize() public initializer {

With the current and contracts, it is not possible to upgrade the contract.

Recommendation: Document the operational plan for contract upgrades. Also, consider using the UUPS proxy pattern to upgrade contract implementation.

[NC-18] No need assert check in _computeAssetAmounts()

The assert check in this function is not needed. Because this check will always be true because of this line: postFeeAmount = assets - feeAmount;

Context:

src/PirexGmx.sol:
  216       */
  217:     function _computeAssetAmounts(Fees f, uint256 assets)
  218:         internal
  219:         view
  220:         returns (uint256 postFeeAmount, uint256 feeAmount)
  221:     {
  222: 
  223: 
  224:         feeAmount = (assets * fees[f]) / FEE_DENOMINATOR;
  225:         postFeeAmount = assets - feeAmount;
  226: 
- 227:       assert(feeAmount + postFeeAmount == assets);
  228:     }

[NC-19] Lack of event emission after critical initialize() functions

To record the init parameters for off-chain monitoring and transparency reasons, please consider emitting an event after the initialize() functions:

src/PirexRewards.sol:
  84  
  85:     function initialize() public initializer {
  86:         __Ownable_init();
  87:     }

[NC-20] Add a timelock to critical functions

It is a good practice to give time for users to react and adjust to critical changes. A timelock provides more guarantees and reduces the level of trust required, thus decreasing risk for users. It also indicates that the project is legitimate (less risk of a malicious owner making a sandwich attack on a user). Consider adding a timelock to:

11 results 


src/PirexGmx.sol:
  351       */
  352:     function setFee(Fees f, uint256 fee) external onlyOwner {
  353          if (fee > FEE_MAX) revert InvalidFee();

  364       */
  365:     function setContract(Contracts c, address contractAddress)
  366          external

  918       */
  919:     function setDelegationSpace(
  920          string memory _delegationSpace,

  940       */
  941:     function setVoteDelegate(address voteDelegate) external onlyOwner {
  942          if (voteDelegate == address(0)) revert ZeroAddress();

  965      */
  966:     function setPauseState(bool state) external onlyOwner {
  967          if (state) {


src/vaults/AutoPxGlp.sol:
   94:     function setWithdrawalPenalty(uint256 penalty) external onlyOwner {
   95          if (penalty > MAX_WITHDRAWAL_PENALTY) revert ExceedsMax();

  106:     function setPlatformFee(uint256 fee) external onlyOwner {
  107          if (fee > MAX_PLATFORM_FEE) revert ExceedsMax();


src/vaults/AutoPxGmx.sol:
  104:     function setPoolFee(uint24 _poolFee) external onlyOwner {
  105          if (_poolFee == 0) revert ZeroAmount();

  116:     function setWithdrawalPenalty(uint256 penalty) external onlyOwner {
  117          if (penalty > MAX_WITHDRAWAL_PENALTY) revert ExceedsMax();

  128:     function setPlatformFee(uint256 fee) external onlyOwner {
  129          if (fee > MAX_PLATFORM_FEE) revert ExceedsMax();

  152:     function setPlatform(address _platform) external onlyOwner {
  153          if (_platform == address(0)) revert ZeroAddress();

[S-01] Generate perfect code headers every time

Description: I recommend using header for Solidity code layout and readability

https://github.com/transmissions11/headers

/*//////////////////////////////////////////////////////////////
                           TESTING 123
//////////////////////////////////////////////////////////////*/

[S-02] Add NatSpec comments to the variables defined in Storage

Description: I recommend adding NatSpec comments explaining the variables defined in Storage, their slots, their contents and definitions.

This improves code readability and control quality

Current Code;

src/vaults/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;
  24:     uint256 public withdrawalPenalty = 300;
  25:     uint256 public platformFee = 1000;
  26:     uint256 public compoundIncentive = 1000;
  27:     address public platform;
  30:     address public immutable rewardsModule;

Recommendation Code;

 //****** Slot 0 ******//
 26:     uint256 public constant MAX_WITHDRAWAL_PENALTY = 500;

 / /****** Slot 1 ******//
 30:     uint256 public constant MAX_PLATFORM_FEE = 2000;

  //****** Slot 2 ******//
 33:     uint256 public constant FEE_DENOMINATOR = 10000;
...

  /****** End of storage ******/

Summary

Gas Optimizations List

Total 10 issues

Suggestions

Total 1 suggestion

[G-01] Remove the initializer modifier [20 K Gas Saved]

if we can just ensure that the initialize() function could only be called from within the constructor, we shouldn't need to worry about it getting called again.

src/PirexRewards.sol:
  84  
  85:     function initialize() public initializer {
  86:         __Ownable_init();
  87:     }

In the EVM, the constructor's job is actually to return the bytecode that will live at the contract's address. So, while inside a constructor, your address (address(this)) will be the deployment address, but there will be no bytecode at that address! So if we check address(this).code.length before the constructor has finished, even from within a delegatecall, we will get 0. So now let's update our initialize() function to only run if we are inside a constructor:

src/PirexRewards.sol:
  84  
  85:     function initialize() public initializer {
+          require(address(this).code.length == 0, 'not in constructor');
  86:         __Ownable_init();
  87:     }

Now the Proxy contract's constructor can still delegatecall initialize(), but if anyone attempts to call it again (after deployment) through the Proxy instance, or tries to call it directly on the above instance, it will revert because address(this).code.length will be nonzero.

Also, because we no longer need to write to any state to track whether initialize() has been called, we can avoid the 20k storage gas cost. In fact, the cost for checking our own code size is only 2 gas, which means we have a 10,000x gas savings over the standard version. Pretty neat!

[G-02] Packing Storage

Context: AutoPxGlp.sol#L24-L30

Recommendation: Storing state variables packaging is more gas efficient.

Current Code:

    uint256 public withdrawalPenalty = 300;
    uint256 public platformFee = 1000;
    uint256 public compoundIncentive = 1000;
    address public platform;

Recommendation Code:

    address public platform;
    uint64 public withdrawalPenalty = 300;
    uint64 public platformFee = 1000;
    uint64 public compoundIncentive = 1000;

[G-03] No need assert check in _computeAssetAmounts() [19 K Gas Saved ]

The assert check in this function is not needed. Because this check will always be true because of this line: postFeeAmount = assets - feeAmount; Deployment Gas Saved : 19 K Transaction Gas Saved : 90

Context:

src/PirexGmx.sol:
  216       */
  217:     function _computeAssetAmounts(Fees f, uint256 assets)
  218:         internal
  219:         view
  220:         returns (uint256 postFeeAmount, uint256 feeAmount)
  221:     {
  222: 
  223: 
  224:         feeAmount = (assets * fees[f]) / FEE_DENOMINATOR;
  225:         postFeeAmount = assets - feeAmount;
  226: 
  227:         assert(feeAmount + postFeeAmount == assets);
  228:     }

Proof Of Concept: The optimizer was turned on and set to 10000 runs.

contract GasTest is DSTest {
    Contract0 c0;
    Contract1 c1;

    function setUp() public {
        c0 = new Contract0();
        c1 = new Contract1();

    }
        function testGas() public {
            c0.depositGmx(5,200_000);
            c1.depositGmx(5,200_000);            
        }
}

contract Contract0 {

    uint256 constant FEE_DENOMINATOR = 1_000_000;

    function depositGmx(uint256 amount, uint256 f)  external {
        (uint256 postFeeAmount, uint256 feeAmount) = _computeAssetAmounts(f,amount);
    }

     function _computeAssetAmounts(uint f, uint256 assets) internal view returns (uint256 postFeeAmount, uint256 feeAmount) {
        feeAmount = (assets * f ) / FEE_DENOMINATOR;
        postFeeAmount = assets - feeAmount;

        assert(feeAmount + postFeeAmount == assets);
    }
}

contract Contract1 {

    uint256 constant FEE_DENOMINATOR = 1_000_000;

    function depositGmx(uint256 amount, uint256 f)  external {
        (uint256 postFeeAmount, uint256 feeAmount) = _computeAssetAmounts(f,amount);
    }

     function _computeAssetAmounts(uint f, uint256 assets) internal view returns (uint256 postFeeAmount, uint256 feeAmount) {
        feeAmount = (assets * f ) / FEE_DENOMINATOR;
        postFeeAmount = assets - feeAmount;

    }
} 

Gas Report:

| src/test/test.sol:Contract0 contract |                 |     |        |     |         |
|--------------------------------------|-----------------|-----|--------|-----|---------|
| Deployment Cost                      | Deployment Size |     |        |     |         |
| 102347                               | 543             |     |        |     |         |
| Function Name                        | min             | avg | median | max | # calls |
| depositGmx                           | 608             | 608 | 608    | 608 | 1       |


| src/test/test.sol:Contract1 contract |                 |     |        |     |         |
|--------------------------------------|-----------------|-----|--------|-----|---------|
| Deployment Cost                      | Deployment Size |     |        |     |         |
| 83329                                | 448             |     |        |     |         |
| Function Name                        | min             | avg | median | max | # calls |
| depositGmx                           | 518             | 518 | 518    | 518 | 1       |

[G-04] Use assembly to write address storage values

Context:

src/PirexFees.sol:
   50:     constructor(address _treasury, address _contributors) Owned(msg.sender) {
  
  54:         treasury = _treasury;
  55:         contributors = _contributors;

src/PirexGmx.sol:
  167:     constructor(
  168:         address _pxGmx,
  169:         address _pxGlp,
  170:         address _pirexFees,
  171:         address _pirexRewards,
  172:         address _delegateRegistry,
  173:         address _gmxBaseReward,
  174:         address _gmx,
  175:         address _esGmx,
  176:         address _gmxRewardRouterV2,
  177:         address _stakedGlp

  193:         pxGmx = PxERC20(_pxGmx);
  194:         pxGlp = PxERC20(_pxGlp);
  195:         pirexFees = PirexFees(_pirexFees);
  196:         pirexRewards = _pirexRewards;
  197:         delegateRegistry = DelegateRegistry(_delegateRegistry);
  198:         gmxBaseReward = ERC20(_gmxBaseReward);
  199:         gmx = ERC20(_gmx);
  200:         esGmx = ERC20(_esGmx);
  201:         gmxRewardRouterV2 = IRewardRouterV2(_gmxRewardRouterV2);
  202:         stakedGlp = IStakedGlp(_stakedGlp);

src/PxERC20.sol:
  24:     constructor(
  25:         address _pirexRewards,
  
  35:         pirexRewards = PirexRewards(_pirexRewards);

src/vaults/AutoPxGlp.sol:
  66:     constructor(
  67:         address _gmxBaseReward,
  68:         address _asset,
  69:         address _pxGmx,
  70:         string memory _name,
  71:         string memory _symbol,
  72:         address _platform,
  73:         address _rewardsModule
  74:     ) PxGmxReward(_pxGmx) PirexERC4626(ERC20(_asset), _name, _symbol) {

  82:         gmxBaseReward = ERC20(_gmxBaseReward);
  83:         platform = _platform;
  84:         rewardsModule = _rewardsModule;

src/vaults/AutoPxGmx.sol:
  73:     constructor(
  74:         address _gmxBaseReward,
  75:         address _gmx,
  76:         address _asset,
  77:         string memory _name,
  78:         string memory _symbol,
  79:         address _platform,
  80:         address _rewardsModule
  81:     ) Owned(msg.sender) PirexERC4626(ERC20(_asset), _name, _symbol) {

  90:         gmxBaseReward = ERC20(_gmxBaseReward);
  91:         gmx = ERC20(_gmx);
  92:         platform = _platform;
  93:         rewardsModule = _rewardsModule;
  
src/vaults/PirexERC4626.sol:
  48:     constructor(
  49:         ERC20 _asset,

  53:         asset = _asset;

src/vaults/PxGmxReward.sol:
  40:     constructor(address _pxGmx) Owned(msg.sender) {
  43:         pxGmx = ERC20(_pxGmx);

Proof Of Concept: The optimizer was turned on and set to 10000 runs.

contract GasTestFoundry is DSTest {
    Contract1 c1;
    Contract2 c2;
    
    function setUp() public {
        c1 = new Contract1();
        c2 = new Contract2();
    }
    
    function testGas() public {
        c1.setRewardTokenAndAmount(0x5B38Da6a701c568545dCfcB03FcB875f56beddC4,356);
        c2.setRewardTokenAndAmount(0x5B38Da6a701c568545dCfcB03FcB875f56beddC4,356);
    }
}

contract Contract1  {
    address rewardToken ;
    uint256 reward;

    function setRewardTokenAndAmount(address token_, uint256 reward_) external {
        rewardToken = token_;
        reward = reward_;
    }
}

contract Contract2  {
    address rewardToken ;
    uint256 reward;

    function setRewardTokenAndAmount(address token_, uint256 reward_) external {
        assembly {
            sstore(rewardToken.slot, token_)
            sstore(reward.slot, reward_)           

        }
    }
}

Gas Report:

╭───────────────────────────────────────────┬─────────────────┬───────┬────────┬───────┬─────────╮
│ src/test/GasTest.t.sol:Contract1 contract ┆                 ┆       ┆        ┆       ┆         │
╞═══════════════════════════════════════════╪═════════════════╪═══════╪════════╪═══════╪═════════╡
│ Deployment Cost                           ┆ Deployment Size ┆       ┆        ┆       ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ 50899                                     ┆ 285             ┆       ┆        ┆       ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ Function Name                             ┆ min             ┆ avg   ┆ median ┆ max   ┆ # calls │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ setRewardTokenAndAmount                   ┆ 44490           ┆ 44490 ┆ 44490  ┆ 44490 ┆ 1       │
╰───────────────────────────────────────────┴─────────────────┴───────┴────────┴───────┴─────────╯
╭───────────────────────────────────────────┬─────────────────┬───────┬────────┬───────┬─────────╮
│ src/test/GasTest.t.sol:Contract2 contract ┆                 ┆       ┆        ┆       ┆         │
╞═══════════════════════════════════════════╪═════════════════╪═══════╪════════╪═══════╪═════════╡
│ Deployment Cost                           ┆ Deployment Size ┆       ┆        ┆       ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ 38087                                     ┆ 221             ┆       ┆        ┆       ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ Function Name                             ┆ min             ┆ avg   ┆ median ┆ max   ┆ # calls │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ setRewardTokenAndAmount                   ┆ 44457           ┆ 44457 ┆ 44457  ┆ 44457 ┆ 1       │
╰───────────────────────────────────────────┴─────────────────┴───────┴────────┴───────┴─────────╯

[G-05] The solady Library's Ownable contract is significantly gas-optimized, which can be used

Description: The project uses the onlyOwner authorization model I recommend using Solady's highly gas optimized contract.

https://github.com/Vectorized/solady/blob/main/src/auth/OwnableRoles.sol

[G-06] Setting The Constructor To Payable [13 gas per instance]

Description: You can cut out 10 opcodes in the creation-time EVM bytecode if you declare a constructor payable. Making the constructor payable eliminates the need for an initial check of msg.value == 0 and saves 13 gas on deployment with no security risks.

Context:

8 results

src/PirexFees.sol:
  50:     constructor(address _treasury, address _contributors) Owned(msg.sender) {

src/PirexGmx.sol:
  167:     constructor(

src/PxERC20.sol:
  24:     constructor(

src/PxGmx.sol:
  10:     constructor(address _pirexRewards)


src/vaults/AutoPxGlp.sol:
  66:     constructor(

src/vaults/AutoPxGmx.sol:
  73:     constructor(

src/vaults/PirexERC4626.sol:
  48:     constructor(

src/vaults/PxGmxReward.sol:
  40:     constructor(address _pxGmx) Owned(msg.sender) {

Recommendation: Set the constructor to payable

Proof Of Concept: https://forum.openzeppelin.com/t/a-collection-of-gas-optimisation-tricks/19966/5?u=pcaversaccio

The optimizer was turned on and set to 10000 runs

contract GasTestFoundry is DSTest {
    
    Contract1 c1;
    Contract2 c2;
    
    function setUp() public {
        c1 = new Contract1();
        c2 = new Contract2();
    }
    
    function testGas() public {
        c1.x();
        c2.x();
    }
}

contract Contract1 {
    
    uint256 public dummy;
    constructor() payable {
        dummy = 1;
    }
    
    function x() public {

    }
}

contract Contract2 {
    
    uint256 public dummy;
    constructor() {
        dummy = 1;
    }
    
    function x() public {
    }
}

Gas Report:

╭───────────────────────────────────────────┬─────────────────┬─────┬────────┬─────┬─────────╮
│ src/test/GasTest.t.sol:Contract1 contract ┆                 ┆     ┆        ┆     ┆         │
╞═══════════════════════════════════════════╪═════════════════╪═════╪════════╪═════╪═════════╡
│ Deployment Cost                           ┆ Deployment Size ┆     ┆        ┆     ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ 49563                                     ┆ 159             ┆     ┆        ┆     ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤

╭───────────────────────────────────────────┬─────────────────┬─────┬────────┬─────┬─────────╮
│ src/test/GasTest.t.sol:Contract2 contract ┆                 ┆     ┆        ┆     ┆         │
╞═══════════════════════════════════════════╪═════════════════╪═════╪════════╪═════╪═════════╡
│ Deployment Cost                           ┆ Deployment Size ┆     ┆        ┆     ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ 49587                                     ┆ 172             ┆     ┆        ┆     ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤

[G-07] Functions guaranteed to revert_ when callled by normal users can be marked payable

Context: 

18 results - 5 files

src/PirexFees.sol:

  63:     function setFeeRecipient(FeeRecipient f, address recipient)
  64          external
  65          onlyOwner

  
  83:     function setTreasuryFeePercent(uint8 _treasuryFeePercent)
  84          external
  85          onlyOwner

src/PirexGmx.sol:

  300:     function setFee(Fees f, uint256 fee) external onlyOwner {
  301          if (fee > FEE_MAX) revert InvalidFee();

 
  313:     function setContract(Contracts c, address contractAddress)
  314          external
  315          onlyOwner

 
  862:     function setDelegationSpace(
  863          string memory _delegationSpace,
  864      bool shouldClear
  865      ) external onlyOwner {


  884:     function setVoteDelegate(address voteDelegate) external onlyOwner {

  909:     function setPauseState(bool state) external onlyOwner {
  

src/PirexRewards.sol:
   
   93:     function setProducer(address _producer) external onlyOwner {
 

  432:     function setRewardRecipientPrivileged(
  433          address lpContract,
  434        ERC20 producerToken,
  435      ERC20 rewardToken,
  436      address recipient
  437      ) external onlyOwner {


src/vaults/AutoPxGlp.sol:
   
   94:     function setWithdrawalPenalty(uint256 penalty) external onlyOwner {
   
  106:     function setPlatformFee(uint256 fee) external onlyOwner {

  118:     function setCompoundIncentive(uint256 incentive) external onlyOwner {

  130:     function setPlatform(address _platform) external onlyOwner {


src/vaults/AutoPxGmx.sol:
 
  104:     function setPoolFee(uint24 _poolFee) external onlyOwner {

  116:     function setWithdrawalPenalty(uint256 penalty) external onlyOwner {
  
  128:     function setPlatformFee(uint256 fee) external onlyOwner {

  140:     function setCompoundIncentive(uint256 incentive) external onlyOwner {

  152:     function setPlatform(address _platform) external onlyOwner {

Description: If a function modifier or require such as onlyOwner-admin is used, the function will revert if a normal user tries to pay the function. Marking the function as payable will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided. The extra opcodes avoided are CALLVALUE(2), DUP1(3), ISZERO(3), PUSH2(3), JUMPI(10), PUSH1(3), DUP1(3), REVERT(0), JUMPDEST(1), POP(2) which costs an average of about 21 gas per call to the function, in addition to the extra deployment cost.

Recommendation: Functions guaranteed to revert when called by normal users can be marked payable  (for only onlyowner or admin functions)

Proof Of Concept: The optimizer was turned on and set to 10000 runs.

contract GasTest is DSTest {
    Contract0 c0;
    Contract1 c1;
    
    function setUp() public {
        c0 = new Contract0();
        c1 = new Contract1();
    }
    
    function testGas() public {
        c0.foo();
        c1.foo();
    }
}

contract Contract0 {
    uint256inverse;
    
    function foo() external {
       inverse++;
    }
}

contract Contract1 {
    uint256inverse;
    
    function foo() external payable {
       inverse++;
    }
}

Gas Report:

╭───────────────────────────────────────────┬─────────────────┬───────┬────────┬───────┬─────────╮
│ src/test/GasTest.t.sol:Contract0 contract ┆                 ┆       ┆        ┆       ┆         │
╞═══════════════════════════════════════════╪═════════════════╪═══════╪════════╪═══════╪═════════╡
│ Deployment Cost                           ┆ Deployment Size ┆       ┆        ┆       ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ 44293                                     ┆ 252             ┆       ┆        ┆       ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ Function Name                             ┆ min             ┆ avg   ┆ median ┆ max   ┆ # calls │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ foo                                       ┆ 22308           ┆ 22308 ┆ 22308  ┆ 22308 ┆ 1       │
╰───────────────────────────────────────────┴─────────────────┴───────┴────────┴───────┴─────────╯
╭───────────────────────────────────────────┬─────────────────┬───────┬────────┬───────┬─────────╮
│ src/test/GasTest.t.sol:Contract1 contract ┆                 ┆       ┆        ┆       ┆         │
╞═══════════════════════════════════════════╪═════════════════╪═══════╪════════╪═══════╪═════════╡
│ Deployment Cost                           ┆ Deployment Size ┆       ┆        ┆       ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ 41893                                     ┆ 240             ┆       ┆        ┆       ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ Function Name                             ┆ min             ┆ avg   ┆ median ┆ max   ┆ # calls │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ foo                                       ┆ 22284           ┆ 22284 ┆ 22284  ┆ 22284 ┆ 1       │
╰───────────────────────────────────────────┴─────────────────┴───────┴────────┴───────┴─────────╯

[G-08] Project file uses Transparent Proxy model but UUPS model is cheaper

UUPS proxies are a lot simpler than Transparent, and they only have the logic for delegating the call. This makes them much cheaper to deploy, and each all has half the overhead (just a single SLOAD).

https://twitter.com/smpalladino/status/1389939166109212675?s=20&t=Nd7ssD9sC_BNTtQF8lnxNg

[G-09] Optimize names to save gas [22 gas per instance]

Context:  All Contracts

Description:  Contracts most called functions could simply save gas by function ordering via Method ID. Calling a function at runtime will be cheaper if the function is positioned earlier in the order (has a relatively lower Method ID) because 22 gas are added to the cost of a function for every position that came before it. The caller can save on gas if you prioritize most called functions. 

Recommendation:  Find a lower method ID name for the most called functions for example Call() vs. Call1() is cheaper by 22 gas For example, the function IDs in the PirexGmx.sol contract will be the most used; A lower method ID may be given.

Proof of Consept: https://medium.com/joyso/solidity-how-does-function-name-affect-gas-consumption-in-smart-contract-47d270d8ac92

PirexGmx.sol function names can be named and sorted according to METHOD ID

Sighash   |   Function Signature
========================
57508589  =>  _calculateRewards(bool,bool)
59524400  =>  _computeAssetAmounts(Fees,uint256)
74874323  =>  setVoteDelegate(address)
492b669d  =>  configureGmxState()
20587afb  =>  setFee(Fees,uint256)
dae84c59  =>  setContract(Contracts,address)
437a8d0a  =>  depositGmx(uint256,address)
f83e36e6  =>  depositFsGlp(uint256,address)
d21fe1ee  =>  _depositGlp(address,uint256,uint256,uint256,address)
f64d0094  =>  depositGlpETH(uint256,uint256,address)
c2ae96ef  =>  depositGlp(address,uint256,uint256,uint256,address)
a7fe1a7f  =>  _redeemPxGlp(address,uint256,uint256,address)
6151f1b7  =>  redeemPxGlpETH(uint256,uint256,address)
414cc4ce  =>  redeemPxGlp(address,uint256,uint256,address)
372500ab  =>  claimRewards()
3e9f3619  =>  claimUserReward(address,uint256,address)
1d3c2b2f  =>  setDelegationSpace(string,bool)
edf187f0  =>  clearVoteDelegate()
cdb88ad1  =>  setPauseState(bool)
71726c92  =>  initiateMigration(address)
24fcd907  =>  migrateReward()
6ac844be  =>  completeMigration(address)

[G-10] Remove tokenAmount and msg.value check in the _depositGlp function

Checking tokenAmount and msg.value in _depositGlp function if (tokenAmount == 0) revert ZeroAmount();. done with, but it is not necessary Because these are done in the interacted RewardRouterV2.sol

By removing this Require, approximately 3k gas is saved

RewardRouterV2.sol - mintAndStakeGlp RewardRouterV2.sol - mintAndStakeETH

src/PirexGmx.sol:
  534       */
  535:     function _depositGlp(
  536:         address token,
  537:         uint256 tokenAmount,
  538:         uint256 minUsdg,
  539:         uint256 minGlp,
  540:         address receiver
  541:     )
  542:         internal
  543:         returns (
  544:             uint256 deposited,
  545:             uint256 postFeeAmount,
  546:             uint256 feeAmount
  547:         )
  548:     {
  549:         if (tokenAmount == 0) revert ZeroAmount();

[S-01] Use v4.8.0 OpenZeppelin contracts

Description: The upcoming v4.8.0 version of OpenZeppelin provides many small gas optimizations.

https://github.com/OpenZeppelin/openzeppelin-contracts/releases/tag/v4.8.0-rc.0

v4.8.0-rc.0
⛽ Many small optimizations