Open code423n4 opened 1 year ago
bytes032 marked the issue as primary issue
Related to #598
bytes032 marked the issue as sufficient quality report
The re-LP contract and process will be modified.
psytama (sponsor) confirmed
I would have liked to see a Coded POC
Removing Liquidity is not as simple to exploit as swap
GalloDaSballo changed the severity to 2 (Med Risk)
UniV2 Code is as follows https://github.com/Uniswap/v2-core/blob/ee547b17853e71ed4e0101ccfd52e70d5acded58/contracts/UniswapV2Pair.sol#L144-L155
Offering Pro Rata Distribution
In lack of a coded POC
The maximum skimmable is the delta excess value in the UniV2 Reserves vs the value that the caller has to pay to imbalance them
The cost of imbalancing and the nature of it + the fact that other people would have ownership of the LP token means that profitability is dependent on those factors, which IMO have not been properly factored in.
In the presence of a Coded POC as part of the original submission, I would have considered a High Severity
In lack of it, Medium Severity for Skimming the excess value seems more appropriate
GalloDaSballo marked the issue as selected for report
Hi @GalloDaSballo,
I understand your judgement as you have raised valid points, though the circumstances here are different (explained below), which make it more profitable as compared to a typical sandwich attack on removeLiquidity().
1. Coded POC
First, to clarify, as requested on 12 Oct, I have submitted the coded POC as a text file to PaperParachute on 19 Oct, before judging QA started. My apologies that it took a while as I travelling and this POC was complicated and required internet access to code due to the forking. Below is the printout from the POC, which shows it is more than just skimming of excess value (profit of 42.79 ETH using 1000 ETH flash loan vs minimal cost in point 2 below).
For your reference, i have uploaded it here now https://gist.github.com/peakbolt/f2d9a44714b1a2a693debea737128696
-------------- setup copied from testReLpContract() ------------
-------------- 1. attacker perform first swap to get rDPX with flashloaned WETH ------------
attacker weth balance (initial) : 10000000000000000000000 [from flashloan]
attacker rdpx balance (initial) : 0
attacker weth balance (after 1st swap): 0
attacker rdpx balance (after 1st swap): 33799781371440793668463
rDPX price in WETH (after 1st swap): 435429977934145959
-------------- 2. Attacker triggers bond() that indirectly perform removeLiquidity() in reLPContract.reLP() ------------
rDPX price in WETH (after bond): 440827346696573406
-------------- 3. Attacker performs 2nd swap rDPX back to WETH and profit ------------
attacker weth balance (after 2nd swap): 10042795347724107931994
attacker rdpx balance (after 2nd swap): 0
attacker profit in WETH (after repaying flashloan): 42795347724107931994
2. Cost of imbalancing There are 3 main attack cost implicitly mentioned in the original submission. Based on the POC, they are ~11 ETH, minimal as compared to the 42.79 ETH profit.
3. Control over amount of LP tokens to remove As mentioned in my submission, in this case, the attacker has control of the trade size of removeLiquidity() via bond() amount. By increasing the bond amount, which increase amount of LP tokens to remove, the attacker can increase the profitability of the attack.
4. During the bootstrap phase, majority of LP holders are dopex funds/partners The docs stated that during bootstrap phase, Dopex Treasury funds and Partners will partake in the dpxETH bonding first to ensure sufficient dpxETH in circulation. In addition, the backing reserve will be used to seed the liquidity for the Uniswap V2 Pool. So during that period, there are little other LP holders. I did not state this in submission as it is already in the docs. https://dopex.notion.site/rDPX-V2-RI-b45b5b402af54bcab758d62fb7c69cb4 (see Launch Details)
@peakbolt -What's the value in Pool?
Hi @GalloDaSballo,
I have adjusted the POC to a liquidity pool value of ~800 ETH with flashloan attack amount of 15e20 and ran it to provide the answers below. Used 800ETH TVL as the assumed amount to bootstrap, judging by the lower range of TVL in the top uniswap pools. Note that the initial TVL value in the test suite is actually 4000 ETH. See link for updated poc: https://gist.github.com/peakbolt/a8fdbe18759ac2e25d15716af6faa76b.
bond()
iterations. bond()
will hit a revert as there will be insufficient WETH collateral in PerpetualAltanticVault
for purchase of put options (during bonding). Total Liquidity (in WETH) for RDPX/WETH UniswapV2 Pool (before attack): 804803971980485176292
attacker weth balance (initial) : 1500000000000000000000 [from flashloan]
attacker rdpx balance (initial) : 0
bond() Iteration 1
bond() Iteration 2
bond() Iteration 3
bond() Iteration 4
bond() Iteration 5
attacker profit in WETH (after repaying flashloan): 22067892940781745930
GalloDaSballo changed the severity to 3 (High Risk)
GalloDaSballo changed the severity to 2 (Med Risk)
GalloDaSballo changed the severity to 3 (High Risk)
The finding shows that the attacker has access to the "button", this allows them to cause the contract to auto-rebalance
The auto-rebalancing is part of the "button" that attack can trigger
Initially I believed that the attacker only had control over the amounts from remove liquidity, which by definition will give pro-rata of the tokens
The pro-rata can be manipulated based on attacking the X * Y = k uniswap invariant to have the contract take a loss that is relative to the new spot balance, vs the fair LP pricing.
That was Medium Severity imo and most Judges would agree with me, however, after talking with 2 more judges, we agreed that the code also impacts the rebalancing, done via swapExactTokensForTokens
meaning that the attacker is not only causing the "skimmin" of the LP token value, they are able to influence the swap via swapExactTokensForTokens
However, unpon further inspection, that is protected by the Oracle Pricing, which could be argued to protect against it
That said, the default slippage protection is 50 BPS, which is above the 30BPS avg cost of a swap on UnIV2, the oracles also can have drift which in general should make the cost of backrunning sustainable
The profit of the attack would come due to: -> Swap taking a Loss -> generally seen as Med but since it's repeatable High is acceptable -> Ability to trigger the Withdrawal at any time, which causes the Pro-Rata received X * Y being less valuable than the fair valuation of the LP Token
This means that under most circumnstances, a risk free trade is available for attacker at the detriment of the reLP
contract, leading me to believe High Severity to be appropriate.
However, given the circumnstances, and given the original submissions not containing the POC, also considering that the POC shows an impact of 2% of funds, I'm maintaining Medium Severity due to the POC not being present in the original submission
GalloDaSballo changed the severity to 2 (Med Risk)
Lines of code
https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L873 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L930
Vulnerability details
RdpxV2Core.bond()
callsreLPContract.reLP()
at the end of the bonding to adjust the LP for the rDPX/ETH pool and swap ETH for rDPX. WithinreLPContract.reLP()
, it first callsremoveLiquidity()
to remove liquidity (rDPX and ETH), followed byswapExactTokensForTokens()
andaddLiquidity()
.However, the issue is that an attacker has control over
RdpxV2Core.bond()
and can use it to indirectly callreLPContract.reLP()
. That means the attacker can basically frontrunned/backrunnedreLP()
without a mempool. It also reduces the attack cost as attacker does not pay high gas to frontrun it.One may argue that the slippage protection is in place, but that only makes it less profitable and would not help in this case because the attacker is able to increase the trade size of the UniswapV2 transactions. That is because the attacker can adjust the bonding amount to increase the amount of liquidity removed by
reLP()
(and also swap/add liquidity), allowing the attacker to craft a profitable sandwich attack.It is not recommended to allow users to have indirect control over the UniswapV2 transaction in
reLPContract
.https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L873
Impact
An attacker can steal from
rdpxV2Core
by crafting an profitable sandwich attack onbond()
.Proof of Concept
An attacker can perform a sandwich attack as follows:
reLPContract.reLP()
.rdpxV2Core.bond()
to indirectly triggerreLPContract.reLP()
, which willremoveLiquidity()
, causing it receive lesser rDPX due to the price manipulation.reLPContract.reLP()
will thenswapExactTokensForTokens()
to swap ETH-to-rDPX using ETH from removed liquidity. This further increase price of rDPX.addLiquidity()
is then called byreLPContract.reLP()
to return part of the liquidity into the pool.Recommended Mitigation Steps
Remove
reLPContract.ReLP()
frombond()
and trigger separately to prevent user access to it.Assessed type
Uniswap