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.

Sign up for GitHub

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

Jump to bottom

Fix rewards accrual #860

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Fix rewards accrual #860

wants to merge 5 commits into from

Conversation

Rubilmax
Copy link
Collaborator

@Rubilmax Rubilmax commented Jun 8, 2023 •
edited
Loading

Pull Request

Issue(s) fixed

This pull request:

This is a fix to the vulnerability disclosed by Spearbit, which cannot be used in production currently because:

  • no rewards are currently distributed by Aave on mainnet
  • no RewardsManager is deployed nor set on MorphoAaveV3-ETH

But this PR should be reviewed and ready to deploy as soon as the Morpho DAO decides to distribute Aave-emitted rewards to users, following an Aave governance proposal.

@Rubilmax Rubilmax marked this pull request as ready for review June 8, 2023 13:03
Tristan22400
Tristan22400 previously approved these changes Jun 8, 2023
View reviewed changes
pakim249CAL
pakim249CAL reviewed Jun 8, 2023
View reviewed changes
src/MorphoInternal.sol Outdated Show resolved Hide resolved
Tristan22400
Tristan22400 previously approved these changes Jun 9, 2023
View reviewed changes
MerlinEgalite
MerlinEgalite approved these changes Jun 9, 2023
View reviewed changes
Copy link
Contributor

@MerlinEgalite MerlinEgalite left a comment

Choose a reason for hiding this comment

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

Changes look good but with lot's of redundant code. Not sure that internalizing them would be better though

QGarchery
QGarchery previously approved these changes Jun 9, 2023
View reviewed changes
Copy link
Contributor

@QGarchery QGarchery left a comment

Choose a reason for hiding this comment

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

LGTM

pakim249CAL
pakim249CAL previously approved these changes Jun 9, 2023
View reviewed changes
MerlinEgalite
MerlinEgalite requested changes Jun 9, 2023
View reviewed changes
Copy link
Contributor

@MerlinEgalite MerlinEgalite left a comment

Choose a reason for hiding this comment

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

Requesting changes to prevent merge

pakim249CAL
pakim249CAL previously approved these changes Jun 19, 2023
View reviewed changes
Tristan22400
Tristan22400 reviewed Jun 19, 2023
View reviewed changes
src/PositionsManagerInternal.sol Show resolved Hide resolved
src/PositionsManagerInternal.sol Outdated Show resolved Hide resolved
Rubilmax
Rubilmax commented Jun 19, 2023
View reviewed changes
Copy link
Collaborator Author

@Rubilmax Rubilmax left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM! But pending because rewards are not available on mainnet so upgrade is not yet required.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@makcandrov makcandrov makcandrov approved these changes

@Tristan22400 Tristan22400 Tristan22400 approved these changes

@MathisGD MathisGD Awaiting requested review from MathisGD

@QGarchery QGarchery Awaiting requested review from QGarchery

@MerlinEgalite MerlinEgalite Awaiting requested review from MerlinEgalite

@pakim249CAL pakim249CAL Awaiting requested review from pakim249CAL

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Rubilmax @QGarchery @Tristan22400 @MerlinEgalite @pakim249CAL @makcandrov