Merge pull request #838 from chriseth/ecrecover

Make ecrecover return zero for malformed input.

docs/miscellaneous.rst

@@ -286,7 +286,7 @@ Global Variables
 - ``sha3(...) returns (bytes32)``: compute the Ethereum-SHA-3 (KECCAK-256) hash of the (tightly packed) arguments
 - ``sha256(...) returns (bytes32)``: compute the SHA-256 hash of the (tightly packed) arguments
 - ``ripemd160(...) returns (bytes20)``: compute the RIPEMD-160 hash of the (tightly packed) arguments
-- ``ecrecover(bytes32 hash, uint8 v, bytes32 r, bytes32 s) returns (address)``: recover address associated with the public key from elliptic curve signature
+- ``ecrecover(bytes32 hash, uint8 v, bytes32 r, bytes32 s) returns (address)``: recover address associated with the public key from elliptic curve signature, return zero on error
 - ``addmod(uint x, uint y, uint k) returns (uint)``: compute ``(x + y) % k`` where the addition is performed with arbitrary precision and does not wrap around at ``2**256``
 - ``mulmod(uint x, uint y, uint k) returns (uint)``: compute ``(x * y) % k`` where the multiplication is performed with arbitrary precision and does not wrap around at ``2**256``
 - ``this`` (current contract's type): the current contract, explicitly convertible to ``address``

docs/units-and-global-variables.rst

@@ -95,7 +95,7 @@ Mathematical and Cryptographic Functions
 ``ripemd160(...) returns (bytes20)``:
     compute RIPEMD-160 hash of the (tightly packed) arguments
 ``ecrecover(bytes32 hash, uint8 v, bytes32 r, bytes32 s) returns (address)``:
-    recover the address associated with the public key from elliptic curve signature
+    recover the address associated with the public key from elliptic curve signature or return zero on error
 
 In the above, "tightly packed" means that the arguments are concatenated without padding.
 This means that the following are all identical::

libsolidity/codegen/ExpressionCompiler.cpp

@@ -1449,6 +1449,19 @@ void ExpressionCompiler::appendExternalFunctionCall(
 		argumentTypes.push_back(_arguments[i]->annotation().type);
 	}
 
+	if (funKind == FunctionKind::ECRecover)
+	{
+		// Clears 32 bytes of currently free memory and advances free memory pointer.
+		// Output area will be "start of input area" - 32.
+		// The reason is that a failing ECRecover cannot be detected, it will just return
+		// zero bytes (which we cannot detect).
+		solAssert(0 < retSize && retSize <= 32, "");
+		utils().fetchFreeMemoryPointer();
+		m_context << Instruction::DUP1 << u256(0) << Instruction::MSTORE;
+		m_context << u256(32) << Instruction::ADD;
+		utils().storeFreeMemoryPointer();
+	}
+
 	// Copy function identifier to memory.
 	utils().fetchFreeMemoryPointer();
 	if (!_functionType.isBareCall() || manualFunctionId)
@@ -1457,7 +1470,7 @@ void ExpressionCompiler::appendExternalFunctionCall(
 		utils().storeInMemoryDynamic(IntegerType(8 * CompilerUtils::dataStartOffset), false);
 	}
 	// If the function takes arbitrary parameters, copy dynamic length data in place.
-	// Move argumenst to memory, will not update the free memory pointer (but will update the memory
+	// Move arguments to memory, will not update the free memory pointer (but will update the memory
 	// pointer on the stack).
 	utils().encodeToMemory(
 		argumentTypes,
@@ -1475,12 +1488,24 @@ void ExpressionCompiler::appendExternalFunctionCall(
 	// function identifier [unless bare]
 	// contract address
 
-	// Output data will replace input data.
+	// Output data will replace input data, unless we have ECRecover (then, output
+	// area will be 32 bytes just before input area).
 	// put on stack: <size of output> <memory pos of output> <size of input> <memory pos of input>
 	m_context << u256(retSize);
-	utils().fetchFreeMemoryPointer();
+	utils().fetchFreeMemoryPointer(); // This is the start of input
+	if (funKind == FunctionKind::ECRecover)
+	{
+		// In this case, output is 32 bytes before input and has already been cleared.
+		m_context << u256(32) << Instruction::DUP2 << Instruction::SUB << Instruction::SWAP1;
+		// Here: <input end> <output size> <outpos> <input pos>
+		m_context << Instruction::DUP1 << Instruction::DUP5 << Instruction::SUB;
+		m_context << Instruction::SWAP1;
+	}
+	else
+	{
 		m_context << Instruction::DUP1 << Instruction::DUP4 << Instruction::SUB;
 		m_context << Instruction::DUP2;
+	}
 
 	// CALL arguments: outSize, outOff, inSize, inOff (already present up to here)
 	// [value,] addr, gas (stack top)
@@ -1543,6 +1568,14 @@ void ExpressionCompiler::appendExternalFunctionCall(
 		utils().loadFromMemoryDynamic(IntegerType(160), false, true, false);
 		utils().convertType(IntegerType(160), FixedBytesType(20));
 	}
+	else if (funKind == FunctionKind::ECRecover)
+	{
+		// Output is 32 bytes before input / free mem pointer.
+		// Failing ecrecover cannot be detected, so we clear output before the call.
+		m_context << u256(32);
+		utils().fetchFreeMemoryPointer();
+		m_context << Instruction::SUB << Instruction::MLOAD;
+	}
 	else if (!_functionType.returnParameterTypes().empty())
 	{
 		utils().fetchFreeMemoryPointer();

test/libsolidity/SolidityEndToEndTest.cpp

@@ -6880,6 +6880,22 @@ BOOST_AUTO_TEST_CASE(create_dynamic_array_with_zero_length)
 	BOOST_CHECK(callContractFunction("f()") == encodeArgs(u256(7)));
 }
 
+BOOST_AUTO_TEST_CASE(failing_ecrecover_invalid_input)
+{
+	// ecrecover should return zero for malformed input
+	// (v should be 27 or 28, not 1)
+	// Note that the precompile does not return zero but returns nothing.
+	char const* sourceCode = R"(
+		contract C {
+			function f() returns (address) {
+				return ecrecover(bytes32(uint(-1)), 1, 2, 3);
+			}
+		}
+	)";
+	compileAndRun(sourceCode, 0, "C");
+	BOOST_CHECK(callContractFunction("f()") == encodeArgs(u256(0)));
+}
+
 BOOST_AUTO_TEST_SUITE_END()
 
 }