Merge pull request #1381 from ethereum/overflown_enum_compared

Enum overflow checking before writing to storage

libsolidity/codegen/CompilerUtils.cpp

@@ -358,7 +358,7 @@ void CompilerUtils::pushCombinedFunctionEntryLabel(Declaration const& _function)
 			Instruction::OR;
 }
 
-void CompilerUtils::convertType(Type const& _typeOnStack, Type const& _targetType, bool _cleanupNeeded)
+void CompilerUtils::convertType(Type const& _typeOnStack, Type const& _targetType, bool _cleanupNeeded, bool _chopSignBits)
 {
 	// For a type extension, we need to remove all higher-order bits that we might have ignored in
 	// previous operations.
@@ -370,6 +370,12 @@ void CompilerUtils::convertType(Type const& _typeOnStack, Type const& _targetTyp
 	Type::Category targetTypeCategory = _targetType.category();
 
 	bool enumOverflowCheckPending = (targetTypeCategory == Type::Category::Enum || stackTypeCategory == Type::Category::Enum);
+	bool chopSignBitsPending = _chopSignBits && targetTypeCategory == Type::Category::Integer;
+	if (chopSignBitsPending)
+	{
+		const IntegerType& targetIntegerType = dynamic_cast<const IntegerType &>(_targetType);
+		chopSignBitsPending = targetIntegerType.isSigned();
+	}
 
 	switch (stackTypeCategory)
 	{
@@ -482,6 +488,14 @@ void CompilerUtils::convertType(Type const& _typeOnStack, Type const& _targetTyp
 					cleanHigherOrderBits(typeOnStack);
 				else if (_cleanupNeeded)
 					cleanHigherOrderBits(targetType);
+				if (chopSignBitsPending)
+				{
+					if (typeOnStack.numBits() < 256)
+						m_context
+							<< ((u256(1) << typeOnStack.numBits()) - 1)
+							<< Instruction::AND;
+					chopSignBitsPending = false;
+				}
 			}
 		}
 		break;
@@ -724,10 +738,15 @@ void CompilerUtils::convertType(Type const& _typeOnStack, Type const& _targetTyp
 	default:
 		// All other types should not be convertible to non-equal types.
 		solAssert(_typeOnStack == _targetType, "Invalid type conversion requested.");
+		if (_cleanupNeeded && _targetType.canBeStored() && _targetType.storageBytes() < 32)
+				m_context
+					<< ((u256(1) << (8 * _targetType.storageBytes())) - 1)
+					<< Instruction::AND;
 		break;
 	}
 
 	solAssert(!enumOverflowCheckPending, "enum overflow checking missing.");
+	solAssert(!chopSignBitsPending, "forgot to chop the sign bits.");
 }
 
 void CompilerUtils::pushZeroValue(Type const& _type)

libsolidity/codegen/CompilerUtils.h

@@ -130,7 +130,8 @@ public:
 	/// if a reference type is converted from calldata or storage to memory.
 	/// If @a _cleanupNeeded, high order bits cleanup is also done if no type conversion would be
 	/// necessary.
-	void convertType(Type const& _typeOnStack, Type const& _targetType, bool _cleanupNeeded = false);
+	/// If @a _chopSignBits, the function resets the signed bits out of the width of the signed integer.
+	void convertType(Type const& _typeOnStack, Type const& _targetType, bool _cleanupNeeded = false, bool _chopSignBits = false);
 
 	/// Creates a zero-value for the given type and puts it onto the stack. This might allocate
 	/// memory for memory references.

libsolidity/codegen/LValue.cpp

@@ -216,11 +216,14 @@ void StorageItem::retrieveValue(SourceLocation const&, bool _remove) const
 void StorageItem::storeValue(Type const& _sourceType, SourceLocation const& _location, bool _move) const
 {
 	CompilerUtils utils(m_context);
+	solAssert(m_dataType, "");
+
 	// stack: value storage_key storage_offset
 	if (m_dataType->isValueType())
 	{
 		solAssert(m_dataType->storageBytes() <= 32, "Invalid storage bytes size.");
 		solAssert(m_dataType->storageBytes() > 0, "Invalid storage bytes size.");
+
 		if (m_dataType->storageBytes() == 32)
 		{
 			solAssert(m_dataType->sizeOnStack() == 1, "Invalid stack size.");
@@ -228,6 +231,11 @@ void StorageItem::storeValue(Type const& _sourceType, SourceLocation const& _loc
 			m_context << Instruction::POP;
 			if (!_move)
 				m_context << Instruction::DUP2 << Instruction::SWAP1;
+
+			m_context << Instruction::SWAP1;
+			utils.convertType(_sourceType, *m_dataType, true);
+			m_context << Instruction::SWAP1;
+
 			m_context << Instruction::SSTORE;
 		}
 		else
@@ -248,6 +256,7 @@ void StorageItem::storeValue(Type const& _sourceType, SourceLocation const& _loc
 			// stack: value storage_ref cleared_value multiplier value
 			if (FunctionType const* fun = dynamic_cast<decltype(fun)>(m_dataType))
 			{
+				solAssert(_sourceType == *m_dataType, "function item stored but target is not equal to source");
 				if (fun->location() == FunctionType::Location::External)
 					// Combine the two-item function type into a single stack slot.
 					utils.combineExternalFunctionType(false);
@@ -257,19 +266,17 @@ void StorageItem::storeValue(Type const& _sourceType, SourceLocation const& _loc
 						Instruction::AND;
 			}
 			else if (m_dataType->category() == Type::Category::FixedBytes)
+			{
+				solAssert(_sourceType.category() == Type::Category::FixedBytes, "source not fixed bytes");
 				m_context
 					<< (u256(0x1) << (256 - 8 * dynamic_cast<FixedBytesType const&>(*m_dataType).numBytes()))
 					<< Instruction::SWAP1 << Instruction::DIV;
+			}
 			else
 			{
 				solAssert(m_dataType->sizeOnStack() == 1, "Invalid stack size for opaque type.");
 				// remove the higher order bits
-				m_context
+				utils.convertType(_sourceType, *m_dataType, true, true);
-					<< (u256(1) << (8 * (32 - m_dataType->storageBytes())))
-					<< Instruction::SWAP1
-					<< Instruction::DUP2
-					<< Instruction::MUL
-					<< Instruction::DIV;
 			}
 			m_context  << Instruction::MUL << Instruction::OR;
 			// stack: value storage_ref updated_value

test/libsolidity/SolidityEndToEndTest.cpp

@@ -4506,6 +4506,102 @@ BOOST_AUTO_TEST_CASE(external_types_in_calls)
 	BOOST_CHECK(callContractFunction("t2()") == encodeArgs(u256(9)));
 }
 
+BOOST_AUTO_TEST_CASE(invalid_enum_compared)
+{
+	char const* sourceCode = R"(
+		contract C {
+			enum X { A, B }
+
+			function test_eq() returns (bool) {
+				X garbled;
+				assembly {
+					garbled := 5
+				}
+				return garbled == garbled;
+			}
+			function test_eq_ok() returns (bool) {
+				X garbled = X.A;
+				return garbled == garbled;
+			}
+			function test_neq() returns (bool) {
+				X garbled;
+				assembly {
+					garbled := 5
+				}
+				return garbled != garbled;
+			}
+		}
+	)";
+	compileAndRun(sourceCode, 0, "C");
+	BOOST_CHECK(callContractFunction("test_eq_ok()") == encodeArgs(u256(1)));
+	// both should throw
+	BOOST_CHECK(callContractFunction("test_eq()") == encodeArgs());
+	BOOST_CHECK(callContractFunction("test_neq()") == encodeArgs());
+}
+
+BOOST_AUTO_TEST_CASE(invalid_enum_logged)
+{
+	char const* sourceCode = R"(
+		contract C {
+			enum X { A, B }
+			event Log(X);
+
+			function test_log() returns (uint) {
+				X garbled = X.A;
+				assembly {
+					garbled := 5
+				}
+				Log(garbled);
+				return 1;
+			}
+			function test_log_ok() returns (uint) {
+				X x = X.A;
+				Log(x);
+				return 1;
+			}
+		}
+		)";
+	compileAndRun(sourceCode, 0, "C");
+	BOOST_CHECK(callContractFunction("test_log_ok()") == encodeArgs(u256(1)));
+	BOOST_REQUIRE_EQUAL(m_logs.size(), 1);
+	BOOST_CHECK_EQUAL(m_logs[0].address, m_contractAddress);
+	BOOST_REQUIRE_EQUAL(m_logs[0].topics.size(), 1);
+	BOOST_REQUIRE_EQUAL(m_logs[0].topics[0], dev::keccak256(string("Log(uint8)")));
+	BOOST_CHECK_EQUAL(h256(m_logs[0].data), h256(u256(0)));
+
+	// should throw
+	BOOST_CHECK(callContractFunction("test_log()") == encodeArgs());
+}
+
+BOOST_AUTO_TEST_CASE(invalid_enum_stored)
+{
+	char const* sourceCode = R"(
+		contract C {
+			enum X { A, B }
+			X public x;
+
+			function test_store() returns (uint) {
+				X garbled = X.A;
+				assembly {
+					garbled := 5
+				}
+				x = garbled;
+				return 1;
+			}
+			function test_store_ok() returns (uint) {
+				x = X.A;
+				return 1;
+			}
+		}
+		)";
+	compileAndRun(sourceCode, 0, "C");
+	BOOST_CHECK(callContractFunction("test_store_ok()") == encodeArgs(u256(1)));
+	BOOST_CHECK(callContractFunction("x()") == encodeArgs(u256(0)));
+
+	// should throw
+	BOOST_CHECK(callContractFunction("test_store()") == encodeArgs());
+}
+
 BOOST_AUTO_TEST_CASE(invalid_enum_as_external_ret)
 {
 	char const* sourceCode = R"(