Potential Reentrancy Vulnerability in EMA Oracle Pallet

[c4-bot-5]

Lines of code

https://github.com/code-423n4/2024-02-hydradx/blob/main/HydraDX-node/pallets/ema-oracle/src/lib.rs#L243-L253 https://github.com/code-423n4/2024-02-hydradx/blob/main/HydraDX-node/pallets/ema-oracle/src/lib.rs#L256-L265 https://github.com/code-423n4/2024-02-hydradx/blob/main/HydraDX-node/pallets/ema-oracle/src/lib.rs#L284-L295

Vulnerability details

Impact

The contract may be susceptible to reentrancy attacks, potentially allowing an attacker to manipulate the state during external calls and perform malicious actions. In the context of the EMA Oracle Pallet, the vulnerable functions are on_trade and on_liquidity_changed.

Proof of Concept

file: HydraDX-node/pallets/ema-oracle/src/lib.rs

on_trade Function

    pub(crate) fn on_trade(
        src: Source,
        assets: (AssetId, AssetId),
        oracle_entry: OracleEntry<BlockNumberFor<T>>,
    ) -> Result<Weight, (Weight, DispatchError)> {
        let weight = OnActivityHandler::<T>::on_trade_weight();
        Self::on_entry(src, assets, oracle_entry)
            .map(|_| weight)
            .map_err(|_| (weight, Error::<T>::TooManyUniqueEntries.into()))
    }

Example vulnerability: Assume an external call is made to a malicious contract within this function. The malicious contract performs an unexpected reentry, affecting the state of the EMA Oracle. This can potentially lead to inaccurate or manipulated oracle values.

Proof of Concept (simplified):

maliciousContract.callOnTrade();

on_liquidity_changed Function

    pub(crate) fn on_liquidity_changed(
        src: Source,
        assets: (AssetId, AssetId),
        oracle_entry: OracleEntry<BlockNumberFor<T>>,
    ) -> Result<Weight, (Weight, DispatchError)> {
        let weight = OnActivityHandler::<T>::on_liquidity_changed_weight();
        Self::on_entry(src, assets, oracle_entry)
            .map(|_| weight)
            .map_err(|_| (weight, Error::<T>::TooManyUniqueEntries.into()))
    }

Example vulnerability: Similar to on_trade, if an external call is made to a malicious contract within this function, and the malicious contract performs an unexpected reentry, it may impact the EMA Oracle's state.

Proof of Concept (simplified):

maliciousContract.callOnLiquidityChanged();

Tools Used

Manual code review.

Recommended Mitigation Steps

Review and enhance the on_trade and on_liquidity_changed functions to minimize state changes during external calls. Ensure that critical state changes are made after external calls to prevent potential reentrancy attacks. Consider using reentrancy guards, such as the checks-effects-interactions pattern, to mitigate reentrancy vulnerabilities. Perform additional testing and code review to identify and address any other potential reentrancy risks.

Assessed type

Reentrancy

[0xRobocop]

Assume an external call is made to a malicious contract within this function.

No evidence for the above claim.

[c4-pre-sort]

0xRobocop marked the issue as insufficient quality report

[c4-sponsor]

enthusiastmartin (sponsor) disputed

[enthusiastmartin]

It is not possible to deploy custom code for attackers.

Therefore, it is no valid unless we made a mistake. But there is no such evidence.

[c4-judge]

OpenCoreCH marked the issue as unsatisfactory: Insufficient proof