diff --git a/.semgrep/rules/sol-rules.yaml b/.semgrep/rules/sol-rules.yaml index 729fb7e199291..3fe1754098d3e 100644 --- a/.semgrep/rules/sol-rules.yaml +++ b/.semgrep/rules/sol-rules.yaml @@ -421,3 +421,37 @@ rules: paths: include: - packages/contracts-bedrock/src/L1/opcm/OPContractsManagerV2.sol + + - id: sol-safety-initialize-assert-proxy-admin + languages: [solidity] + severity: ERROR + message: Initialize functions must call _assertOnlyProxyAdminOrProxyAdminOwner() to restrict initialization to authorized callers + patterns: + - pattern: | + function initialize(...) { + ... + } + - pattern-not: | + function initialize(...) { + ... + _assertOnlyProxyAdminOrProxyAdminOwner(); + ... + } + paths: + include: + - packages/contracts-bedrock/src + exclude: + # TODO(#19001): We should eventually have this on the L2 contracts. + - packages/contracts-bedrock/src/L2 + - packages/contracts-bedrock/src/universal/OptimismMintableERC20Factory.sol + # Dispute games use initialize() in a different context - they are clone-deployed + # via DisputeGameFactory and use their own AlreadyInitialized check pattern. + - packages/contracts-bedrock/src/dispute/FaultDisputeGame.sol + - packages/contracts-bedrock/src/dispute/PermissionedDisputeGame.sol + - packages/contracts-bedrock/src/dispute/SuperFaultDisputeGame.sol + - packages/contracts-bedrock/src/dispute/SuperPermissionedDisputeGame.sol + - packages/contracts-bedrock/src/dispute/zk/OptimisticZkGame.sol + # ProtocolVersions is being deprecated. + - packages/contracts-bedrock/src/L1/ProtocolVersions.sol + # DataAvailabilityChallenge is a beta/non-standard contract. + - packages/contracts-bedrock/src/L1/DataAvailabilityChallenge.sol diff --git a/.semgrep/tests/sol-rules.sol-safety-initialize-assert-proxy-admin.t.sol b/.semgrep/tests/sol-rules.sol-safety-initialize-assert-proxy-admin.t.sol new file mode 100644 index 0000000000000..aee6968dabe34 --- /dev/null +++ b/.semgrep/tests/sol-rules.sol-safety-initialize-assert-proxy-admin.t.sol @@ -0,0 +1,45 @@ +// 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: " to assert that the rule catches the code. +// Use comments like "ok: " to assert that the rule does not catch the code. + +contract SemgrepTest__sol_safety_initialize_assert_proxy_admin { + // ok: sol-safety-initialize-assert-proxy-admin + function initialize(address _param) external reinitializer(1) { + _assertOnlyProxyAdminOrProxyAdminOwner(); + _setParam(_param); + } + + // ok: sol-safety-initialize-assert-proxy-admin + function initialize() external initializer { + _assertOnlyProxyAdminOrProxyAdminOwner(); + } + + // ok: sol-safety-initialize-assert-proxy-admin + function initialize(address _a, uint256 _b) public reinitializer(2) { + // Comment before assertion + _assertOnlyProxyAdminOrProxyAdminOwner(); + // More initialization + param = _a; + } + + // ruleid: sol-safety-initialize-assert-proxy-admin + function initialize(address _param) external reinitializer(1) { + _setParam(_param); + } + + // ruleid: sol-safety-initialize-assert-proxy-admin + function initialize() external initializer { + __Ownable_init(); + } + + // ruleid: sol-safety-initialize-assert-proxy-admin + function initialize(address _a, uint256 _b) public initializer { + param = _a; + value = _b; + } +} diff --git a/.semgrep/tests/sol-rules.sol-safety-proper-initializer.t.sol b/.semgrep/tests/sol-rules.sol-safety-proper-initializer.t.sol index 23ad245b1e912..fd5fa665369d7 100644 --- a/.semgrep/tests/sol-rules.sol-safety-proper-initializer.t.sol +++ b/.semgrep/tests/sol-rules.sol-safety-proper-initializer.t.sol @@ -15,11 +15,13 @@ contract SemgrepTest__sol_safety_proper_initializer { // ok: sol-safety-proper-initializer function initialize() external { + _assertOnlyProxyAdminOrProxyAdminOwner(); // ... } // ok: sol-safety-proper-initializer function initialize() public { + _assertOnlyProxyAdminOrProxyAdminOwner(); // ... } } @@ -29,36 +31,43 @@ contract SemgrepTest__sol_safety_proper_initializer { contract SemgrepTest__sol_safety_proper_initializer { // ok: sol-safety-proper-initializer function initialize() external initializer { + _assertOnlyProxyAdminOrProxyAdminOwner(); // ... } // ok: sol-safety-proper-initializer function initialize() public initializer { + _assertOnlyProxyAdminOrProxyAdminOwner(); // ... } // ok: sol-safety-proper-initializer function initialize() external reinitializer(1) { + _assertOnlyProxyAdminOrProxyAdminOwner(); // ... } // ok: sol-safety-proper-initializer function initialize() external reinitializer(1) { + _assertOnlyProxyAdminOrProxyAdminOwner(); // ... } // ok: sol-safety-proper-initializer function initialize() public reinitializer(2) { + _assertOnlyProxyAdminOrProxyAdminOwner(); // ... } // ruleid: sol-safety-proper-initializer function initialize() internal { + _assertOnlyProxyAdminOrProxyAdminOwner(); // ... } // ruleid: sol-safety-proper-initializer function initialize() public { + _assertOnlyProxyAdminOrProxyAdminOwner(); // ... } }