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

add extra precaution to maxDeposit when yield buffer is low #113

Merged

Conversation

trmid
Copy link
Member

@trmid trmid commented Jun 21, 2024

The issue being addressed is an edge case in the ERC4626 compatibility of maxDeposit / maxMint when the yield buffer is low and a deposit reverts due to the LossyDeposit check. The 4626 spec states that a deposit MUST not revert with the value returned by maxDeposit (same with the mint function in this case), but due to potential rounding errors, it's possible that the prize vault's deposit function reverts if the yield buffer is depleted due to the rounding error.

This is not an issue with the deposit functionality, since it's important that we ensure there can be no lossy deposits; rather this is an issue with the maxDeposit function not returning zero if there is a possibility of the yield buffer being depleted.

The proposed fix adds a simple additional check to the maxDeposit function to ensure that the yield buffer is at least half full. If not, then maxDeposit will return zero, assuming that there is some possibility of a rounding error causing a reversion on deposit. Most prize vaults will only have rounding errors of 1 wei, but there have been some yield sources (specifically compound v2 and similar forks) that have additional rounding errors for 18 decimal tokens. Prize vaults for these kinds of yield sources will be configured with increased yield buffers proportional to that loss, but since there is no way for the contract to know the expected loss size, the more conservative approach for maxDeposit is to check if the yield buffer is at least half full.

This means that there may be a scenario where the yield buffer is less than half full and maxDeposit returns zero, but a deposit of some value goes through without issue. This conservative behaviour does not break the 4626 spec for maxDeposit.

Copy link

linear bot commented Jun 21, 2024

Copy link

LCOV of commit 60be8fc during coverage #578

Summary coverage rate:
  lines......: 99.6% (237 of 238 lines)
  functions..: 100.0% (65 of 65 functions)
  branches...: no data found

Files changed coverage rate:
                              |Lines       |Functions  |Branches    
  Filename                    |Rate     Num|Rate    Num|Rate     Num
  ==================================================================
  src/PrizeVault.sol          |99.5%    200| 100%    51|    -      0

@trmid trmid requested a review from asselstine June 27, 2024 17:23
Copy link
Contributor

@asselstine asselstine left a comment

Choose a reason for hiding this comment

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

Great explanation

@trmid trmid merged commit 2acdde5 into fix-review Jun 28, 2024
2 checks passed
@trmid trmid deleted the gen-1776-134-deposit-can-revert-in-some-cases-when-using-the branch July 16, 2024 16:04
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.

2 participants