search

code-423n4 / 2024-01-salty-findings

11 stars 6 forks source link

formPOL lacks slippage and deadline protection #224

Open c4-bot-10 opened 7 months ago

c4-bot-10 commented 7 months ago

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/DAO.sol#L316 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/DAO.sol#L360

Vulnerability details

Impact

formPOL can be vulnerable to a well known liquidity deposit sandwich attack causing loss of funds for the protocol.

Proof of Concept

In formPOL, there are no slippage parameters when calling depositLiquidityAndIncreaseShare

All liquidity deposits in a Uniswap v2 style pool can be frontrun by this sequence of steps:

  1. attacker pushes pool away from correct ratio
  2. victims liquidity deposit goes through at wrong ratio
  3. attacker sells tokens into the new liquidity for a profit

Liquidity withdrawals in a x * y = k pool are actually rarely profitable to frontrun attack (it requires certain edge cases), as the same sequence actually loses money if it is a liquidity withdrawals, which is why withdrawPOL wasn't explicitly mentioned. However, this issue also applies to withdrawPOL and adding slippage protection in that function could be an extra safety measure.

Tools Used

Manual review

Recommended Mitigation Steps

formPOL should have some form of slippage protection. This could be based off a maximum deviation off some other price feed such a Uniswap v3 TWAP or Chainlink pricefeed. If the pool reserves deviates too much from the expected ratio from the price feed, formPOL should revert.

Assessed type

MEV

c4-judge commented 7 months ago

Picodes marked the issue as primary issue

othernet-global commented 7 months ago

The automatic arbitrage on the DEX provides some built in protection against front running as the attacker is not able to move in and out without experiencing friction from the arbitrage itself.

Simulations (see Sandwich.t.sol) show that when sandwich attacks are used on Salty, the arbitrage earned by the protocol sometimes exceeds any amount lost due to the sandwich attack itself. The actual swap loss (taking arbitrage profits generated by the sandwich swaps into account) is dependent on the multiple pool reserves involved in the arbitrage (which are encouraged by rewards distribution to create more reasonable arbitrage opportunities).

c4-sponsor commented 7 months ago

othernet-global (sponsor) acknowledged

c4-judge commented 6 months ago

Picodes changed the severity to 2 (Med Risk)

c4-judge commented 6 months ago

Picodes marked the issue as satisfactory

c4-judge commented 6 months ago

Picodes marked the issue as selected for report

Picodes commented 6 months ago

Regrouping issues about missing slippage checks here as the root cause is the same - the assumption that AAA is enough to prevent MEV bots from sandwiching maintenance transactions doesn't always hold

othernet-global commented 6 months ago

POL has been removed from the protocol

eaf40ef0fa27314c6e674db6830990df68e5d70e https://github.com/othernet-global/salty-io/commit/8e3231d3f444e9851881d642d6dd03021fade5ed