Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Require shares > 0 upon supply/withdraw/borrow/repay? #155

Closed
Rubilmax opened this issue Jul 21, 2023 · 10 comments · Fixed by #194
Closed

Require shares > 0 upon supply/withdraw/borrow/repay? #155

Rubilmax opened this issue Jul 21, 2023 · 10 comments · Fixed by #194
Labels

Comments

@Rubilmax
Copy link
Collaborator

Should we prevent supplying amount if the number of shares is 0?

Originally posted by @MerlinEgalite in #123 (comment)

From @makcandrov's comment.

@Rubilmax
Copy link
Collaborator Author

In the case of supply/repay, (amount != 0 && share == 0) would be equivalent to doing a giveaway, which is not really an issue. However, in the case of borrow/withdraw, it would mean withdrawing money without updating the user's share, which is more of an issue. Ofc we are talking about negligible amounts here, so it is not a big deal

From @makcandrov.

@MathisGD
Copy link
Contributor

MathisGD commented Aug 1, 2023

Note that this is fixed by #194

@MathisGD MathisGD linked a pull request Aug 1, 2023 that will close this issue
@Rubilmax
Copy link
Collaborator Author

Rubilmax commented Aug 1, 2023

The problem is symmetric though: do we want to revert when the corresponding amount is zero?
I feel like we don't, but it's worth considering.

@MathisGD
Copy link
Contributor

MathisGD commented Aug 1, 2023

Good question

@makcandrov
Copy link
Contributor

It's not exactly symmetric: we don't care if the users receive zero tokens, as long as their shares decrease correctly (so that they can't withdraw tokens without updating their position)

@makcandrov
Copy link
Contributor

makcandrov commented Aug 22, 2023

With #248 being merged, I think this issue should be reopened, with new edge cases:

  • If it is possible to supply/repay x > 0 shares such that the corresponding amount is null, one's position can be increased indefinitely without transferring any tokens.
  • If it is possible to borrow/withdraw x > 0 amount such that the corresponding shares are null, one can siphon liquidity from the market without updating their position.

@Rubilmax
Copy link
Collaborator Author

Rubilmax commented Aug 22, 2023

Note that:

  1. it's not guaranteed to be economically viable (on the other hand, Blue only relies on weak assumptions on tokens and prices)
  2. shares have 6 decimals of additional precision compared to assets, so it can only happen that an asset amount corresponding to input shares is zero (not the other way around)

2 guarantees that only your first example is possible in practice, right?

@QGarchery
Copy link
Contributor

QGarchery commented Aug 22, 2023

I agree with @Rubilmax and also the ratio shares / asset is starting at a high value (VIRTUAL_SHARES = 1e6) and can only increase due to the interests, so 2 should hold for a long time

@Rubilmax
Copy link
Collaborator Author

Rubilmax commented Aug 22, 2023

After looking at the code, it happens that we only use toAssetsDown on borrow and withdraw, which to me nullifies this issue (the attacker would only be capable of borrowing/withdrawing 0 assets corresponding to a non-zero amount of shares)

@makcandrov
Copy link
Contributor

Indeed, the roundings seem to always go towards the correct side. Mb, this issue can remain closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants