Merge pull request #4085 from ethereum/calldatapad

[BREAKING] Properly pad data from calldata.

4 files changed, 19 insertions, 14 deletions

diff --git a/Changelog.md b/Changelog.md
index 8c895ee0..e3383eae 100644
--- a/Changelog.md
+++ b/Changelog.md
@@ -1,6 +1,7 @@
 ### 0.5.0 (unreleased)
 
 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!
  * Commandline interface: Require ``-`` if standard input is used as source.
  * General: ``continue`` in a ``do...while`` loop jumps to the condition (it used to jump to the loop body). Warning: this may silently change the semantics of existing code.
diff --git a/libsolidity/codegen/ArrayUtils.cpp b/libsolidity/codegen/ArrayUtils.cpp
index 14c887c3..2b77db8f 100644
--- a/libsolidity/codegen/ArrayUtils.cpp
+++ b/libsolidity/codegen/ArrayUtils.cpp
@@ -303,12 +303,17 @@ void ArrayUtils::copyArrayToMemory(ArrayType const& _sourceType, bool _padToWord
 			m_context << _sourceType.length();
 		if (baseSize > 1)
 			m_context << u256(baseSize) << Instruction::MUL;
-		// stack: target source_offset source_len
-		m_context << Instruction::DUP1 << Instruction::DUP3 << Instruction::DUP5;
-		// stack: target source_offset source_len source_len source_offset target
-		m_context << Instruction::CALLDATACOPY;
-		m_context << Instruction::DUP3 << Instruction::ADD;
-		m_context << Instruction::SWAP2 << Instruction::POP << Instruction::POP;
+
+		string routine = "calldatacopy(target, source, len)\n";
+		if (_padToWordBoundaries)
+			routine += R"(
+				// Set padding suffix to zero
+				mstore(add(target, len), 0)
+				len := and(add(len, 0x1f), not(0x1f))
+			)";
+		routine += "target := add(target, len)\n";
+		m_context.appendInlineAssembly("{" + routine + "}", {"target", "source", "len"});
+		m_context << Instruction::POP << Instruction::POP;
 	}
 	else if (_sourceType.location() == DataLocation::Memory)
 	{
diff --git a/test/libsolidity/ABIEncoderTests.cpp b/test/libsolidity/ABIEncoderTests.cpp
index 49db9ce1..5f15b28f 100644
--- a/test/libsolidity/ABIEncoderTests.cpp
+++ b/test/libsolidity/ABIEncoderTests.cpp
@@ -374,15 +374,12 @@ BOOST_AUTO_TEST_CASE(calldata)
 	)";
 	string s("abcdef");
 	string t("abcdefgggggggggggggggggggggggggggggggggggggggghhheeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeggg");
-	bool newEncoder = false;
 	BOTH_ENCODERS(
 		compileAndRun(sourceCode);
 		callContractFunction("f(bytes)", 0x20, s.size(), s);
-		// The old encoder did not pad to multiples of 32 bytes
-		REQUIRE_LOG_DATA(encodeArgs(0x20, s.size()) + (newEncoder ? encodeArgs(s) : asBytes(s)));
+		REQUIRE_LOG_DATA(encodeArgs(0x20, s.size(), s));
 		callContractFunction("f(bytes)", 0x20, t.size(), t);
-		REQUIRE_LOG_DATA(encodeArgs(0x20, t.size()) + (newEncoder ? encodeArgs(t) : asBytes(t)));
-		newEncoder = true;
+		REQUIRE_LOG_DATA(encodeArgs(0x20, t.size(), t));
 	)
 }
 
diff --git a/test/libsolidity/SolidityEndToEndTest.cpp b/test/libsolidity/SolidityEndToEndTest.cpp
index 2afb4e1c..236d83ef 100644
--- a/test/libsolidity/SolidityEndToEndTest.cpp
+++ b/test/libsolidity/SolidityEndToEndTest.cpp
@@ -3457,7 +3457,7 @@ BOOST_AUTO_TEST_CASE(event_really_lots_of_data)
 	callContractFunction("deposit()");
 	BOOST_REQUIRE_EQUAL(m_logs.size(), 1);
 	BOOST_CHECK_EQUAL(m_logs[0].address, m_contractAddress);
-	BOOST_CHECK_EQUAL(toHex(m_logs[0].data), toHex(encodeArgs(10, 0x60, 15, 4) + FixedHash<4>(dev::keccak256("deposit()")).asBytes()));
+	BOOST_CHECK_EQUAL(toHex(m_logs[0].data), toHex(encodeArgs(10, 0x60, 15, 4, asString(FixedHash<4>(dev::keccak256("deposit()")).asBytes()))));
 	BOOST_REQUIRE_EQUAL(m_logs[0].topics.size(), 1);
 	BOOST_CHECK_EQUAL(m_logs[0].topics[0], dev::keccak256(string("Deposit(uint256,bytes,uint256)")));
 }
@@ -3964,7 +3964,8 @@ BOOST_AUTO_TEST_CASE(call_forward_bytes_length)
 	compileAndRun(sourceCode, 0, "sender");
 
 	// No additional data, just function selector
-	ABI_CHECK(callContractFunction("viaCalldata()"), encodeArgs(4));
+	ABI_CHECK(callContractFunction("viaCalldata()"), encodeArgs(0x20));
+	// Should be this with 0.5.0: encodeArgs(4));
 	ABI_CHECK(callContractFunction("viaMemory()"), encodeArgs(0x20));
 	// Should be this with 0.5.0: encodeArgs(4));
 	ABI_CHECK(callContractFunction("viaStorage()"), encodeArgs(0x20));
@@ -3972,7 +3973,8 @@ BOOST_AUTO_TEST_CASE(call_forward_bytes_length)
 
 	// Some additional unpadded data
 	bytes unpadded = asBytes(string("abc"));
-	ABI_CHECK(callContractFunctionNoEncoding("viaCalldata()", unpadded), encodeArgs(7));
+	ABI_CHECK(callContractFunctionNoEncoding("viaCalldata()", unpadded), encodeArgs(0x20));
+	// Should be this with 0.5.0: encodeArgs(7));
 	ABI_CHECK(callContractFunctionNoEncoding("viaMemory()", unpadded), encodeArgs(0x20));
 	// Should be this with 0.5.0: encodeArgs(7));
 	ABI_CHECK(callContractFunctionNoEncoding("viaStorage()", unpadded), encodeArgs(0x20));