PirexCvx Manual Review Findings

PCV-01M: Incorrect Proportionate Claim Mechanism

Type	Severity	Location
Mathematical Operations		PirexCvx.sol:L891-L933

Description:

The redeemFuturesRewards mechanism will fail to function properly when more than one parties have token futures for a particular epoch as the total supply is reduced via the burn mechanism whilst the futuresRewards stored in pxCvx remain the same.

Impact:

Any user beyond the first will claim a disproportionate amount of rewards, a trait that can be exploited. Additionally, a portion of users will be unable to claim rewards at all as their disproportionate rewards would not be fulfillable by the ERC20 transfer the function executes.

Example:

Copy
891/**
892    @notice Redeem Futures rewards for rpCVX holders for an epoch
893    @param  epoch     uint256  Epoch (ERC1155 token id)
894    @param  receiver  address  Receives futures rewards
895*/
896function redeemFuturesRewards(uint256 epoch, address receiver)
897    external
898    whenNotPaused
899    nonReentrant
900{
901    if (epoch == 0) revert InvalidEpoch();
902    if (epoch > getCurrentEpoch()) revert InvalidEpoch();
903    if (receiver == address(0)) revert ZeroAddress();
904
905    // Prevent users from burning their futures notes before rewards are claimed
906    (, bytes32[] memory rewards, , uint256[] memory futuresRewards) = pxCvx
907        .getEpoch(epoch);
908
909    if (rewards.length == 0) revert NoRewards();
910
911    emit RedeemFuturesRewards(epoch, receiver, rewards);
912
913    // Check sender rpCVX balance
914    uint256 rpCvxBalance = rpCvx.balanceOf(msg.sender, epoch);
915    if (rpCvxBalance == 0) revert InsufficientBalance();
916
917    // Store rpCVX total supply before burning
918    uint256 rpCvxTotalSupply = rpCvx.totalSupply(epoch);
919
920    // Burn rpCVX tokens
921    rpCvx.burn(msg.sender, epoch, rpCvxBalance);
922
923    uint256 rLen = rewards.length;
924
925    // Loop over rewards and transfer the amount entitled to the rpCVX token holder
926    for (uint256 i; i < rLen; ++i) {
927        // Proportionate to the % of rpCVX owned out of the rpCVX total supply
928        ERC20(address(uint160(bytes20(rewards[i])))).safeTransfer(
929            receiver,
930            (futuresRewards[i] * rpCvxBalance) / rpCvxTotalSupply
931        );
932    }
933}

Recommendation:

We advise the futuresRewards entry to also be updated within pxCvx to subtract the claimed proportionate rewards ensuring that consequent percentage calculations are done correctly.

Alleviation:

The local futuresReards is now properly subtracted in the for loop operation and an external call to updateEpochFuturesRewards is also performed ensurign that the futuresRewards are synchronized in the relevant contract, alleviating this exhibit in full.

PCV-02M: Improper Prevention of Emergency Migration

Type	Severity	Location
Logical Fault		PirexCvx.sol:L323, L326

Description:

The logic within the emergencyMigrateTokens will fail to execute properly if the outstandingRedemptions match the currently held CVX tokens exactly as in such a case the consequent revert statement will execute as the migration amount will be zero.

Impact:

The emergency migration function will not be executable thereby locking the tokens within the contract. The scenario whereby the outstandingRedemptions match the amount held by the contract is likely as users panic in emergency scenarios and would request redemptions at an alarming pace.

Example:

Copy
318for (uint256 i; i < tLen; ++i) {
319    uint256 amount = ERC20(tokens[i]).balanceOf(address(this));
320
321    // If it's CVX, make sure we take into account the outstandingRedemptions
322    if (tokens[i] == address(CVX)) {
323        amount -= outstandingRedemptions;
324    }
325
326    if (amount == 0) revert ZeroAmount();
327
328    amounts[i] = amount;
329    ERC20(tokens[i]).safeTransfer(pirexCvxMigration, amount);
330}

Recommendation:

We advise the if (amount == 0) revert ZeroAmount(); statement to be relocated before the special CVX logic. Alternatively, we advise the duplicate token entry finding to be assimilated to the codebase that relocates the entire CVX logic outside the for loop and alleviates this exhibit as well.

Alleviation:

The Pirex team evaluated this exhibit and stated that this is an intentional design decision as users with outstanding redemptions should still be able to redeem their tokens using the legacy redemption path. After highlighting the emergency word usage to their team, Pirex responded by stating that the emergency convention is not utilized to indicate a failure scenario of the system but rather an abnormal circumstance of the protocols the contract interfaces with (namely Convex). Given the intentional design of the code we consider this exhibit acknowledged.

PCV-03M: Mismatch of Maximum Redemption Time

Type	Severity	Location
Standard Conformity		PirexCvx.sol:L76

Description:

The maximum redemption time present on the Convex V2 Locker instance is 16 weeks rather than the 17 weeks detailed in the variable's comment as well as represented by the variable itself.

Impact:

All redemptions will be unfairly penalized with a fee greater than the minimum regardless of whether the maximum unlock time has been specified.

Example:

Copy
75// Maximum wait time (seconds) for a CVX redemption (17 weeks)
76uint32 public constant MAX_REDEMPTION_TIME = 10281600;

Recommendation:

We advise this trait to be re-evaluated and if a fee increase is expected at all times the trait to be documented as otherwise the minimum fee for redemptions will be always unattainable.

Alleviation:

In light of supplemental material provided by the Pirex team, we concluded that our original finding's rationale is invalid given that all locks performed by the Pirex system are "fresh locks" from the CvxLockerV2 perspective. As a result, the maximum redemption time specified in the contract is correct and we consider the exhibit nullified.

PCV-04M: Disassociation of Votium Time Rewards

Type	Severity	Location
Logical Fault		PirexCvx.sol:L741, L752-L754, L772-L779

Description:

The Votium rewards that are submitted to the contract are then meant to be distributed to users based on the currently active epoch, however, the Votium rewards themselves are not time-restrained with regards to their claim and the contract also meets a fatal failure in case any of the claims fail to execute properly. This can cause significant issues such as race-conditions to arise whereby a user detects a high amount of Votium claims seconds before an epoch ends, submits their funds for claim-age on the next epoch, hijacks the Votium claim mechanism, and ultimately claims a disproportionate amount of rewards for as little as a few seconds of lock time for their funds.

Impact:

Any reward submissions of more than one member can be hijacked via a race-condition by submitting a claim of the first reward with a higher priority fee thereby causing the transaction including all rewards to fail as the first one will not be claimable. In time-based edge case scenarios, a user would be able to exploit this fact to momentarily stake their pxCvx to the contract, acquire a disproportionate amount of futures, and unlock their stake in the next few seconds at no penalty.

Example:

Copy
737/**
738    @notice Claim multiple Votium rewards
739    @param  votiumRewards  VotiumRewards[]  Votium rewards metadata
740*/
741function claimVotiumRewards(VotiumReward[] calldata votiumRewards)
742    external
743    whenNotPaused
744    nonReentrant
745{
746    uint256 tLen = votiumRewards.length;
747    if (tLen == 0) revert EmptyArray();
748
749    // Take snapshot before claiming rewards, if necessary
750    pxCvx.takeEpochSnapshot();
751
752    uint256 epoch = getCurrentEpoch();
753    (uint256 snapshotId, , , ) = pxCvx.getEpoch(epoch);
754    uint256 rpCvxSupply = rpCvx.totalSupply(epoch);
755
756    for (uint256 i; i < tLen; ++i) {
757        address token = votiumRewards[i].token;
758        uint256 index = votiumRewards[i].index;
759        uint256 amount = votiumRewards[i].amount;
760        bytes32[] memory merkleProof = votiumRewards[i].merkleProof;
761
762        if (token == address(0)) revert ZeroAddress();
763        if (amount == 0) revert ZeroAmount();
764
765        emit ClaimVotiumReward(token, index, amount);
766
767        ERC20 t = ERC20(token);
768
769        // Used for calculating the actual token amount received
770        uint256 prevBalance = t.balanceOf(address(this));
771
772        // Validates `token`, `index`, `amount`, and `merkleProof`
773        votiumMultiMerkleStash.claim(
774            token,
775            index,
776            address(this),
777            amount,
778            merkleProof
779        );
780
781        (
782            uint256 rewardFee,
783            uint256 snapshotRewards,
784            uint256 futuresRewards
785        ) = _calculateRewards(
786                fees[Fees.Reward],
787                pxCvx.totalSupplyAt(snapshotId),
788                rpCvxSupply,
789                t.balanceOf(address(this)) - prevBalance
790            );
791
792        // Add reward token address and snapshot/futuresRewards amounts (same index for all)
793        pxCvx.addEpochRewardMetadata(
794            epoch,
795            token.fillLast12Bytes(),
796            snapshotRewards,
797            futuresRewards
798        );
799
800        // Distribute fees
801        t.safeApprove(address(pirexFees), rewardFee);
802        pirexFees.distributeFees(address(this), token, rewardFee);
803    }
804}

Recommendation:

Firstly, we advise the claim process to go through even if one of the votiumRewards has already been claimed. This will prevent claim hijacking from malicious actors. Lastly, we advise some form of time association to be enforced for Votium claims. As an example, the contract could distirbute the rewards for the previous epoch rather than the current one thereby disallowing edge-case scenarios of transaction submissions close to an epoch's end date.

Alleviation:

The Pirex team has stated that this is not a concern as users should be able to stake for one round and acquire their respective rewards. We would like to note that the real concern here is that the user is not obligated to stake for a full round; they are able to stake close to the end of a particular round and claim in the next thus staking their tokens only for mere seconds. This might cause a profitable attack scenario to unfold. The Pirex team reconsidered this exhibit based on our supplemental information and desired to acknowledge it.

PCV-05M: Improper Sanitization of New Fee

Type	Severity	Location
Input Sanitization		PirexCvx.sol:L351

Description:

While the newFee is validated to be within the denominator bounds, it is not validated to be different than the existing one.

Impact:

Queueing the same fee will cause multiple abnormal events to be emitted, will ultimately result in a no-op, and does not protect against multiple pending transactions extending the effectiveAfter timestamp.

Example:

Copy
345/** 
346    @notice Queue fee
347    @param  f       enum    Fee
348    @param  newFee  uint32  New fee
349 */
350function queueFee(Fees f, uint32 newFee) external onlyOwner {
351    if (newFee > FEE_DENOMINATOR) revert InvalidNewFee();
352
353    uint224 effectiveAfter = uint224(block.timestamp + EPOCH_DURATION);
354
355    // Queue up the fee change, which can be set after 2 weeks
356    queuedFees[f].newFee = newFee;
357    queuedFees[f].effectiveAfter = effectiveAfter;
358
359    emit QueueFee(f, newFee, effectiveAfter);
360}
361
362/** 
363    @notice Set fee
364    @param  f  enum  Fee
365 */
366function setFee(Fees f) external onlyOwner {
367    QueuedFee memory q = queuedFees[f];
368
369    if (q.effectiveAfter > block.timestamp)
370        revert BeforeEffectiveTimestamp();
371
372    fees[f] = q.newFee;
373
374    emit SetFee(f, q.newFee);
375}

Recommendation:

We advise the if conditional to additionally evaluate that the newFee is different than the current fees[f] data entry.

Alleviation:

Additional sanitization was introduced ensuring that the newFee is not equal to the fees[f] on successful execution and alleviating this exhibit.

PCV-06M: Inexistent Migration Delay

Type	Severity	Location
Centralization Concern		PirexCvx.sol:L307

Description:

The emergency migration mechanism of the contract is available for at-will invocation by the owner as they can invoke the setPirexCvxMigration and emergencyMigrateTokens in a single chain of transactions.

Example:

Copy
287/** 
288    @notice Set pirexCvxMigration address
289    @param  _pirexCvxMigration  address  PirexCvxMigration address
290 */
291function setPirexCvxMigration(address _pirexCvxMigration)
292    external
293    onlyOwner
294    whenPaused
295{
296    if (_pirexCvxMigration == address(0)) revert ZeroAddress();
297
298    pirexCvxMigration = _pirexCvxMigration;
299
300    emit SetPirexCvxMigration(pirexCvxMigration);
301}
302
303/**
304    @notice Withdraw ERC20 tokens to the pirexCvxMigration address in case of emergency
305    @param  tokens  address[]  Token addresses
306 */
307function emergencyMigrateTokens(address[] calldata tokens)
308    external
309    onlyOwner
310    whenPaused
311{
312    if (pirexCvxMigration == address(0)) revert ZeroAddress();
313
314    uint256 tLen = tokens.length;
315    if (tLen == 0) revert EmptyArray();
316    uint256[] memory amounts = new uint256[](tLen);
317
318    for (uint256 i; i < tLen; ++i) {
319        uint256 amount = ERC20(tokens[i]).balanceOf(address(this));
320
321        // If it's CVX, make sure we take into account the outstandingRedemptions
322        if (tokens[i] == address(CVX)) {
323            amount -= outstandingRedemptions;
324        }
325
326        if (amount == 0) revert ZeroAmount();
327
328        amounts[i] = amount;
329        ERC20(tokens[i]).safeTransfer(pirexCvxMigration, amount);
330    }
331
332    emit MigrateTokens(pirexCvxMigration, tokens, amounts);
333}

Recommendation:

We advise a time delay to be enforced instead to ensure users have adequate time to withdraw their tokens manually if they are able to. To achieve this, a dedicated user-oriented emergency withdrawal mechanism should be available at all times regardless of the pause status of the contract, similarly to SushiSwap.

Alleviation:

The Pirex team has acknowledged this exhibit and stated that the migration actions will sit behind a multisignature of yet-to-be-confirmed reputable parties in the blockchain space thus ensuring that the protocol's owners cannot single-handedly manipulate the protocol. We consider this sufficient acknowledgement of the exhibit.

PCV-07M: Inexistent Prevention of Duplicate Entries

Type	Severity	Location
Input Sanitization		PirexCvx.sol:L321-L324

Description:

The tokens array can contain duplicate entries and as such permit the caller to withdraw more than amount - outstandingRedemptions for the CVX token in particular, circumventing the redemption protection mechanism.

Impact:

The CVX tokens can be improperly extracted from the contract disallowing it to fulfill outstanding redemptions and causing significant impact to its users.

Example:

Copy
303/**
304    @notice Withdraw ERC20 tokens to the pirexCvxMigration address in case of emergency
305    @param  tokens  address[]  Token addresses
306 */
307function emergencyMigrateTokens(address[] calldata tokens)
308    external
309    onlyOwner
310    whenPaused
311{
312    if (pirexCvxMigration == address(0)) revert ZeroAddress();
313
314    uint256 tLen = tokens.length;
315    if (tLen == 0) revert EmptyArray();
316    uint256[] memory amounts = new uint256[](tLen);
317
318    for (uint256 i; i < tLen; ++i) {
319        uint256 amount = ERC20(tokens[i]).balanceOf(address(this));
320
321        // If it's CVX, make sure we take into account the outstandingRedemptions
322        if (tokens[i] == address(CVX)) {
323            amount -= outstandingRedemptions;
324        }
325
326        if (amount == 0) revert ZeroAmount();
327
328        amounts[i] = amount;
329        ERC20(tokens[i]).safeTransfer(pirexCvxMigration, amount);
330    }
331
332    emit MigrateTokens(pirexCvxMigration, tokens, amounts);
333}

Recommendation:

We advise the CVX token to be prevented from being present in the tokens array via a require check and it to be emptied with its special logic outside the for loop.

Alleviation:

The Pirex team has significantly adjusted the emergency migration mechanism to be composed of a two-party system with the configuration parameters of the emergency execution being adjusted by the Pirex team's multisignature wallet while the actual execution of the migration procedure is meant to be conducted by a dedicated, non-Pirex party that is publicly declared on-chain. While on-chain sanitization of the duplicate token entries is still advised, the current implementation in place can be considered a sufficient alleviation as long as the actors of the system act faithfully.

PCV-08M: Potentially Harmful Epoch Rounding

Type	Severity	Location
Mathematical Operations		PirexCvx.sol:L377-L383, L680-L711, L713-L735

Description:

All epoch calculations that utilize getCurrentEpoch contain a rounding-down operation that can cause harmful side-effects. Such a side-effect illustrated in the linked lines is the ability to stake pxCvx for one round and unstake a few seconds later if the current block.timestamp is close to the rounding threshold. As a result, a user can acquire a significant amount of _mintFutures unfairly by momentarily staking their funds and can cause reward distributions to malfunction.

Impact:

As locks are evaluated on an epoch basis rather than a timestamp basis, it is possible to perform a "full epoch" lock in a few seconds if timed properly via on-chain transactions that execute before and after the end point of an epoch.

Example:

Copy
680/**
681    @notice Stake pCVX
682    @param  rounds    uint256    Rounds (i.e. Convex voting rounds)
683    @param  f         enum     Futures enum
684    @param  assets    uint256  pCVX amount
685    @param  receiver  address  Receives spCVX
686*/
687function stake(
688    uint256 rounds,
689    Futures f,
690    uint256 assets,
691    address receiver
692) external whenNotPaused nonReentrant {
693    if (rounds == 0) revert ZeroAmount();
694    if (assets == 0) revert ZeroAmount();
695    if (receiver == address(0)) revert ZeroAddress();
696
697    // Burn pCVX
698    pxCvx.burn(msg.sender, assets);
699
700    emit Stake(rounds, f, assets, receiver);
701
702    // Mint spCVX with the stake expiry timestamp as the id
703    spCvx.mint(
704        receiver,
705        getCurrentEpoch() + EPOCH_DURATION * rounds,
706        assets,
707        UNUSED_1155_DATA
708    );
709
710    _mintFutures(rounds, f, assets, receiver);
711}
712
713/**
714    @notice Unstake pCVX
715    @param  id        uint256  spCVX id (an epoch timestamp)
716    @param  assets    uint256  spCVX amount
717    @param  receiver  address  Receives pCVX
718*/
719function unstake(
720    uint256 id,
721    uint256 assets,
722    address receiver
723) external whenNotPaused nonReentrant {
724    if (id > block.timestamp) revert BeforeStakingExpiry();
725    if (assets == 0) revert ZeroAmount();
726    if (receiver == address(0)) revert ZeroAddress();
727
728    // Mint pCVX for receiver
729    pxCvx.mint(receiver, assets);
730
731    emit Unstake(id, assets, receiver);
732
733    // Burn spCVX from sender
734    spCvx.burn(msg.sender, id, assets);
735}

Recommendation:

We advise the rounding approach to be re-evaluated and a ceiling rather than flooring approach to be used for calculating the current epoch. Alternatively, we advise the impact of this mechanism to be evaluated on a case-by-case basis (such as the stake / unstake misbehaviour), the impact to be documented properly and wherever needed safeguards to be put in place.

Alleviation:

The Pirex team has stated that this is desired behaviour as the only requirement for staking for a particular round is to contain sufficient pxCVX tokens. As a result, we consider this exhibit nullified.

PCV-09M: Unaudited External Contract Interaction

Type	Severity	Location
Standard Conformity		PirexCvx.sol:L83

Description:

The linked contract utilized for the Votium-related reward mechanisms is unaudited.

Example:

Copy
83IVotiumMultiMerkleStash public votiumMultiMerkleStash;

Recommendation:

We advise its integration to the system to be performed with caution as it can contain un-identified flaws as well as undocumented future features that can compromise the integration of the contract with the Votium system.

Alleviation:

The Pirex team has stated that they will proceed with including the dependency without performing an audit for it as they deem it secure enough based on on-chain utilization.