Back to Rolezn's contests

Redacted Cartel contest - Rolezn's results

Boosted GMX assets from your favorite liquid token wrapper, Pirex - brought to you by Redacted Cartel.

General Information

Platform: Code4rena

Start Date: 21/11/2022

Pot Size: $90,500 USDC

Total HM: 18

Participants: 101

Period: 7 days

Judge: Picodes

Total Solo HM: 4

Id: 183

League: ETH

Redacted Cartel

Findings Distribution

Researcher Performance

Rank: 44/101

Findings: 2

Award: $93.14

QA:
grade-b
Gas:
grade-b

🌟 Selected for report: 0

🚀 Solo Findings: 0

External Links

Code4rena Contest PageGithub Contest RepositoryGithub Findings RepositoryRedacted Cartel

Findings Information

🌟 Selected for report: 0xSmartContract

Also found by: 0xAgro, 0xNazgul, 0xPanda, 0xbepresent, 0xfuje, Awesome, B2, Bnke0x0, Deivitto, Diana, Funen, Jeiwan, JohnSmith, Josiah, R2, RaymondFam, Rolezn, Sathish9098, Waze, adriro, aphak5010, brgltd, btk, carrotsmuggler, ch0bu, chaduke, codeislight, codexploder, cryptostellar5, csanuragjain, danyams, datapunk, delfin454000, deliriusz, eierina, erictee, fatherOfBlocks, gz627, gzeon, hansfriese, hihen, jadezti, joestakey, keccak123, martin, nameruse, oyc_109, pedr02b2, perseverancesuccess, rbserver, rotcivegaf, rvierdiiev, sakshamguruji, shark, simon135, subtle77, unforgiven, xiaoming90, yixxas

Awards

53.4851 USDC - $53.49

Labels

bug
grade-b
QA (Quality Assurance)
edited-by-warden
Q-14

External Links

Link to GitHub issue

Summary<a name="Summary">

Low Risk Issues

IssueContexts
LOW‑1decimals() not part of ERC20 standard1
LOW‑2Contracts are not using their OZ Upgradeable counterparts2
LOW‑3Missing parameter validation in constructor1
LOW‑4The nonReentrant modifier should occur before all other modifiers6
LOW‑5Owner can renounce Ownership1
LOW‑6require() should be used instead of assert()1
LOW‑7whenPaused modifier should apply to additional functions1
LOW‑8Loss of precision due to rounding1

Total: 14 contexts over 8 issues

Non-critical Issues

IssueContexts
NC‑1Critical Changes Should Use Two-step Procedure19
NC‑2Use a more recent version of Solidity1
NC‑3Event Is Missing Indexed Fields5
NC‑4Implementation contract may not be initialized8
NC‑5Use Underscores for Number Literals2
NC‑6Use of Block.Timestamp5
NC‑7Expressions for constant values such as a call to keccak256(), should use immutable rather than constant2
NC‑8emit should happen after clearDelegate and setVoteDelegate2
NC‑9block.timestamp is already used when emitting events, no need to input timestamp1
NC‑10Event emit should emit a parameter1

Total: 46 contexts over 10 issues

Low Risk Issues

<a href="#Summary">[LOW‑1]</a><a name="LOW&#x2011;1"> decimals() not part of ERC20 standard

decimals() is not part of the official ERC20 standard and might fail for tokens that do not implement it. While in practice it is very unlikely, as usually most of the tokens implement it, this should still be considered as a potential issue.

<ins>Proof Of Concept</ins>
48: constructor(
49:      ERC20 _asset,
50:      string memory _name,
51:      string memory _symbol
52:  ) ERC20(_name, _symbol, _asset.decimals()) {

https://github.com/code-423n4/2022-11-redactedcartel/tree/main/src/vaults/PirexERC4626.sol#LL48-52

<a href="#Summary">[LOW‑2]</a><a name="LOW&#x2011;2"> Contracts are not using their OZ Upgradeable counterparts

The non-upgradeable standard version of OpenZeppelin’s library are inherited / used by the contracts. It would be safer to use the upgradeable versions of the library contracts to avoid unexpected behaviour.

<ins>Proof of Concept</ins>
6: import {Pausable} from "openzeppelin-contracts/contracts/security/Pausable.sol";

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

5: import {AccessControl} from "openzeppelin-contracts/contracts/access/AccessControl.sol";

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

<ins>Recommended Mitigation Steps</ins>

Where applicable, use the contracts from openzeppelin-contracts-upgradeable instead of openzeppelin-contracts.

<a href="#Summary">[LOW‑3]</a><a name="LOW&#x2011;3"> Missing parameter validation in constructor

Some parameters of constructors are not checked for invalid values.

<ins>Proof Of Concept</ins>
69: address _pxGmx

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

<ins>Recommended Mitigation Steps</ins>

Validate the parameters.

<a href="#Summary">[LOW‑4]</a><a name="LOW&#x2011;4"> The nonReentrant modifier should occur before all other modifiers

Currently the nonReentrant modifier is not the first to occur, it should occur before all other modifiers.

<ins>Proof Of Concept</ins>
378: function depositGmx(uint256 amount, address receiver)
        external
        whenNotPaused
        nonReentrant
        returns (
            uint256,
            uint256,
            uint256
        )
    {

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

422: function depositFsGlp(uint256 amount, address receiver)
        external
        whenNotPaused
        nonReentrant
        returns (
            uint256,
            uint256,
            uint256
        )
    {

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

554: function depositGlpETH(
        uint256 minUsdg,
        uint256 minGlp,
        address receiver
    )
        external
        payable
        whenNotPaused
        nonReentrant
        returns (
            uint256,
            uint256,
            uint256
        )
    {

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

583: function depositGlp(
        address token,
        uint256 tokenAmount,
        uint256 minUsdg,
        uint256 minGlp,
        address receiver
    )
        external
        whenNotPaused
        nonReentrant
        returns (
            uint256,
            uint256,
            uint256
        )
    {

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

685: function redeemPxGlpETH(
        uint256 amount,
        uint256 minOut,
        address receiver
    )
        external
        whenNotPaused
        nonReentrant
        returns (
            uint256,
            uint256,
            uint256
        )
    {

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

712: function redeemPxGlp(
        address token,
        uint256 amount,
        uint256 minOut,
        address receiver
    )
        external
        whenNotPaused
        nonReentrant
        returns (
            uint256,
            uint256,
            uint256
        )
    {
<ins>Recommended Mitigation Steps</ins>

Re-sort modifiers so that the nonReentrant modifier occurs first.

<a href="#Summary">[LOW‑5]</a><a name="LOW&#x2011;5"> Owner can renounce Ownership

Typically, the contract’s owner is the account that deploys the contract. As a result, the owner is able to perform certain privileged activities. PirexRewards.sol contract inherits the OwnableUpgradeable contract in this project which implements the renounceOwnership() call. 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.

<ins>Proof Of Concept</ins>
15: contract PirexRewards is OwnableUpgradeable {

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

<ins>Recommended Mitigation Steps</ins>

It is recommended to override the function an disable the option to renounce if its not needed.

<a href="#Summary">[LOW‑6]</a><a name="LOW&#x2011;6"> require() Should Be Used Instead Of assert()

Prior to solidity version 0.8.0, hitting an assert consumes the remainder of the transaction's available gas rather than returning it, as require()/revert() do. assert() should be avoided even past solidity version 0.8.0 as its <a href="https://docs.soliditylang.org/en/v0.8.14/control-structures.html#panic-via-assert-and-error-via-require">documentation</a> states that "The assert function creates an error of type Panic(uint256). ... Properly functioning code should never create a Panic, not even on invalid external input. If this happens, then there is a bug in your contract which you should fix".

<ins>Proof Of Concept</ins>
225: assert(feeAmount + postFeeAmount == assets);

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

<a href="#Summary">[LOW‑7]</a><a name="LOW&#x2011;7"> whenPaused modifier should apply to additional functions

In PirexGmx.sol contract the whenPaused modifier is applied to states changes such as the function configureGmxState. As a result, the whenPaused modifier should also apply to other similar functions.

Proof of Concept

whenPaused modifier should apply to following functions as well:

function setContract(Contracts c, address contractAddress)

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

<a href="#Summary">[LOW‑8]</a><a name="LOW&#x2011;8"> Loss of precision due to rounding

Due to division by precision, _calculateRewards can return incorrect results when below the divided precision value

<ins>Proof Of Concept</ins>
261: return
262:     r.claimableReward(address(this)) +
263:     ((r.stakedAmounts(address(this)) *
264:         (cumulativeRewardPerToken -
265:             r.previousCumulatedRewardPerToken(address(this)))) /
266:         precision);

https://github.com/code-423n4/2022-11-redactedcartel/tree/main/src/PirexGmx.sol#L61-L266

<ins>Recommended Mitigation Steps</ins>

Add a lower limit to calculation affected by the division of precision

Non Critical Issues

<a href="#Summary">[NC‑1]</a><a name="NC&#x2011;1"> Critical Changes Should Use Two-step Procedure

The critical procedures should be two step process.

See similar findings in previous Code4rena contests for reference: https://code4rena.com/reports/2022-06-illuminate/#2-critical-changes-should-use-two-step-procedure

<ins>Proof Of Concept</ins>
63: function setFeeRecipient(FeeRecipient f, address recipient)

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

83: function setTreasuryFeePercent(uint8 _treasuryFeePercent)

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

300: function setFee(Fees f, uint256 fee) external onlyOwner {

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

313: function setContract(Contracts c, address contractAddress)

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

862: function setDelegationSpace(

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

884: function setVoteDelegate(address voteDelegate) external onlyOwner {

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

909: function setPauseState(bool state) external onlyOwner {

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

93: function setProducer(address _producer) external onlyOwner {

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

107: function setRewardRecipient(

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

432: function setRewardRecipientPrivileged(

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

94: function setWithdrawalPenalty(uint256 penalty) external onlyOwner {

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

106: function setPlatformFee(uint256 fee) external onlyOwner {

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

118: function setCompoundIncentive(uint256 incentive) external onlyOwner {

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

130: function setPlatform(address _platform) external onlyOwner {

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

104: function setPoolFee(uint24 _poolFee) external onlyOwner {

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

116: function setWithdrawalPenalty(uint256 penalty) external onlyOwner {

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

128: function setPlatformFee(uint256 fee) external onlyOwner {

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

140: function setCompoundIncentive(uint256 incentive) external onlyOwner {

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

152: function setPlatform(address _platform) external onlyOwner {

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

<ins>Recommended Mitigation Steps</ins>

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

<a href="#Summary">[NC‑2]</a><a name="NC&#x2011;2"> Use a more recent version of Solidity

<a href="https://blog.soliditylang.org/2021/04/21/solidity-0.8.4-release-announcement/">0.8.4</a>: bytes.concat() instead of abi.encodePacked(<bytes>,<bytes>)

<a href="https://blog.soliditylang.org/2022/02/16/solidity-0.8.12-release-announcement/">0.8.12</a>: string.concat() instead of abi.encodePacked(<str>,<str>)

<a href="https://blog.soliditylang.org/2022/03/16/solidity-0.8.13-release-announcement/">0.8.13</a>: Ability to use using for with a list of free functions

<a href="https://blog.soliditylang.org/2022/05/18/solidity-0.8.14-release-announcement/">0.8.14</a>:

ABI Encoder: When ABI-encoding values from calldata that contain nested arrays, correctly validate the nested array length against calldatasize() in all cases. Override Checker: Allow changing data location for parameters only when overriding external functions.

<a href="https://blog.soliditylang.org/2022/06/15/solidity-0.8.15-release-announcement/">0.8.15</a>:

Code Generation: Avoid writing dirty bytes to storage when copying bytes arrays. Yul Optimizer: Keep all memory side-effects of inline assembly blocks.

<a href="https://blog.soliditylang.org/2022/08/08/solidity-0.8.16-release-announcement/">0.8.16</a>:

Code Generation: Fix data corruption that affected ABI-encoding of calldata values represented by tuples: structs at any nesting level; argument lists of external functions, events and errors; return value lists of external functions. The 32 leading bytes of the first dynamically-encoded value in the tuple would get zeroed when the last component contained a statically-encoded array.

<a href="https://blog.soliditylang.org/2022/09/08/solidity-0.8.17-release-announcement/">0.8.17</a>:

Yul Optimizer: Prevent the incorrect removal of storage writes before calls to Yul functions that conditionally terminate the external EVM call.

<ins>Proof Of Concept</ins>
pragma solidity >=0.8.0;

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

<ins>Recommended Mitigation Steps</ins>

Consider updating to a more recent solidity version.

<a href="#Summary">[NC‑3]</a><a name="NC&#x2011;3"> Event Is Missing Indexed Fields

Index event fields make the field more quickly accessible to off-chain tools that parse events. However, note that each index field costs extra gas during emission, so it's not necessarily best to index the maximum allowed per event (three fields).

Each event should use three indexed fields if there are three or more fields, and gas usage is not particularly of concern for the events in question. If there are fewer than three fields, all of the fields should be indexed.

<ins>Proof Of Concept</ins>
event SetFeeRecipient(FeeRecipient f, address recipient);

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

event ClaimRewards(
        uint256 baseRewards,
        uint256 esGmxRewards,
        uint256 gmxBaseRewards,
        uint256 glpBaseRewards,
        uint256 gmxEsGmxRewards,
        uint256 glpEsGmxRewards
    );

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

event SetDelegationSpace(string delegationSpace, bool shouldClear);

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

event Harvest(
        ERC20[] producerTokens,
        ERC20[] rewardTokens,
        uint256[] rewardAmounts
    );

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

event GlobalAccrue(uint256 lastUpdate, uint256 lastSupply, uint256 rewards);

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

<a href="#Summary">[NC‑4]</a><a name="NC&#x2011;4"> Implementation contract may not be initialized

OpenZeppelin recommends that the initializer modifier be applied to constructors. Per OZs Post implementation contract should be initialized to avoid potential griefs or exploits. https://forum.openzeppelin.com/t/uupsupgradeable-vulnerability-post-mortem/15680/5

<ins>Proof Of Concept</ins>
50: constructor(address _treasury, address _contributors) Owned(msg.sender)

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

167: constructor(
        address _pxGmx,
        address _pxGlp,
        address _pirexFees,
        address _pirexRewards,
        address _delegateRegistry,
        address _gmxBaseReward,
        address _gmx,
        address _esGmx,
        address _gmxRewardRouterV2,
        address _stakedGlp
    ) Owned(msg.sender)

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

24: constructor(
        address _pirexRewards,
        string memory _name,
        string memory _symbol,
        uint8 _decimals
    ) ERC20(_name, _symbol, _decimals)

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

10: constructor(address _pirexRewards)
        PxERC20(_pirexRewards, "Pirex GMX", "pxGMX", 18)

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

66: constructor(
        address _gmxBaseReward,
        address _asset,
        address _pxGmx,
        string memory _name,
        string memory _symbol,
        address _platform,
        address _rewardsModule
    ) PxGmxReward(_pxGmx) PirexERC4626(ERC20(_asset), _name, _symbol)

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

73: constructor(
        address _gmxBaseReward,
        address _gmx,
        address _asset,
        string memory _name,
        string memory _symbol,
        address _platform,
        address _rewardsModule
    ) Owned(msg.sender) PirexERC4626(ERC20(_asset), _name, _symbol)

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

48: constructor(
        ERC20 _asset,
        string memory _name,
        string memory _symbol
    ) ERC20(_name, _symbol, _asset.decimals())

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

40: constructor(address _pxGmx) Owned(msg.sender)

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

<a href="#Summary">[NC‑5]</a><a name="NC&#x2011;5"> Use Underscores for Number Literals

There are multiple occasions where certain numbers have been hardcoded, either in variables or in the code itself. Large numbers can become hard to read.

<ins>Proof Of Concept</ins>
20: uint256 public constant FEE_DENOMINATOR = 10000;

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

22: uint256 public constant FEE_DENOMINATOR = 10000;

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

<ins>Recommended Mitigation Steps</ins>

Use 10_000 instead of 10000 to improve readability.

<a href="#Summary">[NC‑6]</a><a name="NC&#x2011;6"> Use of Block.Timestamp

Block timestamps have historically been used for a variety of applications, such as entropy for random numbers (see the Entropy Illusion for further details), locking funds for periods of time, and various state-changing conditional statements that are time-dependent. Miners have the ability to adjust timestamps slightly, which can prove to be dangerous if block timestamps are used incorrectly in smart contracts. References: SWC ID: 116

<ins>Proof Of Concept</ins>
291: (block.timestamp - u.lastUpdate);

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

314: if (block.timestamp != lastUpdate || totalSupply != lastSupply) {

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

316: (block.timestamp - lastUpdate) *

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

54: (block.timestamp - globalState.lastUpdate) *

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

77: (block.timestamp - u.lastUpdate);

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

<ins>Recommended Mitigation Steps</ins>

Block timestamps should not be used for entropy or generating random numbers—i.e., they should not be the deciding factor (either directly or through some derivation) for winning a game or changing an important state.

Time-sensitive logic is sometimes required; e.g., for unlocking contracts (time-locking), completing an ICO after a few weeks, or enforcing expiry dates. It is sometimes recommended to use block.number and an average block time to estimate times; with a 10 second block time, 1 week equates to approximately, 60480 blocks. Thus, specifying a block number at which to change a contract state can be more secure, as miners are unable to easily manipulate the block number.

<a href="#Summary">[NC‑7]</a><a name="NC&#x2011;7"> Expressions for constant values such as a call to keccak256(), should use immutable rather than constant

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. There is a difference between constant variables and immutable variables, and they should each be used in their appropriate contexts. 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.

<ins>Proof Of Concept</ins>
9: bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE");

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

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

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

<a href="#Summary">[NC‑8]</a><a name="NC&#x2011;8"> emit should happen after clearDelegate and setVoteDelegate

Events should be emitted only after the relevant action has occurred and not before.

<ins>Proof Of Concept</ins>
895: function clearVoteDelegate() public onlyOwner {
896:     emit ClearVoteDelegate();
897:
898:     delegateRegistry.clearDelegate(delegationSpace);
899: }

https://github.com/code-423n4/2022-11-redactedcartel/tree/main/src/PirexGmx.sol#L895-L899

106: emit DistributeFees(
107:    token,
108:    distribution,
109:    treasuryDistribution,
110:    contributorsDistribution
111: );

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

<a href="#Summary">[NC‑9]</a><a name="NC&#x2011;9"> block.timestamp is already used when emitting events, no need to input timestamp

<ins>Proof Of Concept</ins>
297: emit UserAccrue(producerToken, user, block.timestamp, balance, rewards);

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

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

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

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

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

<a href="#Summary">[NC‑10]</a><a name="NC&#x2011;10"> Event emit should emit a parameter

Some emitted events do not have any emitted parameters. It is recommended to add some parameter such as state changes or value changes when events are emitted

<ins>Proof Of Concept</ins>
896: emit ClearVoteDelegate()

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

#0 - c4-judge

2022-12-05T09:47:57Z

Picodes marked the issue as grade-b

Findings Information

🌟 Selected for report: gzeon

Also found by: 0xPanda, 0xSmartContract, B2, Deivitto, Diana, JohnSmith, PaludoX0, Rahoz, RaymondFam, ReyAdmirado, Rolezn, Schlagatron, Secureverse, Tomio, __141345__, adriro, ajtra, aphak5010, c3phas, chaduke, codeislight, cryptonue, datapunk, dharma09, halden, karanctf, keccak123, oyc_109, pavankv, sakshamguruji, saneryee, unforgiven

Awards

39.6537 USDC - $39.65

Labels

bug
G (Gas Optimization)
grade-b
sponsor confirmed
edited-by-warden
G-09

External Links

Link to GitHub issue

Summary<a name="Summary">

Gas Optimizations

IssueContextsEstimated Gas Saved
GAS‑1Using Private Rather Than Public For Constants, Saves Gas16-
GAS‑2Empty Blocks Should Be Removed Or Emit Something2-
GAS‑3require() Should Be Used Instead Of assert()1-
GAS‑4<x> += <y> Costs M ore Gas Than <x> = <x> + <y> For State Variables6-
GAS‑5++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-loops3105
GAS‑6Public Functions To External19-
GAS‑7Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead10-
GAS‑8Superfluous event fields7-
GAS‑9State variables only set in the constructor should be declared immutable1-
GAS‑10internal functions only called once can be inlined to save gas7-
GAS‑11Variables can be packed into fewer storage slots4-
GAS‑12Setting the constructor to payable8104
GAS‑13Functions guaranteed to revert when called by normal users can be marked payable32672
GAS‑14Use unchecked on calculations that cannot go negative3-

Total: 119 contexts over 14 issues

Gas Optimizations

<a href="#Summary">[GAS‑2]</a><a name="GAS&#x2011;2"> Using Private Rather Than Public For Constants, Saves Gas

If needed, the value can be read from the verified contract source code. Savings are due to the compiler not having to create non-payable getter functions for deployment calldata, and not adding another entry to the method ID table

<ins>Proof Of Concept</ins>
20: uint8 public constant FEE_PERCENT_DENOMINATOR = 100;
23: uint8 public constant MAX_TREASURY_FEE_PERCENT = 75;

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

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

44: uint256 public constant FEE_DENOMINATOR = 1_000_000;
47: uint256 public constant FEE_MAX = 200_000;

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

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

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

https://github.com/code-423n4/2022-11-redactedcartel/tree/main/src/PxERC20.sol#L9-L10

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;

https://github.com/code-423n4/2022-11-redactedcartel/tree/main/src/vaults/AutoPxGlp.sol#L18-L22

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;

https://github.com/code-423n4/2022-11-redactedcartel/tree/main/src/vaults/AutoPxGmx.sol#L18-L23

<ins>Recommended Mitigation Steps</ins>

Set variable to private.

<a href="#Summary">[GAS‑3]</a><a name="GAS&#x2011;3"> Empty Blocks Should Be Removed Or Emit Something

The code should be refactored such that they no longer exist, or the block should do something useful, such as emitting an event or reverting. If the contract is meant to be extended, the contract should be abstract and the function signatures be added without any default implementation. If the block is an empty if-statement block to avoid doing subsequent checks in the else-if/else conditions, the else-if/else conditions should be nested under the negation of the if-statement, because they involve different classes of checks, which may lead to the introduction of errors when the code is later modified (if(x){}else if(y){...}else{...} => if(!x){if(y){...}else{...}})

<ins>Proof Of Concept</ins>
12: {}

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

23: {}

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

<a href="#Summary">[GAS‑4]</a><a name="GAS&#x2011;4"> require() Should Be Used Instead Of assert()

<ins>Proof Of Concept</ins>
225: assert(feeAmount + postFeeAmount == assets);

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

<a href="#Summary">[GAS‑5]</a><a name="GAS&#x2011;5"> <x> += <y>/<x> -= <y> Costs More Gas Than <x> = <x> + <y>/<x> = <x> - <y> For State Variables

<ins>Proof Of Concept</ins>
361: producerState.rewardStates[rewardTokens[i]] += r;

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

85: balanceOf[msg.sender] -= amount;

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

90: balanceOf[to] += amount;

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

119: balanceOf[from] -= amount;

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

124: balanceOf[to] += amount;

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

95: rewardState += rewardAmount;

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

<a href="#Summary">[GAS‑6]</a><a name="GAS&#x2011;6"> ++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

<ins>Proof Of Concept</ins>
163: for (uint256 i; i < len; ++i) {

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

351: for (uint256 i; i < pLen; ++i) {

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

396: for (uint256 i; i < rLen; ++i) {

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

<a href="#Summary">[GAS‑10]</a><a name="GAS&#x2011;10"> Public Functions To External

The following functions could be set external to save gas and improve code quality. External call cost is less expensive than of public functions.

<ins>Proof Of Concept</ins>
function clearVoteDelegate() public onlyOwner {

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

function initialize() public initializer {

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

function userAccrue(ERC20 producerToken, address user) public {

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

function transferFrom(
        address from,
        address to,
        uint256 amount
    ) public override returns (bool) {

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

function totalAssets() public view override returns (uint256) {

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

function withdraw(
        uint256 assets,
        address receiver,
        address owner
    ) public override returns (uint256 shares) {

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

function redeem(
        uint256 shares,
        address receiver,
        address owner
    ) public override returns (uint256 assets) {

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

function totalAssets() public view override returns (uint256) {

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

function withdraw(
        uint256 assets,
        address receiver,
        address owner
    ) public override returns (uint256 shares) {

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

function redeem(
        uint256 shares,
        address receiver,
        address owner
    ) public override returns (uint256 assets) {

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

function withdraw(
        uint256 assets,
        address receiver,
        address owner
    ) public virtual returns (uint256 shares) {

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

function redeem(
        uint256 shares,
        address receiver,
        address owner
    ) public virtual returns (uint256 assets) {

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

function totalAssets() public view virtual returns (uint256);

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

function previewMint(uint256 shares) public view virtual returns (uint256) {

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

function maxDeposit(address) public view virtual returns (uint256) {

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

function maxMint(address) public view virtual returns (uint256) {

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

function maxWithdraw(address owner) public view virtual returns (uint256) {

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

function maxRedeem(address owner) public view virtual returns (uint256) {

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

function transferFrom(
        address from,
        address to,
        uint256 amount
    ) public override returns (bool) {

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

<a href="#Summary">[GAS‑11]</a><a name="GAS&#x2011;11"> 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

<ins>Proof Of Concept</ins>
11: uint32 lastUpdate;

https://github.com/code-423n4/2022-11-redactedcartel/tree/main/src/Common.sol#L11

6: uint224 lastSupply;

https://github.com/code-423n4/2022-11-redactedcartel/tree/main/src/Common.sol#L6

12: uint224 lastBalance;

https://github.com/code-423n4/2022-11-redactedcartel/tree/main/src/Common.sol#L12

20: uint8 public constant FEE_PERCENT_DENOMINATOR = 100;

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

23: uint8 public constant MAX_TREASURY_FEE_PERCENT = 75;

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

28: uint8 public treasuryFeePercent = MAX_TREASURY_FEE_PERCENT;

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

26: uint24 public poolFee = 3000;

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

<a href="#Summary">[GAS‑13]</a><a name="GAS&#x2011;13"> Superfluous event fields

block.number and block.timestamp are added to the event information by default, so adding them manually will waste additional gas.

<ins>Proof Of Concept</ins>
96: event SetContract(Contracts indexed c, address contractAddress);

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

50: event GlobalAccrue(
        ERC20 indexed producerToken,
        uint256 lastUpdate,
        uint256 lastSupply,
        uint256 rewards
    );

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

63: event Harvest(
        ERC20[] producerTokens,
        ERC20[] rewardTokens,
        uint256[] rewardAmounts
    );

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

27: event Deposit(
        address indexed caller,
        address indexed owner,
        uint256 assets,
        uint256 shares
    );

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

34: event Withdraw(
        address indexed caller,
        address indexed receiver,
        address indexed owner,
        uint256 assets,
        uint256 shares
    );

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

21: event GlobalAccrue(uint256 lastUpdate, uint256 lastSupply, uint256 rewards);

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

22: event UserAccrue(
        address indexed user,
        uint256 lastUpdate,
        uint256 lastSupply,
        uint256 rewards
    );

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

<a href="#Summary">[GAS‑14]</a><a name="GAS&#x2011;14"> State variables only set in the constructor should be declared immutable

Avoids a Gsset (20000 gas) in the constructor, and replaces each Gwarmacces (100 gas) with a PUSH32 (3 gas).

<ins>Proof Of Concept</ins>
43: pxGmx = ERC20(_pxGmx)

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

<a href="#Summary">[GAS‑15]</a><a name="GAS&#x2011;15"> internal functions only called once can be inlined to save gas

<ins>Proof Of Concept</ins>
474: function afterDeposit

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

487: function afterWithdraw

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

501: function afterTransfer

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

222: function beforeDeposit

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

49: function _globalAccrue

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

68: function _userAccrue

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

90: function _harvest

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

<a href="#Summary">[GAS‑11]</a><a name="GAS&#x2011;11"> Variables can be packed into fewer storage slots

Variables can be packed with lower values to be more gas efficient. If variables occupying the same slot are both written the same function or by the constructor, avoids a separate Gsset (20000 gas). Reads of the variables can also be cheaper. Estimated storage slots that can be saved: 15

<ins>Proof Of Concept</ins>
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;

Can save up to 6 storage slots by changing to:

	uint64 public constant MAX_WITHDRAWAL_PENALTY = 500;
    uint64 public constant MAX_PLATFORM_FEE = 2000;
    uint64 public constant FEE_DENOMINATOR = 10000;
    uint64 public constant MAX_COMPOUND_INCENTIVE = 5000;
    uint256 public constant EXPANDED_DECIMALS = 1e30;

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

https://github.com/code-423n4/2022-11-redactedcartel/tree/main/src/vaults/AutoPxGlp.sol#L18-L30

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;

26:  uint24 public poolFee = 3000;

28:  uint256 public withdrawalPenalty = 300;
29:  uint256 public platformFee = 1000;
30:  uint256 public compoundIncentive = 1000;
31:  address public platform;

34:  address public immutable rewardsModule;

Can save up to 7 storage slots (Technically can save even 8 storage slots with reduced uint size but preferred to keep it at most to uint32/uint64) by changing to:

    uint64 public constant MAX_WITHDRAWAL_PENALTY = 500;
    uint64 public constant MAX_PLATFORM_FEE = 2000;
    uint64 public constant FEE_DENOMINATOR = 10000;
    uint64 public constant MAX_COMPOUND_INCENTIVE = 5000;

    uint24 public poolFee = 3000;
    address public platform;

    uint32 public withdrawalPenalty = 300;
    uint32 public platformFee = 1000;
    uint32 public compoundIncentive = 1000;

    address public immutable rewardsModule;

https://github.com/code-423n4/2022-11-redactedcartel/tree/main/src/vaults/AutoPxGmx.sol#L20-L34

44:  uint256 public constant FEE_DENOMINATOR = 1_000_000;
47:  uint256 public constant FEE_MAX = 200_000;

Can save up to 1 storage slot by changing to:

44:  uint128 public constant FEE_DENOMINATOR = 1_000_000;
47:  uint128 public constant FEE_MAX = 200_000;

https://github.com/code-423n4/2022-11-redactedcartel/tree/main/src/PirexGmx.sol#L44-L47

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;
31:  address public treasury;
32:  address public contributors;

Can save up to 1 storage slot by changing to:

	uint8 public constant FEE_PERCENT_DENOMINATOR = 100;
	address public treasury;
	uint8 public constant MAX_TREASURY_FEE_PERCENT = 75;
	address public contributors;
	uint8 public treasuryFeePercent = MAX_TREASURY_FEE_PERCENT;

https://github.com/code-423n4/2022-11-redactedcartel/tree/main/src/PirexFees.sol#L20-L32

<a href="#Summary">[GAS‑12]</a><a name="GAS&#x2011;12"> Setting the constructor to payable

Saves ~13 gas per instance

<ins>Proof Of Concept</ins>
50: constructor(address _treasury, address _contributors) Owned(msg.sender)

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

167: constructor(
        address _pxGmx,
        address _pxGlp,
        address _pirexFees,
        address _pirexRewards,
        address _delegateRegistry,
        address _gmxBaseReward,
        address _gmx,
        address _esGmx,
        address _gmxRewardRouterV2,
        address _stakedGlp
    ) Owned(msg.sender)

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

24: constructor(
        address _pirexRewards,
        string memory _name,
        string memory _symbol,
        uint8 _decimals
    ) ERC20(_name, _symbol, _decimals)

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

10: constructor(address _pirexRewards)
        PxERC20(_pirexRewards, "Pirex GMX", "pxGMX", 18)

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

66: constructor(
        address _gmxBaseReward,
        address _asset,
        address _pxGmx,
        string memory _name,
        string memory _symbol,
        address _platform,
        address _rewardsModule
    ) PxGmxReward(_pxGmx) PirexERC4626(ERC20(_asset), _name, _symbol)

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

73: constructor(
        address _gmxBaseReward,
        address _gmx,
        address _asset,
        string memory _name,
        string memory _symbol,
        address _platform,
        address _rewardsModule
    ) Owned(msg.sender) PirexERC4626(ERC20(_asset), _name, _symbol)

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

48: constructor(
        ERC20 _asset,
        string memory _name,
        string memory _symbol
    ) ERC20(_name, _symbol, _asset.decimals())

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

40: constructor(address _pxGmx) Owned(msg.sender)

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

<a href="#Summary">[GAS‑13]</a><a name="GAS&#x2011;13"> Functions guaranteed to revert when called by normal users can be marked payable

If a function modifier or require such as onlyOwner/onlyX 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.

<ins>Proof Of Concept</ins>
63: function setFeeRecipient(FeeRecipient f, address recipient)
        external
        onlyOwner
    {

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

83: function setTreasuryFeePercent(uint8 _treasuryFeePercent)
        external
        onlyOwner
    {

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

272: function configureGmxState() external onlyOwner whenPaused {

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

300: function setFee(Fees f, uint256 fee) external onlyOwner {

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

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

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

739: function claimRewards()
        external
        onlyPirexRewards
        returns (
            ERC20[] memory producerTokens,
            ERC20[] memory rewardTokens,
            uint256[] memory rewardAmounts
        )
    {

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

824: function claimUserReward(
        address token,
        uint256 amount,
        address receiver
    ) external onlyPirexRewards {

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

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

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

884: function setVoteDelegate(address voteDelegate) external onlyOwner {

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

895: function clearVoteDelegate() public onlyOwner {

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

909: function setPauseState(bool state) external onlyOwner {

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

921: function initiateMigration(address newContract)
        external
        whenPaused
        onlyOwner
    {

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

956: function completeMigration(address oldContract)
        external
        whenPaused
        onlyOwner
    {

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

93: function setProducer(address _producer) external onlyOwner {

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

151: function addRewardToken(ERC20 producerToken, ERC20 rewardToken)
        external
        onlyOwner
    {

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

179: function removeRewardToken(ERC20 producerToken, uint256 removalIndex)
        external
        onlyOwner
    {

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

432: function setRewardRecipientPrivileged(
        address lpContract,
        ERC20 producerToken,
        ERC20 rewardToken,
        address recipient
    ) external onlyOwner {

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

461: function unsetRewardRecipientPrivileged(
        address lpContract,
        ERC20 producerToken,
        ERC20 rewardToken
    ) external onlyOwner {

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

45: function mint(address to, uint256 amount)
        external
        virtual
        onlyRole(MINTER_ROLE)
    {

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

62: function burn(address from, uint256 amount)
        external
        virtual
        onlyRole(BURNER_ROLE)
    {

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

19: function burn(address from, uint256 amount)
        external
        override
        onlyRole(BURNER_ROLE)
    {

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

94: function setWithdrawalPenalty(uint256 penalty) external onlyOwner {

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

106: function setPlatformFee(uint256 fee) external onlyOwner {

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

118: function setCompoundIncentive(uint256 incentive) external onlyOwner {

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

106: function setPlatform(address _platform) external onlyOwner {
130: function setPlatform(address _platform) external onlyOwner {

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

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

104: function setPoolFee(uint24 _poolFee) external onlyOwner {

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

116: function setWithdrawalPenalty(uint256 penalty) external onlyOwner {

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

128: function setPlatformFee(uint256 fee) external onlyOwner {

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

140: function setCompoundIncentive(uint256 incentive) external onlyOwner {

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

128: function setPlatform(address _platform) external onlyOwner {
152: function setPlatform(address _platform) external onlyOwner {

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

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

<ins>Recommended Mitigation Steps</ins>

Functions guaranteed to revert when called by normal users can be marked payable.

<a href="#Summary">[GAS‑14]</a><a name="GAS&#x2011;14"> Use unchecked on calculations that cannot go negative

<ins>Proof Of Concept</ins>
104: uint256 contributorsDistribution = distribution - treasuryDistribution;

Change to:

unchecked {
    uint256 contributorsDistribution = distribution - treasuryDistribution;
}

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

223: postFeeAmount = assets - feeAmount;

Change to:

unchecked {
    postFeeAmount = assets - feeAmount;
}

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

798: rewardAmounts[1] = baseRewards - rewardAmounts[0];
805: rewardAmounts[3] = esGmxRewards - rewardAmounts[2];

Change to:

unchecked {
    rewardAmounts[1] = baseRewards - rewardAmounts[0];
    rewardAmounts[3] = esGmxRewards - rewardAmounts[2];
}

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

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

#0 - c4-judge

2022-12-05T14:21:54Z

Picodes marked the issue as grade-b

#1 - drahrealm

2022-12-09T05:54:15Z

Many are invalid and/or minor relative to the added complexity, but otherwise some are considered to be implemented

#2 - c4-sponsor

2022-12-09T05:54:20Z

drahrealm marked the issue as sponsor confirmed

AuditHub

A portfolio for auditors, a security profile for protocols, a hub for web3 security.

Built bymalatrax © 2024

Auditors

Browse

Contests

Browse

Get in touch

ContactTwitter