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

fix: add security contact to various contract NatSpecs (OZ N-03) #1028

Merged
merged 8 commits into from
Oct 1, 2024

Conversation

MoonBoi9001
Copy link
Member

Motivation:

Title:

N-03 Lack of Security Contact

Details:

Providing a specific security contact (such as an email or ENS name) within a smart contract significantly simplifies the process for individuals to communicate if they identify a vulnerability in the code. This practice is quite beneficial as it permits the code owners to dictate the communication channel for vulnerability disclosure, eliminating the risk of miscommunication or failure to report due to a lack of knowledge on how to do so. In addition, if the contract incorporates third-party libraries and a bug surfaces in those, it becomes easier for their maintainers to contact the appropriate person about the problem and provide mitigation instructions.

Throughout the codebase, multiple instances of contracts without a security contact were identified:

The DisputeManager contract
The GraphPayments contract
The HorizonStaking contract
The HorizonStakingExtension contract
The PaymentsEscrow contract
The SubgraphService contract
The TAPCollector contract

Consider adding a NatSpec comment containing a security contact above each contract definition. Using the @Custom:security-contact convention is recommended as it has been adopted by the OpenZeppelin Wizard and the ethereum-lists.

Key changes

  • add security contact to DisputeManager.sol
  • add security contact to GraphPayments.sol
  • add security contact to HorizonStaking.sol
  • add security contact to HorizonStakingExtension.sol
  • add security contact to PaymentsEscrow.sol
  • add security contact to SubgraphService.sol + add NatSpec before contract def
  • add security contact to TAPCollector.sol

Copy link

openzeppelin-code bot commented Sep 17, 2024

fix: add security contact to various contract NatSpecs (OZ N-03)

Generated at commit: 8ce493f79e179a6cdd35b9f12c3f8f74056852f3

🚨 Report Summary

Severity Level Results
Contracts Critical
High
Medium
Low
Note
Total
2
4
0
15
41
62
Dependencies Critical
High
Medium
Low
Note
Total
0
0
0
0
0
0

For more details view the full report in OpenZeppelin Code Inspector

@MoonBoi9001
Copy link
Member Author

MoonBoi9001 commented Sep 18, 2024

I'm unsure why this PR is failing the

  • CI - packages/horizon / test-ci (pull_request)

the other PR 1029 passed all checks implying that I introduced an issue in this test specifically, rather than the issue already being part of the horizon branch that I'm trying to merge into. It appears the issue is with the test_RevertWhen_TheContractIsDeployedWithAnInvalidController where the test expects the function to revert but it's not reverting.

@MoonBoi9001
Copy link
Member Author

MoonBoi9001 commented Sep 19, 2024

Re-ran the test and it passed, so looks like a non-deterministic bug.

First test run
second test run

Highlighting part of the test-ci error message:

Ran 5 tests for test/utilities/GraphDirectory.t.sol:GraphDirectoryTest
[PASS] test_RevertWhen_AnInvalidContractGetterIsCalled() (gas: 44372)
[FAIL: next call did not revert as expected; counterexample: calldata=0x38e51f930000000000000000000000000000000000000000000000000000000000000003 args=
[0x0000000000000000000000000000000000000003]] test_RevertWhen_TheContractIsDeployedWithAnInvalidController(address) (runs: 13, μ: 42142, ~: 42142)

@MoonBoi9001
Copy link
Member Author

This PR will be marked as draft while we workout the security email contact.

@MoonBoi9001 MoonBoi9001 marked this pull request as draft September 19, 2024 13:29
@MoonBoi9001 MoonBoi9001 marked this pull request as ready for review September 30, 2024 16:10
@MoonBoi9001 MoonBoi9001 merged commit 157216a into horizon Oct 1, 2024
9 checks passed
@MoonBoi9001 MoonBoi9001 deleted the fix_oz_n-03 branch October 1, 2024 15:45
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.

2 participants