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

Prevent DIV/0 revert calculating MOMP on a pool with no loans #712

Merged
merged 4 commits into from
Apr 3, 2023

Conversation

EdNoepel
Copy link
Contributor

@EdNoepel EdNoepel commented Mar 28, 2023

Description of change

High level

Fix for #702

Description of bug or vulnerability and solution

Consumers should be able to calculate MOMP at any time, regardless of whether there is debt in the pool. When there are no borrowers, we define MOMP as the HPB. This means it will now be MIN_PRICE if there is no liquidity on the book. There is no change to how MOMP is calculated for the first loan.

Contract size

Pre Change

============ Deployment Bytecode Sizes ============
PoolInfoUtils - 11,773B (47.90%)

Post Change

============ Deployment Bytecode Sizes ============
PoolInfoUtils - 11,828B (48.13%)

Gas usage

This call would only cost gas if used in a transaction from a proxy contract or outside of the Ajna protocol.

Pre Change

src/PoolInfoUtils.sol:PoolInfoUtils contract
Function Name min avg median max # calls
momp 108324 108324 108324 108324 1

Post Change

src/PoolInfoUtils.sol:PoolInfoUtils contract
Function Name min avg median max # calls
momp 34665 55698 36878 108353 9

Copy link
Contributor

@grandizzy grandizzy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

( , , uint256 noOfLoans) = pool.loansInfo();
noOfLoans += pool.totalAuctionsInPool();
if (noOfLoans == 0) return MAX_PRICE;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about returning the price of the top bucket? Basically replace Maths.wdiv(...)) below with 1?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MAX_PRICE is the price of the top bucket.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i mean top priced bucket with deposit > 0

Copy link
Contributor Author

@EdNoepel EdNoepel Mar 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in e433d8cfba109191cdb67b74179836a7405d37d9; please re-review.

@EdNoepel EdNoepel requested a review from ith-harvey April 1, 2023 14:13
Copy link
Contributor

@ith-harvey ith-harvey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, don't see issues with this addition, happy to approve once comments are addressed.

@@ -15,6 +15,7 @@ import {
_priceAt,
_reserveAuctionPrice,
MAX_FENWICK_INDEX,
MAX_PRICE,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was MAX_PRICE added? I don't see it used, suggest it is removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed in 111ce68.

Copy link
Contributor

@ith-harvey ith-harvey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, ship it!

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

Successfully merging this pull request may close these issues.

4 participants