Commit ea5d8305 authored by Michael Amadi's avatar Michael Amadi Committed by GitHub

Proxy disable initializers check (#13317)

* add script that checks that proxied (non predeploy) contracts call `_disableInitializers` in their constructor

* add check that all initialize functions of proxied contracts (not predeploys) have the external and initializer modifiers

* add to ctb justfile but not in `just check` yet

* fixes

* use semgrep instead

* ...

* fixes

* fixes

* fixes

* fixes

* fixes

* fix comment on semgrep test

* fix semgrep test

* rename rule

* rename rule

* add exception
parent 713dc46b
......@@ -188,3 +188,46 @@ rules:
paths:
exclude:
- packages/contracts-bedrock/test
- id: sol-safety-use-disable-initializer
languages: [solidity]
severity: ERROR
message: Proxied contracts (excluding predeploys) must disable initializers in constructor
patterns:
- pattern-regex: "///\\s*@custom:proxied\\s+true(?P<CONTRACT>[\\s\\S]*)"
- pattern-not-regex: "///\\s*@custom:predeploy.*(?P<REST>[\\s\\S]*)"
- focus-metavariable: $CONTRACT
- pattern: |
constructor(...) {
...
}
- pattern-not: |
constructor(...) {
...
_disableInitializers();
...
}
paths:
exclude:
- packages/contracts-bedrock/src/L1/SystemConfigInterop.sol
- packages/contracts-bedrock/src/L1/OptimismPortalInterop.sol
- id: sol-safety-proper-initializer
languages: [solidity]
severity: ERROR
message: Proxied contracts must have an initialize function with the initializer modifier and external visibility
patterns:
- pattern-regex: "///\\s*@custom:proxied\\s+true(?P<CONTRACT>[\\s\\S]*)"
- focus-metavariable: $CONTRACT
- pattern: |
function initialize(...) {
...
}
- pattern-not: |
function initialize(...) external initializer {
...
}
paths:
exclude:
- packages/contracts-bedrock/src/L1/SystemConfig.sol
- packages/contracts-bedrock/src/L1/SystemConfigInterop.sol
// Semgrep tests for Solidity rules are defined in this file.
// Semgrep tests do not need to be valid Solidity code but should be syntactically correct so that
// Semgrep can parse them. You don't need to be able to *run* the code here but it should look like
// the code that you expect to catch with the rule.
//
// Semgrep testing 101
// Use comments like "ruleid: <rule-id>" to assert that the rule catches the code.
// Use comments like "ok: <rule-id>" to assert that the rule does not catch the code.
/// NOTE: Semgrep limitations mean that the rule for this check is defined as a relatively loose regex that searches the
/// remainder of the file after the `@custom:proxied` natspec tag is detected. This means that we must test the case
/// without this natspec tag BEFORE the case with the tag or the rule will apply to the remainder of the file.
// If no proxied natspec, initialize functions can have no initializer modifier and be public or external
contract SemgrepTest__sol_safety_proper_initializer {
// ok: sol-safety-proper-initializer
function initialize() external {
// ...
}
// ok: sol-safety-proper-initializer
function initialize() public {
// ...
}
}
/// NOTE: the proxied natspec below is valid for all contracts after this one
/// @custom:proxied true
contract SemgrepTest__sol_safety_proper_initializer {
// ok: sol-safety-proper-initializer
function initialize() external initializer {
// ...
}
// ruleid: sol-safety-proper-initializer
function initialize() external {
// ...
}
// ruleid: sol-safety-proper-initializer
function initialize() public initializer {
// ...
}
// ruleid: sol-safety-proper-initializer
function initialize() public {
// ...
}
}
// Semgrep tests for Solidity rules are defined in this file.
// Semgrep tests do not need to be valid Solidity code but should be syntactically correct so that
// Semgrep can parse them. You don't need to be able to *run* the code here but it should look like
// the code that you expect to catch with the rule.
//
// Semgrep testing 101
// Use comments like "ruleid: <rule-id>" to assert that the rule catches the code.
// Use comments like "ok: <rule-id>" to assert that the rule does not catch the code.
/// NOTE: Semgrep limitations mean that the rule for this check is defined as a relatively loose regex that searches the
/// remainder of the file after the `@custom:proxied` natspec tag is detected.
/// This means that we must test the case without this natspec tag BEFORE the case with the tag or the rule will apply
/// to the remainder of the file.
// If no predeploy natspec, disableInitializers can or cannot be called in constructor
contract SemgrepTest__sol_safety_use_disable_initializer {
// ok: sol-safety-use-disable-initializer
constructor() {
// ...
_disableInitializers();
// ...
}
// ok: sol-safety-use-disable-initializer
constructor() {
// ...
}
}
// if no predeploy natspec, disableInitializers must be called in constructor
/// @custom:proxied true
contract SemgrepTest__sol_safety_use_disable_initializer {
// ok: sol-safety-use-disable-initializer
constructor() {
// ...
_disableInitializers();
// ...
}
// ruleid: sol-safety-use-disable-initializer
constructor() {
// ...
}
}
/// NOTE: the predeploy natspec below is valid for all contracts after this one
/// @custom:predeploy
// if predeploy natspec, disableInitializers may or may not be called in constructor
contract SemgrepTest__sol_safety_use_disable_initializer {
// ok: sol-safety-use-disable-initializer
constructor() {
// ...
}
// ok: sol-safety-use-disable-initializer
constructor() {
// ...
_disableInitializers();
// ...
}
}
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment