feat: semgrep rules and check for reinitializer modifiers

.circleci/config.yml

@@ -900,6 +900,8 @@ jobs:
           command: snapshots-check-no-build
       - run-contracts-check:
           command: interfaces-check-no-build
+      - run-contracts-check:
+          command: reinitializer-check-no-build
       - run-contracts-check:
           command: size-check
       - run-contracts-check:

.semgrep/rules/sol-rules.yaml

@@ -249,7 +249,7 @@ rules:
   - id: sol-safety-proper-initializer
     languages: [solidity]
     severity: ERROR
-    message: Proxied contracts must have an initialize function with the initializer modifier and external visibility
+    message: Proxied contracts must have an initialize function with the initializer or reinitializer modifier and external or public visibility
     patterns:
       - pattern-regex: "///\\s*@custom:proxied\\s+true(?P<CONTRACT>[\\s\\S]*)"
       - focus-metavariable: $CONTRACT
@@ -261,7 +261,34 @@ rules:
           function initialize(...) external initializer {
             ...
           }
+      - pattern-not: |
+          function initialize(...) public initializer {
+            ...
+          }
+      - pattern-not: |
+          function initialize(...) external reinitializer(...) {
+            ...
+          }
+      - pattern-not: |
+          function initialize(...) public reinitializer(...) {
+            ...
+          }
     paths:
       exclude:
-        - packages/contracts-bedrock/src/L1/SystemConfig.sol
         - packages/contracts-bedrock/src/L1/SystemConfigInterop.sol
+
+  - id: sol-safety-proper-upgrade-function
+    languages: [solidity]
+    severity: ERROR
+    message: Upgrade functions must be external and have the reinitializer modifier
+    patterns:
+      - pattern-regex: "///\\s*@custom:proxied\\s+true(?P<CONTRACT>[\\s\\S]*)"
+      - focus-metavariable: $CONTRACT
+      - pattern: |
+          function upgrade(...) {
+            ...
+          }
+      - pattern-not: |
+          function upgrade(...) external reinitializer(...) {
+            ...
+          }

.semgrep/tests/sol-rules.sol-safety-proper-initializer.t.sol

@@ -32,13 +32,28 @@ contract SemgrepTest__sol_safety_proper_initializer {
         // ...
     }
 
-    // ruleid: sol-safety-proper-initializer
-    function initialize() external {
+    // ok: sol-safety-proper-initializer
+    function initialize() public initializer {
+        // ...
+    }
+
+    // ok: sol-safety-proper-initializer
+    function initialize() external reinitializer(1) {
+        // ...
+    }
+
+    // ok: sol-safety-proper-initializer
+    function initialize() external reinitializer(1) {
+        // ...
+    }
+
+    // ok: sol-safety-proper-initializer
+    function initialize() public reinitializer(2) {
         // ...
     }
 
     // ruleid: sol-safety-proper-initializer
-    function initialize() public initializer {
+    function initialize() internal {
         // ...
     }
 

.semgrep/tests/sol-rules.sol-safety-proper-upgrade-function.t.sol

@@ -0,0 +1,64 @@
+// 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, upgrade functions can have no upgrade modifier and be public or external
+contract SemgrepTest__sol_safety_proper_upgrade_function_1 {
+    // ok: sol-safety-proper-upgrade-function
+    function upgrade() external {
+        // ...
+    }
+
+    // ok: sol-safety-proper-upgrade-function
+    function upgrade() public {
+        // ...
+    }
+}
+
+/// NOTE: the proxied natspec below is valid for all contracts after this one
+/// @custom:proxied true
+contract SemgrepTest__sol_safety_proper_upgrade_function_2 {
+    // ok: sol-safety-proper-upgrade-function
+    function upgrade() external reinitializer(1) {
+        // ...
+    }
+
+    // ok: sol-safety-proper-upgrade-function
+    function upgrade() external reinitializer(1) {
+        // ...
+    }
+
+    // ruleid: sol-safety-proper-upgrade-function
+    function upgrade() public reinitializer(2) {
+        // ...
+    }
+
+    // ruleid: sol-safety-proper-upgrade-function
+    function upgrade() external initializer {
+        // ...
+    }
+
+    // ruleid: sol-safety-proper-upgrade-function
+    function upgrade() public initializer {
+        // ...
+    }
+
+    // ruleid: sol-safety-proper-upgrade-function
+    function upgrade() external {
+        // ...
+    }
+
+    // ruleid: sol-safety-proper-upgrade-function
+    function upgrade() public {
+        // ...
+    }
+}

op-chain-ops/solc/types.go

@@ -201,11 +201,13 @@ type AstNode struct {
 	Value    interface{} `json:"value,omitempty"`
 
 	// Other fields
-	Arguments []Expression `json:"arguments,omitempty"`
-	Condition *Expression  `json:"condition,omitempty"`
-	TrueBody  *AstBlock    `json:"trueBody,omitempty"`
-	FalseBody *AstBlock    `json:"falseBody,omitempty"`
-	Operator  string       `json:"operator,omitempty"`
+	ModifierName *Expression  `json:"modifierName,omitempty"`
+	Modifiers    []AstNode    `json:"modifiers,omitempty"`
+	Arguments    []Expression `json:"arguments,omitempty"`
+	Condition    *Expression  `json:"condition,omitempty"`
+	TrueBody     *AstBlock    `json:"trueBody,omitempty"`
+	FalseBody    *AstBlock    `json:"falseBody,omitempty"`
+	Operator     string       `json:"operator,omitempty"`
 }
 
 type AstBaseContract struct {
@@ -259,6 +261,7 @@ type Expression struct {
 	OverloadedDeclarations []int                 `json:"overloadedDeclarations,omitempty"`
 	ReferencedDeclaration  int                   `json:"referencedDeclaration,omitempty"`
 	ArgumentTypes          []AstTypeDescriptions `json:"argumentTypes,omitempty"`
+	Value                  interface{}           `json:"value,omitempty"`
 }
 
 type ForgeArtifact struct {

op-deployer/pkg/deployer/upgrade/v2_0_0/testdata/config.json

@@ -8,4 +8,4 @@
       "absolutePrestate": "0x0000000000000000000000000000000000000000000000000000000000000abc"
     }
   ]
-}
+}

packages/contracts-bedrock/justfile

@@ -212,6 +212,14 @@ interfaces-check-no-build:
 # artifacts can cause the script to detect issues incorrectly.
 interfaces-check: clean build interfaces-check-no-build
 
+# Checks that all upgrade/initialize funcitons have proper reinitializer modifiers.
+reinitializer-check: build-source reinitializer-check-no-build
+
+# Checks that all upgrade/initialize funcitons have proper reinitializer modifiers.
+# Does not build contracts.
+reinitializer-check-no-build:
+  go run ./scripts/checks/reinitializer
+
 # Checks that the size of the contracts is within the limit.
 size-check:
   forge build --sizes --skip "/**/test/**" --skip "/**/scripts/**"

packages/contracts-bedrock/scripts/checks/reinitializer/main.go

@@ -0,0 +1,119 @@
+package main
+
+import (
+	"fmt"
+	"os"
+	"strconv"
+	"strings"
+
+	"github.com/ethereum-optimism/optimism/op-chain-ops/solc"
+	"github.com/ethereum-optimism/optimism/packages/contracts-bedrock/scripts/checks/common"
+)
+
+func main() {
+	if _, err := common.ProcessFilesGlob(
+		[]string{"forge-artifacts/**/*.json"},
+		[]string{},
+		processFile,
+	); err != nil {
+		fmt.Printf("Error: %v\n", err)
+		os.Exit(1)
+	}
+}
+
+func processFile(path string) (*common.Void, []error) {
+	artifact, err := common.ReadForgeArtifact(path)
+	if err != nil {
+		return nil, []error{err}
+	}
+
+	if err := checkArtifact(artifact); err != nil {
+		return nil, []error{err}
+	}
+
+	return nil, nil
+}
+
+func checkArtifact(artifact *solc.ForgeArtifact) error {
+	// Skip interfaces.
+	if strings.HasPrefix(artifact.Ast.AbsolutePath, "interfaces/") {
+		return nil
+	}
+
+	// Skip if we have no upgrade function.
+	upgradeFn := getFunctionByName(artifact, "upgrade")
+	if upgradeFn == nil {
+		return nil
+	}
+
+	// We can have an upgrade function without an initialize function.
+	initializeFn := getFunctionByName(artifact, "initialize")
+	if initializeFn == nil {
+		return nil
+	}
+
+	// Grab the reinitializer value from the upgrade function.
+	upgradeFnReinitializerValue, err := getReinitializerValue(upgradeFn)
+	if err != nil {
+		return fmt.Errorf("error getting reinitializer value from upgrade function: %w", err)
+	}
+
+	// Grab the reinitializer value from the initialize function.
+	initializeFnReinitializerValue, err := getReinitializerValue(initializeFn)
+	if err != nil {
+		return fmt.Errorf("error getting reinitializer value from initialize function: %w", err)
+	}
+
+	// If the reinitializer values are different, return an error.
+	if upgradeFnReinitializerValue != initializeFnReinitializerValue {
+		return fmt.Errorf("upgrade function and initialize function have different reinitializer values")
+	}
+
+	return nil
+}
+
+func getContractDefinition(artifact *solc.ForgeArtifact) *solc.AstNode {
+	for _, node := range artifact.Ast.Nodes {
+		if node.NodeType == "ContractDefinition" {
+			return &node
+		}
+	}
+	return nil
+}
+
+func getFunctionByName(artifact *solc.ForgeArtifact, name string) *solc.AstNode {
+	contract := getContractDefinition(artifact)
+	if contract == nil {
+		return nil
+	}
+	for _, node := range contract.Nodes {
+		if node.NodeType == "FunctionDefinition" {
+			if node.Name == name {
+				return &node
+			}
+		}
+	}
+	return nil
+}
+
+func getReinitializerValue(node *solc.AstNode) (uint64, error) {
+	if node.Modifiers == nil {
+		return 0, fmt.Errorf("no modifiers found")
+	}
+
+	for _, modifier := range node.Modifiers {
+		if modifier.ModifierName.Name == "reinitializer" {
+			valStr, ok := modifier.Arguments[0].Value.(string)
+			if !ok {
+				return 0, fmt.Errorf("reinitializer value is not a string")
+			}
+			val, err := strconv.Atoi(valStr)
+			if err != nil {
+				return 0, fmt.Errorf("reinitializer value is not an integer")
+			}
+			return uint64(val), nil
+		}
+	}
+
+	return 0, fmt.Errorf("reinitializer modifier not found")
+}