Merge pull request #2662 from ethereum/fixEcrecover2

Fix ecrecover memory clear

5 files changed, 93 insertions, 4 deletions

diff --git a/Changelog.md b/Changelog.md
index cedb5859..6bf998b5 100644
--- a/Changelog.md
+++ b/Changelog.md
@@ -13,6 +13,7 @@ Features:
  * Type checker: Warn when existing symbols, including builtins, are overwritten.
 
 Bugfixes:
+ * Code Generator: Properly clear return memory area for ecrecover.
  * Type Checker: Fix crash for some assignment to non-lvalue.
  * Type Checker: Fix invalid "specify storage keyword" warning for reference members of structs.
  * Type Checker: Mark modifiers as internal.
diff --git a/docs/bugs.json b/docs/bugs.json
index a0c0e7c4..4fd73492 100644
--- a/docs/bugs.json
+++ b/docs/bugs.json
@@ -1,5 +1,12 @@
 [
     {
+        "name": "ECRecoverMalformedInput",
+	"summary": "The ecrecover() builtin can return garbage for malformed input.",
+        "description": "The ecrecover precompile does not properly signal failure for malformed input (especially in the 'v' argument) and thus the Solidity function can return data that was previously present in the return area in memory.",
+        "fixed": "0.4.14",
+        "severity": "medium"
+    },
+    {
         "name": "SkipEmptyStringLiteral",
 	"summary": "If \"\" is used in a function call, the following function arguments will not be correctly passed to the function.",
         "description": "If the empty string literal \"\" is used as an argument in a function call, it is skipped by the encoder. This has the effect that the encoding of all arguments following this is shifted left by 32 bytes and thus the function call data is corrupted.",
@@ -107,4 +114,4 @@
         "severity": "high",
         "fixed": "0.3.0"
     }
-]
\ No newline at end of file
+]
diff --git a/docs/bugs_by_version.json b/docs/bugs_by_version.json
index d6802eec..e67a0845 100644
--- a/docs/bugs_by_version.json
+++ b/docs/bugs_by_version.json
@@ -1,6 +1,7 @@
 {
     "0.1.0": {
         "bugs": [
+            "ECRecoverMalformedInput", 
             "SkipEmptyStringLiteral", 
             "ConstantOptimizerSubtraction", 
             "IdentityPrecompileReturnIgnored", 
@@ -16,6 +17,7 @@
     }, 
     "0.1.1": {
         "bugs": [
+            "ECRecoverMalformedInput", 
             "SkipEmptyStringLiteral", 
             "ConstantOptimizerSubtraction", 
             "IdentityPrecompileReturnIgnored", 
@@ -31,6 +33,7 @@
     }, 
     "0.1.2": {
         "bugs": [
+            "ECRecoverMalformedInput", 
             "SkipEmptyStringLiteral", 
             "ConstantOptimizerSubtraction", 
             "IdentityPrecompileReturnIgnored", 
@@ -46,6 +49,7 @@
     }, 
     "0.1.3": {
         "bugs": [
+            "ECRecoverMalformedInput", 
             "SkipEmptyStringLiteral", 
             "ConstantOptimizerSubtraction", 
             "IdentityPrecompileReturnIgnored", 
@@ -61,6 +65,7 @@
     }, 
     "0.1.4": {
         "bugs": [
+            "ECRecoverMalformedInput", 
             "SkipEmptyStringLiteral", 
             "ConstantOptimizerSubtraction", 
             "IdentityPrecompileReturnIgnored", 
@@ -76,6 +81,7 @@
     }, 
     "0.1.5": {
         "bugs": [
+            "ECRecoverMalformedInput", 
             "SkipEmptyStringLiteral", 
             "ConstantOptimizerSubtraction", 
             "IdentityPrecompileReturnIgnored", 
@@ -91,6 +97,7 @@
     }, 
     "0.1.6": {
         "bugs": [
+            "ECRecoverMalformedInput", 
             "SkipEmptyStringLiteral", 
             "ConstantOptimizerSubtraction", 
             "IdentityPrecompileReturnIgnored", 
@@ -107,6 +114,7 @@
     }, 
     "0.1.7": {
         "bugs": [
+            "ECRecoverMalformedInput", 
             "SkipEmptyStringLiteral", 
             "ConstantOptimizerSubtraction", 
             "IdentityPrecompileReturnIgnored", 
@@ -123,6 +131,7 @@
     }, 
     "0.2.0": {
         "bugs": [
+            "ECRecoverMalformedInput", 
             "SkipEmptyStringLiteral", 
             "ConstantOptimizerSubtraction", 
             "IdentityPrecompileReturnIgnored", 
@@ -139,6 +148,7 @@
     }, 
     "0.2.1": {
         "bugs": [
+            "ECRecoverMalformedInput", 
             "SkipEmptyStringLiteral", 
             "ConstantOptimizerSubtraction", 
             "IdentityPrecompileReturnIgnored", 
@@ -155,6 +165,7 @@
     }, 
     "0.2.2": {
         "bugs": [
+            "ECRecoverMalformedInput", 
             "SkipEmptyStringLiteral", 
             "ConstantOptimizerSubtraction", 
             "IdentityPrecompileReturnIgnored", 
@@ -171,6 +182,7 @@
     }, 
     "0.3.0": {
         "bugs": [
+            "ECRecoverMalformedInput", 
             "SkipEmptyStringLiteral", 
             "ConstantOptimizerSubtraction", 
             "IdentityPrecompileReturnIgnored", 
@@ -186,6 +198,7 @@
     }, 
     "0.3.1": {
         "bugs": [
+            "ECRecoverMalformedInput", 
             "SkipEmptyStringLiteral", 
             "ConstantOptimizerSubtraction", 
             "IdentityPrecompileReturnIgnored", 
@@ -200,6 +213,7 @@
     }, 
     "0.3.2": {
         "bugs": [
+            "ECRecoverMalformedInput", 
             "SkipEmptyStringLiteral", 
             "ConstantOptimizerSubtraction", 
             "IdentityPrecompileReturnIgnored", 
@@ -214,6 +228,7 @@
     }, 
     "0.3.3": {
         "bugs": [
+            "ECRecoverMalformedInput", 
             "SkipEmptyStringLiteral", 
             "ConstantOptimizerSubtraction", 
             "IdentityPrecompileReturnIgnored", 
@@ -227,6 +242,7 @@
     }, 
     "0.3.4": {
         "bugs": [
+            "ECRecoverMalformedInput", 
             "SkipEmptyStringLiteral", 
             "ConstantOptimizerSubtraction", 
             "IdentityPrecompileReturnIgnored", 
@@ -240,6 +256,7 @@
     }, 
     "0.3.5": {
         "bugs": [
+            "ECRecoverMalformedInput", 
             "SkipEmptyStringLiteral", 
             "ConstantOptimizerSubtraction", 
             "IdentityPrecompileReturnIgnored", 
@@ -253,6 +270,7 @@
     }, 
     "0.3.6": {
         "bugs": [
+            "ECRecoverMalformedInput", 
             "SkipEmptyStringLiteral", 
             "ConstantOptimizerSubtraction", 
             "IdentityPrecompileReturnIgnored", 
@@ -264,6 +282,7 @@
     }, 
     "0.4.0": {
         "bugs": [
+            "ECRecoverMalformedInput", 
             "SkipEmptyStringLiteral", 
             "ConstantOptimizerSubtraction", 
             "IdentityPrecompileReturnIgnored", 
@@ -275,6 +294,7 @@
     }, 
     "0.4.1": {
         "bugs": [
+            "ECRecoverMalformedInput", 
             "SkipEmptyStringLiteral", 
             "ConstantOptimizerSubtraction", 
             "IdentityPrecompileReturnIgnored", 
@@ -286,6 +306,7 @@
     }, 
     "0.4.10": {
         "bugs": [
+            "ECRecoverMalformedInput", 
             "SkipEmptyStringLiteral", 
             "ConstantOptimizerSubtraction"
         ], 
@@ -293,20 +314,26 @@
     }, 
     "0.4.11": {
         "bugs": [
+            "ECRecoverMalformedInput", 
             "SkipEmptyStringLiteral"
         ], 
         "released": "2017-05-03"
     }, 
     "0.4.12": {
-        "bugs": [], 
+        "bugs": [
+            "ECRecoverMalformedInput"
+        ], 
         "released": "2017-07-03"
     }, 
     "0.4.13": {
-        "bugs": [], 
+        "bugs": [
+            "ECRecoverMalformedInput"
+        ], 
         "released": "2017-07-06"
     }, 
     "0.4.2": {
         "bugs": [
+            "ECRecoverMalformedInput", 
             "SkipEmptyStringLiteral", 
             "ConstantOptimizerSubtraction", 
             "IdentityPrecompileReturnIgnored", 
@@ -317,6 +344,7 @@
     }, 
     "0.4.3": {
         "bugs": [
+            "ECRecoverMalformedInput", 
             "SkipEmptyStringLiteral", 
             "ConstantOptimizerSubtraction", 
             "IdentityPrecompileReturnIgnored", 
@@ -326,6 +354,7 @@
     }, 
     "0.4.4": {
         "bugs": [
+            "ECRecoverMalformedInput", 
             "SkipEmptyStringLiteral", 
             "ConstantOptimizerSubtraction", 
             "IdentityPrecompileReturnIgnored"
@@ -334,6 +363,7 @@
     }, 
     "0.4.5": {
         "bugs": [
+            "ECRecoverMalformedInput", 
             "SkipEmptyStringLiteral", 
             "ConstantOptimizerSubtraction", 
             "IdentityPrecompileReturnIgnored", 
@@ -343,6 +373,7 @@
     }, 
     "0.4.6": {
         "bugs": [
+            "ECRecoverMalformedInput", 
             "SkipEmptyStringLiteral", 
             "ConstantOptimizerSubtraction", 
             "IdentityPrecompileReturnIgnored"
@@ -351,6 +382,7 @@
     }, 
     "0.4.7": {
         "bugs": [
+            "ECRecoverMalformedInput", 
             "SkipEmptyStringLiteral", 
             "ConstantOptimizerSubtraction"
         ], 
@@ -358,6 +390,7 @@
     }, 
     "0.4.8": {
         "bugs": [
+            "ECRecoverMalformedInput", 
             "SkipEmptyStringLiteral", 
             "ConstantOptimizerSubtraction"
         ], 
@@ -365,6 +398,7 @@
     }, 
     "0.4.9": {
         "bugs": [
+            "ECRecoverMalformedInput", 
             "SkipEmptyStringLiteral", 
             "ConstantOptimizerSubtraction"
         ], 
diff --git a/libsolidity/codegen/ExpressionCompiler.cpp b/libsolidity/codegen/ExpressionCompiler.cpp
index a35008bf..02cc62be 100644
--- a/libsolidity/codegen/ExpressionCompiler.cpp
+++ b/libsolidity/codegen/ExpressionCompiler.cpp
@@ -1622,7 +1622,7 @@ void ExpressionCompiler::appendExternalFunctionCall(
 		// zero bytes (which we cannot detect).
 		solAssert(0 < retSize && retSize <= 32, "");
 		utils().fetchFreeMemoryPointer();
-		m_context << Instruction::DUP1 << u256(0) << Instruction::MSTORE;
+		m_context << u256(0) << Instruction::DUP2 << Instruction::MSTORE;
 		m_context << u256(32) << Instruction::ADD;
 		utils().storeFreeMemoryPointer();
 	}
diff --git a/test/libsolidity/SolidityEndToEndTest.cpp b/test/libsolidity/SolidityEndToEndTest.cpp
index 5bcde441..db7f59ee 100644
--- a/test/libsolidity/SolidityEndToEndTest.cpp
+++ b/test/libsolidity/SolidityEndToEndTest.cpp
@@ -8251,6 +8251,53 @@ BOOST_AUTO_TEST_CASE(failing_ecrecover_invalid_input)
 	BOOST_CHECK(callContractFunction("f()") == encodeArgs(u256(0)));
 }
 
+BOOST_AUTO_TEST_CASE(failing_ecrecover_invalid_input_proper)
+{
+	char const* sourceCode = R"(
+		contract C {
+			function f() returns (address) {
+				return recover(
+					0x77e5189111eb6557e8a637b27ef8fbb15bc61d61c2f00cc48878f3a296e5e0ca,
+					0, // invalid v value
+					0x6944c77849b18048f6abe0db8084b0d0d0689cdddb53d2671c36967b58691ad4,
+					0xef4f06ba4f78319baafd0424365777241af4dfd3da840471b4b4b087b7750d0d,
+					0xca35b7d915458ef540ade6068dfe2f44e8fa733c,
+					0xca35b7d915458ef540ade6068dfe2f44e8fa733c
+				);
+			}
+			function recover(bytes32 hash, uint8 v, bytes32 r, bytes32 s, uint blockExpired, bytes32 salt)
+				returns (address)
+			{
+				require(hash == keccak256(blockExpired, salt));
+				return ecrecover(hash, v, r, s);
+			}
+		}
+	)";
+	compileAndRun(sourceCode, 0, "C");
+	BOOST_CHECK(callContractFunction("f()") == encodeArgs(u256(0)));
+}
+
+BOOST_AUTO_TEST_CASE(failing_ecrecover_invalid_input_asm)
+{
+	char const* sourceCode = R"(
+		contract C {
+			function f() returns (address) {
+				assembly {
+					mstore(mload(0x40), 0xca35b7d915458ef540ade6068dfe2f44e8fa733c)
+				}
+				return ecrecover(
+					0x77e5189111eb6557e8a637b27ef8fbb15bc61d61c2f00cc48878f3a296e5e0ca,
+					0, // invalid v value
+					0x6944c77849b18048f6abe0db8084b0d0d0689cdddb53d2671c36967b58691ad4,
+					0xef4f06ba4f78319baafd0424365777241af4dfd3da840471b4b4b087b7750d0d
+				);
+			}
+		}
+	)";
+	compileAndRun(sourceCode, 0, "C");
+	BOOST_CHECK(callContractFunction("f()") == encodeArgs(u256(0)));
+}
+
 BOOST_AUTO_TEST_CASE(calling_nonexisting_contract_throws)
 {
 	char const* sourceCode = R"(