PASCAL - Slippage isn't implmented properly on mint() and burn() functions

[sherlock-admin4]

PASCAL

High

Slippage isn't implmented properly on mint() and burn() functions

Summary

Slippage isn't implmented properly on mint() and burn() functions especially on burn() function.

Vulnerability Detail

Slippage is enforced by using price discreprancy in oracle.vy (current price, desired price and slippage ), so current slippage implementation helps check sudden price movement not actual token payout. But in pool.vy the actual amount that core.vy pays out the LP provider when minting and burning LP tokens is calculated using the total pool value in the form of quote tokens and current circulating LP tokens https://github.com/sherlock-audit/2024-08-velar-artha/blob/main/gl-sherlock/contracts/pools.vy#L163-L178 ,
https://github.com/sherlock-audit/2024-08-velar-artha/blob/main/gl-sherlock/contracts/pools.vy#L213-L228 , the issue is that current circulating LP tokens and pool reserve will be changing constantly because of minting, burning and closing positions (especially profitable ones).

Proof of concept and impact

Let's look at this scenario ; User wants to burn 100 Lp tokens he currently checks pool.calc_burn() a view function for how much USDT he will get in return (converting pool value to usdt) . Let's assign some varaibles; lp token total supply = 1000; pool reserve value in usdt = 2000.
Using the calculation in the link I pasted above (lp * pv) /ts = (100 * 2000) / 1000 = 200 usdt will be returned as the result.
Happy with the result he goes ahead to call burn in apy.vy to burn 100 lp tokens but before he called it a profitable trader closed their positions taking out a net 1,000 usdt value out of the pool (I used 'net' because the collateral the trader deposited is absorbed into the pool reserve ).
Then as the user that wants to burn calls burn() new pay out value is calculated as (100 * 1000)/ 1000 = 100. So Lp provider ends up getting 100 USDT instead of 200 and there was no slippage to ensure he will get 200 as the price of tokens do not affect the value in the pool.

Tools Used

Manual Review

Recommendation

Set a minimum amount of quote/LP tokens user is supposed to expect

Duplicate of #74

[Pascal4me]

Reason for escalation is clearly stated in impact
Lead judge didn't even give reason for invalidation or a blind spot they didn't understand

[IWildSniperI]

Escalate

On behalf of @Pascal4me

[sherlock-admin3]

Escalate

On behalf of @Pascal4me

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[msheikhattari]

Invalid, the issue itself states that slippage is provided by a user when he calls mint or burn in apy.vy

[ami0x226]

Invalid, the issue itself states that slippage is provided by a user when he calls mint or burn in apy.vy

The codebase only provides a slippage check for price movement, which is insufficient to prevent receiving tokens in an amount less than expected.
The report states that a frontrunned (not malicious) closing position can reduce the amount received.
As a result, this is valid medium severity

[msheikhattari]

BOB has no public mempool so positions can't be frontrun either way

[ami0x226]

BOB has no public mempool so positions can't be frontrun either way

I mentioned in my comment: "The report states that a frontrunned (not malicious) closing position can reduce the amount received."
Here, frontrunned (not malicious) closing position means accidental frontrunning, not malicious.
In other words, the closing position transaction before execution of burning transaction influences to LPer's expected amount.

[WangSecurity]

a slippage check for price movement

I'm not sure I understand how the current slippage works in the protocol, could you please elaborate on it?

[Pascal4me]

Answering your question wang; the current slippage is just for price movement. Let's say you come with 1 bitcoin to add to the Btc/usdt (base/quote) LP, the amount of LP given to you(minted) is calculated based on the total value of the tokens in the pool as quote token present in the pool that is all btc are converted to usdt with the current oracle price to calculate how many Lp tokens you will be minted. So the current slippage implemented by the protocol only enforces price movements. So if btc is currently $60k you can put a slippage of $500, then if the oracle returns a price that is < $59500 and >$60,500 the transaction reverts.

[rickkk137]

this issue talks about LP token price and as we know LP token price depend on pool reserve and LP total supply
its mean when LPs want burn their tokens if a profitable position closes before their burn TX finally they get less asset than expected

• pashap9990 - Lp amount will be minted lower than what it really expected in mint function #104
• pashap9990 - LPs cannot specify min amount received in burn function, causing loss of fund for them #74

[WangSecurity]

After additionally considering this issue and based on the discussion under #74, I agree this issue is valid and indeed, the slippage is checked only on the Oracle price, but when it comes to conversions, there can be slippage-related issues due to a decrease of pool reserves. Planning to accept the escalation and duplicate with #74.

[Pascal4me]

According to the rules slippages are high severity

[WangSecurity]

The rules don't say any slippage related issues are necessarily high and I believe that here the medium is more appropriate. The reasons are the following:

1. If it's the attack via front-running, then Bob has a private mempool, which makes this very unreliable. Hence, extensive constraint.
2. If it's unintended situation, then the 2 calls (one to mint/burn and the one affecting LP reserves) should be in the same block and the transaction affecting LP reserves. I see it as an extensive limitation as well, because Bob has low block time and these 2 transaction would have been made within ~2 secs (taking into account the average Bob blockchain timestamp).

Hence, medium severity. The decision remains, accept the escalation and validate with medium severity and duplicate with #74.

[4gontuk]

@WangSecurity #65 is dup of #105

[WangSecurity]

Unfortunately, 65 is not a sufficient duplicate of 105. 65 just states that the check_slippage function is not utilised when trying to deposit into the LP, but it's utilised. 105 explains that the slippage is checked, but the slippage is not sufficient.
Hence, 65 is incorrect and doesn't identify the issue.

Moreover, 65 doesn't show a definite loss of funds with a detailed explanation, which is required:

Slippage related issues showing a definite loss of funds with a detailed explanation for the same can be considered valid high

[WangSecurity]

Result:
Medium
Duplicate of #74

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• IWildSniperI: accepted