Merge pull request #4590 from ethereum/msgValueModifier

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

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.

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

@@ -684,9 +684,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)

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,72 @@ 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 == 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, "");
+	if (_mutability > m_bestMutabilityAndLocation.mutability)
+		m_bestMutabilityAndLocation = MutabilityAndLocation{_mutability, _location};
+	if (!m_currentFunction || _mutability <= m_currentFunction->stateMutability())
+		return;
 
-		solAssert(
-			m_currentFunction->stateMutability() == StateMutability::View ||
-			m_currentFunction->stateMutability() == StateMutability::Pure,
-			""
+	// 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
+	))
+	{
+		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.typeError(
+					_location,
+					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.typeError(
+					_location,
+					"\"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
+		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 +329,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 +390,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/SolidityEndToEndTest.cpp

@@ -10537,11 +10537,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");
@@ -10564,6 +10571,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;
 			}
 		}

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 {}
+}
+// ----
+// TypeError: (101-115): This modifier uses "msg.value" and thus the function has to be payable or internal.

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.

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