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

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``.

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)

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.

libsolidity/analysis/TypeChecker.cpp

@@ -686,9 +686,7 @@ bool TypeChecker::visit(StructDefinition const& _struct)
 
 bool TypeChecker::visit(FunctionDefinition const& _function)
 {
-	bool isLibraryFunction =
+	bool isLibraryFunction = _function.inContractKind() == ContractDefinition::ContractKind::Library;
-		dynamic_cast<ContractDefinition const*>(_function.scope()) &&
-		dynamic_cast<ContractDefinition const*>(_function.scope())->isLibrary();
 	if (_function.isPayable())
 	{
 		if (isLibraryFunction)

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,17 +219,33 @@ 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\"."
 		);
+		m_errors = true;
+	}
 	else if (_mutability == StateMutability::NonPayable)
+	{
 		m_errorReporter.typeError(
 			_location,
 			"Function declared as " +
@@ -237,18 +253,36 @@ void ViewPureChecker::reportMutability(StateMutability _mutability, SourceLocati
 			", 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::Pure ||
+		m_currentFunction->stateMutability() == StateMutability::NonPayable,
 		""
 	);
-		m_errors = true;
-	}
-	if (_mutability > m_currentBestMutability)
-		m_currentBestMutability = _mutability;
 }
 
 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
+		using MagicMember = pair<MagicType::Kind, string>;
-		set<string> static const pureMembers{
+		set<MagicMember> static const pureMembers{
-			"encode", "encodePacked", "encodeWithSelector", "encodeWithSignature", "decode", "data", "sig", "blockhash"
+			{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), "");

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;
 };
 
 }

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 {}
+}

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".

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.

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".

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?