Redacted Cartel contest - Koolex's results

Lines of code

https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/vaults/AutoPxGmx.sol#L370-L403

Vulnerability details

Impact

A malicious early actor can set a minimum X amount for deposit . Therefore, the contract AutoPxGmx doesn't accept deposits from other depositers with amount less than X which can be determined by the actor.

Proof of Concept

1 - Malicious actor user A deposits 1e18 pxGMX into AutoPxGmx. - neglecting the fees for simplicity, shares of A = assets of A = 1e18 since totalSupply is zero as this the first deposit (Check depositGmx function)

uint256 supply = totalSupply;

if (
    (shares = supply == 0
                ? assets
                : assets.mulDivDown(supply, totalAssets() - assets)) == 0
   ) revert ZeroShares();

2 - A sends 100e18 pxGMX to AutoPxGmx contract (can buy it before from PirexGmx via depositGmx function). - totalSupply = 1e18 - shares of A = 1e18 (still the same) - totalAssets = 1e18+100e18 = 101e18

3 - User B tries to deposit 90e18 pxGMX, following the logic in the code:

uint256 supply = totalSupply;

if (
    (shares = supply == 0
                ? assets
                : assets.mulDivDown(supply, totalAssets() - assets)) == 0
   ) revert ZeroShares();

shares = assets.mulDivDown(supply, totalAssets() - assets)) shares = 90e18 * 1e18 / (191e18-90e18) = 90e18 / 101e18 = 0 since shares == 0, the deposit will revert.

Conclusion: User B has to deposit at least 101e18 to satisfy the logic. shares = 101e18 * 1e18 / (202e18-101e18) = 101e18 / 101e18 = 1

Notes: 1- If A sends a big amount of pxGMX, it will result in DoS and make it nearly impossible for others to deposit. 2- This applies to both depositGmx function and the deposit function inherited from PirexERC4626 contract.

Tools Used

Manual review

Recommended Mitigation Steps

The totalAssets function returns the actual contract balance of pxGMX as follows:

function totalAssets() public view override returns (uint256) {
        return asset.balanceOf(address(this));
    }

Keep track of the total assets in a state variable instead of relying on 'balanceOf' which can be affected externally .

Another option (not very convenient though) is to set a maximum for deposit. If the user wants to deposit more, then he/she has to do it in multiple transactions.

Lines of code

https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/PirexRewards.sol#L373-L417

Vulnerability details

Impact

If reward tokens array happen to be empty when the user calims, he/she loses the rewards. That means, the user rewards is set to zero even if he/she doesn't receive any rewards (e.g. WETH or pxGMX).

Proof of Concept

To calim rewards, the user would call the function claim in PirexRewards.sol.

(https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/PirexRewards.sol#L373-L417)

The function: 1- resets the user rewards to 0

	 ERC20[] memory rewardTokens = p.rewardTokens;
     uint256 rLen = rewardTokens.length;

     // Update global and user reward states to reflect the claim
     p.globalState.rewards = globalRewards - userRewards;
     p.userStates[user].rewards = 0;

     emit Claim(producerToken, user);

2- goes over the rewardTokens array of the producer token calculating the rewards amount for each token, and then calls claimUserReward function of the producer PirexGmx.sol.

 // Transfer the proportionate reward token amounts to the recipient
   for (uint256 i; i < rLen; ++i) {
     ERC20 rewardToken = rewardTokens[i];
     address rewardRecipient = p.rewardRecipients[user][rewardToken];
     address recipient = rewardRecipient != address(0)
                    ? rewardRecipient
                    : user;
     uint256 rewardState = p.rewardStates[rewardToken];
                uint256 amount = (rewardState * userRewards) / globalRewards;

     if (amount != 0) {
        // Update reward state (i.e. amount) to reflect reward tokens transferred out
        p.rewardStates[rewardToken] = rewardState - amount;

                    producer.claimUserReward(
                        address(rewardToken),
                        amount,
                        recipient
                    );
                }
            }

If the rewards tokens array happen to be empty then the for-loop would be skipped.

An example of when this would happen, if the owner of PirexRewards.sol is adding or removing rewardsToken via addRewardToken and removeRewardToken functions as it is possible to migrate the producer PirexGmx.sol to a new one.

Tools Used

Manual review

Recommended Mitigation Steps

In the function claim, revert if the rewardsToken array is empty.

Example:

   ERC20[] memory rewardTokens = p.rewardTokens;
   uint256 rLen = rewardTokens.length;

   	// this is the new line added
   	if (rLen == 0) revert RewardTokensEmpty();