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

Added Check to CeloDistributionSchedule activate function #11109

Merged

Conversation

soloseng
Copy link
Contributor

@soloseng soloseng commented Jul 5, 2024

Description

Added a check that prevents activating the CeloDistributionSchedule contract if the distributionSchedule contract address if not set in CELO token contract.

Previously, it was expected that the CeloDistributionSchedule address would be manually set in the GoldToken contract by calling setCeloTokenDistributionSchedule(). This was expected to be done before calling activate() in the CeloDistributionSchedule. However, the CeloDistributionSchedule contract did not verify that the CeloDistributionSchedule address was set in the GoldToken contract, allowing for the CeloDistributionSchedule to be incorrectly activated.

These new changes get the CeloDistributionSchedule address from the registry, instead of setting it in the GoldToken contract. Hence, no longer requiring that the CeloDistributionSchedule address be manually set in the GoldToken contract before activating the CeloDistributionSchedule contract.

Tested

unit tested

@soloseng soloseng changed the base branch from master to release/core-contracts/12 July 5, 2024 17:23
@soloseng soloseng changed the title Soloseng/add-check-to-distribution-schedule Added Check to CeloDistributionSchedule activate function Jul 5, 2024
@soloseng soloseng self-assigned this Jul 5, 2024
@soloseng soloseng marked this pull request as ready for review July 5, 2024 17:25
@soloseng soloseng requested a review from a team as a code owner July 5, 2024 17:25
Copy link

gitguardian bot commented Jul 12, 2024

⚠️ GitGuardian has uncovered 1 secret following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

🔎 Detected hardcoded secret in your pull request
GitGuardian id GitGuardian status Secret Commit Filename
10538986 Triggered Generic High Entropy Secret e379b5c packages/protocol/scripts/foundry/constants.sh View secret
🛠 Guidelines to remediate hardcoded secrets
  1. Understand the implications of revoking this secret by investigating where it is used in your code.
  2. Replace and store your secret safely. Learn here the best practices.
  3. Revoke and rotate this secret.
  4. If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.

To avoid such incidents in the future consider


🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

arthurgousset
arthurgousset previously approved these changes Jul 15, 2024
Copy link
Contributor

@arthurgousset arthurgousset left a comment

Choose a reason for hiding this comment

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

Left a couple nit comments, but looks good to me. I like how you wrote the two new unit tests. It's clever to expect the contract to revert.

Nit: registryAddress and proxyAdminAddress (as examples) are constants that could live in test-sol/constants.sol. Probably not worth refactoring in this PR, but something that might be worth doing separately.

@arthurgousset arthurgousset self-requested a review July 15, 2024 13:16
Copy link
Contributor

@arthurgousset arthurgousset left a comment

Choose a reason for hiding this comment

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

I'm not 100% sure I understand the original bug, and how this PR fixes it, so not super comfortable approving. But left some nit comments from reading through the PR.

@arthurgousset arthurgousset dismissed their stale review July 15, 2024 13:20

I mistakenly approved the PR when I meant to leave a comment

@soloseng
Copy link
Contributor Author

I'm not 100% sure I understand the original bug, and how this PR fixes it, so not super comfortable approving. But left some nit comments from reading through the PR.

Updated the PR description to make it more clear what the changes are and why they were needed.

Copy link
Contributor

@arthurgousset arthurgousset left a comment

Choose a reason for hiding this comment

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

Pending this conversation the changes look good to me. But, I'm always a little nervous approving changes to core contracts. So take my approval with a pinch of salt. I'd prefer if @martinvol or @pahor167 gave this an approval too.

@soloseng soloseng merged commit 562fcf0 into release/core-contracts/12 Jul 18, 2024
23 checks passed
@soloseng soloseng deleted the soloseng/add-check-to-distribution-schedule branch July 18, 2024 17:58
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.

4 participants