Merge pull request #8924 from ethereum/arraySliceBug

Fix array slice bug.

Changelog.md

@@ -2,6 +2,7 @@
 
 Important Bugfixes:
  * Add missing callvalue check to the creation code of a contract that does not define a constructor but has a base that does define a constructor.
+ * Disallow index range accesses for arrays with dynamically encoded base types.
 
 
 Language Features:

docs/bugs.json

@@ -1,4 +1,15 @@
 [
+    {
+        "name": "ArraySliceDynamicallyEncodedBaseType",
+        "summary": "Accessing array slices of arrays with dynamically encoded base types (e.g. multi-dimensional arrays) can result in invalid data being read.",
+        "description": "For arrays with dynamically sized base types, index range accesses that use a start expression that is non-zero will result in invalid array slices. Any index access to such array slices will result in data being read from incorrect calldata offsets. Array slices are only supported for dynamic calldata types and all problematic type require ABIEncoderV2 to be enabled.",
+        "introduced": "0.6.0",
+        "fixed": "0.6.8",
+        "severity": "very low",
+        "conditions": {
+            "ABIEncoderV2": true
+        }
+    },
     {
         "name": "ImplicitConstructorCallvalueCheck",
         "summary": "The creation code of a contract that does not define a constructor but has a base that does define a constructor did not revert for calls with non-zero value.",

docs/bugs_by_version.json

@@ -1082,6 +1082,7 @@
     },
     "0.6.0": {
         "bugs": [
+            "ArraySliceDynamicallyEncodedBaseType",
             "ImplicitConstructorCallvalueCheck",
             "TupleAssignmentMultiStackSlotComponents",
             "MemoryArrayCreationOverflow",
@@ -1091,6 +1092,7 @@
     },
     "0.6.1": {
         "bugs": [
+            "ArraySliceDynamicallyEncodedBaseType",
             "ImplicitConstructorCallvalueCheck",
             "TupleAssignmentMultiStackSlotComponents",
             "MemoryArrayCreationOverflow"
@@ -1099,6 +1101,7 @@
     },
     "0.6.2": {
         "bugs": [
+            "ArraySliceDynamicallyEncodedBaseType",
             "ImplicitConstructorCallvalueCheck",
             "TupleAssignmentMultiStackSlotComponents",
             "MemoryArrayCreationOverflow"
@@ -1107,6 +1110,7 @@
     },
     "0.6.3": {
         "bugs": [
+            "ArraySliceDynamicallyEncodedBaseType",
             "ImplicitConstructorCallvalueCheck",
             "TupleAssignmentMultiStackSlotComponents",
             "MemoryArrayCreationOverflow"
@@ -1115,6 +1119,7 @@
     },
     "0.6.4": {
         "bugs": [
+            "ArraySliceDynamicallyEncodedBaseType",
             "ImplicitConstructorCallvalueCheck",
             "TupleAssignmentMultiStackSlotComponents",
             "MemoryArrayCreationOverflow"
@@ -1123,6 +1128,7 @@
     },
     "0.6.5": {
         "bugs": [
+            "ArraySliceDynamicallyEncodedBaseType",
             "ImplicitConstructorCallvalueCheck",
             "TupleAssignmentMultiStackSlotComponents"
         ],
@@ -1130,12 +1136,14 @@
     },
     "0.6.6": {
         "bugs": [
+            "ArraySliceDynamicallyEncodedBaseType",
             "ImplicitConstructorCallvalueCheck"
         ],
         "released": "2020-04-09"
     },
     "0.6.7": {
         "bugs": [
+            "ArraySliceDynamicallyEncodedBaseType",
             "ImplicitConstructorCallvalueCheck"
         ],
         "released": "2020-05-04"

libsolidity/analysis/TypeChecker.cpp

@@ -2864,6 +2864,8 @@ bool TypeChecker::visit(IndexRangeAccess const& _access)
 
 	if (arrayType->location() != DataLocation::CallData || !arrayType->isDynamicallySized())
 		m_errorReporter.typeError(1227_error, _access.location(), "Index range access is only supported for dynamic calldata arrays.");
+	else if (arrayType->baseType()->isDynamicallyEncoded())
+		m_errorReporter.typeError(1878_error, _access.location(), "Index range access is not supported for arrays with dynamically encoded base types.");
 	_access.annotation().type = TypeProvider::arraySlice(*arrayType);
 	_access.annotation().isLValue = isLValue;
 	_access.annotation().isPure = isPure;

libsolidity/codegen/CompilerUtils.cpp

@@ -990,7 +990,8 @@ void CompilerUtils::convertType(
 		solAssert(_targetType == typeOnStack.arrayType(), "");
 		solUnimplementedAssert(
 			typeOnStack.arrayType().location() == DataLocation::CallData &&
-			typeOnStack.arrayType().isDynamicallySized(),
+			typeOnStack.arrayType().isDynamicallySized() &&
+			!typeOnStack.arrayType().baseType()->isDynamicallyEncoded(),
 			""
 		);
 		break;

libsolidity/codegen/ExpressionCompiler.cpp

@@ -1769,7 +1769,12 @@ bool ExpressionCompiler::visit(IndexAccess const& _indexAccess)
 		case Type::Category::ArraySlice:
 		{
 			auto const& arrayType = dynamic_cast<ArraySliceType const&>(baseType).arrayType();
-			solAssert(arrayType.location() == DataLocation::CallData && arrayType.isDynamicallySized(), "");
+			solAssert(
+				arrayType.location() == DataLocation::CallData &&
+				arrayType.isDynamicallySized() &&
+				!arrayType.baseType()->isDynamicallyEncoded(),
+				""
+			);
 			solAssert(_indexAccess.indexExpression(), "Index expression expected.");
 
 			acceptAndConvert(*_indexAccess.indexExpression(), *TypeProvider::uint256(), true);
@@ -1852,7 +1857,12 @@ bool ExpressionCompiler::visit(IndexRangeAccess const& _indexAccess)
 			arrayType = &sliceType->arrayType();
 
 	solAssert(arrayType, "");
-	solUnimplementedAssert(arrayType->location() == DataLocation::CallData && arrayType->isDynamicallySized(), "");
+	solUnimplementedAssert(
+		arrayType->location() == DataLocation::CallData &&
+		arrayType->isDynamicallySized() &&
+		!arrayType->baseType()->isDynamicallyEncoded(),
+		""
+	);
 
 	if (_indexAccess.startExpression())
 		acceptAndConvert(*_indexAccess.startExpression(), *TypeProvider::uint256());

libsolidity/codegen/YulUtilFunctions.cpp

@@ -1599,6 +1599,7 @@ string YulUtilFunctions::conversionFunction(Type const& _from, Type const& _to)
 		ArraySliceType const& fromType = dynamic_cast<ArraySliceType const&>(_from);
 		ArrayType const& targetType = dynamic_cast<ArrayType const&>(_to);
 
+		solAssert(!fromType.arrayType().baseType()->isDynamicallyEncoded(), "");
 		solAssert(
 			*fromType.arrayType().baseType() == *targetType.baseType(),
 			"Converting arrays of different type is not possible"

test/libsolidity/syntaxTests/parsing/array_range_nested.sol

@@ -2,9 +2,8 @@ pragma experimental ABIEncoderV2;
 contract C {
     function f(uint256[][] calldata x) external pure {
         x[0][1:2];
-        x[1:2][1:2];
+        uint256 a = x[0][1:2][1:2][1:][3:][0];
-        uint256 a = x[1:2][1:2][1:][3:][0][2];
+        uint256 b = x[1][1:][3:4][1:][2:3][0];
-        uint256 b = x[1:][3:4][1][1:][2:3][0];
         a; b;
     }
 }

test/libsolidity/syntaxTests/parsing/array_range_nested_invalid.sol

@@ -0,0 +1,14 @@
+pragma experimental ABIEncoderV2;
+contract C {
+    function f(uint256[][] calldata x) external pure {
+        x[1:2];
+        x[:];
+        x[1:];
+        x[:2];
+    }
+}
+// ----
+// TypeError: (110-116): Index range access is not supported for arrays with dynamically encoded base types.
+// TypeError: (126-130): Index range access is not supported for arrays with dynamically encoded base types.
+// TypeError: (140-145): Index range access is not supported for arrays with dynamically encoded base types.
+// TypeError: (155-160): Index range access is not supported for arrays with dynamically encoded base types.