Curve price calculation is manipultable due to read only reentrancy

[hats-bug-reporter[bot]]

Github username: @abhishekvispute Submission hash (on-chain): 0x3493f3d0fc03c6b5e255a995df2cbb3fc747d107a49d6d7d29f819a21f749724 Severity: high

Description: Description\ CVG is currently using last prices of curve to derive the price. However it fails to check if vault is in active trade or not, by doing a reentrancy checks. This allows attackers to reenter the curve pool, and use the in between state for price.

Attack Scenario\ Swap in any curve pool handling natve ETH And do action on CVG

Recommedation Consider checking reentrancy before Example implementation: https://github.com/VMEX-finance/vmex/blob/master/packages/contracts/contracts/protocol/oracles/CurveOracle.sol

[0xcuriousapple]

Associated code https://github.com/hats-finance/Convergence-Finance---IBO-0x0e410e7af8e70fc5bffcdbfbdf1673ee7b3d0777/blob/f43c5d9bc6b30c9f488e34836f09dc04d8f7361f/contracts/Oracles/CvgOracle.sol#L141-L142

   function _getCrvPoolPrice(
        address crvPool,
        bool isReversed,
        bool isEthPriceRelated
    ) internal view returns (uint256, bool) {
        return _postTreatmentAndVerifyEth(ICrvPool(crvPool).last_prices(), isReversed, isEthPriceRelated);
    }

[0xcuriousapple]

Hmm, you dont even need to reenter, I think the call of .last_prices() itself is wrong Please check this code of tri-crypto pool, https://etherscan.deth.net/address/0xd51a44d3fae010294c616388b506acda1bfaae46

you will see the last prices being updated in each liquidity action. (swap, add, remove) This is simple spot price manipulation you should consider using price_oracle() instead

https://resources.curve.fi/factory-pools/understanding-oracles/#exponential-moving-average

def price_oracle(k: uint256) -> uint256:
    return self._packed_view(k, self.price_oracle_packed)

[0xcuriousapple]

I see now that you have safeguards implemented in place by double-checking prices derived from uniswap or curve with chainlink.

      IOracleStruct.OracleParams memory oracleParams = oracleParametersPerERC20[erc20Address];
        (uint256 poolOraclePrice, bool isEthVerified) = oracleParams.isStable
            ? (10 ** 18, true)
            : _getPriceOracle(erc20Address);
        (uint256 aggregatorOraclePrice, uint256 lastUpdateDate) = _getPriceAggregator(oracleParams.aggregatorOracle);
        uint256 delta = (oracleParams.deltaAggregatorCvgOracle * poolOraclePrice) / 10_000;

        require(poolOraclePrice + delta > aggregatorOraclePrice, "ORACLE_TO_LOW");
        require(poolOraclePrice - delta < aggregatorOraclePrice, "ORACLE_TO_HIGH");
        require(isEthVerified, "ETH_NOT_VERIFIED");

Even though your implementations are wrong, this decreases the severity of this issue Since the impact at max could be delta

Apologies for the incorrect severity classification. However, you should consider fixing this anyway. Reliance on last prices is not recommended, its equivalent to spot reserves Consider reducing the severity of this issue to medium or low.

[0xR3vert]

Hello, Thanks a lot for your attention. We have designed the oracle delta check with the last_price purposely, so it's assumed on our side. In our oracle contract, we are allowing a delta difference that protects us from liquidity attack. In conclusion we have so to consider this issue as invalid.