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

Reserve Updates #272

Merged
merged 36 commits into from
Dec 5, 2024
Merged

Reserve Updates #272

merged 36 commits into from
Dec 5, 2024

Conversation

sirpy
Copy link
Contributor

@sirpy sirpy commented Feb 14, 2024

No description provided.

Copy link

openzeppelin-code bot commented May 23, 2024

Reserve Updates

Generated at commit: 494ba1164b9a1e009e2213aa79dd8d21d26bfa9d

🚨 Report Summary

Severity Level Results
Contracts Critical
High
Medium
Low
Note
Total
3
5
0
16
44
68
Dependencies Critical
High
Medium
Low
Note
Total
0
0
1
0
104
105

For more details view the full report in OpenZeppelin Code Inspector

@peterje-tr
Copy link

Hi Hadar. Looking forward to review this :) Just one request, can you clean up the commit history and add more descriptive commit messages? Also, potentially splitting into multiple independent PRs if possible?

Thanks,
Peter Emil

@sirpy
Copy link
Contributor Author

sirpy commented Oct 10, 2024

@peterje-tr sorry not gonna happen:) i'm happy to walk you through.

Copy link

@peterje-tr peterje-tr left a comment

Choose a reason for hiding this comment

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

Super nice work. Glad to see the changes. I have some comments

General Notes:

  • This branch does not include recent changes from master. I think it would be proper to update it
  • This PR would greatly benefit from being split into multiple independent PRs. It would make it much easier to review

Comment on lines +780 to +783
require(
_msgSender() == address(avatar) || _msgSender() == guardian,
"CompoundVotingMachine: not avatar or guardian"
);

Choose a reason for hiding this comment

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

The Sayfer finding SAY-02 "Inconsistent Guardian Approval Requirement" seems to be still present

Copy link
Contributor Author

Choose a reason for hiding this comment

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

line 429 was changed to make it a little bit clearer
here the status is fixed
https://sayfer.io/audits/smart-contract-audit-report-for-gooddollar/

Choose a reason for hiding this comment

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

I can't find it in the link? Can you give me a more precise link?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

contracts/staking/GoodFundManager.sol Show resolved Hide resolved
contracts/reserve/GoodMarketMaker.sol Outdated Show resolved Hide resolved
contracts/reserve/GoodMarketMaker.sol Show resolved Hide resolved
contracts/reserve/GoodMarketMaker.sol Show resolved Hide resolved
scripts/proposals/reserveRestore.ts Show resolved Hide resolved
scripts/proposals/reserveRestore.ts Outdated Show resolved Hide resolved
scripts/proposals/reserveRestore.ts Show resolved Hide resolved
scripts/proposals/reserveRestore.ts Outdated Show resolved Hide resolved
contracts/governance/StakersDistribution.sol Outdated Show resolved Hide resolved
@sirpy sirpy merged commit bba0de1 into master Dec 5, 2024
30 checks passed
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.

3 participants