CVX staking contract's deposit and withdraw functions should use deadline

[hats-bug-reporter[bot]]

Github username: @0xRizwan Twitter username: 0xRizwann Submission hash (on-chain): 0xe9b2057d4db315f16178206cba0d17135e357a60e15d3f1dc18828b4622ac9c5 Severity: medium

Description: Description\ CvxAssetStakingService.sol and CvgCvxStakingPositionService.sol are the contracts which are in charge of registering deposits and withdraws of staking position. Both of these contracts inherits from the StakingServiceBase which implements all the staking logic. Any user can own a staking position by depositing ETH, CVX or cvgCVX tokens depending on both contracts.

The deposit and withraw functions in both contracts does not use deadline and performs the token swaps via curve pools.

The different token swaps via curve pools implemented in deposit and withdraw functions in both contracts can be checked below:

1) In CvxAssetStakingService.sol of deposit() function at L-251

2) In CvgCvxStakingPositionService.sol of deposit() function at L-272

3) In CvgCvxStakingPositionService.sol of withdraw() function at L-185 and L-191

Missing the deadline while swapping the tokens in both deposit and withdraw functions can have severe impacts. deadline is a useful tool to ensure that transaction can not be “saved for later”. It should be noted that, the contracts would be deployed on Ethereum mainnet.

Due to missing deadline check in deposit and withdraw functions, it may be more profitable for a validator to deny the transaction from being added until the transaction incurs the maximum amount of slippage.

Consider below scenario to understand the importance of deadline in Swap mechanisms:

Let's take, CvgCvxStakingPositionService.withdraw() function,

    function withdraw(
        uint256 tokenId,
        uint256 amount,
        TOKEN_TYPE tokenType,
        uint256 minCvx1AmountOut
    ) external checkCompliance(tokenId) {
        require(amount != 0, "WITHDRAW_LTE_0");

        uint256 _cvgStakingCycle = stakingCycle;

        /// @dev Update the CycleInfo & the TokenInfo for the current & next cycle
        _updateAmountStakedWithdraw(tokenId, amount, _cvgStakingCycle);

        /// @dev transfer tokens back to user depending on the selected option
        if (tokenType == TOKEN_TYPE.CVX) {
            ICvx1 _cvx1 = cvx1;

            /// @dev swap cvgCVX to CVX1 through the Curve pool
            uint256 exchangedAmount = curvePool.exchange(1, 0, amount, minCvx1AmountOut, address(this));

            /// @dev withdraw CVX from CVX1 contract
            _cvx1.withdraw(exchangedAmount, msg.sender);

       @audit // focus this part in below explained scenario
       . . . . . . .  . . . . . . . . . . . . . . .. . .  . . . . . . .  ..  .. . . . . . . . . . . . . . . . .  .
        } else if (tokenType == TOKEN_TYPE.CVX1) {
            /// @dev swap cvgCVX to CVX1 through the Curve pool and directly send tokens to user
            curvePool.exchange(1, 0, amount, minCvx1AmountOut, msg.sender);
       . . . .  . . ..  .. . . . . . .  .    . . . . . . . . . . . . . . . . . . . . . . .. . . . . . . . .  . . . . . 

        } else {
            /// @dev Transfers cvgCVX back to user
            cvxConvergenceLocker.transfer(msg.sender, amount);
        }

        emit Withdraw(tokenId, msg.sender, _cvgStakingCycle, amount);
    }

1) Alice wants to withdraw her cvgCVX tokens and she calls CvgCvxStakingPositionService.withdraw() with all arguments and sets tokenType as CVX1 means she want to withdraw the CVX1 tokens to her wallet address. She has set minCvx1AmountOut as slippage protection so at atleast this much amount she is expected to receive.

2) Alice sends a withdraw transaction to the mempool, but with a very low gas fee.

3) Miners/validators see the transaction but the fee is not attractive, so the transaction is stale and pending for a long time by validators as they dont find it profitable to include her transaction.

4) Let's say, After two days or a week, the average gas fees drop low enough for the miners/validators to execute the transaction but the price of the assets has changed drastically.

5) Now the value Alice receives is much lower and possibly close to the max slippage she set.

6) Say, Alice had set minCvx1AmountOut as 100 but due to hold of her transaction the there was surge in cvgCVX to CVX1 swaps and CVX1 tokens would have been, say 120. This hold of Alice transaction will create her loss of 20 tokens which is her loss as user due to missing deadline functionality so Alice would receive 100 CVX1 tokens instead of 120 CVX1 tokens if she had used deadline in withdraw function.

This would create loss of users due to crypto market volatility due to pending transaction. An exact opposite scenario, i.e Profit for Alice can be understood.

Recommendation\ Use deadline param in deposit and withdraw function as highlighted above.

For example understanding, consider below changes:

+    /* =-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=--=-=-=-=
+                                           MODIFIERS 
+    =-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=--=-=-=-= */

+    /**
+     * @notice Modifier to check deadline against current timestamp
+     * @param deadline latest timestamp to accept this transaction
+     */
+    modifier deadlineCheck(uint256 deadline) {
+        require(block.timestamp <= deadline, "Deadline expired!");
+        _;
+    }

    function withdraw(
        uint256 tokenId,
        uint256 amount,
        TOKEN_TYPE tokenType,
        uint256 minCvx1AmountOut,
+      uint256 deadline
-    ) external checkCompliance(tokenId) {
+    ) external deadlineCheck(deadline) checkCompliance(tokenId) {
        require(amount != 0, "WITHDRAW_LTE_0");

        uint256 _cvgStakingCycle = stakingCycle;

        /// @dev Update the CycleInfo & the TokenInfo for the current & next cycle
        _updateAmountStakedWithdraw(tokenId, amount, _cvgStakingCycle);

        /// @dev transfer tokens back to user depending on the selected option
        if (tokenType == TOKEN_TYPE.CVX) {
            ICvx1 _cvx1 = cvx1;

            /// @dev swap cvgCVX to CVX1 through the Curve pool
            uint256 exchangedAmount = curvePool.exchange(1, 0, amount, minCvx1AmountOut, address(this));

            /// @dev withdraw CVX from CVX1 contract
            _cvx1.withdraw(exchangedAmount, msg.sender);
        } else if (tokenType == TOKEN_TYPE.CVX1) {
            /// @dev swap cvgCVX to CVX1 through the Curve pool and directly send tokens to user
            curvePool.exchange(1, 0, amount, minCvx1AmountOut, msg.sender);
        } else {
            /// @dev Transfers cvgCVX back to user
            cvxConvergenceLocker.transfer(msg.sender, amount);
        }

        emit Withdraw(tokenId, msg.sender, _cvgStakingCycle, amount);
    }

[0xRizwan]

Curve also recommends to use deadline for the above described issue. This can be checked here

def exchange_underlying(i: int128, j: int128, dx: uint256, min_dy: uint256):

This method exchanges dx of underlying token i into token j. You specify min_dy as the minimal amount of token j to get to avoid front-running. You can also use deadline to make transaction revert if it wasn't accepted for too long.

[PlamenTSV]

Depositing swaps can happen through uniswap and they feature a built-in deadline which is utilized. Curve does not implement it and only recommends it, this could be informational since the slippage prevents the user from being on a loss. On the mainnet block-stuffing is too high cost and the scenario for all validators to skip our transaction is not realistic

[0xRizwan]

@PlamenTSV

Depositing swaps can happen through uniswap and they feature a built-in deadline which is utilized.

The highlighted instances have used curve pool exchange for swaps and the issue is not related with uniswap deadline.

I have described the user would face issues due lack of deadline check either with opportunity of profit or vise versa. I think for this issue, judgement similar to #1 should be applicable here. Both issues main intention is to provide the user specified deadline to prevent the issue. #1 issue is with uniswap and this one is focussed on curve pools. The curve pool recommendation shouldn't be ignored as you can see there is more benefit in implementing the deadline check than surely happening of this issue in volatile markets.

[rodiontr]

@PlamenTSV not an issue, "You can also use deadline to make transaction revert if it wasn't accepted for too long".

"you can also use" != "must use" hence it's not a vulnerability

[PlamenTSV]

The fix is the same and it could be interpreted as dupe, will remove labeling and let sponsor decide if this is a dupe of #1, an invalid or a standalone issue

[shalbe-cvg]

Hello,

Thank you for reporting this issue. This issue closely resembles to issue #1 which propose to have a deadline on a swap. Even though Curve recommends to put one, this is unlikely to happen on Ethereum mainnet since validators are trusted entities.

Regards.