Prevent libraries from being called.

12 files changed, 143 insertions, 7 deletions

diff --git a/Changelog.md b/Changelog.md
index 1c279dfb..4909aca3 100644
--- a/Changelog.md
+++ b/Changelog.md
@@ -1,6 +1,7 @@
 ### 0.4.20 (unreleased)
 
 Features:
+ * Code Generator: Prevent non-view functions in libraries from being called directly.
  * Commandline interface: Support strict mode of assembly with the ``--strict--assembly`` switch.
  * Limit the number of warnings raised for creating abstract contracts.
  * Inline Assembly: Issue warning for using jump labels (already existed for jump instructions).
diff --git a/libevmasm/Assembly.cpp b/libevmasm/Assembly.cpp
index 5fab24e1..b9fedf26 100644
--- a/libevmasm/Assembly.cpp
+++ b/libevmasm/Assembly.cpp
@@ -283,6 +283,11 @@ Json::Value Assembly::assemblyJSON(StringMap const& _sourceCodes) const
 				createJsonValue("PUSHLIB", i.location().start, i.location().end, m_libraries.at(h256(i.data())))
 			);
 			break;
+		case PushDeployTimeAddress:
+			collection.append(
+				createJsonValue("PUSHDEPLOYADDRESS", i.location().start, i.location().end)
+			);
+			break;
 		case Tag:
 			collection.append(
 				createJsonValue("tag", i.location().start, i.location().end, string(i.data())));
@@ -590,6 +595,10 @@ LinkerObject const& Assembly::assemble() const
 			ret.linkReferences[ret.bytecode.size()] = m_libraries.at(i.data());
 			ret.bytecode.resize(ret.bytecode.size() + 20);
 			break;
+		case PushDeployTimeAddress:
+			ret.bytecode.push_back(byte(Instruction::PUSH20));
+			ret.bytecode.resize(ret.bytecode.size() + 20);
+			break;
 		case Tag:
 			assertThrow(i.data() != 0, AssemblyException, "Invalid tag position.");
 			assertThrow(i.splitForeignPushTag().first == size_t(-1), AssemblyException, "Foreign tag.");
diff --git a/libevmasm/AssemblyItem.cpp b/libevmasm/AssemblyItem.cpp
index 64963021..5af618ff 100644
--- a/libevmasm/AssemblyItem.cpp
+++ b/libevmasm/AssemblyItem.cpp
@@ -68,6 +68,7 @@ unsigned AssemblyItem::bytesRequired(unsigned _addressLength) const
 	case PushSub:
 		return 1 + _addressLength;
 	case PushLibraryAddress:
+	case PushDeployTimeAddress:
 		return 1 + 20;
 	default:
 		break;
@@ -97,6 +98,7 @@ int AssemblyItem::returnValues() const
 	case PushSubSize:
 	case PushProgramSize:
 	case PushLibraryAddress:
+	case PushDeployTimeAddress:
 		return 1;
 	case Tag:
 		return 0;
@@ -119,6 +121,7 @@ bool AssemblyItem::canBeFunctional() const
 	case PushSubSize:
 	case PushProgramSize:
 	case PushLibraryAddress:
+	case PushDeployTimeAddress:
 		return true;
 	case Tag:
 		return false;
@@ -190,6 +193,9 @@ string AssemblyItem::toAssemblyText() const
 	case PushLibraryAddress:
 		text = string("linkerSymbol(\"") + toHex(data()) + string("\")");
 		break;
+	case PushDeployTimeAddress:
+		text = string("deployTimeAddress()");
+		break;
 	case UndefinedItem:
 		assertThrow(false, AssemblyException, "Invalid assembly item.");
 		break;
@@ -252,6 +258,9 @@ ostream& dev::eth::operator<<(ostream& _out, AssemblyItem const& _item)
 		_out << " PushLibraryAddress " << hash.substr(0, 8) + "..." + hash.substr(hash.length() - 8);
 		break;
 	}
+	case PushDeployTimeAddress:
+		_out << " PushDeployTimeAddress";
+		break;
 	case UndefinedItem:
 		_out << " ???";
 		break;
diff --git a/libevmasm/AssemblyItem.h b/libevmasm/AssemblyItem.h
index d38db927..5319a2b6 100644
--- a/libevmasm/AssemblyItem.h
+++ b/libevmasm/AssemblyItem.h
@@ -46,7 +46,8 @@ enum AssemblyItemType {
 	PushProgramSize,
 	Tag,
 	PushData,
-	PushLibraryAddress ///< Push a currently unknown address of another (library) contract.
+	PushLibraryAddress, ///< Push a currently unknown address of another (library) contract.
+	PushDeployTimeAddress ///< Push an address to be filled at deploy time. Should not be touched by the optimizer.
 };
 
 class Assembly;
diff --git a/libevmasm/GasMeter.cpp b/libevmasm/GasMeter.cpp
index dad952bc..543f1cbc 100644
--- a/libevmasm/GasMeter.cpp
+++ b/libevmasm/GasMeter.cpp
@@ -52,6 +52,7 @@ GasMeter::GasConsumption GasMeter::estimateMax(AssemblyItem const& _item, bool _
 	case PushSubSize:
 	case PushProgramSize:
 	case PushLibraryAddress:
+	case PushDeployTimeAddress:
 		gas = runGas(Instruction::PUSH1);
 		break;
 	case Tag:
diff --git a/libevmasm/SemanticInformation.cpp b/libevmasm/SemanticInformation.cpp
index 4c2290c4..03870f7c 100644
--- a/libevmasm/SemanticInformation.cpp
+++ b/libevmasm/SemanticInformation.cpp
@@ -35,6 +35,7 @@ bool SemanticInformation::breaksCSEAnalysisBlock(AssemblyItem const& _item)
 	default:
 	case UndefinedItem:
 	case Tag:
+	case PushDeployTimeAddress:
 		return true;
 	case Push:
 	case PushString:
diff --git a/libsolidity/codegen/Compiler.cpp b/libsolidity/codegen/Compiler.cpp
index 44264a07..d3afada5 100644
--- a/libsolidity/codegen/Compiler.cpp
+++ b/libsolidity/codegen/Compiler.cpp
@@ -51,6 +51,7 @@ void Compiler::compileClone(
 	map<ContractDefinition const*, eth::Assembly const*> const& _contracts
 )
 {
+	solAssert(!_contract.isLibrary(), "");
 	ContractCompiler runtimeCompiler(nullptr, m_runtimeContext, m_optimize);
 	ContractCompiler cloneCompiler(&runtimeCompiler, m_context, m_optimize);
 	m_runtimeSub = cloneCompiler.compileClone(_contract, _contracts);
diff --git a/libsolidity/codegen/CompilerContext.h b/libsolidity/codegen/CompilerContext.h
index 0e8b639c..a155a3a5 100644
--- a/libsolidity/codegen/CompilerContext.h
+++ b/libsolidity/codegen/CompilerContext.h
@@ -174,6 +174,9 @@ public:
 	eth::AssemblyItem appendData(bytes const& _data) { return m_asm->append(_data); }
 	/// Appends the address (virtual, will be filled in by linker) of a library.
 	void appendLibraryAddress(std::string const& _identifier) { m_asm->appendLibraryAddress(_identifier); }
+	/// Appends a zero-address that can be replaced by something else at deploy time (if the
+	/// position in bytecode is known).
+	void appendDeployTimeAddress() { m_asm->append(eth::PushDeployTimeAddress); }
 	/// Resets the stack of visited nodes with a new stack having only @c _node
 	void resetVisitedNodes(ASTNode const* _node);
 	/// Pops the stack of visited nodes
diff --git a/libsolidity/codegen/ContractCompiler.cpp b/libsolidity/codegen/ContractCompiler.cpp
index a81ba518..f463db94 100644
--- a/libsolidity/codegen/ContractCompiler.cpp
+++ b/libsolidity/codegen/ContractCompiler.cpp
@@ -64,6 +64,12 @@ void ContractCompiler::compileContract(
 )
 {
 	CompilerContext::LocationSetter locationSetter(m_context, _contract);
+
+	if (_contract.isLibrary())
+		// Check whether this is a call (true) or a delegatecall (false).
+		// This has to be the first code in the contract.
+		appendDelegatecallCheck();
+
 	initializeContext(_contract, _contracts);
 	appendFunctionSelector(_contract);
 	appendMissingFunctions();
@@ -75,8 +81,13 @@ size_t ContractCompiler::compileConstructor(
 )
 {
 	CompilerContext::LocationSetter locationSetter(m_context, _contract);
-	initializeContext(_contract, _contracts);
-	return packIntoContractCreator(_contract);
+	if (_contract.isLibrary())
+		return deployLibrary(_contract);
+	else
+	{
+		initializeContext(_contract, _contracts);
+		return packIntoContractCreator(_contract);
+	}
 }
 
 size_t ContractCompiler::compileClone(
@@ -122,6 +133,7 @@ void ContractCompiler::appendCallValueCheck()
 
 void ContractCompiler::appendInitAndConstructorCode(ContractDefinition const& _contract)
 {
+	solAssert(!_contract.isLibrary(), "Tried to initialize library.");
 	CompilerContext::LocationSetter locationSetter(m_context, _contract);
 	// Determine the arguments that are used for the base constructors.
 	std::vector<ContractDefinition const*> const& bases = _contract.annotation().linearizedBaseContracts;
@@ -163,6 +175,7 @@ void ContractCompiler::appendInitAndConstructorCode(ContractDefinition const& _c
 size_t ContractCompiler::packIntoContractCreator(ContractDefinition const& _contract)
 {
 	solAssert(!!m_runtimeCompiler, "");
+	solAssert(!_contract.isLibrary(), "Tried to use contract creator or library.");
 
 	appendInitAndConstructorCode(_contract);
 
@@ -188,6 +201,34 @@ size_t ContractCompiler::packIntoContractCreator(ContractDefinition const& _cont
 	return m_context.runtimeSub();
 }
 
+size_t ContractCompiler::deployLibrary(ContractDefinition const& _contract)
+{
+	solAssert(!!m_runtimeCompiler, "");
+	solAssert(_contract.isLibrary(), "Tried to deploy contract as library.");
+
+	CompilerContext::LocationSetter locationSetter(m_context, _contract);
+
+	solAssert(m_context.runtimeSub() != size_t(-1), "Runtime sub not registered");
+	m_context.pushSubroutineSize(m_context.runtimeSub());
+	m_context.pushSubroutineOffset(m_context.runtimeSub());
+	m_context.appendInlineAssembly(R"(
+	{
+		// If code starts at 11, an mstore(0) writes to the full PUSH20 plus data
+		// without the need for a shift.
+		let codepos := 11
+		codecopy(codepos, subOffset, subSize)
+		// Check that the first opcode is a PUSH20
+		switch eq(0x73, byte(0, mload(codepos)))
+		case 0 { invalid() }
+		mstore(0, address())
+		mstore8(codepos, 0x73)
+		return(codepos, subSize)
+	}
+	)", {"subSize", "subOffset"});
+
+	return m_context.runtimeSub();
+}
+
 void ContractCompiler::appendBaseConstructor(FunctionDefinition const& _constructor)
 {
 	CompilerContext::LocationSetter locationSetter(m_context, _constructor);
@@ -244,11 +285,26 @@ void ContractCompiler::appendConstructor(FunctionDefinition const& _constructor)
 	_constructor.accept(*this);
 }
 
+void ContractCompiler::appendDelegatecallCheck()
+{
+	// Special constant that will be replaced by the address at deploy time.
+	// At compilation time, this is just "PUSH20 00...000".
+	m_context.appendDeployTimeAddress();
+	m_context << Instruction::ADDRESS << Instruction::EQ;
+	// The result on the stack is
+	// "We have not been called via DELEGATECALL".
+}
+
 void ContractCompiler::appendFunctionSelector(ContractDefinition const& _contract)
 {
 	map<FixedHash<4>, FunctionTypePointer> interfaceFunctions = _contract.interfaceFunctions();
 	map<FixedHash<4>, const eth::AssemblyItem> callDataUnpackerEntryPoints;
 
+	if (_contract.isLibrary())
+	{
+		solAssert(m_context.stackHeight() == 1, "CALL / DELEGATECALL flag expected.");
+	}
+
 	FunctionDefinition const* fallback = _contract.fallbackFunction();
 	eth::AssemblyItem notFound = m_context.newTag();
 	// directly jump to fallback if the data is too short to contain a function selector
@@ -260,7 +316,7 @@ void ContractCompiler::appendFunctionSelector(ContractDefinition const& _contrac
 	if (!interfaceFunctions.empty())
 		CompilerUtils(m_context).loadFromMemory(0, IntegerType(CompilerUtils::dataStartOffset * 8), true);
 
-	// stack now is: 1 0 <funhash>
+	// stack now is: <can-call-non-view-functions>? <funhash>
 	for (auto const& it: interfaceFunctions)
 	{
 		callDataUnpackerEntryPoints.insert(std::make_pair(it.first, m_context.newTag()));
@@ -272,6 +328,7 @@ void ContractCompiler::appendFunctionSelector(ContractDefinition const& _contrac
 	m_context << notFound;
 	if (fallback)
 	{
+		solAssert(!_contract.isLibrary(), "");
 		if (!fallback->isPayable())
 			appendCallValueCheck();
 
@@ -291,6 +348,13 @@ void ContractCompiler::appendFunctionSelector(ContractDefinition const& _contrac
 		CompilerContext::LocationSetter locationSetter(m_context, functionType->declaration());
 
 		m_context << callDataUnpackerEntryPoints.at(it.first);
+		if (_contract.isLibrary() && functionType->stateMutability() > StateMutability::View)
+		{
+			// If the function is not a view function and is called without DELEGATECALL,
+			// we revert.
+			m_context << dupInstruction(2);
+			m_context.appendConditionalRevert();
+		}
 		m_context.setStackOffset(0);
 		// We have to allow this for libraries, because value of the previous
 		// call is still visible in the delegatecall.
@@ -441,6 +505,7 @@ void ContractCompiler::registerStateVariables(ContractDefinition const& _contrac
 
 void ContractCompiler::initializeStateVariables(ContractDefinition const& _contract)
 {
+	solAssert(!_contract.isLibrary(), "Tried to initialize state variables of library.");
 	for (VariableDeclaration const* variable: _contract.stateVariables())
 		if (variable->value() && !variable->isConstant())
 			ExpressionCompiler(m_context, m_optimise).appendStateVariableInitialization(*variable);
diff --git a/libsolidity/codegen/ContractCompiler.h b/libsolidity/codegen/ContractCompiler.h
index 7c5ee59f..1fd80d05 100644
--- a/libsolidity/codegen/ContractCompiler.h
+++ b/libsolidity/codegen/ContractCompiler.h
@@ -75,10 +75,19 @@ private:
 	/// with a new and initialized context. Adds the constructor code.
 	/// @returns the identifier of the runtime sub assembly
 	size_t packIntoContractCreator(ContractDefinition const& _contract);
+	/// Appends code that deploys the given contract as a library.
+	/// Will also add code that modifies the contract in memory by injecting the current address
+	/// for the call protector.
+	size_t deployLibrary(ContractDefinition const& _contract);
 	/// Appends state variable initialisation and constructor code.
 	void appendInitAndConstructorCode(ContractDefinition const& _contract);
 	void appendBaseConstructor(FunctionDefinition const& _constructor);
 	void appendConstructor(FunctionDefinition const& _constructor);
+	/// Appends code that returns a boolean flag on the stack that tells whether
+	/// the contract has been called via delegatecall (false) or regular call (true).
+	/// This is done by inserting a specific push constant as the first instruction
+	/// whose data will be modified in memory at deploy time.
+	void appendDelegatecallCheck();
 	void appendFunctionSelector(ContractDefinition const& _contract);
 	void appendCallValueCheck();
 	/// Creates code that unpacks the arguments for the given function represented by a vector of TypePointers.
diff --git a/libsolidity/interface/CompilerStack.cpp b/libsolidity/interface/CompilerStack.cpp
index 5713256a..3b5e65e8 100644
--- a/libsolidity/interface/CompilerStack.cpp
+++ b/libsolidity/interface/CompilerStack.cpp
@@ -734,9 +734,12 @@ void CompilerStack::compileContract(
 
 	try
 	{
-		Compiler cloneCompiler(m_optimize, m_optimizeRuns);
-		cloneCompiler.compileClone(_contract, _compiledContracts);
-		compiledContract.cloneObject = cloneCompiler.assembledObject();
+		if (!_contract.isLibrary())
+		{
+			Compiler cloneCompiler(m_optimize, m_optimizeRuns);
+			cloneCompiler.compileClone(_contract, _compiledContracts);
+			compiledContract.cloneObject = cloneCompiler.assembledObject();
+		}
 	}
 	catch (eth::AssemblyException const&)
 	{
diff --git a/test/libsolidity/SolidityEndToEndTest.cpp b/test/libsolidity/SolidityEndToEndTest.cpp
index f5f7e64a..5b98d979 100644
--- a/test/libsolidity/SolidityEndToEndTest.cpp
+++ b/test/libsolidity/SolidityEndToEndTest.cpp
@@ -3543,6 +3543,39 @@ BOOST_AUTO_TEST_CASE(library_call_in_homestead)
 	ABI_CHECK(callContractFunction("sender()"), encodeArgs(u160(m_sender)));
 }
 
+BOOST_AUTO_TEST_CASE(library_call_protection)
+{
+	// This tests code that reverts a call if it is a direct call to a library
+	// as opposed to a delegatecall.
+	char const* sourceCode = R"(
+		library Lib {
+			struct S { uint x; }
+			// a direct call to this should revert
+			function np(S storage s) public returns (address) { s.x = 3; return msg.sender; }
+			// a direct call to this is fine
+			function v(S storage) public view returns (address) { return msg.sender; }
+			// a direct call to this is fine
+			function pu() public pure returns (uint) { return 2; }
+		}
+		contract Test {
+			Lib.S public s;
+			function np() public returns (address) { return Lib.np(s); }
+			function v() public view returns (address) { return Lib.v(s); }
+			function pu() public pure returns (uint) { return Lib.pu(); }
+		}
+	)";
+	compileAndRun(sourceCode, 0, "Lib");
+	ABI_CHECK(callContractFunction("np(Lib.S storage)"), encodeArgs());
+	ABI_CHECK(callContractFunction("v(Lib.S storage)"), encodeArgs(u160(m_sender)));
+	ABI_CHECK(callContractFunction("pu()"), encodeArgs(2));
+	compileAndRun(sourceCode, 0, "Test", bytes(), map<string, Address>{{"Lib", m_contractAddress}});
+	ABI_CHECK(callContractFunction("s()"), encodeArgs(0));
+	ABI_CHECK(callContractFunction("np()"), encodeArgs(u160(m_sender)));
+	ABI_CHECK(callContractFunction("s()"), encodeArgs(3));
+	ABI_CHECK(callContractFunction("v()"), encodeArgs(u160(m_sender)));
+	ABI_CHECK(callContractFunction("pu()"), encodeArgs(2));
+}
+
 BOOST_AUTO_TEST_CASE(store_bytes)
 {
 	// this test just checks that the copy loop does not mess up the stack