From 75a92b0ffd0946c17a27a58e6e02abe96cd3fa00 Mon Sep 17 00:00:00 2001 From: Erik Kundt Date: Thu, 26 Jul 2018 21:45:24 +0200 Subject: Warns if modifier uses msg.value in non-payable function. --- Changelog.md | 1 + libsolidity/analysis/StaticAnalyzer.cpp | 10 -- libsolidity/analysis/StaticAnalyzer.h | 3 - libsolidity/analysis/TypeChecker.cpp | 4 +- libsolidity/analysis/ViewPureChecker.cpp | 123 +++++++++++++++------ libsolidity/analysis/ViewPureChecker.h | 25 ++--- .../memberLookup/msg_value_modifier_payable.sol | 4 + .../memberLookup/msg_value_modifier_pure.sol | 6 + .../memberLookup/msg_value_modifier_view.sol | 6 + .../viewPureChecker/msg_value_modifier.sol | 6 + .../viewPureChecker/msg_value_modifier_view.sol | 6 + 11 files changed, 129 insertions(+), 65 deletions(-) create mode 100644 test/libsolidity/syntaxTests/memberLookup/msg_value_modifier_payable.sol create mode 100644 test/libsolidity/syntaxTests/memberLookup/msg_value_modifier_pure.sol create mode 100644 test/libsolidity/syntaxTests/memberLookup/msg_value_modifier_view.sol create mode 100644 test/libsolidity/syntaxTests/viewPureChecker/msg_value_modifier.sol create mode 100644 test/libsolidity/syntaxTests/viewPureChecker/msg_value_modifier_view.sol diff --git a/Changelog.md b/Changelog.md index 1e16df59..30a851c8 100644 --- a/Changelog.md +++ b/Changelog.md @@ -87,6 +87,7 @@ Compiler Features: * Tests: Determine transaction status during IPC calls. * Code Generator: Allocate and free local variables according to their scope. * Removed ``pragma experimental "v0.5.0";``. + * View Pure Checker: Warn about ``msg.value`` used by modifier in non-payable function. Bugfixes: * Build System: Support versions of CVC4 linked against CLN instead of GMP. In case of compilation issues due to the experimental SMT solver support, the solvers can be disabled when configuring the project with CMake using ``-DUSE_CVC4=OFF`` or ``-DUSE_Z3=OFF``. diff --git a/libsolidity/analysis/StaticAnalyzer.cpp b/libsolidity/analysis/StaticAnalyzer.cpp index 60a58665..487a5cca 100644 --- a/libsolidity/analysis/StaticAnalyzer.cpp +++ b/libsolidity/analysis/StaticAnalyzer.cpp @@ -56,7 +56,6 @@ bool StaticAnalyzer::visit(FunctionDefinition const& _function) else solAssert(!m_currentFunction, ""); solAssert(m_localVarUseCount.empty(), ""); - m_nonPayablePublic = _function.isPublic() && !_function.isPayable(); m_constructor = _function.isConstructor(); return true; } @@ -64,7 +63,6 @@ bool StaticAnalyzer::visit(FunctionDefinition const& _function) void StaticAnalyzer::endVisit(FunctionDefinition const&) { m_currentFunction = nullptr; - m_nonPayablePublic = false; m_constructor = false; for (auto const& var: m_localVarUseCount) if (var.second == 0) @@ -154,14 +152,6 @@ bool StaticAnalyzer::visit(MemberAccess const& _memberAccess) ); } - if (m_nonPayablePublic && !m_library) - if (MagicType const* type = dynamic_cast(_memberAccess.expression().annotation().type.get())) - if (type->kind() == MagicType::Kind::Message && _memberAccess.memberName() == "value") - m_errorReporter.warning( - _memberAccess.location(), - "\"msg.value\" used in non-payable function. Do you want to add the \"payable\" modifier to this function?" - ); - if (_memberAccess.memberName() == "callcode") if (auto const* type = dynamic_cast(_memberAccess.annotation().type.get())) if (type->kind() == FunctionType::Kind::BareCallCode) diff --git a/libsolidity/analysis/StaticAnalyzer.h b/libsolidity/analysis/StaticAnalyzer.h index 2a62e391..7f5c743a 100644 --- a/libsolidity/analysis/StaticAnalyzer.h +++ b/libsolidity/analysis/StaticAnalyzer.h @@ -75,9 +75,6 @@ private: /// Flag that indicates whether the current contract definition is a library. bool m_library = false; - /// Flag that indicates whether a public function does not contain the "payable" modifier. - bool m_nonPayablePublic = false; - /// Number of uses of each (named) local variable in a function, counter is initialized with zero. /// Pairs of AST ids and pointers are used as keys to ensure a deterministic order /// when traversing. diff --git a/libsolidity/analysis/TypeChecker.cpp b/libsolidity/analysis/TypeChecker.cpp index fca64f6a..8b941fca 100644 --- a/libsolidity/analysis/TypeChecker.cpp +++ b/libsolidity/analysis/TypeChecker.cpp @@ -686,9 +686,7 @@ bool TypeChecker::visit(StructDefinition const& _struct) bool TypeChecker::visit(FunctionDefinition const& _function) { - bool isLibraryFunction = - dynamic_cast(_function.scope()) && - dynamic_cast(_function.scope())->isLibrary(); + bool isLibraryFunction = _function.inContractKind() == ContractDefinition::ContractKind::Library; if (_function.isPayable()) { if (isLibraryFunction) diff --git a/libsolidity/analysis/ViewPureChecker.cpp b/libsolidity/analysis/ViewPureChecker.cpp index e92ad2fa..291b4681 100644 --- a/libsolidity/analysis/ViewPureChecker.cpp +++ b/libsolidity/analysis/ViewPureChecker.cpp @@ -142,7 +142,7 @@ bool ViewPureChecker::visit(FunctionDefinition const& _funDef) { solAssert(!m_currentFunction, ""); m_currentFunction = &_funDef; - m_currentBestMutability = StateMutability::Pure; + m_bestMutabilityAndLocation = {StateMutability::Pure, _funDef.location()}; return true; } @@ -150,7 +150,7 @@ void ViewPureChecker::endVisit(FunctionDefinition const& _funDef) { solAssert(m_currentFunction == &_funDef, ""); if ( - m_currentBestMutability < _funDef.stateMutability() && + m_bestMutabilityAndLocation.mutability < _funDef.stateMutability() && _funDef.stateMutability() != StateMutability::Payable && _funDef.isImplemented() && !_funDef.isConstructor() && @@ -159,22 +159,22 @@ void ViewPureChecker::endVisit(FunctionDefinition const& _funDef) ) m_errorReporter.warning( _funDef.location(), - "Function state mutability can be restricted to " + stateMutabilityToString(m_currentBestMutability) + "Function state mutability can be restricted to " + stateMutabilityToString(m_bestMutabilityAndLocation.mutability) ); m_currentFunction = nullptr; } -bool ViewPureChecker::visit(ModifierDefinition const&) +bool ViewPureChecker::visit(ModifierDefinition const& _modifier) { solAssert(m_currentFunction == nullptr, ""); - m_currentBestMutability = StateMutability::Pure; + m_bestMutabilityAndLocation = {StateMutability::Pure, _modifier.location()}; return true; } void ViewPureChecker::endVisit(ModifierDefinition const& _modifierDef) { solAssert(m_currentFunction == nullptr, ""); - m_inferredMutability[&_modifierDef] = m_currentBestMutability; + m_inferredMutability[&_modifierDef] = std::move(m_bestMutabilityAndLocation); } void ViewPureChecker::endVisit(Identifier const& _identifier) @@ -219,36 +219,70 @@ void ViewPureChecker::endVisit(InlineAssembly const& _inlineAssembly) }(_inlineAssembly.operations()); } -void ViewPureChecker::reportMutability(StateMutability _mutability, SourceLocation const& _location) +void ViewPureChecker::reportMutability( + StateMutability _mutability, + SourceLocation const& _location, + boost::optional const& _nestedLocation +) { - if (m_currentFunction && m_currentFunction->stateMutability() < _mutability) + if (_mutability > m_bestMutabilityAndLocation.mutability) + m_bestMutabilityAndLocation = MutabilityAndLocation{_mutability, _location}; + if (!m_currentFunction || _mutability <= m_currentFunction->stateMutability()) + return; + + // Check for payable here, because any occurrence of `msg.value` + // will set mutability to payable. + if (_mutability == StateMutability::View || ( + _mutability == StateMutability::Payable && + m_currentFunction->stateMutability() == StateMutability::Pure + )) { - if (_mutability == StateMutability::View) - m_errorReporter.typeError( - _location, - "Function declared as pure, but this expression (potentially) reads from the " - "environment or state and thus requires \"view\"." - ); - else if (_mutability == StateMutability::NonPayable) - m_errorReporter.typeError( - _location, - "Function declared as " + - stateMutabilityToString(m_currentFunction->stateMutability()) + - ", but this expression (potentially) modifies the state and thus " - "requires non-payable (the default) or payable." - ); - else - solAssert(false, ""); - - solAssert( - m_currentFunction->stateMutability() == StateMutability::View || - m_currentFunction->stateMutability() == StateMutability::Pure, - "" + m_errorReporter.typeError( + _location, + "Function declared as pure, but this expression (potentially) reads from the " + "environment or state and thus requires \"view\"." ); m_errors = true; } - if (_mutability > m_currentBestMutability) - m_currentBestMutability = _mutability; + else if (_mutability == StateMutability::NonPayable) + { + m_errorReporter.typeError( + _location, + "Function declared as " + + stateMutabilityToString(m_currentFunction->stateMutability()) + + ", but this expression (potentially) modifies the state and thus " + "requires non-payable (the default) or payable." + ); + m_errors = true; + } + else if (_mutability == StateMutability::Payable) + { + // We do not warn for library functions because they cannot be payable anyway. + // Also internal functions should be allowed to use `msg.value`. + if (m_currentFunction->isPublic() && m_currentFunction->inContractKind() != ContractDefinition::ContractKind::Library) + { + if (_nestedLocation) + m_errorReporter.warning( + _location, + "This modifier uses \"msg.value\" and thus the function should be payable.", + SecondarySourceLocation().append("\"msg.value\" appears here inside the modifier.", *_nestedLocation) + ); + else + m_errorReporter.warning( + _location, + "\"msg.value\" used in non-payable function. Do you want to add the \"payable\" modifier to this function?" + ); + } + } + else + solAssert(false, ""); + + solAssert( + m_currentFunction->stateMutability() == StateMutability::View || + m_currentFunction->stateMutability() == StateMutability::Pure || + m_currentFunction->stateMutability() == StateMutability::NonPayable, + "" + ); } void ViewPureChecker::endVisit(FunctionCall const& _functionCall) @@ -293,12 +327,28 @@ void ViewPureChecker::endVisit(MemberAccess const& _memberAccess) break; case Type::Category::Magic: { - // we can ignore the kind of magic and only look at the name of the member - set static const pureMembers{ - "encode", "encodePacked", "encodeWithSelector", "encodeWithSignature", "decode", "data", "sig", "blockhash" + using MagicMember = pair; + set static const pureMembers{ + {MagicType::Kind::ABI, "decode"}, + {MagicType::Kind::ABI, "encode"}, + {MagicType::Kind::ABI, "encodePacked"}, + {MagicType::Kind::ABI, "encodeWithSelector"}, + {MagicType::Kind::ABI, "encodeWithSignature"}, + {MagicType::Kind::Block, "blockhash"}, + {MagicType::Kind::Message, "data"}, + {MagicType::Kind::Message, "sig"} }; - if (!pureMembers.count(member)) + set static const payableMembers{ + {MagicType::Kind::Message, "value"} + }; + + auto const& type = dynamic_cast(*_memberAccess.expression().annotation().type); + MagicMember magicMember(type.kind(), member); + + if (!pureMembers.count(magicMember)) mutability = StateMutability::View; + if (payableMembers.count(magicMember)) + mutability = StateMutability::Payable; break; } case Type::Category::Struct: @@ -338,7 +388,8 @@ void ViewPureChecker::endVisit(ModifierInvocation const& _modifier) if (ModifierDefinition const* mod = dynamic_cast(_modifier.name()->annotation().referencedDeclaration)) { solAssert(m_inferredMutability.count(mod), ""); - reportMutability(m_inferredMutability.at(mod), _modifier.location()); + auto const& mutAndLocation = m_inferredMutability.at(mod); + reportMutability(mutAndLocation.mutability, _modifier.location(), mutAndLocation.location); } else solAssert(dynamic_cast(_modifier.name()->annotation().referencedDeclaration), ""); diff --git a/libsolidity/analysis/ViewPureChecker.h b/libsolidity/analysis/ViewPureChecker.h index 3db52e7e..faa5b698 100644 --- a/libsolidity/analysis/ViewPureChecker.h +++ b/libsolidity/analysis/ViewPureChecker.h @@ -31,16 +31,6 @@ namespace dev namespace solidity { -class ASTNode; -class FunctionDefinition; -class ModifierDefinition; -class Identifier; -class MemberAccess; -class IndexAccess; -class ModifierInvocation; -class FunctionCall; -class InlineAssembly; - class ViewPureChecker: private ASTConstVisitor { public: @@ -50,6 +40,11 @@ public: bool check(); private: + struct MutabilityAndLocation + { + StateMutability mutability; + SourceLocation location; + }; virtual bool visit(FunctionDefinition const& _funDef) override; virtual void endVisit(FunctionDefinition const& _funDef) override; @@ -65,15 +60,19 @@ private: /// Called when an element of mutability @a _mutability is encountered. /// Creates appropriate warnings and errors and sets @a m_currentBestMutability. - void reportMutability(StateMutability _mutability, SourceLocation const& _location); + void reportMutability( + StateMutability _mutability, + SourceLocation const& _location, + boost::optional const& _nestedLocation = {} + ); std::vector> const& m_ast; ErrorReporter& m_errorReporter; bool m_errors = false; - StateMutability m_currentBestMutability = StateMutability::Payable; + MutabilityAndLocation m_bestMutabilityAndLocation = MutabilityAndLocation{StateMutability::Payable, SourceLocation()}; FunctionDefinition const* m_currentFunction = nullptr; - std::map m_inferredMutability; + std::map m_inferredMutability; }; } diff --git a/test/libsolidity/syntaxTests/memberLookup/msg_value_modifier_payable.sol b/test/libsolidity/syntaxTests/memberLookup/msg_value_modifier_payable.sol new file mode 100644 index 00000000..6e93626f --- /dev/null +++ b/test/libsolidity/syntaxTests/memberLookup/msg_value_modifier_payable.sol @@ -0,0 +1,4 @@ +contract C { + modifier costs(uint _amount) { require(msg.value >= _amount); _; } + function f() costs(1 ether) public payable {} +} diff --git a/test/libsolidity/syntaxTests/memberLookup/msg_value_modifier_pure.sol b/test/libsolidity/syntaxTests/memberLookup/msg_value_modifier_pure.sol new file mode 100644 index 00000000..398c127d --- /dev/null +++ b/test/libsolidity/syntaxTests/memberLookup/msg_value_modifier_pure.sol @@ -0,0 +1,6 @@ +contract C { + modifier costs(uint _amount) { require(msg.value >= _amount); _; } + function f() costs(1 ether) public pure {} +} +// ---- +// TypeError: (101-115): Function declared as pure, but this expression (potentially) reads from the environment or state and thus requires "view". diff --git a/test/libsolidity/syntaxTests/memberLookup/msg_value_modifier_view.sol b/test/libsolidity/syntaxTests/memberLookup/msg_value_modifier_view.sol new file mode 100644 index 00000000..49a3139c --- /dev/null +++ b/test/libsolidity/syntaxTests/memberLookup/msg_value_modifier_view.sol @@ -0,0 +1,6 @@ +contract C { + modifier costs(uint _amount) { require(msg.value >= _amount); _; } + function f() costs(1 ether) public view {} +} +// ---- +// Warning: (101-115): This modifier uses "msg.value" and thus the function should be payable. diff --git a/test/libsolidity/syntaxTests/viewPureChecker/msg_value_modifier.sol b/test/libsolidity/syntaxTests/viewPureChecker/msg_value_modifier.sol new file mode 100644 index 00000000..160b20a7 --- /dev/null +++ b/test/libsolidity/syntaxTests/viewPureChecker/msg_value_modifier.sol @@ -0,0 +1,6 @@ +contract C { + modifier m(uint _amount, uint _avail) { require(_avail >= _amount); _; } + function f() m(1 ether, msg.value) public pure {} +} +// ---- +// TypeError: (118-127): Function declared as pure, but this expression (potentially) reads from the environment or state and thus requires "view". diff --git a/test/libsolidity/syntaxTests/viewPureChecker/msg_value_modifier_view.sol b/test/libsolidity/syntaxTests/viewPureChecker/msg_value_modifier_view.sol new file mode 100644 index 00000000..4cad4439 --- /dev/null +++ b/test/libsolidity/syntaxTests/viewPureChecker/msg_value_modifier_view.sol @@ -0,0 +1,6 @@ +contract C { + modifier m(uint _amount, uint _avail) { require(_avail >= _amount); _; } + function f() m(1 ether, msg.value) public view {} +} +// ---- +// Warning: (118-127): "msg.value" used in non-payable function. Do you want to add the "payable" modifier to this function? -- cgit From 431c2fbcf370926d6c3e7e93667d8472f2f6b73b Mon Sep 17 00:00:00 2001 From: chriseth Date: Thu, 16 Aug 2018 17:28:49 +0200 Subject: Turn warning into error. --- libsolidity/analysis/ViewPureChecker.cpp | 12 +++++++----- .../syntaxTests/memberLookup/msg_value_modifier_view.sol | 2 +- .../397_warns_msg_value_in_non_payable_public_function.sol | 2 +- .../syntaxTests/viewPureChecker/msg_value_modifier_view.sol | 2 +- 4 files changed, 10 insertions(+), 8 deletions(-) diff --git a/libsolidity/analysis/ViewPureChecker.cpp b/libsolidity/analysis/ViewPureChecker.cpp index 291b4681..be6b7ef7 100644 --- a/libsolidity/analysis/ViewPureChecker.cpp +++ b/libsolidity/analysis/ViewPureChecker.cpp @@ -262,16 +262,18 @@ void ViewPureChecker::reportMutability( if (m_currentFunction->isPublic() && m_currentFunction->inContractKind() != ContractDefinition::ContractKind::Library) { if (_nestedLocation) - m_errorReporter.warning( + m_errorReporter.typeError( _location, - "This modifier uses \"msg.value\" and thus the function should be payable.", - SecondarySourceLocation().append("\"msg.value\" appears here inside the modifier.", *_nestedLocation) + SecondarySourceLocation().append("\"msg.value\" appears here inside the modifier.", *_nestedLocation), + "This modifier uses \"msg.value\" and thus the function has to be payable or internal." ); else - m_errorReporter.warning( + m_errorReporter.typeError( _location, - "\"msg.value\" used in non-payable function. Do you want to add the \"payable\" modifier to this function?" + "\"msg.value\" can only be used in payable public functions. Make the function " + "\"payable\" or use an internal function to avoid this error." ); + m_errors = true; } } else diff --git a/test/libsolidity/syntaxTests/memberLookup/msg_value_modifier_view.sol b/test/libsolidity/syntaxTests/memberLookup/msg_value_modifier_view.sol index 49a3139c..8430c5c3 100644 --- a/test/libsolidity/syntaxTests/memberLookup/msg_value_modifier_view.sol +++ b/test/libsolidity/syntaxTests/memberLookup/msg_value_modifier_view.sol @@ -3,4 +3,4 @@ contract C { function f() costs(1 ether) public view {} } // ---- -// Warning: (101-115): This modifier uses "msg.value" and thus the function should be payable. +// TypeError: (101-115): This modifier uses "msg.value" and thus the function has to be payable or internal. diff --git a/test/libsolidity/syntaxTests/nameAndTypeResolution/397_warns_msg_value_in_non_payable_public_function.sol b/test/libsolidity/syntaxTests/nameAndTypeResolution/397_warns_msg_value_in_non_payable_public_function.sol index 4e1f62e1..c56ad25f 100644 --- a/test/libsolidity/syntaxTests/nameAndTypeResolution/397_warns_msg_value_in_non_payable_public_function.sol +++ b/test/libsolidity/syntaxTests/nameAndTypeResolution/397_warns_msg_value_in_non_payable_public_function.sol @@ -4,4 +4,4 @@ contract C { } } // ---- -// Warning: (52-61): "msg.value" used in non-payable function. Do you want to add the "payable" modifier to this function? +// TypeError: (52-61): "msg.value" can only be used in payable public functions. Make the function "payable" or use an internal function to avoid this error. diff --git a/test/libsolidity/syntaxTests/viewPureChecker/msg_value_modifier_view.sol b/test/libsolidity/syntaxTests/viewPureChecker/msg_value_modifier_view.sol index 4cad4439..613b0198 100644 --- a/test/libsolidity/syntaxTests/viewPureChecker/msg_value_modifier_view.sol +++ b/test/libsolidity/syntaxTests/viewPureChecker/msg_value_modifier_view.sol @@ -3,4 +3,4 @@ contract C { function f() m(1 ether, msg.value) public view {} } // ---- -// Warning: (118-127): "msg.value" used in non-payable function. Do you want to add the "payable" modifier to this function? +// TypeError: (118-127): "msg.value" can only be used in payable public functions. Make the function "payable" or use an internal function to avoid this error. -- cgit From b7c6e53d3d74b649ee6275254d30162aef403e17 Mon Sep 17 00:00:00 2001 From: Leonardo Alt Date: Mon, 20 Aug 2018 15:13:38 +0200 Subject: Fix endToEnd test --- test/libsolidity/SolidityEndToEndTest.cpp | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/test/libsolidity/SolidityEndToEndTest.cpp b/test/libsolidity/SolidityEndToEndTest.cpp index 2bf20126..f058458d 100644 --- a/test/libsolidity/SolidityEndToEndTest.cpp +++ b/test/libsolidity/SolidityEndToEndTest.cpp @@ -10528,11 +10528,18 @@ BOOST_AUTO_TEST_CASE(non_payable_throw) contract C { uint public a; function f() public returns (uint) { + return msgvalue(); + } + function msgvalue() internal returns (uint) { return msg.value; } function() external { + update(); + } + function update() internal { a = msg.value + 1; } + } )"; compileAndRun(sourceCode, 0, "C"); @@ -10555,6 +10562,9 @@ BOOST_AUTO_TEST_CASE(no_nonpayable_circumvention_by_modifier) if (false) _; // avoid the function, we should still not accept ether } function f() tryCircumvent public returns (uint) { + return msgvalue(); + } + function msgvalue() internal returns (uint) { return msg.value; } } -- cgit From de9f566a7cda48a8a23f91be380e8cd917ecaf34 Mon Sep 17 00:00:00 2001 From: chriseth Date: Mon, 3 Sep 2018 18:37:51 +0200 Subject: Update changelog. --- Changelog.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Changelog.md b/Changelog.md index 30a851c8..c61d1007 100644 --- a/Changelog.md +++ b/Changelog.md @@ -62,6 +62,7 @@ Breaking Changes: * Type Checker: Disallow "loose assembly" syntax entirely. This means that jump labels, jumps and non-functional instructions cannot be used anymore. * Type System: Disallow explicit and implicit conversions from decimal literals to ``bytesXX`` types. * Type System: Disallow explicit and implicit conversions from hex literals to ``bytesXX`` types of different size. + * View Pure Checker: Disallow ``msg.value`` in (or introducing it via a modifier to) a non-payable function. * Remove obsolete ``std`` directory from the Solidity repository. This means accessing ``https://github.com/ethereum/solidity/blob/develop/std/*.sol`` (or ``https://github.com/ethereum/solidity/std/*.sol`` in Remix) will not be possible. * References Resolver: Turn missing storage locations into an error. This was already the case in the experimental 0.5.0 mode. * Syntax Checker: Disallow functions without implementation to use modifiers. This was already the case in the experimental 0.5.0 mode. @@ -87,7 +88,6 @@ Compiler Features: * Tests: Determine transaction status during IPC calls. * Code Generator: Allocate and free local variables according to their scope. * Removed ``pragma experimental "v0.5.0";``. - * View Pure Checker: Warn about ``msg.value`` used by modifier in non-payable function. Bugfixes: * Build System: Support versions of CVC4 linked against CLN instead of GMP. In case of compilation issues due to the experimental SMT solver support, the solvers can be disabled when configuring the project with CMake using ``-DUSE_CVC4=OFF`` or ``-DUSE_Z3=OFF``. -- cgit