code-423n4 / 2023-10-ethena-findings

5 stars 5 forks source link

There is no slippage protection for orders #66

Closed c4-submissions closed 11 months ago

c4-submissions commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-10-ethena/blob/main/contracts/EthenaMinting.sol#L162-L187

Vulnerability details

Proof of Concept

From the docs

Ethena has designed its minting process to minimize slippage for its users. Before submitting a minting transaction, users receive a "price" from the Ethena Pricing API that includes a predefined slippage range. The user then signs this price, confirming their acceptance of the potential variation within the specified range. When the transaction is executed, the smart contract logic ensures that the final minting settlement price falls within the signed slippage range. This approach provides users with a level of certainty about the price they will receive and makes the transaction predictable. Slippage management is a key part of Ethena's minting design strategy, aiming to provide users with a more stable and transparent transaction experience. It's one more way that we are working to make the world of decentralized finance more accessible and user-friendly.

Documentation describes that slippage protection is very important for the protocol, however there is no such check inside mint function. User signs order where he provides asset, amount of asset and amount of usde, this values are just verified to be not 0 and then assets are transfered and usde is minted. But there is no any check that usde price at the moment is indeed in some range from amount of assets that user sent for 1 usde.

As i understand minter role will have some backend service that will check prices to be fair(or maybe not) before calling mint function. But after he called the mint function, then tx can seat in the mempool for some time and during that time price can be changed, which can lead to loss for the usde purchaser. As protocol expects that arbitragers will use it to arbitrage dex prices and minting prices, then control of price is very important for them, and it's very important for other users.

Each order has expiry param, but it can't protect users from flash price spikes.

Tools Used

VsCode

Recommended Mitigation Steps

Price validation should be done on chain.

Assessed type

Error

c4-pre-sort commented 1 year ago

raymondfam marked the issue as low quality report

c4-pre-sort commented 1 year ago

raymondfam marked the issue as primary issue

raymondfam commented 1 year ago

The sponsor already explained this does not apply to their PMM.

c4-judge commented 11 months ago

fatherGoose1 marked the issue as unsatisfactory: Invalid

rvierdiiev commented 11 months ago

hello @fatherGoose1 can you pls explain the reason for the invalidation of this issue?

thanks

flowercrimson commented 11 months ago

Hey @fatherGoose1, I think this issue should be valid medium and there might be a misunderstanding. Let me clarify: (1) Lookout says that The sponsor already explained this does not apply to their PMM, which I think refers to sponsors discord comment. But this issue has nothing to do with the sponsor's comment. Sponsor's comment here:

I think the idea of slippage is more for AMMs. our implementation is more like PMM. Although we ask for 30s expiry in the signing, we in fact require market makers to submit the signature and order back to us within 500ms of us sending them the RFQ. In case the market moves too much even in those 500ms, we simply reject the order.

If this is what the lookout was referring to, then I don't think it applies to this issue. Because the sponsor is only saying there is slippage protection for the off-chain part of order submission, but this issue is saying the vulnerability is on-chain tx settlement - the time between order submission and order settlements is not protected, which has nothing to do with off-chain.

In fact, only off-chain protection is insufficient against slippage: a) Suppose, the order price didn't move within 500ms and is accepted off-chain and then submitted on-chain. The time taken for the order settlement on-chain is still uncertain. And even the order has an expiry param, it only prevents stale order and does nothing to protect slippage.
b) Even expiry requires tx to be settled in the same block, but still this means that this transaction can settle anywhere within a 12 second timeframe, which is far more than sponsor's off-chain 500ms control.

(2) The sponsor says the concept of slippage only applies to AMM, not to this implementation, which I don't agree. Slippage affects on-chain mechanisms that rely on token price valuation. In Ethena, slippage protection is key to ensure 1:1 collateralization ratio. Although Ethena spent great efforts ensuring 1:1 ratio through 'delta-neutral hedging strategies' with CEX, but the missing area is still the initial order on-chain settlement window.

Suppose that the order settled on-chain with a price slippage when tx at mempool, then usde will be under-collateralized from the beginning. And price slippage is very likely because the accepted collaterals are not stable coins. From now on, 'delta-neutral hedging` will have no effect on the initial under-collateralization. Because delta-neutral only protects price fluctuation after CEX positions are created. The initial under-collateralization on-chain will lead to it is always under-collateralized.

Based on the impact of price evaluation and 1:1 collateralization ratio key to ethena, the fact that this issue points to a window of no protection, I think this is at least Medium severity.

deHB6 commented 11 months ago

We run our own full ETH node that we submit transactions too to mitigate frontrunning. Minters and redeemers at least for the first several months are whitelisted professional market makers familiar with PMMs. We offer a price, they accept, and that is the price the trade will take place at. It may take some time to settle, but there is no deviation in settlement price from what the parties have agreed to. That's my understanding of what slippage is, so I don't think there is any.

deHB6 commented 11 months ago

Hmm there are definitely risks especially if we grow large in size but that risk is born by the protocol overall, the individual user is assured that they will get the USDe amount they have signed and a full hedge of that amount. With a full ETH node to submit TX to and perpetual swap trading on 6 exchanges including the biggest which together are > half the liquidity we feel we can get the best possible perp price. One thing to note is we generally won't wait for the transaction to be confirmed on chain to place the hedge. Also we did consider an on chain oracle check but not for this reason - to slow down a hack in case minter key compromised however we decided against it. One reason we didn't go for it is the oracle prices are spot prices, not perp prices. Perp prices can deviate from spot by 2-3% when market moves so would result in false positives and block minting/redeeming.

rvierdiiev commented 11 months ago

It may take some time to settle, but there is no deviation in settlement price from what the parties have agreed to.

The moment when order is signed isn't the moment of settlement. The deal that supposed to be profitable, may become unprofitable with time. From perspective of user at the moment t0 they agree to send you some amount of tokens in order to get another amount of usde. But they would like to get it at the moment of agreement(t0). However they don't receive usde at that moment, which means that they can't use it and should wait when tx will be executed on chain(t1). Only then they can get minted amount. During the waiting time price can be changed, which means that the deal can be unprofitable to user or to the protocol. In first case user doesn't receive enough usde, but provided more assets to hedge them, in another case more usde is minted than provided assets that should back position.

flowercrimson commented 11 months ago

Hey @deHB6, (1) Because on-chain settlement is a different time from off-chain order signing or off-chain order acceptance, the risk of token price deviation results in either the user taking a loss or the protocol taking a loss, and this window can be protected against. (2)

We run our own full ETH node that we submit transactions too to mitigate frontrunning.

Running an ETH node eliminates third-part risk and improves efficiency to some extends, but it doesn't shield txs from sitting in the mempool or frontrunning. There is no control of tx ordering and timing.

deHB6 commented 11 months ago

One's loss is the others gain. PMMs don't do oracle checks, the price is set in stone

fatherGoose1 commented 11 months ago

The sponsor has provided significant detail of their off-chain mechanisms, most notably:

For these reasons, this issue will remain invalid.