Users can MAKE EMA-Oracle price outdated with direct transfers to StableSwap

[c4-bot-2]

Lines of code

https://github.com/code-423n4/2024-02-hydradx/blob/main/HydraDX-node/runtime/hydradx/src/system.rs#L65-L87

Vulnerability details

Bug Description

The EMA oracle, designed to utilize HydraDX's Omnipools and StableSwap for exchange rate information, operates by monitoring activities within these liquidity pools. It looks for specific operations like exchanges, deposits, and withdrawals to adjust the assets' exchange rates accordingly. This updating process is not continuous but occurs when the responsible hooks are called by the StableSwap/Omnipool

The system, although thorough, does not account for price update triggers in the event of direct asset transfers to Stableswap, as these do not set off any hooks within the oracle. This lapse means that such direct transfers can alter asset prices within the liquidity pools without the oracle's knowledge, potentially leading to misleading exchange rates.

Moreover, there's a risk of manipulation by bad actors who might use direct transfers to StableSwap in an effort to sway the arbitrage process, especially during periods of network congestion. Such interference could unjustly prevent necessary liquidations within lending protocols.

Impact

The issue allows a malicious user to change the price of the AMM without updating the oracle.

Proof of Concept

The issue can be validated when looking at the runtime configuration. The configuration only restricts transfers to the Omnipool address, but not to the StableSwap address.

// filter transfers of LRNA and omnipool assets to the omnipool account
if let RuntimeCall::Tokens(orml_tokens::Call::transfer { dest, currency_id, .. })
| RuntimeCall::Tokens(orml_tokens::Call::transfer_keep_alive { dest, currency_id, .. })
| RuntimeCall::Tokens(orml_tokens::Call::transfer_all { dest, currency_id, .. })
| RuntimeCall::Currencies(pallet_currencies::Call::transfer { dest, currency_id, .. }) = call
{
	// Lookup::lookup() is not necessary thanks to IdentityLookup
	if dest == &Omnipool::protocol_account() && (*currency_id == hub_asset_id || Omnipool::exists(*currency_id))
	{
		return false;
	}
}
// filter transfers of HDX to the omnipool account
if let RuntimeCall::Balances(pallet_balances::Call::transfer { dest, .. })
| RuntimeCall::Balances(pallet_balances::Call::transfer_keep_alive { dest, .. })
| RuntimeCall::Balances(pallet_balances::Call::transfer_all { dest, .. })
| RuntimeCall::Currencies(pallet_currencies::Call::transfer_native_currency { dest, .. }) = call
{
	// Lookup::lookup() is not necessary thanks to IdentityLookup
	if dest == &Omnipool::protocol_account() {
		return false;
	}
}

Tools Used

Manual Review

Recommended Mitigation Steps

The issue can be mitigated by disabling transfers to the StableSwap pools, similar to how it is implemented for the Omnipool.

Assessed type

Oracle

[c4-pre-sort]

0xRobocop marked the issue as sufficient quality report

[c4-pre-sort]

0xRobocop marked the issue as primary issue

[c4-sponsor]

enthusiastmartin (sponsor) disputed

[enthusiastmartin]

We believe this is not an issue, impact is not obvious. Oracle is not guaranteed to be always correct.

[OpenCoreCH]

The finding itself is valid, but only speculates about potential impacts ("potentially leading to misleading exchange rates."). Because of that, it is a design recommendation -> QA.

[c4-judge]

OpenCoreCH changed the severity to QA (Quality Assurance)

[c4-judge]

OpenCoreCH marked the issue as grade-a

[J4X-98]

Hi @OpenCoreCH ,

thank you very much for reviewing this contest. I am sorry that my issue was a bit inconclusive about the severity/results of this. The same impact (but for omnipool) has already been found in one of the earlier audits at Finding A1, this is why I kept my issue intentionally rather short, which was suboptimal in hindsight.

The oracle should always return the current price of the assets at stableswap/omnipool. As identified in the other audit this could be broken for the Omnipool by transferring assets directly to the Omnipool, making the price outdated. This was confirmed as a medium severity finding by the sponsor and fixed by implementing guards that blocked any transfers of tokens directly to the Omnipool. Unfortunately, the team has forgotten to implement the same safety measures when the StableSwap AMM was added to the protocol. As a result of this, the attack path is once again possible for all assets listed on StableSwap.

While I have not described an attack path that leads to an attacker profiting from this (which might be possible), the "attack" path of donating to make the oracle outdated, that I have described shows a way how the oracle becomes outdated which should never be the case. To keep it simple, this leads to one of the components of the protocol not functioning as intended (the oracle returning a wrong price) leading to the damage scenario of "Assets not at direct risk, but the function of the protocol impacted".

Additionally i would like to add that finding #73 leads to the exact same damage scenario, the oracle returning an incorrect price for an asset, and has been confirmed as medium severity.

[OpenCoreCH]

Keeping at QA because of missing impact / attack path. This might lead to problems in the protocol and be a valid medium or high then, but the issue does not demonstrate that.

#73 mentions a potential impact (third-party protocols) that has external requirements, but is still valid nevertheless.

[J4X-98]

Hi @OpenCoreCH,

thanks for your review but I must tell you that I disagree with you differentiating between this issue and #73. They both result in the exact same state of the oracle returning an incorrect price for an asset.

Additionally, you mention that #73 offers an impact while this one does not. The impact that #73 describes is "So any protocol that uses this oracle as a price source would receive an incorrect price for the re-added asset for a short period of time." which can be shortened down to "external protocols relying on this oracle might break". My issue describes " Such interference could unjustly prevent necessary liquidations within lending protocols." which can also be shortened to "external protocols relying on this oracle might break too". It is just more focussed on lending protocols as this is the first thing that came to mind for me.

I'd kindly ask you to reconsider this and let me know your final judgment.

[OpenCoreCH]

That's a good point, I previously missed the mention of integration with other protocols. Because a potential realistic impact with external requirements is mentioned and the finding itself is valid, I am upgrading it to medium.

[c4-judge]

This previously downgraded issue has been upgraded by OpenCoreCH

[c4-judge]

OpenCoreCH marked the issue as selected for report