Merge pull request #4224 from ethereum/revert_wrong_calldata

[BREAKING] Revert if calldata has wrong size

5 files changed, 39 insertions, 58 deletions

diff --git a/Changelog.md b/Changelog.md
index b1f742bf..24ca5feb 100644
--- a/Changelog.md
+++ b/Changelog.md
@@ -8,6 +8,7 @@ How to update your code:
 Breaking Changes:
  * ABI Encoder: Properly pad data from calldata (``msg.data`` and external function parameters). Use ``abi.encodePacked`` for unpadded encoding.
  * Code Generator: Signed right shift uses proper arithmetic shift, i.e. rounding towards negative infinity. Warning: this may silently change the semantics of existing code!
+ * Code Generator: Revert at runtime if calldata is too short or points out of bounds. This is done inside the ``ABI decoder`` and therefore also applies to ``abi.decode()``.
  * Commandline interface: Remove obsolete ``--formal`` option.
  * Commandline interface: Rename the ``--julia`` option to ``--yul``.
  * Commandline interface: Require ``-`` if standard input is used as source.
diff --git a/libsolidity/codegen/CompilerUtils.cpp b/libsolidity/codegen/CompilerUtils.cpp
index 3446be55..a5e96335 100644
--- a/libsolidity/codegen/CompilerUtils.cpp
+++ b/libsolidity/codegen/CompilerUtils.cpp
@@ -181,7 +181,7 @@ void CompilerUtils::storeInMemoryDynamic(Type const& _type, bool _padToWordBound
 	}
 }
 
-void CompilerUtils::abiDecode(TypePointers const& _typeParameters, bool _fromMemory, bool _revertOnOutOfBounds)
+void CompilerUtils::abiDecode(TypePointers const& _typeParameters, bool _fromMemory)
 {
 	/// Stack: <source_offset> <length>
 	if (m_context.experimentalFeatureActive(ExperimentalFeature::ABIEncoderV2))
@@ -194,14 +194,10 @@ void CompilerUtils::abiDecode(TypePointers const& _typeParameters, bool _fromMem
 	}
 
 	//@todo this does not yet support nested dynamic arrays
-
-	if (_revertOnOutOfBounds)
-	{
-		size_t encodedSize = 0;
-		for (auto const& t: _typeParameters)
-			encodedSize += t->decodingType()->calldataEncodedSize(true);
-		m_context.appendInlineAssembly("{ if lt(len, " + to_string(encodedSize) + ") { revert(0, 0) } }", {"len"});
-	}
+	size_t encodedSize = 0;
+	for (auto const& t: _typeParameters)
+		encodedSize += t->decodingType()->calldataEncodedSize(true);
+	m_context.appendInlineAssembly("{ if lt(len, " + to_string(encodedSize) + ") { revert(0, 0) } }", {"len"});
 
 	m_context << Instruction::DUP2 << Instruction::ADD;
 	m_context << Instruction::SWAP1;
@@ -231,26 +227,21 @@ void CompilerUtils::abiDecode(TypePointers const& _typeParameters, bool _fromMem
 				{
 					// compute data pointer
 					m_context << Instruction::DUP1 << Instruction::MLOAD;
-					if (_revertOnOutOfBounds)
-					{
-						// Check that the data pointer is valid and that length times
-						// item size is still inside the range.
-						Whiskers templ(R"({
-							if gt(ptr, 0x100000000) { revert(0, 0) }
-							ptr := add(ptr, base_offset)
-							let array_data_start := add(ptr, 0x20)
-							if gt(array_data_start, input_end) { revert(0, 0) }
-							let array_length := mload(ptr)
-							if or(
-								gt(array_length, 0x100000000),
-								gt(add(array_data_start, mul(array_length, <item_size>)), input_end)
-							) { revert(0, 0) }
-						})");
-						templ("item_size", to_string(arrayType.isByteArray() ? 1 : arrayType.baseType()->calldataEncodedSize(true)));
-						m_context.appendInlineAssembly(templ.render(), {"input_end", "base_offset", "offset", "ptr"});
-					}
-					else
-						m_context << Instruction::DUP3 << Instruction::ADD;
+					// Check that the data pointer is valid and that length times
+					// item size is still inside the range.
+					Whiskers templ(R"({
+						if gt(ptr, 0x100000000) { revert(0, 0) }
+						ptr := add(ptr, base_offset)
+						let array_data_start := add(ptr, 0x20)
+						if gt(array_data_start, input_end) { revert(0, 0) }
+						let array_length := mload(ptr)
+						if or(
+							gt(array_length, 0x100000000),
+							gt(add(array_data_start, mul(array_length, <item_size>)), input_end)
+						) { revert(0, 0) }
+					})");
+					templ("item_size", to_string(arrayType.isByteArray() ? 1 : arrayType.baseType()->calldataEncodedSize(true)));
+					m_context.appendInlineAssembly(templ.render(), {"input_end", "base_offset", "offset", "ptr"});
 					// stack: v1 v2 ... v(k-1) input_end base_offset current_offset v(k)
 					moveIntoStack(3);
 					m_context << u256(0x20) << Instruction::ADD;
@@ -273,30 +264,25 @@ void CompilerUtils::abiDecode(TypePointers const& _typeParameters, bool _fromMem
 					loadFromMemoryDynamic(IntegerType(256), !_fromMemory);
 					m_context << Instruction::SWAP1;
 					// stack: input_end base_offset next_pointer data_offset
-					if (_revertOnOutOfBounds)
-						m_context.appendInlineAssembly("{ if gt(data_offset, 0x100000000) { revert(0, 0) } }", {"data_offset"});
+					m_context.appendInlineAssembly("{ if gt(data_offset, 0x100000000) { revert(0, 0) } }", {"data_offset"});
 					m_context << Instruction::DUP3 << Instruction::ADD;
 					// stack: input_end base_offset next_pointer array_head_ptr
-					if (_revertOnOutOfBounds)
-						m_context.appendInlineAssembly(
-							"{ if gt(add(array_head_ptr, 0x20), input_end) { revert(0, 0) } }",
-							{"input_end", "base_offset", "next_ptr", "array_head_ptr"}
-						);
+					m_context.appendInlineAssembly(
+						"{ if gt(add(array_head_ptr, 0x20), input_end) { revert(0, 0) } }",
+						{"input_end", "base_offset", "next_ptr", "array_head_ptr"}
+					);
 					// retrieve length
 					loadFromMemoryDynamic(IntegerType(256), !_fromMemory, true);
 					// stack: input_end base_offset next_pointer array_length data_pointer
 					m_context << Instruction::SWAP2;
 					// stack: input_end base_offset data_pointer array_length next_pointer
-					if (_revertOnOutOfBounds)
-					{
-						unsigned itemSize = arrayType.isByteArray() ? 1 : arrayType.baseType()->calldataEncodedSize(true);
-						m_context.appendInlineAssembly(R"({
-							if or(
-								gt(array_length, 0x100000000),
-								gt(add(data_ptr, mul(array_length, )" + to_string(itemSize) + R"()), input_end)
-							) { revert(0, 0) }
-						})", {"input_end", "base_offset", "data_ptr", "array_length", "next_ptr"});
-					}
+					unsigned itemSize = arrayType.isByteArray() ? 1 : arrayType.baseType()->calldataEncodedSize(true);
+					m_context.appendInlineAssembly(R"({
+						if or(
+							gt(array_length, 0x100000000),
+							gt(add(data_ptr, mul(array_length, )" + to_string(itemSize) + R"()), input_end)
+						) { revert(0, 0) }
+					})", {"input_end", "base_offset", "data_ptr", "array_length", "next_ptr"});
 				}
 				else
 				{
diff --git a/libsolidity/codegen/CompilerUtils.h b/libsolidity/codegen/CompilerUtils.h
index 8e3a8a5d..0ff3ad7c 100644
--- a/libsolidity/codegen/CompilerUtils.h
+++ b/libsolidity/codegen/CompilerUtils.h
@@ -102,7 +102,7 @@ public:
 	/// area. Also has a hard cap of 0x100000000 for any given length/offset field.
 	/// Stack pre: <source_offset> <length>
 	/// Stack post: <value0> <value1> ... <valuen>
-	void abiDecode(TypePointers const& _typeParameters, bool _fromMemory = false, bool _revertOnOutOfBounds = false);
+	void abiDecode(TypePointers const& _typeParameters, bool _fromMemory = false);
 
 	/// Copies values (of types @a _givenTypes) given on the stack to a location in memory given
 	/// at the stack top, encoding them according to the ABI as the given types @a _targetTypes.
diff --git a/libsolidity/codegen/ExpressionCompiler.cpp b/libsolidity/codegen/ExpressionCompiler.cpp
index ecbd0243..2e548e32 100644
--- a/libsolidity/codegen/ExpressionCompiler.cpp
+++ b/libsolidity/codegen/ExpressionCompiler.cpp
@@ -2049,7 +2049,7 @@ void ExpressionCompiler::appendExternalFunctionCall(
 				mstore(0x40, newMem)
 			})", {"start", "size"});
 
-		utils().abiDecode(returnTypes, true, true);
+		utils().abiDecode(returnTypes, true);
 	}
 }
 
diff --git a/test/libsolidity/ABIDecoderTests.cpp b/test/libsolidity/ABIDecoderTests.cpp
index c122d806..ef00103b 100644
--- a/test/libsolidity/ABIDecoderTests.cpp
+++ b/test/libsolidity/ABIDecoderTests.cpp
@@ -266,7 +266,6 @@ BOOST_AUTO_TEST_CASE(calldata_arrays_too_large)
 			}
 		}
 	)";
-	bool newEncoder = false;
 	BOTH_ENCODERS(
 		compileAndRun(sourceCode);
 		bytes args = encodeArgs(
@@ -275,9 +274,8 @@ BOOST_AUTO_TEST_CASE(calldata_arrays_too_large)
 		);
 		ABI_CHECK(
 			callContractFunction("f(uint256,uint256[],uint256)", args),
-			newEncoder ? encodeArgs() : encodeArgs(7)
+			encodeArgs()
 		);
-		newEncoder = true;
 	)
 }
 
@@ -449,13 +447,11 @@ BOOST_AUTO_TEST_CASE(short_input_value_type)
 			function f(uint a, uint b) public pure returns (uint) { return a; }
 		}
 	)";
-	bool newDecoder = false;
 	BOTH_ENCODERS(
 		compileAndRun(sourceCode);
 		ABI_CHECK(callContractFunction("f(uint256,uint256)", 1, 2), encodeArgs(1));
 		ABI_CHECK(callContractFunctionNoEncoding("f(uint256,uint256)", bytes(64, 0)), encodeArgs(0));
-		ABI_CHECK(callContractFunctionNoEncoding("f(uint256,uint256)", bytes(63, 0)), newDecoder ? encodeArgs() : encodeArgs(0));
-		newDecoder = true;
+		ABI_CHECK(callContractFunctionNoEncoding("f(uint256,uint256)", bytes(63, 0)), encodeArgs());
 	)
 }
 
@@ -466,15 +462,13 @@ BOOST_AUTO_TEST_CASE(short_input_array)
 			function f(uint[] a) public pure returns (uint) { return 7; }
 		}
 	)";
-	bool newDecoder = false;
 	BOTH_ENCODERS(
 		compileAndRun(sourceCode);
 		ABI_CHECK(callContractFunctionNoEncoding("f(uint256[])", encodeArgs(0x20, 0)), encodeArgs(7));
-		ABI_CHECK(callContractFunctionNoEncoding("f(uint256[])", encodeArgs(0x20, 1)), newDecoder ? encodeArgs() : encodeArgs(7));
-		ABI_CHECK(callContractFunctionNoEncoding("f(uint256[])", encodeArgs(0x20, 1) + bytes(31, 0)), newDecoder ? encodeArgs() : encodeArgs(7));
+		ABI_CHECK(callContractFunctionNoEncoding("f(uint256[])", encodeArgs(0x20, 1)), encodeArgs());
+		ABI_CHECK(callContractFunctionNoEncoding("f(uint256[])", encodeArgs(0x20, 1) + bytes(31, 0)), encodeArgs());
 		ABI_CHECK(callContractFunctionNoEncoding("f(uint256[])", encodeArgs(0x20, 1) + bytes(32, 0)), encodeArgs(7));
 		ABI_CHECK(callContractFunctionNoEncoding("f(uint256[])", encodeArgs(0x20, 2, 5, 6)), encodeArgs(7));
-		newDecoder = true;
 	)
 }