Redacted Cartel contest - datapunk's results

Lines of code

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexRewards.sol#L179 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexRewards.sol#L387

Vulnerability details

Impact

Any unclaimed rewardTokens will be unclaimable once a rewardToken has been removed from producerTokens[producerToken].rewardTokens list.

Proof of Concept

The owner can call removeRewardToken to remove any rewardToken from a producerToken. However, if the rewardToken still has unclaimed rewards, as evidenced by producerTokens[producerToken].rewardStates, these rewards will be unclaimable.

The claim function, once identified the producerToken, loops through rewardTokens list and claims rewards as marked in rewardStates[rewardToken]. However, if a rewardToken has been removed, even if rewardStates[rewardToken] is non-zero, it would not be claimable.

Tools Used

manual

Recommended Mitigation Steps

Check require(rewardStates[rewardTokens[removalIndex]] == 0) before removing the unwanted rewardToken.

L1. Depending on how many rewardTokens, the owner of PirexRewards will add to producerTokens[producerToken].rewardTokens, looping through the array may result in OOG.

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexRewards.sol#L163

L2. Avoid event spamming from userAccrue in PirexRewards.sol

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexRewards.sol#L281 userAccrue generates UserAccrue event, regardless whether or not there are any changes. bracketing the updating logic and emit inside of the following condition

 if (block.timestamp != u.lastUpdate || balance != u.lastBalance) {
     ...
 }

Similar issues exist in PxGmxReward.sol for _globalAccue and _userAccrue https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/PxGmxReward.sol#L49 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/PxGmxReward.sol#L68

L3. event ClaimRewards may emit rewards values that are inconsistent with the actual rewards.

This is due to the recalculation in place for rewardAmounts. ClaimRewards should emit rewardAmounts values in place the gmxBaseRewards, etc.

L4. PxGmxReward is not inheriting from ERC20, yet it is accessing ERC20 functions.

AutoPxGlp inherits from both PxGmxReward and PirexERC4626, which in turn from ERC20. Therefore, things do work. However it is best practice to make code more explicity by inheriting from ERC20.

L5. withdraw and redeem functions in AutoPxGmx can reuse the functions defined in PirexERC4626 by calling PirexERC4626.withdraw and PirexERC4626.redeem

L6. solidity always rounds down

Fees can be calculated more accurately with rounding up as needed. For example,

feeAmount = (assets * fees[f]) / FEE_DENOMINATOR; can be changed to feeAmount = (assets * fees[f] + FEE_DENOMINATOR / 2) / FEE_DENOMINATOR;

G1. previewWithdraw and previewRedeem in AutoPxGlp.sol can be made external to save gas, since they are not called internally.

G2. harvest() can be made more gas efficient if it keeps track of lastCallTime and returns immediately if lastCallTime is the same as current block.timeStamp

G3. totalAssets() and asset.balanceOf(address(this)) does the same thing, so create one local variable to save one SLOAD https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L288