Warns if modifier uses msg.value in non-payable function.

11 files changed, 129 insertions, 65 deletions

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<MagicType const*>(_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<FunctionType const*>(_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<ContractDefinition const*>(_function.scope()) &&
-		dynamic_cast<ContractDefinition const*>(_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<SourceLocation> 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<string> static const pureMembers{
-			"encode", "encodePacked", "encodeWithSelector", "encodeWithSignature", "decode", "data", "sig", "blockhash"
+		using MagicMember = pair<MagicType::Kind, string>;
+		set<MagicMember> 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<MagicMember> static const payableMembers{
+			{MagicType::Kind::Message, "value"}
+		};
+
+		auto const& type = dynamic_cast<MagicType const&>(*_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<decltype(mod)>(_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<ContractDefinition const*>(_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<SourceLocation> const& _nestedLocation = {}
+	);
 
 	std::vector<std::shared_ptr<ASTNode>> 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<ModifierDefinition const*, StateMutability> m_inferredMutability;
+	std::map<ModifierDefinition const*, MutabilityAndLocation> 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?