diff options
| -rw-r--r-- | Changelog.md | 1 | ||||
| -rw-r--r-- | docs/abi-spec.rst | 10 | ||||
| -rw-r--r-- | docs/contracts.rst | 40 | ||||
| -rw-r--r-- | libevmasm/Assembly.cpp | 9 | ||||
| -rw-r--r-- | libevmasm/AssemblyItem.cpp | 9 | ||||
| -rw-r--r-- | libevmasm/AssemblyItem.h | 3 | ||||
| -rw-r--r-- | libevmasm/GasMeter.cpp | 1 | ||||
| -rw-r--r-- | libevmasm/SemanticInformation.cpp | 1 | ||||
| -rw-r--r-- | libsolidity/codegen/Compiler.cpp | 1 | ||||
| -rw-r--r-- | libsolidity/codegen/CompilerContext.h | 3 | ||||
| -rw-r--r-- | libsolidity/codegen/ContractCompiler.cpp | 71 | ||||
| -rw-r--r-- | libsolidity/codegen/ContractCompiler.h | 9 | ||||
| -rw-r--r-- | libsolidity/interface/CompilerStack.cpp | 9 | ||||
| -rwxr-xr-x | scripts/install_deps.sh | 5 | ||||
| -rw-r--r-- | test/libsolidity/SolidityEndToEndTest.cpp | 33 |
15 files changed, 186 insertions, 19 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/docs/abi-spec.rst b/docs/abi-spec.rst index b81e35a6..0c0d4556 100644 --- a/docs/abi-spec.rst +++ b/docs/abi-spec.rst @@ -40,21 +40,21 @@ The following elementary types exist: - ``int<M>``: two's complement signed integer type of ``M`` bits, ``0 < M <= 256``, ``M % 8 == 0``. -- ``address``: equivalent to ``uint160``, except for the assumed interpretation and language typing. +- ``address``: equivalent to ``uint160``, except for the assumed interpretation and language typing. For computing the function selector, ``address`` is used. -- ``uint``, ``int``: synonyms for ``uint256``, ``int256`` respectively (this shorthand not to be used for computing the function selector). +- ``uint``, ``int``: synonyms for ``uint256``, ``int256`` respectively. For computing the function selector, ``uint256`` and ``int256`` have to be used. -- ``bool``: equivalent to ``uint8`` restricted to the values 0 and 1 +- ``bool``: equivalent to ``uint8`` restricted to the values 0 and 1. For computing the function selector, ``bool`` is used. - ``fixed<M>x<N>``: signed fixed-point decimal number of ``M`` bits, ``8 <= M <= 256``, ``M % 8 ==0``, and ``0 < N <= 80``, which denotes the value ``v`` as ``v / (10 ** N)``. - ``ufixed<M>x<N>``: unsigned variant of ``fixed<M>x<N>``. -- ``fixed``, ``ufixed``: synonyms for ``fixed128x19``, ``ufixed128x19`` respectively (this shorthand not to be used for computing the function selector). +- ``fixed``, ``ufixed``: synonyms for ``fixed128x19``, ``ufixed128x19`` respectively. For computing the function selector, ``fixed128x19`` and ``ufixed128x19`` have to be used. - ``bytes<M>``: binary type of ``M`` bytes, ``0 < M <= 32``. -- ``function``: equivalent to ``bytes24``: an address, followed by a function selector +- ``function``: an address (20 bytes) folled by a function selector (4 bytes). Encoded identical to ``bytes24``. The following (fixed-size) array type exists: diff --git a/docs/contracts.rst b/docs/contracts.rst index fcc26b24..afc32b16 100644 --- a/docs/contracts.rst +++ b/docs/contracts.rst @@ -939,15 +939,15 @@ derived override, but this function will bypass function kill() public { /* do cleanup 2 */ super.kill(); } } - contract Final is Base2, Base1 { + contract Final is Base1, Base2 { } -If ``Base1`` calls a function of ``super``, it does not simply +If ``Base2`` calls a function of ``super``, it does not simply call this function on one of its base contracts. Rather, it calls this function on the next base contract in the final -inheritance graph, so it will call ``Base2.kill()`` (note that +inheritance graph, so it will call ``Base1.kill()`` (note that the final inheritance sequence is -- starting with the most -derived contract: Final, Base1, Base2, mortal, owned). +derived contract: Final, Base2, Base1, mortal, owned). The actual function that is called when using super is not known in the context of the class where it is used, although its type is known. This is similar for ordinary @@ -1100,7 +1100,11 @@ is executed in the context of the calling contract, i.e. ``this`` points to the calling contract, and especially the storage from the calling contract can be accessed. As a library is an isolated piece of source code, it can only access state variables of the calling contract if they are explicitly supplied (it -would have no way to name them, otherwise). +would have no way to name them, otherwise). Library functions can only be +called directly (i.e. without the use of ``DELEGATECALL``) if they do not modify +the state (i.e. if they are ``view`` or ``pure`` functions), +because libraries are assumed to be stateless. In particular, it is +not possible to destroy a library unless Solidity's type system is circumvented. Libraries can be seen as implicit base contracts of the contracts that use them. They will not be explicitly visible in the inheritance hierarchy, but calls @@ -1111,7 +1115,7 @@ if the library were a base contract. Of course, calls to internal functions use the internal calling convention, which means that all internal types can be passed and memory types will be passed by reference and not copied. To realize this in the EVM, code of internal library functions -and all functions called from therein will be pulled into the calling +and all functions called from therein will at compile time be pulled into the calling contract, and a regular ``JUMP`` call will be used instead of a ``DELEGATECALL``. .. index:: using for, set @@ -1268,6 +1272,30 @@ Restrictions for libraries in comparison to contracts: (These might be lifted at a later point.) +Call Protection For Libraries +============================= + +As mentioned in the introduction, if a library's code is executed +using a ``CALL`` instead of a ``DELEGATECALL`` or ``CALLCODE``, +it will revert unless a ``view`` or ``pure`` function is called. + +The EVM does not provide a direct way for a contract to detect +whether it was called using ``CALL`` or not, but a contract +can use the ``ADDRESS`` opcode to find out "where" it is +currently running. The generated code compares this address +to the address used at construction time to determine the mode +of calling. + +More specifically, the runtime code of a library always starts +with a push instruction, which is a zero of 20 bytes at +compilation time. When the deploy code runs, this constant +is replaced in memory by the current address and this +modified code is stored in the contract. At runtime, +this causes the deploy time address to be the first +constant to be pushed onto the stack and the dispatcher +code compares the current address against this constant +for any non-view and non-pure function. + .. index:: ! using for, library .. _using-for: 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/scripts/install_deps.sh b/scripts/install_deps.sh index a8966f96..d62cffb7 100755 --- a/scripts/install_deps.sh +++ b/scripts/install_deps.sh @@ -84,9 +84,12 @@ case $(uname -s) in 10.12) echo "Installing solidity dependencies on macOS 10.12 Sierra." ;; + 10.13) + echo "Installing solidity dependencies on macOS 10.13 High Sierra." + ;; *) echo "Unsupported macOS version." - echo "We only support Mavericks, Yosemite and El Capitan, with work-in-progress on Sierra." + echo "We only support Mavericks, Yosemite, El Capitan, Sierra and High Sierra." exit 1 ;; esac 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 |