Redacted Cartel contest - simon135's results

Lines of code

https://github.com/code-423n4/2022-11-redactedcartel/blob/684627b7889e34ba7799e50074d138361f0f532b/src/vaults/AutoPxGmx.sol#L321

Vulnerability details

Impact

when the router is going to the trade from baseGmxReward to Gmx the slippage set is 1 which is saying we allow the trade to take 99% percent slippage Also a similar thing with sqrtPriceLimitX96

---

Uniswap Docs

amountOutMinimum: we are setting it to zero, but this is a significant risk in production. For a real deployment, this value should be calculated using our SDK or an on-chain price oracle - this helps protect against getting an unusually bad price for a trade due to a front-running sandwich or another type of price manipulation sqrtPriceLimitX96: We set this to zero - which makes this parameter inactive. In production, this value can be used to set the limit for the price the swap will push the pool to, which can help protect against price impact or for setting up logic in a variety of price-relevant mechanisms.

Proof of Concept

     */
    function compound(
        uint24 fee,
        uint256 amountOutMinimum,
        uint160 sqrtPriceLimitX96,
        bool optOutIncentive
    )
        public
        returns (
            uint256 gmxBaseRewardAmountIn,
            uint256 gmxAmountOut,
            uint256 pxGmxMintAmount,
            uint256 totalFee,
            uint256 incentive
        )
    {
        if (fee == 0) revert InvalidParam();
        if (amountOutMinimum == 0) revert InvalidParam();

        uint256 assetsBeforeClaim = asset.balanceOf(address(this));

        PirexRewards(rewardsModule).claim(asset, address(this));

        // Swap the entire reward balance for GMX
        gmxBaseRewardAmountIn = gmxBaseReward.balanceOf(address(this));

        if (gmxBaseRewardAmountIn != 0) {
            gmxAmountOut = SWAP_ROUTER.exactInputSingle(
                IV3SwapRouter.ExactInputSingleParams({
                    tokenIn: address(gmxBaseReward),
                    tokenOut: address(gmx),
                    fee: fee,
                    recipient: address(this),
                    amountIn: gmxBaseRewardAmountIn,
                    amountOutMinimum: amountOutMinimum,
                    sqrtPriceLimitX96: sqrtPriceLimitX96
                })
            );

As you can see slippage and sqrtPrice is user supplied but where most users call them the args are not user-supplied

    function withdraw(
        uint256 assets,
        address receiver,
        address owner
    ) public override returns (uint256 shares) {
        // Compound rewards and ensure they are properly accounted for prior to withdrawal calculation
        compound(poolFee, 1, 0, true);

here the slippage is set to 1 and sqrtPrice is 0 which is bad because the user can be sandwiched and the contract will lose funds. There will be fewer funds to give out as rewards which can be bad for the protocol.

Recommended Mitigation Steps

make it user-supplied or make it the same or close the same amount of what is withdrawn or deposited etc. Also, do the same for sqrtPrice which can also help get the most amount of funds in the contract

make these checks into a single modifier to make the code more readable:

        if (_pxGmx == address(0)) revert ZeroAddress();
        if (_pxGlp == address(0)) revert ZeroAddress();
        if (_pirexFees == address(0)) revert ZeroAddress();
        if (_pirexRewards == address(0)) revert ZeroAddress();
        if (_delegateRegistry == address(0)) revert ZeroAddress();
        if (_gmxBaseReward == address(0)) revert ZeroAddress();
        if (_gmx == address(0)) revert ZeroAddress();
        if (_esGmx == address(0)) revert ZeroAddress();
        if (_gmxRewardRouterV2 == address(0)) revert ZeroAddress();
        if (_stakedGlp == address(0)) revert ZeroAddress();

instead, make it into a modifier

modifer nonzero(adddress t) {
if(t== address(0)) revert ZeroAddress();
}

rename the assets var to amount for readability

  function _computeAssetAmounts(Fees f, uint256 assets)

make into uint256 amount :219L: PirexGmx.sol

rename the postFeeAmount and feeAmount

it's very confusing because the postFeeAmount is the real fee amount and feeAmount is just the fee subtracted instead: feeAmount= the fee postFeeAmount the amount-fee

add parameters to beforeDeposit to stop huge slippage loss and to make it more useable

        if (totalAssets() != 0) beforeDeposit(address(0), 0, 0);

https://github.com/code-423n4/2022-11-redactedcartel/blob/684627b7889e34ba7799e50074d138361f0f532b/src/vaults/AutoPxGmx.sol#L383

if distrbutorbalance= 0 then rewards won't be processed even though there are rewards that need to get processed

     uint256 blockReward = pendingRewards > distributorBalance
            ? distributorBalance
            : pendingRewards;
        uint256 precision = r.PRECISION();
        uint256 cumulativeRewardPerToken = r.cumulativeRewardPerToken() +
            ((blockReward * precision) / r.totalSupply());

so if pendingRewards > distributorBalance The amount is distributorBalance instead of the actual pending amounts ex:

pendingRewards= 1ether distributorBalance=0 The rewards will be 0 and no rewards will be distributed because of the ternary operator wrong use instead, make the calculation into

blockReward=pendingRewards;
        uint256 cumulativeRewardPerToken = r.cumulativeRewardPerToken() +
            ((blockReward * precision) / r.totalSupply());

small amount of tokens won't be distributed

    function distributeFees(ERC20 token) external {
        uint256 distribution = token.balanceOf(address(this));
        uint256 treasuryDistribution = (distribution * treasuryFeePercent) /
            FEE_PERCENT_DENOMINATOR;
        uint256 contributorsDistribution = distribution - treasuryDistribution;
		

if distribution is less than FEE_PERCENT_DENOMINATOR then it will be zero and be a waste of gas by the caller add a zero check to make sure it's not zero.

make sure the total supply can be less than uint256 because for some tokens this can cause an issue

   globalState.lastSupply = totalSupply.safeCastTo224();

the function will revert if totalPxGlpFee is less than the Incentive which is possible with the calculations

    uint256 newAssets = totalAssets() - preClaimTotalAssets;
        if (newAssets != 0) {
            totalPxGlpFee = (newAssets * platformFee) / FEE_DENOMINATOR;
            pxGlpIncentive = optOutIncentive
                ? 0
                : (totalPxGlpFee * compoundIncentive) / FEE_DENOMINATOR;

            if (pxGlpIncentive != 0)
                asset.safeTransfer(msg.sender, pxGlpIncentive);
            asset.safeTransfer(owner, totalPxGlpFee - pxGlpIncentive);
        }

approve all should be used it can cause risk of hacks and loss of funds, on the front end approve how much users need

        gmxBaseReward.safeApprove(address(SWAP_ROUTER), type(uint256).max);
        gmx.safeApprove(_platform, type(uint256).max);

https://github.com/code-423n4/2022-11-redactedcartel/blob/684627b7889e34ba7799e50074d138361f0f532b/src/vaults/AutoPxGmx.sol#L98-L99

if time passes between states then more tokens will cause a more significant balance just make sure it's updated by a right way and that rewards are not updated too much

        uint256 rewards = u.rewards +
            u.lastBalance *
            (block.timestamp - u.lastUpdate);
        u.lastUpdate = block.timestamp.safeCastTo32();
        u.lastBalance = balance.safeCastTo224();
        u.rewards = rewards;

certain times these calculations can revert causing issues

ex: rewardState= 1e18 userRewards=100 globalRewards=10

(1e18 *100) / 10= 1e19

it can really not work when the tokens are going through a loop and the amounts get bigger causing a revert at the end of the loop and making the function revert and rewards won't work.

        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;

https://github.com/code-423n4/2022-11-redactedcartel/blob/684627b7889e34ba7799e50074d138361f0f532b/src/PirexRewards.sol#L418 make sure the calculation is good and make a limit for how many rewardTokens you can loop through

centralization risk, the minter and burner can mint any number of tokens and burn any number of tokens

https://github.com/code-423n4/2022-11-redactedcartel/blob/684627b7889e34ba7799e50074d138361f0f532b/src/PxERC20.sol#L46