Redacted Cartel contest - gz627's results

[L-1]  Using a fixed version of solidity compiler

From security perspective there's always the trade-off between new code fixing known issues and potentially introducing new ones. To make an informed choice you can always consult the List of Known Bugs. A new release never leaves known security issues unfixed. You might want to hold off for a few days after a new release just in case something is discovered once people start using it. rules-on-choosing-a-solidity-version

Contracts should be deployed with the same compiler version and flags that they have been tested the most with. Locking the pragma helps ensure that contracts do not accidentally get deployed using, for example, the compiler which may have high risks of bugs.

Recommend to use pragma solidity 0.8.17; instead of pragma solidity >=0.8.0; or pragma solidity ^0.8.0;.

File: src/vaults/PirexERC4626.sol
2: pragma solidity >=0.8.0;

[L-2] Function parameter shawdows state variable

Instances (6)

instance 1 File: src/vaults/AutoPxGmx.sol Line 342: address owner https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/vaults/AutoPxGmx.sol#L342

Contract AutoPxGmx is a owned contract which has an inherited state variable owner. However, its function redeem() has a parameter address owner on Line 342 shawdows the state variable. This may confuse users. The suggestion is to change the function parameter to address _owner.

Same issues are:

instance 2 File: src/vaults/AutoPxGmx.sol Line 318: address owner https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/vaults/AutoPxGmx.sol#L318

instance 3 File: src/vaults/AutoPxGlp.sol Line 439: address owner https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/vaults/AutoPxGlp.sol#L439

instance 4 File: src/vaults/AutoPxGlp.sol Line 452: address owner https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/vaults/AutoPxGlp.sol#L452

instance 5 File: src/vaults/AutoPxGlp.sol Line 488: address owner https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/vaults/AutoPxGlp.sol#L488

instance 6 File: src/vaults/AutoPxGlp.sol Line 502: address owner https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/vaults/AutoPxGlp.sol#L502

[L-3] NatSpec tag content inconsistent with actual parameter

File:     src/PirexRewards.sol
336:     @return rewardAmounts ERC20[] Reward token amounts

According to Line 343, the above Line 336 should be: @return rewardAmounts int256[] Reward token amounts

[L-4] Uniswap pool fee tier should be bounded

Univerwap V3 currently has four fees tiers (0.01%, 0.05%, 0.30%, and 1%) and more tiers may be added by UNI governance. Setting the fee tier too low may result in transaction failure. It is a good practice to limit the pool fee tier within a reasonable range to prevent accidental errors.

File: src/vaults/AutoPxGmx.sol Function: setPoolFee()

[L-5] fee parameter (Uniswap pool tier fee) should be removed from AutoPxGmx.compound() function

AutoPxGmx.compound() function is public and can be called by any user. If a user sets an inappropriate fee (e.g. too low), Uniswap transaction will fail. On the other hand, there is a state variable poolFee has already been set by the contract owner (on Line 26 and function setPoolFee()) as Uniswap pool tier fee. So, it is reasonable to just use the poolFee directly in the swap function. This also saves gas.

File: https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/vaults/AutoPxGmx.sol Function: compound()

Recommended changes:

• Remove Line 243: uint24 fee,
• Replace Line 272: fee: fee, with fee: poolFee,

Other related changes:

1. Replace Line 321 with compound(1, 0, true);
2. Replace Line 345 with compound(1, 0, true);
3. Replace Line 227 with compound(1, 0, true);

[L-6] Inconsistent comment

File:  src/vaults/AutoPxGlp.sol

86:    // Approve the Uniswap V3 router to manage our base reward (inbound swap token)
87:    gmxBaseReward.safeApprove(address(_platform), type(uint256).max);

The above Line 86 says Approve the Uniswap V3 router to ..., actually it should be Approve the platform (e.g. PirexGmx) to ...