PirexCvxConvex Manual Review Findings

PCC-01M: Improper Adjustment of Delegation Space & Registry

Type	Severity	Location
Logical Fault		PirexCvxConvex.sol:L108-L111, L195-L208

Description:

The delegation space & registry can be adjusted without nullifying previously delegated voting power and as such potentially cause improper delegation of power when it is overwritten improperly.

Impact:

Delegation will be improperly retained in the original delegation space which may ultimately be malicious.

Example:

Copy
195/** 
196    @notice Set delegationSpace
197    @param  _delegationSpace  string  Convex Snapshot delegation space
198 */
199function setDelegationSpace(string memory _delegationSpace)
200    external
201    onlyOwner
202{
203    bytes memory d = bytes(_delegationSpace);
204    if (d.length == 0) revert EmptyString();
205    delegationSpace = bytes32(d);
206
207    emit SetDelegationSpace(_delegationSpace);
208}

Recommendation:

We advise the setDelegationSpace & setConvexContract functions to invoke the clearDelegate function of cvxDelegateRegistry should a delegation exist, a trait that can be validated via the delegation public function of the said contract.

Alleviation:

After a secondary consideration, the Pirex team adjusted the setDelegationSpace code to accept an additional argument that specifies whether any previously set delegation should be cleared via the existing clearVoteDelegate function. As a result, we consider the exhibit fully alleviated.

PCC-02M: Incorrect Usage of Funds at Rest

Type	Severity	Location
Logical Fault		PirexCvxConvex.sol:L143-L145

Description:

The _lock function is meant to calculate how many funds it is supposed to lock in the CVX Locker, however, it incorrectly calculates them leading to unfulfillable redemptions.

Impact:

Users will not be able to fulfill their outstanding redemptions as the pending locks of the system "overpower" the condition of outstandingRedemptions being higher than the current balance of the contract.

Example:

Copy
123/**
124    @notice Unlock CVX and relock excess
125 */
126function _lock() internal {
127    _unlock();
128
129    uint256 balance = CVX.balanceOf(address(this));
130    bool balanceGreaterThanRedemptions = balance > outstandingRedemptions;
131
132    // Lock CVX if the balance is greater than outstanding redemptions or if there are pending locks
133    if (balanceGreaterThanRedemptions || pendingLocks != 0) {
134        uint256 balanceRedemptionsDifference = balanceGreaterThanRedemptions
135            ? balance - outstandingRedemptions
136            : 0;
137
138        // Lock amount is the greater of the two: balanceRedemptionsDifference or pendingLocks
139        // balanceRedemptionsDifference is greater if there is unlocked CVX that isn't reserved for redemptions + deposits
140        // pendingLocks is greater if there are more new deposits than unlocked CVX that is reserved for redemptions
141        cvxLocker.lock(
142            address(this),
143            balanceRedemptionsDifference > pendingLocks
144                ? balanceRedemptionsDifference
145                : pendingLocks,
146            0
147        );
148
149        pendingLocks = 0;
150    }
151}

Recommendation:

The _lock function logic should be refactored to reduce pendingLocks by the amount of funds that are within outstandingRedemptions. As such, if outstandingRedemptions match the pendingLocks then only excess funds should be locked rather than pendingLocks.

Alleviation:

The Pirex team has evaluated our exhibit and deemed invalid based on their internal fuzz tests as well as general test suites. As a result, we consider this exhibit nullified per the client's request.

PCC-03M: Unlimited CVX Allowance Centralization

Type	Severity	Location
Centralization Concern		PirexCvxConvex.sol:L104

Description:

The setConvexContract function permits the owner to set an unlimited CVX allowance to an arbitrary address, a trait we strongly advise against.

Impact:

Should the owner of the system be compromised, they are capable of completely compromising the system in relatively few actions.

Example:

Copy
87/** 
88    @notice Set a contract address
89    @param  c                enum     Contract enum
90    @param  contractAddress  address  Contract address    
91 */
92function setConvexContract(ConvexContract c, address contractAddress)
93    external
94    onlyOwner
95{
96    if (contractAddress == address(0)) revert ZeroAddress();
97
98    emit SetConvexContract(c, contractAddress);
99
100    if (c == ConvexContract.CvxLocker) {
101        // Revoke approval from the old locker and add allowances to the new locker
102        CVX.safeApprove(address(cvxLocker), 0);
103        cvxLocker = ICvxLocker(contractAddress);
104        CVX.safeApprove(contractAddress, type(uint256).max);
105        return;
106    }
107
108    if (c == ConvexContract.CvxDelegateRegistry) {
109        cvxDelegateRegistry = ICvxDelegateRegistry(contractAddress);
110        return;
111    }
112}

Recommendation:

We advise some alternative migration methodologies to be utilized instead, such as by ensuring any locker changes are accompanied by a time delay to allow sufficient time for users to extract their funds from the system.

Alleviation:

The Pirex team has acknowledged this exhibit and stated that the administrative actions of the ecosystem will sit behind a multisignature of reputable team members thus ensuring that each protocol owner cannot single-handedly manipulate the protocol. We consider this sufficient acknowledgement of the exhibit.

PCC-04M: Unsafe & Inefficient Type

Type	Severity	Location
Mathematical Operations		PirexCvxConvex.sol:L174, L178

Description:

The linked statements utilize the uint8 data type and perform an unsafe cast of the reward array yielded by Convex.

Impact:

While relatively unlikely, should the total available rewards of Convex exceed 255 entries the cast operation will overflow silently causing the system to misbehave.

Example:

Copy
160/**
161    @notice Get claimable rewards and balances
162    @return rewards  ConvexReward[]  Claimable rewards and balances
163 */
164function _claimableRewards()
165    internal
166    view
167    returns (ConvexReward[] memory rewards)
168{
169    address addr = address(this);
170
171    // Get claimable rewards
172    ICvxLocker.EarnedData[] memory c = cvxLocker.claimableRewards(addr);
173
174    uint8 cLen = uint8(c.length);
175    rewards = new ConvexReward[](cLen);
176
177    // Get the current balances for each token to calculate the amount received
178    for (uint8 i; i < cLen; ++i) {
179        rewards[i] = ConvexReward({
180            token: c[i].token,
181            amount: c[i].amount,
182            balance: ERC20(c[i].token).balanceOf(addr)
183        });
184    }
185}

Recommendation:

We advise the cast and corresponding loop iterator type to be adjusted to uint256 as any sub-256-bit data type incurs extra gas on the EVM.

Alleviation:

The original uint256 data type is now utilized as advised.