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

fix: make sure value is not user manipulable #337

Merged
4 commits merged into from May 13, 2021
Merged

fix: make sure value is not user manipulable #337

4 commits merged into from May 13, 2021

Conversation

ghost
Copy link

@ghost ghost commented May 12, 2021

closes #316 & closes #306

@ghost
Copy link
Author

ghost commented May 12, 2021

#307 (review)

If the self.totalDebt -= loss isn't done before we calculate the ratio_change then the ratio_change change is calculated on the amount before the loss.

  • Start: vault = 4500 strategy=500
  • Strategy loss 100: vault = 4500 strategy=400 but totalAssets = 5000
  • We add back 100 to the vault. vault = 4600 strategy=400 but totalAssets = 5100
  • without changes: strategy report -> ratio_change = 10_000 * 100 / 5000 = 200
  • with the changes: strategy report -> ratio_change = 10_000 * 100 / 5100 = 196

With these changes, the strategy that has losses will keep a higher debt ratio.
Is that what you expect with the changes you suggested @fubuloubu ?

@fubuloubu
Copy link
Member

Is that what you expect with the changes you suggested @fubuloubu ?

No. In your example, 20% of the strategy funds were lost. The strategy has 10% of the total funds. Therefore a 2% adjustment (10% of 20%) should be made to its ratio.

However in my suggested change in #316 (comment), the value would be 10_000 * 100 / (500 * 10_000 / 1_000) as the strategy total debt would be normalized using the current debt ratio to whatever the total should be considered to be prior to any updates.

I thibk these two fixes should be worked together.

@fubuloubu
Copy link
Member

fubuloubu commented May 12, 2021

Also can you add @fp-crypto as a co-author on the first commit?

@ghost
Copy link
Author

ghost commented May 12, 2021

@fubuloubu updated with changes needed for #316

contracts/Vault.vy Outdated Show resolved Hide resolved
@ghost
Copy link
Author

ghost commented May 13, 2021

With a vault with a single strategy and that strategy is to EmergencyExit mode we have a division by zero.

steffel and others added 3 commits May 13, 2021 10:41
contracts/Vault.vy Outdated Show resolved Hide resolved
Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com>
@ghost ghost merged commit cd75f52 into yearn:develop May 13, 2021
@ghost ghost changed the title fix: prevent division per zero fix: make sure value is not user manipulable May 14, 2021
@ghost ghost mentioned this pull request May 14, 2021
19 tasks
fubuloubu added a commit that referenced this pull request May 16, 2021
* docs: withdraw can empty available funds (#302)

* fix: prevent revoking non existing strategy (#303)

* fix: prevent revoking non existing strategy

* refactor: fix typo

Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com>

* chore: remove guestList (#308)

* fix: strategy migration only through Vault (#309)

* fix: migratiion should only be perforned by vault

* docs: document new strategy prerequisites

* fix: typo

Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com>

Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com>

* fix: make sure _assessFees isn't called twice within the same block (#326)

* fix: avoid duplicates on withdrawal queue (#299)

* fix: avoid duplicates on withdrawal queue

* fix: use assert instead of raise

Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com>

* fix: use only one loop

* fix: use min approach

* refactor: queue should not add or remove entries

* feat: do not replace strategy

* refactor: clarity

Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com>

* docs: comment

Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com>

* fix: refactor for clarity

Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com>

* docs: token should not be inflationary, deflationary or transfer fees (#331)

* docs: loss debt ratio flow (#332)

* docs: loss will udate debt ratio, debt is adjusted on next report

* docs: fixup comment

Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com>

* fix: prevent fees to be above 100% (#328)

Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com>

* fix: precision factor is useless (#335)

* fix: precision factor is useless most of the time

* fix: remove entirely precision factor

Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com>

* fix: prevent division per zero (#337)

* fix: prevent division per zero

Co-authored-by: fp-crypto <83050944+fp-crypto@users.noreply.github.com>

* fix: do not relly on user manipulable values

* fix: strategy emergency mode

* fix: symplify math

Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com>

Co-authored-by: fp-crypto <83050944+fp-crypto@users.noreply.github.com>
Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com>

Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com>
Co-authored-by: fp-crypto <83050944+fp-crypto@users.noreply.github.com>
orbxball pushed a commit to orbxball/yearn-vaults that referenced this pull request Aug 8, 2021
* docs: withdraw can empty available funds (yearn#302)

* fix: prevent revoking non existing strategy (yearn#303)

* fix: prevent revoking non existing strategy

* refactor: fix typo

Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com>

* chore: remove guestList (yearn#308)

* fix: strategy migration only through Vault (yearn#309)

* fix: migratiion should only be perforned by vault

* docs: document new strategy prerequisites

* fix: typo

Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com>

Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com>

* fix: make sure _assessFees isn't called twice within the same block (yearn#326)

* fix: avoid duplicates on withdrawal queue (yearn#299)

* fix: avoid duplicates on withdrawal queue

* fix: use assert instead of raise

Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com>

* fix: use only one loop

* fix: use min approach

* refactor: queue should not add or remove entries

* feat: do not replace strategy

* refactor: clarity

Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com>

* docs: comment

Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com>

* fix: refactor for clarity

Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com>

* docs: token should not be inflationary, deflationary or transfer fees (yearn#331)

* docs: loss debt ratio flow (yearn#332)

* docs: loss will udate debt ratio, debt is adjusted on next report

* docs: fixup comment

Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com>

* fix: prevent fees to be above 100% (yearn#328)

Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com>

* fix: precision factor is useless (yearn#335)

* fix: precision factor is useless most of the time

* fix: remove entirely precision factor

Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com>

* fix: prevent division per zero (yearn#337)

* fix: prevent division per zero

Co-authored-by: fp-crypto <83050944+fp-crypto@users.noreply.github.com>

* fix: do not relly on user manipulable values

* fix: strategy emergency mode

* fix: symplify math

Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com>

Co-authored-by: fp-crypto <83050944+fp-crypto@users.noreply.github.com>
Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com>

Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com>
Co-authored-by: fp-crypto <83050944+fp-crypto@users.noreply.github.com>
sambacha pushed a commit to sambacha/yearn-vaults that referenced this pull request Sep 7, 2021
* docs: withdraw can empty available funds (yearn#302)

* fix: prevent revoking non existing strategy (yearn#303)

* fix: prevent revoking non existing strategy

* refactor: fix typo

Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com>

* chore: remove guestList (yearn#308)

* fix: strategy migration only through Vault (yearn#309)

* fix: migratiion should only be perforned by vault

* docs: document new strategy prerequisites

* fix: typo

Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com>

Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com>

* fix: make sure _assessFees isn't called twice within the same block (yearn#326)

* fix: avoid duplicates on withdrawal queue (yearn#299)

* fix: avoid duplicates on withdrawal queue

* fix: use assert instead of raise

Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com>

* fix: use only one loop

* fix: use min approach

* refactor: queue should not add or remove entries

* feat: do not replace strategy

* refactor: clarity

Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com>

* docs: comment

Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com>

* fix: refactor for clarity

Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com>

* docs: token should not be inflationary, deflationary or transfer fees (yearn#331)

* docs: loss debt ratio flow (yearn#332)

* docs: loss will udate debt ratio, debt is adjusted on next report

* docs: fixup comment

Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com>

* fix: prevent fees to be above 100% (yearn#328)

Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com>

* fix: precision factor is useless (yearn#335)

* fix: precision factor is useless most of the time

* fix: remove entirely precision factor

Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com>

* fix: prevent division per zero (yearn#337)

* fix: prevent division per zero

Co-authored-by: fp-crypto <83050944+fp-crypto@users.noreply.github.com>

* fix: do not relly on user manipulable values

* fix: strategy emergency mode

* fix: symplify math

Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com>

Co-authored-by: fp-crypto <83050944+fp-crypto@users.noreply.github.com>
Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com>

Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com>
Co-authored-by: fp-crypto <83050944+fp-crypto@users.noreply.github.com>
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant