ajna-finance / ajna-core

The Ajna protocol is a non-custodial, peer-to-peer, permissionless lending, borrowing and trading system that requires no governance or external price feeds to function.

https://www.ajna.finance/

Other

31 stars 11 forks source link

Sherlock-72: Lenders pay deposit fees due to no slippage control #915

Closed grandizzy closed 1 year ago

grandizzy commented 1 year ago

Description of change

High level

see https://github.com/sherlock-audit/2023-04-ajna-judging/issues/72
- new revertIfBelowLup param added to Pool.addQuoteToken function
- if true then the tx will revert if price of the bucket to which the quote tokens will be added is below LUP price (hence avoid paying fee for deposit below LUP).
- if false deposit below LUP will go through and deposit fee paid

Contract size

Pre Change

LenderActions            -  10,635B  (43.27%)

Post Change

LenderActions      -  10,669B  (43.41%)

Gas usage

Pre Change

| addQuoteToken                        | 122500          | 179774 | 164363 | 715163  | 60004   |

Post Change

| addQuoteToken                        | 122683          | 179965 | 164546 | 715487  | 60004   |

grandizzy commented 1 year ago

I am uncomfortable with:
* Half the parameters of `addQuoteToken` used for stale TX/frontrunning mitigation.  It is not good DX.

* Interface changes this late in the development cycle.
If we are more concerned about frontrunning than stale TXes, we could just replace expiry_ with revertIfBelowLup_.

I don't think one exclude the other (stale tx vs frontrunning) but happy to get rid of expiry_ and stick with revertIfBelowLup_ @ith-harvey @mattcushman wdyt?

mattcushman commented 1 year ago

I am uncomfortable with:
* Half the parameters of `addQuoteToken` used for stale TX/frontrunning mitigation.  It is not good DX.

* Interface changes this late in the development cycle.
If we are more concerned about frontrunning than stale TXes, we could just replace expiry_ with revertIfBelowLup_.
I don't think one exclude the other (stale tx vs frontrunning) but happy to get rid of expiry_ and stick with revertIfBelowLup_ @ith-harvey @mattcushman wdyt?

If the concern is about interface changes, I think we just keep expiry_ and say to lenders that they need to accept the risk that they get hit with the penalty.

I don't see the sheer number of parameters being a big deal, I do think that this provides real functionality, but it's something we can live without as well. My vote would be 60/40 to include but not strongly.

prateek105 commented 1 year ago

My vote is to keep both expiry and revertIfBelowLup_ if the interface change is not that big of an issue. The expiry was added in this PR here https://github.com/ajna-finance/contracts/pull/600. As per this PR the expiry is required to decrease the probability of deposits at sub-optimal price. But expiry doesn't always prevent from deposit penalty and hence revertIfBelowLup_ is also needed.

ith-harvey commented 1 year ago

I'm Switzerland on this one... Don't care either way.

I believe the question we are debating is:

Does the value of the front run protection outweigh the downstream work and possible timeline extension of altering the interface at this stage in the project?

I'm unaware of the later implications... Maybe @EdNoepel can share with us what he thinks those are in standup?