N-02 Overly Restrictive Validation #2548
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sparrowDom
requested review from
DanielVF,
naddison36,
shahthepro and
clement-ux
as code owners
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
OpenZeppelin issue:
Throughout the codebase, there are several validation issues that can be corrected:
The NatSpec comment for ACTION_THRESHOLD describes it as the "Minimum liquidity required to continue," which implies the value 1e12 is an inclusive lower bound for an action to proceed. However, within the _burnOethOnTheContract function, this threshold must be exceeded, making the threshold an exclusive boundary and contradicting the NatSpec comment. Consider updating the NatSpec comment to clarify that the amount must be "greater than" the threshold to align with the code's logic.
The validation checks in _getAddLiquidityParams use a strict greater-than (>) comparison to ensure the calculated required token amounts are less than the maximum available balances. This logic is overly restrictive and will incorrectly cause the transaction to revert in valid scenarios where the required amount is exactly equal to the contract's available balance. To fix this and allow the strategy to correctly deploy its full token balance when necessary, consider changing the comparison operators from > to >=.
Consider fixing these issues to ensure the validations are correctly aligned with intended behavior