Omniscia Pirex Audit

PirexCvxConvex Code Style Findings

PirexCvxConvex Code Style Findings

PCC-01C: Inexplicable if-if Structure

Description:

The linked if-if structure is inexplicable as each conditional evaluates an enum that can only take one value.

Example:

contracts/PirexCvxConvex.sol
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 the conditionals to be chained in a single if-else clause with the last clause not evaluating any conditional and being set as simply else as the enum value is guaranteed once all other values have been exhausted.

Alleviation:

Instead of using a single if-else clause the code was updated to omit the secondary if clause and corresponding return statement and retain the original if clause. These statements are functionally equivalent and as such we consider the exhibit dealt with.

PCC-02C: Redundant Return Statements

Description:

The linked return statements are redundant as the function will not execute any supplemental statements (valid for the second if clause and the first if clause if a proper if-else chain is used).

Example:

contracts/PirexCvxConvex.sol
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 them to be safely omitted from the codebase.

Alleviation:

The Pirex team has instead removed the latter return statement and retained the current simple if structure in place, implying that they wish to retain the current code style in place. As a result, we consider this exhibit alleviated.

Prev

PirexCvx.sol (PCV-C)

Next

Finding Types

On this page