hats-finance / Convergence---Convex-integration-0xb3df23e155b74ad2b93777f58980d6727e8b40bb

0 stars 1 forks source link

`CvgCvxStakingPositionService.sol#Deposit` conversion logic `_convertCvxToCvgCvx` is completely bricked. #55

Open hats-bug-reporter[bot] opened 6 months ago

hats-bug-reporter[bot] commented 6 months ago

Github username: -- Twitter username: -- Submission hash (on-chain): 0xe7417261b7f219e97402d2e9be528c105c2cc24ea745fb41bc49d5a6f3bb746e Severity: high

Description: Description\ The method used for converting CVX tokens to CVGCVX is incorrectly divides((cvxAmount - feesAmount) * depegPercentage) with 1000 ( THOUSAND) instead of 100 ( HUNDRED)

although the constant name is hundred but the value is used is 1000

   /// @dev internal constant used for divisions
    uint256 internal constant HUNDRED = 1_000;

which leaves this calculation to be always incorrect when ((cvxAmount - feesAmount) * depegPercentage)/100 is supposed to be greater than or equal to left side of the equation _curvePool.get_dy(0, 1, cvxAmount)

  if (_curvePool.get_dy(0, 1, cvxAmount) > ((cvxAmount - feesAmount) * depegPercentage) / HUNDRED) {

this will lead to incorrect calculations and potentially and potentially minting and exchanging incorrect amount of tokwns .


   function _convertCvxToCvgCvx(
        uint256 cvxAmount,
        uint256 minCvgCvxAmountOut,
        bool isEthDeposit
    ) internal returns (uint256) {
        uint256 balance = cvxConvergenceLocker.balanceOf(address(this));
        if (!isEthDeposit) {
            /// @dev transfer CVX from user to this contract
            CVX.transferFrom(msg.sender, address(this), cvxAmount);
        }

        /// @dev Acts as a peg keeper and will prefers swap in the liquid pool in case of a depeg
        uint256 feesAmount = (cvxAmount * cvxConvergenceLocker.mintFees()) / 10_000;
        ICrvPoolPlain _curvePool = curvePool;
@audit issue here , value of HUNDRED should be 100 and not 1000 
        if (_curvePool.get_dy(0, 1, cvxAmount) > ((cvxAmount - feesAmount) * depegPercentage) / HUNDRED) {
            require(minCvgCvxAmountOut >= cvxAmount, "INVALID_SLIPPAGE");

            /// @dev peg is too low, we swap in the LP with the CVX sent
            cvx1.mint(address(this), cvxAmount);
            _curvePool.exchange(0, 1, cvxAmount, minCvgCvxAmountOut, address(this));
        } else {
            /// @dev peg OK, we pass through the mint process 1:1
            cvxConvergenceLocker.mint(address(this), cvxAmount, false);
        }

        return cvxConvergenceLocker.balanceOf(address(this)) - balance;
    }

Attack Scenario Whenever someone initiates the deposit transaction inside CvgCvxStakingPositionService.sol , the deposit function calls


 function _deposit(
        uint256 tokenId,
        uint256 cvgCvxAmount,
        DepositCvxData memory cvxData,
        bool isEthDeposit
    ) internal {

//snip

  /// @dev convert CVX to cvgCVX if amount is specified
        if (cvxData.amount != 0) {
            totalAmount += _convertCvxToCvgCvx(cvxData.amount, cvxData.minAmountOut, isEthDeposit);
        }

//snip

}

the conversion from the target function will be incorrect minting more or less CVX1 than intended . This issue escalates when the incorrect amount of assets is exchanged through curve pools

This can also lead to Denial of service of deposit and other functionalities relying on this function due to incorrect parameters [assed for curve pool exchange function and the pool won't have enough funds and rhe transaction might revert in most cases

  /// @dev peg is too low, we swap in the LP with the CVX sent
            cvx1.mint(address(this), cvxAmount);
            _curvePool.exchange(0, 1, cvxAmount, minCvgCvxAmountOut, address(this));

Attachments

  1. Proof of Concept (PoC) File Please check the detail section

  2. Revised Code File (Optional) modify HUNDRED variable as follows

/// @dev internal constant used for divisions uint256 internal constant HUNDRED = 100;

PlamenTSV commented 6 months ago

The variable is named HUNDRED because it represents 100% The depeg percentage is defined as 1025. This way the calculation becomes cvxAmount*1025/1000, effectively increasing the amount by 2.5% to check for the correct slippage. If the cvxAmount is 1000, 2.5% depeg slippage would give it 1025. Nothing wrong with the code