stopthecap - No slippage or deadline control for swapping while stability burning

[sherlock-admin]

stopthecap

high

No slippage or deadline control for swapping while stability burning

Summary

No slippage or deadline control for swapping while stability burning

Vulnerability Detail

Even though Unitas claims there will not be slippage because if it is burned from one side, it is minted in the other one, there is an edge-case where it does create a slight slippage depending on the burn amount. Unitas intends to make stability burns when the reserve ratio is below 130% to try and get it back to normal levels. This burn, is a one sided burn of usd1, which reduces the totalSupply of usd1 unilaterally. https://github.com/sherlock-audit/2023-04-unitasprotocol/blob/main/Unitas-Protocol/src/Unitas.sol#L208

If any user is trying to swap usd1 while there is a stability burn being conducted, they will be affected by that slippage.

The other recommendation is the usage of a deadline param. Without a deadline parameter, the transaction may sit in the mempool and be executed at a much later time potentially resulting in a swap after/before a stability burn.

Impact

User will be affected by unintended and unhandled slippage, potentially affecting the funds they get back from the swap

Code Snippet

https://github.com/sherlock-audit/2023-04-unitasprotocol/blob/main/Unitas-Protocol/src/Unitas.sol#L208

Tool used

Manual Review

Recommendation

Allow a user to specify to key parameters, a deadline and a minOutAmount. And make a check for both at start and end of the execution in the swap function.

[SunXrex]

Sun: In this phase we don’t support stability burn.

Aditya: Slippage on a swap DEX like Uniswap is part of the design. This is to ensure that the pool does not get fully empty. So if the user places a large order, they see a slippage in price. Every order on a DEX like Uniswap, moves the price. Large orders move the price in higher degree. As the order grows, the slippage also grows. This is inherent property of xy=k bonding curve on which Uniswap operate.

In case of Unitas, every swap order is essentially mint/burn order. As long as there is enough USDT in reserve and insurance pool, protocol can mint and burn any amount. This leads to 1:1 value transfer without slippage. Unitas does not mint on a bondig curve, instead the protocol simply mints new tokens or burns the existing. Eg: On uniswap if I buy 1 ETH, I will face low slippage but if I buy 1000 ETH, the slippage will be much higher. However, in case of Unitas whether the user mints 1000 USD91 or 1million USD91, the user will get all the tokens at Oracle price.

In case of Uniswap, the DEX does not have the authority to mint/burn but only facilitate a swap. In case of Unitas, the protocol has the authority to mint and burn. This leads to no slippage for minters/burners.

[Adityaxrex]

This style of slippage is low probability for our design. We may fix it in the future but edge case scenario for now.

[SunXrex]

It should be considered a low-medium risk because the chances of encountering it are very low.

[ctf-sec]

This style of slippage is low probability for our design. We may fix it in the future but edge case scenario for now.

medium

[hrishibhat]

Considering this issue as a medium based on the above comments

[Shogoki]

Escalate for 10 USDC I think this issue should be rated as a valid low instead of medium. Reasons:

• Sponsor stated here already that they consider it low probability.
• In the contest Readme: “getting different price for price update in the same block” was excluded, which is similiar.
• according to the sponsor price updates are not that frequent, so the tx would have to stay a long time in the mempool, for this issue
• Issue #67 was downgraded because the prices are in general quite stable, same goes here and diminishes the impact of the issue

[sherlock-admin]

Escalate for 10 USDC I think this issue should be rated as a valid low instead of medium. Reasons:

• Sponsor stated here already that they consider it low probability.
• In the contest Readme: “getting different price for price update in the same block” was excluded, which is similiar.
• according to the sponsor price updates are not that frequent, so the tx would have to stay a long time in the mempool, for this issue
• Issue #67 was downgraded because the prices are in general quite stable, same goes here and diminishes the impact of the issue

You've created a valid escalation for 10 USDC!

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.

[juntzhan]

Escalate for 10 USDC

Hi @Shogoki glad to know your thoughts.

This issue is a valid medium, because price is based on currency exchange rates and it is not fair to say the price is stable or not (depends on the currency itself).

When EMC appriciates / depreciates, the sponsor HAVE TO update price or the protocol will not work properly.

Please see #96 for a reference.

[sherlock-admin]

Escalate for 10 USDC

Hi @Shogoki glad to know your thoughts.

This issue is a valid medium, because price is based on currency exchange rates and it is not fair to say the price is stable or not (depends on the currency itself).

When EMC appriciates / depreciates, the sponsor HAVE TO update price or the protocol will not work properly.

Please see #96 for a reference.

You've created a valid escalation for 10 USDC!

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.

[ctf-sec]

Can keep the medium severity

[0xffff11]

I strongly believe it should keep a med too.

[jacksanford1]

In order to be Medium severity, the magnitude of the loss has to be quite material.

I don't see anything that points to the magnitude of loss with this slippage issue being very material. @Shogoki makes a good point that there also needs to be proof that this issue doesn't fall into the "known issues" category from the README:

In the contest Readme: “getting different price for price update in the same block” was excluded, which is similiar.

I also agree with @juntzhan that we can't rely on the idea that the price is stable:

because price is based on currency exchange rates and it is not fair to say the price is stable or not (depends on the currency itself).

The probability of this loss seems fairly low, but even if the probability were medium (instead of low) the impact needs to be large enough to be considered material. And it seems hard to prove that the magnitude of loss could be large, especially considering the stability burn mechanism is probably set up in a way where it shouldn't cause large slippage (someone can check me on that).

@0xffff11 @ctf-sec In order for this to be considered Medium I think two things need to be cleared up: 1) This technically does not fall under the README known issue above 2) The magnitude of loss can be large enough to warrant a Medium

[juntzhan]

In order to be Medium severity, the magnitude of the loss has to be quite material.

I don't see anything that points to the magnitude of loss with this slippage issue being very material. @Shogoki makes a good point that there also needs to be proof that this issue doesn't fall into the "known issues" category from the README:

In the contest Readme: “getting different price for price update in the same block” was excluded, which is similiar.

I also agree with @juntzhan that we can't rely on the idea that the price is stable:

because price is based on currency exchange rates and it is not fair to say the price is stable or not (depends on the currency itself).

The probability of this loss seems fairly low, but even if the probability were medium (instead of low) the impact needs to be large enough to be considered material. And it seems hard to prove that the magnitude of loss could be large, especially considering the stability burn mechanism is probably set up in a way where it shouldn't cause large slippage (someone can check me on that).

@0xffff11 @ctf-sec In order for this to be considered Medium I think two things need to be cleared up:

1. This technically does not fall under the README known issue above
2. The magnitude of loss can be large enough to warrant a Medium

1. the README known issue "getting different price for price update in the same block" assumes the price update and swap happen at the same POINT of time and the possibility is very low, while this issue decribes a PERIOD of time during which the price update happens, the possibility is much higher, so they are different issues
2. similar to traditional DEX, user may suffer a large loss without slippage protection, (assume USDEMC/USD1 is 1 : 10) a swap with 10000 USDEMC could result in 100 LESS USD1 due to 10% slippage caused by depreciation, and user could lose more if he swaps more or USDEMC depreciates more

UPDATE: I don't think stability burn mechanism does much help to migitate slippage, as the price would be significantly influenced by the currency exchage rates in external financial markets (as confirmed by sponsor, price data is from multiple cex)

[jacksanford1]

@Shogoki What do you think about juntzhan's points?

[Shogoki]

@Shogoki What do you think about juntzhan's points?

Thanks for enquiring my feedback. Appreciate it :-)

Regarding 1. the mentioned issue in the Readme: It does not say, it happens at the same time, but only “in the same block”. One might say it is probably assumed like this, but it is not clearly stated…

However, as also stated in my escalation on #67, if we do not count the argument from the sponsor of stable prices, which was the reason why #67 was downgraded, i believe this one can also be counted as Medium.

[hrishibhat]

Result: Medium Has duplicates

[sherlock-admin]

Escalations have been resolved successfully!

Escalation status:

• juntzhan: accepted
• Shogoki: rejected

[jacksanford1]

Acknowledged by protocol team (won't fix).