c4-bot-6 commented 6 months ago

Lines of code

https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/pallets/omnipool/src/lib.rs#L447-L549

Vulnerability details

Impact

The function add_token expects the caller to provide an initial_price for the token being added. This price is used to calculate the amount of hub tokens to mint to the pool.

The price to be used is the LRNA-tokenA price, assuming tokenA is being added. This price can be calculated by the caller by using the other tokens (say tokenB) in the pool. We assume that tokenA-tokenB price is already available from CEXs or other DEXs, and the LRNA-tokenB price is available from the pool spot price or the EMA oracle.

The issue is that this initial_price can be abused with a sandwich attack to extract value from the pool. A malicious user can frontrun tokenA addition by seeling LRNA tokens for tokenB, devaluing the LRNA tokens, and then swapping back the tokenB for tokenA after the addition. This would result in profit for the attacker.

Since it is impossible to institute slippage control in the pool (like done with ensure_price) since the tokenA doesnt even exist in the pool yet, this is a potential issue.

Proof of Concept

The attack can be carried out in the following steps:

ALICE sells LRNA tokens for tokenB.
Admin calls add_token with tokenA.
Alice sells the acquired tokenB from step 1 for tokenA.
Alice is shown to profit from the attack.

For simplicity, lets assume that all tokens (tokenA, tokenB, LRNA) are equal and worth 1 usd.

Initially, the pool is assumed to have 20 tokenB, and 20 LRNA tokens corresponding to the 20 tokenB since they are all valued at the same price.

In the below table, the assets of ALICE are tracked in columns 2-4, and the assets of the pool are tracked in columns 5-8. The pool's LRNA tokens are tracked in columns 6 and 7, and they contain the corresponding LRNA tokens for tokenA and tokenB respectively.

pA is the spot price of LRNA-tokenA, calculated by column_6/column_5. pB is the spot price of LRNA-tokenB, calculated by column_7/column_8. pAB is the spot price of tokenA-tokenB, calculated pB/pA.

The initial condition of the pool looks like so: Assuming there are 20 tokenB in the pool, and no tokenA.

Operation	ALICE's tokenA	ALICE's LRNA	ALICE's tokenB	Pool's tokenA	Pool's LRNA for tokenA	Pool's LRNA for tokenB	Pool's tokenB	pA	pB	pAB
Initial	20	20	20	0	0	20	20	-	1	-

pA and pAB cannot be calculated yet since theres no liquidity.

Now say Alice sells 5 LRNA tokens for tokenB. There will now be 25 LRNA corresponding to tokenB, and only 16 tokenB in the pool according to the x*y=k invariant.

Operation	ALICE tokenA	ALICE LRNA	ALICE tokenB	Pool tokenA	Pool LRNA for tokenA	Pool LRNA for tokenB	Pool tokenB	pA	pB	pAB
Initial	20	20	20	0	0	20	20	-	1	-
swap LRNA->B	20	15	24	0	0	25	16	-	1.5625	-

Next, the admin adds tokenA. Since they are unaware of the previous swap which was a frontrunning attack, they assume the price of A is still 1. They add 30 tokenA with a price of 1, so 30 LRNA tokens are minted corresponding to the tokenA position.

Operation	ALICE tokenA	ALICE LRNA	ALICE tokenB	Pool tokenA	Pool LRNA for tokenA	Pool LRNA for tokenB	Pool tokenB	pA	pB	pAB
Initial	20	20	20	0	0	20	20	-	1	-
swap LRNA->B	20	15	24	0	0	25	16	-	1.5625	-
add_token	20	15	24	30	30	25	16	1	1.5625	1.5625

Next, ALICE turn around and sells the extra 4 tokenB she earned from the first swap and exchanges it for tokenA. This A-B swap again happens following the x*y=k invariant, and the LRNA assets in the pool remain the same. So the final tokenB in the pool grows back to 20 tokens, and 6 tokenA is paid out following the invariant.

Operation	ALICE tokenA	ALICE LRNA	ALICE tokenB	Pool tokenA	Pool LRNA for tokenA	Pool LRNA for tokenB	Pool tokenB	pA	pB	pAB
Initial	20	20	20	0	0	20	20	-	1	-
swap LRNA->B	20	15	24	0	0	25	16	-	1.5625	-
add_token	20	15	24	30	30	25	16	1	1.5625	1.5625
swap B->A	26	15	20	24	30	25	20	1.25	1.25	1.0

We see that the pAB returns back to a value of 1.0, ensuring that the A-B swap price is now 1 and there is no more arbitrage opportunity.

Considering that all the tokens were valued at 1 USD, ALICE started with 20+20+20 = 60 USD worth of assets, and ended with 26+15+20 = 61 USD worth of assets. This is a profit of 1 USD.

This profit is assuming no swap fees, and can grow if the sandwich attacks were done with larger amounts. In effect, this shows that there is a chance for MEV in the add_token call if the token addition is done with a large amount.

The MEV comes from the admin assuming an initial price of 1, but the LRNA token being actually devalued due to the frontrun swap. So the protocol credits the tokenA position with less LRNA tokens than ideal, and thus makes A artificially cheaper to purchase. This is taken advantage of

Tools Used

Manual Review

Recommended Mitigation Steps

Consider adding expected_hub_asset_amount parameter to the add_liquidity function. If the current hub asset does not match this amount, a frontrun must have happened and the function should revert.

Assessed type

MEV

c4-pre-sort commented 6 months ago

0xRobocop marked the issue as primary issue

c4-pre-sort commented 6 months ago

0xRobocop marked the issue as sufficient quality report

c4-sponsor commented 6 months ago

enthusiastmartin (sponsor) disputed

enthusiastmartin commented 6 months ago

In our opinion - not feasible.

Suggested mitigation does not do anything.

It might be broadly correct, but we would like to require more details, practical POC of an exploit.

OpenCoreCH commented 6 months ago

The warden demonstrated that the addition of the initial liquidity can be sandwiched and that the system currently has no parameters for the admin to prevent that (slippage parameters or something similar). Therefore, in line with the other issues related to slippage / frontrunning, judging as a valid medium.

c4-judge commented 6 months ago

OpenCoreCH marked the issue as selected for report

Frankcastleauditor commented 6 months ago

hi @OpenCoreCH , I think that you need to reconsider the validity of this finding to the following reasons .

This report said that when swapping tokens, the LRNA assets in the pool remain the same as shown here

This A-B swap again happens following the x*y=k invariant, and the LRNA assets in the pool remain the same.

The report assumed that there is no change in the LRNA reserves of the swapped assets, but according to the code the amount of LRNA is taken out from the pool of asset_in and then will be transferred to the pool of the asset_out. this claim is not correct at all , since swapping between to different asset will change the amount of hub reserves corresponding to each pool as shown here in the math of the omnipool : https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/math/src/omnipool/math.rs#L62-L71

                asset_in: AssetStateChange {
                        delta_reserve: Increase(amount),
               -->      delta_hub_reserve: Decrease(delta_hub_reserve_in),
                        ..Default::default()
                },
                asset_out: AssetStateChange {
                        delta_reserve: Decrease(delta_reserve_out),
                -->     delta_hub_reserve: Increase(delta_hub_reserve_out),
                        ..Default::default()
                },

So this mistake made this attack profitable for the attacker .

this report broke the invariant x*y=k of the omnipool , this report broke the invariant in two calculations after the report said that the admin add 30 units of tokenA to the omnipool at a price of 1 so the LRNA minted were 30 units : 1) the invariant of the pool of tokenB after the add_token function got called was 16*25 =400 , which should not be affected through the attack since there was no liquidity added or removed from the pool of tokenB , after the swap that made by the attacker the report set the state of the pool to own 20 of tokenB and 25 of LRNA for tokenB , so this broke the invariant since swapping tokens should not change the pool invariant as shown here so the right amount of LRNA is the pool should be 20 instead of 25 to keep the invariant 20*20=400 , so this mistake in the calculation lead to greater amount of LRNA got transferred to be swapped for the tokenA .

Screenshot from 2024-03-13 17-43-38

as shown in the screenshot taken from the table from the report , the invariant was broken in the last row of the table , the invariant x*y=k got changed without any liquidity addition or removal so this broke the invariant .
to emphasis on where in the code the LRNA reserve should be updated when swap happened to keep the invariant you can see this code snippet here from the math file which decrease the amount of hub_asset from the pool state of the token_in to keep the invariant , so the correct pool state should be

pool tokenB = 20 ; 
pool LRNA for tokenB = 20 ;

and the 5 units of LRNA coming out from this trade will be traded for tokenA .

2) the invariant of the pool of tokenA after swaping tokenB for tokenA was broken in the report , since the invariant after 30 units of the tokenA got added by the admin to the pool was

Pool tokenA = 30 ; 
Pool LRNA for tokenA = 30 ;

so the invariant is 30*30 = 900 , this invariant should be hold constant when swapping the 5 units of LRNA for tokenA , but the report broke this invariant and set the state of the pool after the swap to be :

Pool tokenA = 24 ; 
Pool LRNA for tokenA = 30 ;

so the invariant was broken by the report 24*30 != 900 , so the right reserve of the LRNA token was 35 , which came from 30 form the initial addition by the admin + 5 from the swapping the report did not update the LRNA reserves on each pool , although they should be updated according to the code here

Screenshot from 2024-03-13 18-10-28

so as shown the invariant was broken in the last row , and LRNA reserve did not get updated . and then the amount of tokenA will be returned to the attacker Alice should be equal to tokenA returned to Alice = 4.2857 units , so keep the invariant x*y = 900 , so the final state will hold the invariant 25.71*35 = 900 , so the attacker will receive 4.2857 instead of 6 units mentioned by the report .

so I think this report should be marked as invalid .

OpenCoreCH commented 6 months ago

Thanks for your comment. The problem in the report indeed seems to be the "This A-B swap again happens following the x*y=k invariant, and the LRNA assets in the pool remain the same. " It is not actually the A-B swap that follows the invariant, but the A/LRNA & B/LRNA swap. This ultimately makes the analysis invalid. There might be still ways how a frontrunning / sandwiching is possible, but the report does not demonstrate that convincingly.