-
Notifications
You must be signed in to change notification settings - Fork 8
refactor(contracts): internal M0 and Sherlock audit changes #55
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
Merged
Merged
Changes from 10 commits
Commits
Show all changes
19 commits
Select commit
Hold shift + click to select a range
f53f493
unit tests for invalid transfers, no claimable yield, proper principa…
mesozoic-technology aaf1a71
added _disableInitializers() to constructor, fixes to test
mesozoic-technology b8bc884
returning if totalPrincipal is zero before index update
mesozoic-technology b64bee5
added test_claimYield()
mesozoic-technology cb5fffb
added test_claimYieldMultipleUsers()
mesozoic-technology d67e1b5
added test_claimYieldAfterTransfer()
mesozoic-technology 6e21279
added test_indexUpdateWithZeroTotalPrincipal()
mesozoic-technology 9e17f6c
added test_indexUpdateAfterBurn()
mesozoic-technology 93db590
updated tests, passing with forked state
mesozoic-technology 6e94ce3
comments in burn test
mesozoic-technology 46f6869
included min256() in claim() to never dos a claim
mesozoic-technology f65ac1d
changed NobleDollar.sol pragma to be 0.8.22 to allow compilation in l…
mesozoic-technology 8f47134
changing pragma back to 0.8.20 since this is fixed by another branch
mesozoic-technology fb82a58
included word on rounding behavior in readme
mesozoic-technology 56a5e11
Merge pull request #2 from m0-foundation/sherlock-5
mesozoic-technology 2b51f88
Merge pull request #3 from m0-foundation/sherlock-6
mesozoic-technology b84f0bb
Merge pull request #4 from m0-foundation/sherlock-7
mesozoic-technology a11037c
Fix formatting issues and delete commented out code
toninorair 9aaf923
Fix compilation warnings
toninorair File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have doubts this is needed,
getIndexRoundedDownshould revert withDivisionByZeroUh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mesozoic-technology https://github.com/m0-foundation/noble-dollar/blob/contracts/contracts/utils/IndexingMath.sol#L97
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we were to revert, we'd be doing this on a cross chain message. In that case, what would happen to the yield the message intended to send? By returning instead, there is no ambiguity here, the yield is received by the contract, and the next time there is principal present to avoid a division by zero, the index is still updated to the correct value.
Maybe this would not be a problem given how Hyperlane works, but I chose this as I am not 100% familiar with that - would be good to know that, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fair take, worth adding comment for auditors, just in case