CVE-2023-42441: Improper Locking (CWE-667)
value: Vyper is a Pythonic Smart Contract Language for the Ethereum Virtual Machine (EVM). Starting in version 0.2.9 and prior to version 0.3.10, locks of the type `@nonreentrant("")` or `@nonreentrant('')` do not produce reentrancy checks at runtime. This issue is fixed in version 0.3.10. As a workaround, ensure the lock name is a non-empty string.
CWE description: The product does not properly acquire or release a lock on a resource, leading to unexpected resource state changes and behaviors.
CVSS v3 base score: 5.3
Published: 2023-09-18T21:16Z
Repository: https://github.com/vyperlang/vyper
Language: Python
Files changed: tests/parser/exceptions/test_structure_exception.py, tests/parser/features/decorators/test_nonreentrant.py, tests/parser/test_call_graph_stability.py, tests/parser/types/test_identifier_naming.py, vyper/ast/identifiers.py, vyper/exceptions.py, vyper/semantics/namespace.py, vyper/semantics/types/base.py, vyper/semantics/types/function.py
Commit date: 2023-09-15 22:01:03 +0000
Commit message: fix: only allow valid identifiers to be nonreentrant keys (#3605)

disallow invalid identifiers like `" "`, `"123abc"` from being keys for
non-reentrant locks.

this commit also refactors the `validate_identifiers` helper function to
be in the `ast/` subdirectory, and slightly improves the VyperException
constructor by allowing None (optional) annotations.
Diff: diff --git a/tests/parser/exceptions/test_structure_exception.py b/tests/parser/exceptions/test_structure_exception.py
index 08794b75f2..97ac2b139d 100644
--- a/tests/parser/exceptions/test_structure_exception.py
+++ b/tests/parser/exceptions/test_structure_exception.py
@@ -54,13 +54,30 @@ def double_nonreentrant():
 struct X:
     int128[5]: int128[7]
     """,
     """
 @external
-@nonreentrant("B")
-@nonreentrant("C")
-def double_nonreentrant():
+@nonreentrant(" ")
+def invalid_nonreentrant_key():
+    pass
+    """,
+    """
+@external
+@nonreentrant("")
+def invalid_nonreentrant_key():
+    pass
+    """,
+    """
+@external
+@nonreentrant("123")
+def invalid_nonreentrant_key():
+    pass
+    """,
+    """
+@external
+@nonreentrant("!123abcd")
+def invalid_nonreentrant_key():
     pass
     """,
     """
 @external
 def foo():
diff --git a/tests/parser/features/decorators/test_nonreentrant.py b/tests/parser/features/decorators/test_nonreentrant.py
index ac73b35bec..9e74019250 100644
--- a/tests/parser/features/decorators/test_nonreentrant.py
+++ b/tests/parser/features/decorators/test_nonreentrant.py
@@ -140,11 +140,11 @@ callback: public(Callback)
 def set_callback(c: address):
     self.callback = Callback(c)
 
 @external
 @payable
-@nonreentrant('default')
+@nonreentrant("lock")
 def protected_function(val: String[100], do_callback: bool) -> uint256:
     self.special_value = val
     _amount: uint256 = msg.value
     send(self.callback.address, msg.value)
 
@@ -164,11 +164,11 @@ def unprotected_function(val: String[100], do_callback: bool):
     if do_callback:
         self.callback.send_funds(_amount)
 
 @external
 @payable
-@nonreentrant('default')
+@nonreentrant("lock")
 def __default__():
     pass
     """
 
     reentrant_contract = get_contract(reentrant_code)
diff --git a/tests/parser/test_call_graph_stability.py b/tests/parser/test_call_graph_stability.py
index b651092d16..a6193610e2 100644
--- a/tests/parser/test_call_graph_stability.py
+++ b/tests/parser/test_call_graph_stability.py
@@ -4,12 +4,12 @@ import string
 import hypothesis.strategies as st
 import pytest
 from hypothesis import given, settings
 
 import vyper.ast as vy_ast
+from vyper.ast.identifiers import RESERVED_KEYWORDS
 from vyper.compiler.phases import CompilerData
-from vyper.semantics.namespace import RESERVED_KEYWORDS
 
 
 def _valid_identifier(attr):
     return attr not in RESERVED_KEYWORDS
 
diff --git a/tests/parser/types/test_identifier_naming.py b/tests/parser/types/test_identifier_naming.py
index f4f602f471..5cfc7e8ed7 100755
--- a/tests/parser/types/test_identifier_naming.py
+++ b/tests/parser/types/test_identifier_naming.py
@@ -1,12 +1,12 @@
 import pytest
 
 from vyper.ast.folding import BUILTIN_CONSTANTS
+from vyper.ast.identifiers import RESERVED_KEYWORDS
 from vyper.builtins.functions import BUILTIN_FUNCTIONS
 from vyper.codegen.expr import ENVIRONMENT_VARIABLES
 from vyper.exceptions import NamespaceCollision, StructureException, SyntaxException
-from vyper.semantics.namespace import RESERVED_KEYWORDS
 from vyper.semantics.types.primitives import AddressT
 
 BUILTIN_CONSTANTS = set(BUILTIN_CONSTANTS.keys())
 ALL_RESERVED_KEYWORDS = (
     BUILTIN_CONSTANTS | BUILTIN_FUNCTIONS | RESERVED_KEYWORDS | ENVIRONMENT_VARIABLES
diff --git a/vyper/ast/identifiers.py b/vyper/ast/identifiers.py
new file mode 100644
index 0000000000..985b04e5cd
--- /dev/null
+++ b/vyper/ast/identifiers.py
@@ -0,0 +1,111 @@
+import re
+
+from vyper.exceptions import StructureException
+
+
+def validate_identifier(attr, ast_node=None):
+    if not re.match("^[_a-zA-Z][a-zA-Z0-9_]*$", attr):
+        raise StructureException(f"'{attr}' contains invalid character(s)", ast_node)
+    if attr.lower() in RESERVED_KEYWORDS:
+        raise StructureException(f"'{attr}' is a reserved keyword", ast_node)
+
+
+# https://docs.python.org/3/reference/lexical_analysis.html#keywords
+# note we don't technically need to block all python reserved keywords,
+# but do it for hygiene
+_PYTHON_RESERVED_KEYWORDS = {
+    "False",
+    "None",
+    "True",
+    "and",
+    "as",
+    "assert",
+    "async",
+    "await",
+    "break",
+    "class",
+    "continue",
+    "def",
+    "del",
+    "elif",
+    "else",
+    "except",
+    "finally",
+    "for",
+    "from",
+    "global",
+    "if",
+    "import",
+    "in",
+    "is",
+    "lambda",
+    "nonlocal",
+    "not",
+    "or",
+    "pass",
+    "raise",
+    "return",
+    "try",
+    "while",
+    "with",
+    "yield",
+}
+_PYTHON_RESERVED_KEYWORDS = {s.lower() for s in _PYTHON_RESERVED_KEYWORDS}
+
+# Cannot be used for variable or member naming
+RESERVED_KEYWORDS = _PYTHON_RESERVED_KEYWORDS | {
+    # decorators
+    "public",
+    "external",
+    "nonpayable",
+    "constant",
+    "immutable",
+    "transient",
+    "internal",
+    "payable",
+    "nonreentrant",
+    # "class" keywords
+    "interface",
+    "struct",
+    "event",
+    "enum",
+    # EVM operations
+    "unreachable",
+    # special functions (no name mangling)
+    "init",
+    "_init_",
+    "___init___",
+    "____init____",
+    "default",
+    "_default_",
+    "___default___",
+    "____default____",
+    # more control flow and special operations
+    "range",
+    # more special operations
+    "indexed",
+    # denominations
+    "ether",
+    "wei",
+    "finney",
+    "szabo",
+    "shannon",
+    "lovelace",
+    "ada",
+    "babbage",
+    "gwei",
+    "kwei",
+    "mwei",
+    "twei",
+    "pwei",
+    # sentinal constant values
+    # TODO remove when these are removed from the language
+    "zero_address",
+    "empty_bytes32",
+    "max_int128",
+    "min_int128",
+    "max_decimal",
+    "min_decimal",
+    "max_uint256",
+    "zero_wei",
+}
diff --git a/vyper/exceptions.py b/vyper/exceptions.py
index aa23614e85..defca7cc53 100644
--- a/vyper/exceptions.py
+++ b/vyper/exceptions.py
@@ -52,11 +52,13 @@ class _BaseVyperException(Exception):
 
         if len(items) == 1 and isinstance(items[0], tuple) and isinstance(items[0][0], int):
             # support older exceptions that don't annotate - remove this in the future!
             self.lineno, self.col_offset = items[0][:2]
         else:
-            self.annotations = items
+            # strip out None sources so that None can be passed as a valid
+            # annotation (in case it is only available optionally)
+            self.annotations = [k for k in items if k is not None]
 
     def with_annotation(self, *annotations):
         """
         Creates a copy of this exception with a modified source annotation.
 
diff --git a/vyper/semantics/namespace.py b/vyper/semantics/namespace.py
index b88bc3d817..613ac0c03b 100644
--- a/vyper/semantics/namespace.py
+++ b/vyper/semantics/namespace.py
@@ -1,14 +1,9 @@
 import contextlib
-import re
-
-from vyper.exceptions import (
-    CompilerPanic,
-    NamespaceCollision,
-    StructureException,
-    UndeclaredDefinition,
-)
+
+from vyper.ast.identifiers import validate_identifier
+from vyper.exceptions import CompilerPanic, NamespaceCollision, UndeclaredDefinition
 from vyper.semantics.analysis.levenshtein_utils import get_levenshtein_error_suggestions
 
 
 class Namespace(dict):
     """
@@ -119,113 +114,5 @@ def override_global_namespace(ns):
         _namespace = ns
         yield
     finally:
         # unclobber
         _namespace = tmp
-
-
-def validate_identifier(attr):
-    if not re.match("^[_a-zA-Z][a-zA-Z0-9_]*$", attr):
-        raise StructureException(f"'{attr}' contains invalid character(s)")
-    if attr.lower() in RESERVED_KEYWORDS:
-        raise StructureException(f"'{attr}' is a reserved keyword")
-
-
-# https://docs.python.org/3/reference/lexical_analysis.html#keywords
-# note we don't technically need to block all python reserved keywords,
-# but do it for hygiene
-_PYTHON_RESERVED_KEYWORDS = {
-    "False",
-    "None",
-    "True",
-    "and",
-    "as",
-    "assert",
-    "async",
-    "await",
-    "break",
-    "class",
-    "continue",
-    "def",
-    "del",
-    "elif",
-    "else",
-    "except",
-    "finally",
-    "for",
-    "from",
-    "global",
-    "if",
-    "import",
-    "in",
-    "is",
-    "lambda",
-    "nonlocal",
-    "not",
-    "or",
-    "pass",
-    "raise",
-    "return",
-    "try",
-    "while",
-    "with",
-    "yield",
-}
-_PYTHON_RESERVED_KEYWORDS = {s.lower() for s in _PYTHON_RESERVED_KEYWORDS}
-
-# Cannot be used for variable or member naming
-RESERVED_KEYWORDS = _PYTHON_RESERVED_KEYWORDS | {
-    # decorators
-    "public",
-    "external",
-    "nonpayable",
-    "constant",
-    "immutable",
-    "transient",
-    "internal",
-    "payable",
-    "nonreentrant",
-    # "class" keywords
-    "interface",
-    "struct",
-    "event",
-    "enum",
-    # EVM operations
-    "unreachable",
-    # special functions (no name mangling)
-    "init",
-    "_init_",
-    "___init___",
-    "____init____",
-    "default",
-    "_default_",
-    "___default___",
-    "____default____",
-    # more control flow and special operations
-    "range",
-    # more special operations
-    "indexed",
-    # denominations
-    "ether",
-    "wei",
-    "finney",
-    "szabo",
-    "shannon",
-    "lovelace",
-    "ada",
-    "babbage",
-    "gwei",
-    "kwei",
-    "mwei",
-    "twei",
-    "pwei",
-    # sentinal constant values
-    # TODO remove when these are removed from the language
-    "zero_address",
-    "empty_bytes32",
-    "max_int128",
-    "min_int128",
-    "max_decimal",
-    "min_decimal",
-    "max_uint256",
-    "zero_wei",
-}
diff --git a/vyper/semantics/types/base.py b/vyper/semantics/types/base.py
index af955f6071..c5af5c2a39 100644
--- a/vyper/semantics/types/base.py
+++ b/vyper/semantics/types/base.py
@@ -1,20 +1,20 @@
 from functools import cached_property
 from typing import Any, Dict, Optional, Tuple, Union
 
 from vyper import ast as vy_ast
 from vyper.abi_types import ABIType
+from vyper.ast.identifiers import validate_identifier
 from vyper.exceptions import (
     CompilerPanic,
     InvalidLiteral,
     InvalidOperation,
     NamespaceCollision,
     StructureException,
     UnknownAttribute,
 )
 from vyper.semantics.analysis.levenshtein_utils import get_levenshtein_error_suggestions
-from vyper.semantics.namespace import validate_identifier
 
 
 # Some fake type with an overridden `compare_type` which accepts any RHS
 # type of type `type_`
 class _GenericTypeAcceptor:
diff --git a/vyper/semantics/types/function.py b/vyper/semantics/types/function.py
index 506dae135c..77b9efb13d 100644
--- a/vyper/semantics/types/function.py
+++ b/vyper/semantics/types/function.py
@@ -3,10 +3,11 @@ import warnings
 from dataclasses import dataclass
 from functools import cached_property
 from typing import Any, Dict, List, Optional, Tuple
 
 from vyper import ast as vy_ast
+from vyper.ast.identifiers import validate_identifier
 from vyper.ast.validation import validate_call_args
 from vyper.exceptions import (
     ArgumentException,
     CallViolation,
     CompilerPanic,
@@ -218,11 +219,14 @@ class ContractFunctionT(VyperType):
 
                     if node.name == "__init__":
                         msg = "Nonreentrant decorator disallowed on `__init__`"
                         raise FunctionDeclarationException(msg, decorator)
 
-                    kwargs["nonreentrant"] = decorator.args[0].value
+                    nonreentrant_key = decorator.args[0].value
+                    validate_identifier(nonreentrant_key, decorator.args[0])
+
+                    kwargs["nonreentrant"] = nonreentrant_key
 
                 elif isinstance(decorator, vy_ast.Name):
                     if FunctionVisibility.is_valid_value(decorator.id):
                         if "function_visibility" in kwargs:
                             raise FunctionDeclarationException(

Vulnerable code: @nonreentrant("B")
@nonreentrant("C")
def double_nonreentrant():
@nonreentrant('default')
@nonreentrant('default')
from vyper.semantics.namespace import RESERVED_KEYWORDS
from vyper.semantics.namespace import RESERVED_KEYWORDS
            self.annotations = items
import re

from vyper.exceptions import (
    CompilerPanic,
    NamespaceCollision,
    StructureException,
    UndeclaredDefinition,
)


def validate_identifier(attr):
    if not re.match("^[_a-zA-Z][a-zA-Z0-9_]*$", attr):
        raise StructureException(f"'{attr}' contains invalid character(s)")
    if attr.lower() in RESERVED_KEYWORDS:
        raise StructureException(f"'{attr}' is a reserved keyword")


# https://docs.python.org/3/reference/lexical_analysis.html#keywords
# note we don't technically need to block all python reserved keywords,
# but do it for hygiene
_PYTHON_RESERVED_KEYWORDS = {
    "False",
    "None",
    "True",
    "and",
    "as",
    "assert",
    "async",
    "await",
    "break",
    "class",
    "continue",
    "def",
    "del",
    "elif",
    "else",
    "except",
    "finally",
    "for",
    "from",
    "global",
    "if",
    "import",
    "in",
    "is",
    "lambda",
    "nonlocal",
    "not",
    "or",
    "pass",
    "raise",
    "return",
    "try",
    "while",
    "with",
    "yield",
}
_PYTHON_RESERVED_KEYWORDS = {s.lower() for s in _PYTHON_RESERVED_KEYWORDS}

# Cannot be used for variable or member naming
RESERVED_KEYWORDS = _PYTHON_RESERVED_KEYWORDS | {
    # decorators
    "public",
    "external",
    "nonpayable",
    "constant",
    "immutable",
    "transient",
    "internal",
    "payable",
    "nonreentrant",
    # "class" keywords
    "interface",
    "struct",
    "event",
    "enum",
    # EVM operations
    "unreachable",
    # special functions (no name mangling)
    "init",
    "_init_",
    "___init___",
    "____init____",
    "default",
    "_default_",
    "___default___",
    "____default____",
    # more control flow and special operations
    "range",
    # more special operations
    "indexed",
    # denominations
    "ether",
    "wei",
    "finney",
    "szabo",
    "shannon",
    "lovelace",
    "ada",
    "babbage",
    "gwei",
    "kwei",
    "mwei",
    "twei",
    "pwei",
    # sentinal constant values
    # TODO remove when these are removed from the language
    "zero_address",
    "empty_bytes32",
    "max_int128",
    "min_int128",
    "max_decimal",
    "min_decimal",
    "max_uint256",
    "zero_wei",
}
from vyper.semantics.namespace import validate_identifier
                    kwargs["nonreentrant"] = decorator.args[0].value
Fixed code: @nonreentrant(" ")
def invalid_nonreentrant_key():
    pass
    """,
    """
@external
@nonreentrant("")
def invalid_nonreentrant_key():
    pass
    """,
    """
@external
@nonreentrant("123")
def invalid_nonreentrant_key():
    pass
    """,
    """
@external
@nonreentrant("!123abcd")
def invalid_nonreentrant_key():
@nonreentrant("lock")
@nonreentrant("lock")
from vyper.ast.identifiers import RESERVED_KEYWORDS
from vyper.ast.identifiers import RESERVED_KEYWORDS
import re

from vyper.exceptions import StructureException


def validate_identifier(attr, ast_node=None):
    if not re.match("^[_a-zA-Z][a-zA-Z0-9_]*$", attr):
        raise StructureException(f"'{attr}' contains invalid character(s)", ast_node)
    if attr.lower() in RESERVED_KEYWORDS:
        raise StructureException(f"'{attr}' is a reserved keyword", ast_node)


# https://docs.python.org/3/reference/lexical_analysis.html#keywords
# note we don't technically need to block all python reserved keywords,
# but do it for hygiene
_PYTHON_RESERVED_KEYWORDS = {
    "False",
    "None",
    "True",
    "and",
    "as",
    "assert",
    "async",
    "await",
    "break",
    "class",
    "continue",
    "def",
    "del",
    "elif",
    "else",
    "except",
    "finally",
    "for",
    "from",
    "global",
    "if",
    "import",
    "in",
    "is",
    "lambda",
    "nonlocal",
    "not",
    "or",
    "pass",
    "raise",
    "return",
    "try",
    "while",
    "with",
    "yield",
}
_PYTHON_RESERVED_KEYWORDS = {s.lower() for s in _PYTHON_RESERVED_KEYWORDS}

# Cannot be used for variable or member naming
RESERVED_KEYWORDS = _PYTHON_RESERVED_KEYWORDS | {
    # decorators
    "public",
    "external",
    "nonpayable",
    "constant",
    "immutable",
    "transient",
    "internal",
    "payable",
    "nonreentrant",
    # "class" keywords
    "interface",
    "struct",
    "event",
    "enum",
    # EVM operations
    "unreachable",
    # special functions (no name mangling)
    "init",
    "_init_",
    "___init___",
    "____init____",
    "default",
    "_default_",
    "___default___",
    "____default____",
    # more control flow and special operations
    "range",
    # more special operations
    "indexed",
    # denominations
    "ether",
    "wei",
    "finney",
    "szabo",
    "shannon",
    "lovelace",
    "ada",
    "babbage",
    "gwei",
    "kwei",
    "mwei",
    "twei",
    "pwei",
    # sentinal constant values
    # TODO remove when these are removed from the language
    "zero_address",
    "empty_bytes32",
    "max_int128",
    "min_int128",
    "max_decimal",
    "min_decimal",
    "max_uint256",
    "zero_wei",
}
            # strip out None sources so that None can be passed as a valid
            # annotation (in case it is only available optionally)
            self.annotations = [k for k in items if k is not None]

from vyper.ast.identifiers import validate_identifier
from vyper.exceptions import CompilerPanic, NamespaceCollision, UndeclaredDefinition
from vyper.ast.identifiers import validate_identifier
from vyper.ast.identifiers import validate_identifier
                    nonreentrant_key = decorator.args[0].value
                    validate_identifier(nonreentrant_key, decorator.args[0])

                    kwargs["nonreentrant"] = nonreentrant_key