-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
feat: semgrep rules and check for reinitializer modifiers
Adds semgrep rules and a new golang contracts check for reinitializer modifiers. Important safety checks now that we are using upgrade functions.
- Loading branch information
1 parent
f45e785
commit d5e28ee
Showing
9 changed files
with
746 additions
and
11 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
64 changes: 64 additions & 0 deletions
64
.semgrep/tests/sol-rules.sol-safety-proper-upgrade-function.t.sol
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 { | ||
| // ... | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,4 +8,4 @@ | |
| "absolutePrestate": "0x0000000000000000000000000000000000000000000000000000000000000abc" | ||
| } | ||
| ] | ||
| } | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
119 changes: 119 additions & 0 deletions
119
packages/contracts-bedrock/scripts/checks/reinitializer/main.go
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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") | ||
| } |
Oops, something went wrong.