Merge pull request #12850 from ethereum/dataLocationInInheritance

Properly check data location in inheritance.
ethereum · May 17, 2022 · c5cc553 · c5cc553
2 parents bef348a + f427247
commit c5cc553
Show file tree

Hide file tree

Showing 20 changed files with 222 additions and 13 deletions.
diff --git a/Changelog.md b/Changelog.md
@@ -2,6 +2,7 @@
 
 Important Bugfixes:
  * ABI Encoder: When ABI-encoding values from calldata that contain nested arrays, correctly validate the nested array length against ``calldatasize()`` in all cases.
+ * Override Checker: Allow changing data location for parameters only when overriding external functions.
 
 
 Compiler Features:

diff --git a/docs/bugs.json b/docs/bugs.json
@@ -1,4 +1,15 @@
 [
+    {
+        "uid": "SOL-2022-3",
+        "name": "DataLocationChangeInInternalOverride",
+        "summary": "It was possible to change the data location of the parameters or return variables from ``calldata`` to ``memory`` and vice-versa while overriding internal and public functions. This caused invalid code to be generated when calling such a function internally through virtual function calls.",
+        "description": "When calling external functions, it is irrelevant if the data location of the parameters is ``calldata`` or ``memory``, the encoding of the data does not change. Because of that, changing the data location when overriding external functions is allowed. The compiler incorrectly also allowed a change in the data location for overriding public and internal functions. Since public functions can be called internally as well as externally, this causes invalid code to be generated when such an incorrectly overridden function is called internally through the base contract. The caller provides a memory pointer, but the called function interprets it as a calldata pointer or vice-versa.",
+        "link": "https://blog.soliditylang.org/2022/05/17/data-location-inheritance-bug/",
+        "introduced": "0.6.9",
+        "fixed": "0.8.14",
+        "severity": "very low"
+
+    },
     {
         "uid": "SOL-2022-2",
         "name": "NestedCallataArrayAbiReencodingSizeValidation",

diff --git a/docs/bugs_by_version.json b/docs/bugs_by_version.json
@@ -1345,6 +1345,7 @@
     },
     "0.6.10": {
         "bugs": [
+            "DataLocationChangeInInternalOverride",
             "NestedCallataArrayAbiReencodingSizeValidation",
             "SignedImmutables",
             "ABIDecodeTwoDimensionalArrayMemory",
@@ -1356,6 +1357,7 @@
     },
     "0.6.11": {
         "bugs": [
+            "DataLocationChangeInInternalOverride",
             "NestedCallataArrayAbiReencodingSizeValidation",
             "SignedImmutables",
             "ABIDecodeTwoDimensionalArrayMemory",
@@ -1367,6 +1369,7 @@
     },
     "0.6.12": {
         "bugs": [
+            "DataLocationChangeInInternalOverride",
             "NestedCallataArrayAbiReencodingSizeValidation",
             "SignedImmutables",
             "ABIDecodeTwoDimensionalArrayMemory",
@@ -1477,6 +1480,7 @@
     },
     "0.6.9": {
         "bugs": [
+            "DataLocationChangeInInternalOverride",
             "NestedCallataArrayAbiReencodingSizeValidation",
             "SignedImmutables",
             "ABIDecodeTwoDimensionalArrayMemory",
@@ -1489,6 +1493,7 @@
     },
     "0.7.0": {
         "bugs": [
+            "DataLocationChangeInInternalOverride",
             "NestedCallataArrayAbiReencodingSizeValidation",
             "SignedImmutables",
             "ABIDecodeTwoDimensionalArrayMemory",
@@ -1500,6 +1505,7 @@
     },
     "0.7.1": {
         "bugs": [
+            "DataLocationChangeInInternalOverride",
             "NestedCallataArrayAbiReencodingSizeValidation",
             "SignedImmutables",
             "ABIDecodeTwoDimensionalArrayMemory",
@@ -1512,6 +1518,7 @@
     },
     "0.7.2": {
         "bugs": [
+            "DataLocationChangeInInternalOverride",
             "NestedCallataArrayAbiReencodingSizeValidation",
             "SignedImmutables",
             "ABIDecodeTwoDimensionalArrayMemory",
@@ -1523,6 +1530,7 @@
     },
     "0.7.3": {
         "bugs": [
+            "DataLocationChangeInInternalOverride",
             "NestedCallataArrayAbiReencodingSizeValidation",
             "SignedImmutables",
             "ABIDecodeTwoDimensionalArrayMemory",
@@ -1533,6 +1541,7 @@
     },
     "0.7.4": {
         "bugs": [
+            "DataLocationChangeInInternalOverride",
             "NestedCallataArrayAbiReencodingSizeValidation",
             "SignedImmutables",
             "ABIDecodeTwoDimensionalArrayMemory",
@@ -1542,6 +1551,7 @@
     },
     "0.7.5": {
         "bugs": [
+            "DataLocationChangeInInternalOverride",
             "NestedCallataArrayAbiReencodingSizeValidation",
             "SignedImmutables",
             "ABIDecodeTwoDimensionalArrayMemory",
@@ -1551,6 +1561,7 @@
     },
     "0.7.6": {
         "bugs": [
+            "DataLocationChangeInInternalOverride",
             "NestedCallataArrayAbiReencodingSizeValidation",
             "SignedImmutables",
             "ABIDecodeTwoDimensionalArrayMemory",
@@ -1560,6 +1571,7 @@
     },
     "0.8.0": {
         "bugs": [
+            "DataLocationChangeInInternalOverride",
             "NestedCallataArrayAbiReencodingSizeValidation",
             "SignedImmutables",
             "ABIDecodeTwoDimensionalArrayMemory",
@@ -1569,6 +1581,7 @@
     },
     "0.8.1": {
         "bugs": [
+            "DataLocationChangeInInternalOverride",
             "NestedCallataArrayAbiReencodingSizeValidation",
             "SignedImmutables",
             "ABIDecodeTwoDimensionalArrayMemory",
@@ -1578,32 +1591,37 @@
     },
     "0.8.10": {
         "bugs": [
+            "DataLocationChangeInInternalOverride",
             "NestedCallataArrayAbiReencodingSizeValidation"
         ],
         "released": "2021-11-09"
     },
     "0.8.11": {
         "bugs": [
+            "DataLocationChangeInInternalOverride",
             "NestedCallataArrayAbiReencodingSizeValidation",
             "AbiEncodeCallLiteralAsFixedBytesBug"
         ],
         "released": "2021-12-20"
     },
     "0.8.12": {
         "bugs": [
+            "DataLocationChangeInInternalOverride",
             "NestedCallataArrayAbiReencodingSizeValidation",
             "AbiEncodeCallLiteralAsFixedBytesBug"
         ],
         "released": "2022-02-16"
     },
     "0.8.13": {
         "bugs": [
+            "DataLocationChangeInInternalOverride",
             "NestedCallataArrayAbiReencodingSizeValidation"
         ],
         "released": "2022-03-16"
     },
     "0.8.2": {
         "bugs": [
+            "DataLocationChangeInInternalOverride",
             "NestedCallataArrayAbiReencodingSizeValidation",
             "SignedImmutables",
             "ABIDecodeTwoDimensionalArrayMemory",
@@ -1613,6 +1631,7 @@
     },
     "0.8.3": {
         "bugs": [
+            "DataLocationChangeInInternalOverride",
             "NestedCallataArrayAbiReencodingSizeValidation",
             "SignedImmutables",
             "ABIDecodeTwoDimensionalArrayMemory"
@@ -1621,34 +1640,39 @@
     },
     "0.8.4": {
         "bugs": [
+            "DataLocationChangeInInternalOverride",
             "NestedCallataArrayAbiReencodingSizeValidation",
             "SignedImmutables"
         ],
         "released": "2021-04-21"
     },
     "0.8.5": {
         "bugs": [
+            "DataLocationChangeInInternalOverride",
             "NestedCallataArrayAbiReencodingSizeValidation",
             "SignedImmutables"
         ],
         "released": "2021-06-10"
     },
     "0.8.6": {
         "bugs": [
+            "DataLocationChangeInInternalOverride",
             "NestedCallataArrayAbiReencodingSizeValidation",
             "SignedImmutables"
         ],
         "released": "2021-06-22"
     },
     "0.8.7": {
         "bugs": [
+            "DataLocationChangeInInternalOverride",
             "NestedCallataArrayAbiReencodingSizeValidation",
             "SignedImmutables"
         ],
         "released": "2021-08-11"
     },
     "0.8.8": {
         "bugs": [
+            "DataLocationChangeInInternalOverride",
             "NestedCallataArrayAbiReencodingSizeValidation",
             "UserDefinedValueTypesBug",
             "SignedImmutables"
@@ -1657,6 +1681,7 @@
     },
     "0.8.9": {
         "bugs": [
+            "DataLocationChangeInInternalOverride",
             "NestedCallataArrayAbiReencodingSizeValidation"
         ],
         "released": "2021-09-29"

diff --git a/libsolidity/analysis/ContractLevelChecker.cpp b/libsolidity/analysis/ContractLevelChecker.cpp
@@ -39,7 +39,7 @@ namespace
 {
 
 template <class T, class B>
-bool hasEqualParameters(T const& _a, B const& _b)
+bool hasEqualExternalCallableParameters(T const& _a, B const& _b)
 {
 	return FunctionType(_a).asExternallyCallableFunction(false)->hasEqualParameterTypes(
 		*FunctionType(_b).asExternallyCallableFunction(false)
@@ -204,7 +204,7 @@ void ContractLevelChecker::findDuplicateDefinitions(map<string, vector<T>> const
 			SecondarySourceLocation ssl;
 
 			for (size_t j = i + 1; j < overloads.size(); ++j)
-				if (hasEqualParameters(*overloads[i], *overloads[j]))
+				if (hasEqualExternalCallableParameters(*overloads[i], *overloads[j]))
 				{
 					solAssert(
 						(

diff --git a/libsolidity/analysis/OverrideChecker.cpp b/libsolidity/analysis/OverrideChecker.cpp
@@ -313,7 +313,7 @@ Token OverrideProxy::functionKind() const
 	}, m_item);
 }
 
-FunctionType const* OverrideProxy::functionType() const
+FunctionType const* OverrideProxy::externalFunctionType() const
 {
 	return std::visit(GenericVisitor{
 		[&](FunctionDefinition const* _item) { return FunctionType(*_item).asExternallyCallableFunction(false); },
@@ -322,6 +322,15 @@ FunctionType const* OverrideProxy::functionType() const
 	}, m_item);
 }
 
+FunctionType const* OverrideProxy::originalFunctionType() const
+{
+	return std::visit(GenericVisitor{
+		[&](FunctionDefinition const* _item) { return TypeProvider::function(*_item); },
+		[&](VariableDeclaration const*) -> FunctionType const* { solAssert(false, "Requested specific function type of variable."); return nullptr; },
+		[&](ModifierDefinition const*) -> FunctionType const* { solAssert(false, "Requested specific function type of modifier."); return nullptr; }
+	}, m_item);
+}
+
 ModifierType const* OverrideProxy::modifierType() const
 {
 	return std::visit(GenericVisitor{
@@ -413,7 +422,7 @@ OverrideProxy::OverrideComparator const& OverrideProxy::overrideComparator() con
 			[&](FunctionDefinition const* _function)
 			{
 				vector<string> paramTypes;
-				for (Type const* t: functionType()->parameterTypes())
+				for (Type const* t: externalFunctionType()->parameterTypes())
 					paramTypes.emplace_back(t->richIdentifier());
 				return OverrideComparator{
 					_function->name(),
@@ -424,7 +433,7 @@ OverrideProxy::OverrideComparator const& OverrideProxy::overrideComparator() con
 			[&](VariableDeclaration const* _var)
 			{
 				vector<string> paramTypes;
-				for (Type const* t: functionType()->parameterTypes())
+				for (Type const* t: externalFunctionType()->parameterTypes())
 					paramTypes.emplace_back(t->richIdentifier());
 				return OverrideComparator{
 					_var->name(),
@@ -589,21 +598,54 @@ void OverrideChecker::checkOverride(OverrideProxy const& _overriding, OverridePr
 
 	if (_super.isFunction())
 	{
-		FunctionType const* functionType = _overriding.functionType();
-		FunctionType const* superType = _super.functionType();
+		FunctionType const* functionType = _overriding.externalFunctionType();
+		FunctionType const* superType = _super.externalFunctionType();
 
+		bool returnTypesDifferAlready = false;
 		if (_overriding.functionKind() != Token::Fallback)
 		{
 			solAssert(functionType->hasEqualParameterTypes(*superType), "Override doesn't have equal parameters!");
 
 			if (!functionType->hasEqualReturnTypes(*superType))
+			{
+				returnTypesDifferAlready = true;
 				overrideError(
 					_overriding,
 					_super,
 					4822_error,
 					"Overriding " + _overriding.astNodeName() + " return types differ.",
 					"Overridden " + _overriding.astNodeName() + " is here:"
 				);
+			}
+		}
+
+		// The override proxy considers calldata and memory the same data location.
+		// Here we do a more specific check:
+		// Data locations of parameters and return variables have to match
+		// unless we have a public function overriding an external one.
+		if (
+			_overriding.isFunction() &&
+			!returnTypesDifferAlready &&
+			_super.visibility() != Visibility::External &&
+			_overriding.functionKind() != Token::Fallback
+		)
+		{
+			if (!_overriding.originalFunctionType()->hasEqualParameterTypes(*_super.originalFunctionType()))
+				overrideError(
+					_overriding,
+					_super,
+					7723_error,
+					"Data locations of parameters have to be the same when overriding non-external functions, but they differ.",
+					"Overridden " + _overriding.astNodeName() + " is here:"
+				);
+			if (!_overriding.originalFunctionType()->hasEqualReturnTypes(*_super.originalFunctionType()))
+				overrideError(
+					_overriding,
+					_super,
+					1443_error,
+					"Data locations of return variables have to be the same when overriding non-external functions, but they differ.",
+					"Overridden " + _overriding.astNodeName() + " is here:"
+				);
 		}
 
 		// Stricter mutability is always okay except when super is Payable

diff --git a/libsolidity/analysis/OverrideChecker.h b/libsolidity/analysis/OverrideChecker.h
@@ -84,7 +84,10 @@ class OverrideProxy
 	/// @returns receive / fallback / function (only the latter for modifiers and variables);
 	langutil::Token functionKind() const;
 
-	FunctionType const* functionType() const;
+	/// @returns the externally callable function type
+	FunctionType const* externalFunctionType() const;
+	/// @returns the (unmodified) function type
+	FunctionType const* originalFunctionType() const;
 	ModifierType const* modifierType() const;
 
 	Declaration const* declaration() const;
@@ -101,6 +104,7 @@ class OverrideProxy
 
 	/**
 	 * Struct to help comparing override items about whether they override each other.
+	 * Compares functions based on their "externally callable" type.
 	 * Does not produce a total order.
 	 */
 	struct OverrideComparator

diff --git a/libsolidity/ast/AST.cpp b/libsolidity/ast/AST.cpp
@@ -480,7 +480,9 @@ FunctionDefinition const& FunctionDefinition::resolveVirtual(
 	solAssert(isOrdinary(), "");
 	solAssert(!libraryFunction(), "");
 
-	FunctionType const* functionType = TypeProvider::function(*this)->asExternallyCallableFunction(false);
+	// We actually do not want the externally callable function here.
+	// This is just to add an assertion since the comparison used to be less strict.
+	FunctionType const* externalFunctionType = TypeProvider::function(*this)->asExternallyCallableFunction(false);
 
 	bool foundSearchStart = (_searchStart == nullptr);
 	for (ContractDefinition const* c: _mostDerivedContract.annotation().linearizedBaseContracts)
@@ -495,9 +497,12 @@ FunctionDefinition const& FunctionDefinition::resolveVirtual(
 				// With super lookup analysis guarantees that there is an implemented function in the chain.
 				// With virtual lookup there are valid cases where returning an unimplemented one is fine.
 				(function->isImplemented() || _searchStart == nullptr) &&
-				FunctionType(*function).asExternallyCallableFunction(false)->hasEqualParameterTypes(*functionType)
+				FunctionType(*function).asExternallyCallableFunction(false)->hasEqualParameterTypes(*externalFunctionType)
 			)
+			{
+				solAssert(FunctionType(*function).hasEqualParameterTypes(*TypeProvider::function(*this)));
 				return *function;
+			}
 	}
 
 	solAssert(false, "Virtual function " + name() + " not found.");

diff --git a/test/externalTests/bleeps.sh b/test/externalTests/bleeps.sh
@@ -86,6 +86,14 @@ function bleeps_test
     npm install npm-run-all
     npm install
 
+    # TODO: Bleeps depends on OpenZeppelin 4.3.2, which is affected by
+    # https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3293.
+    # Forcing OZ >= 4.6.0 fixes this but it also causes a lot of unrelated compilation errors.
+    # Remove this when Bleeps gets updated to support newer OpenZeppelin.
+    perl -i -0pe \
+        "s/(function hashProposal\(\n        address\[\] )calldata( targets,\n        uint256\[\] )calldata( values,\n        bytes\[\] )calldata( calldatas,)/\1memory\2memory\3memory\4/g" \
+        node_modules/@openzeppelin/contracts/governance/IGovernor.sol
+
     replace_version_pragmas
 
     for preset in $SELECTED_PRESETS; do