Merge pull request #1464 from federicobond/warn-msgvalue

Warn about using msg.value in non-payable function

libsolidity/analysis/StaticAnalyzer.cpp

@@ -0,0 +1,78 @@
+/*
+	This file is part of solidity.
+
+	solidity is free software: you can redistribute it and/or modify
+	it under the terms of the GNU General Public License as published by
+	the Free Software Foundation, either version 3 of the License, or
+	(at your option) any later version.
+
+	solidity is distributed in the hope that it will be useful,
+	but WITHOUT ANY WARRANTY; without even the implied warranty of
+	MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+	GNU General Public License for more details.
+
+	You should have received a copy of the GNU General Public License
+	along with solidity.  If not, see <http://www.gnu.org/licenses/>.
+*/
+/**
+ * @author Federico Bond <federicobond@gmail.com>
+ * @date 2016
+ * Static analyzer and checker.
+ */
+
+#include <libsolidity/analysis/StaticAnalyzer.h>
+#include <memory>
+#include <libsolidity/ast/AST.h>
+
+using namespace std;
+using namespace dev;
+using namespace dev::solidity;
+
+
+bool StaticAnalyzer::analyze(SourceUnit const& _sourceUnit)
+{
+	_sourceUnit.accept(*this);
+	return Error::containsOnlyWarnings(m_errors);
+}
+
+bool StaticAnalyzer::visit(ContractDefinition const& _contract)
+{
+	m_library = _contract.isLibrary();
+	return true;
+}
+
+void StaticAnalyzer::endVisit(ContractDefinition const&)
+{
+	m_library = false;
+}
+
+bool StaticAnalyzer::visit(FunctionDefinition const& _function)
+{
+	m_nonPayablePublic = _function.isPublic() && !_function.isPayable();
+	return true;
+}
+
+void StaticAnalyzer::endVisit(FunctionDefinition const&)
+{
+	m_nonPayablePublic = false;
+}
+
+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")
+				warning(_memberAccess.location(), "\"msg.value\" used in non-payable function. Do you want to add the \"payable\" modifier to this function?");
+
+	return true;
+}
+
+void StaticAnalyzer::warning(SourceLocation const& _location, string const& _description)
+{
+	auto err = make_shared<Error>(Error::Type::Warning);
+	*err <<
+		errinfo_sourceLocation(_location) <<
+		errinfo_comment(_description);
+
+	m_errors.push_back(err);
+}

libsolidity/analysis/StaticAnalyzer.h

@@ -0,0 +1,72 @@
+/*
+	This file is part of solidity.
+
+	solidity is free software: you can redistribute it and/or modify
+	it under the terms of the GNU General Public License as published by
+	the Free Software Foundation, either version 3 of the License, or
+	(at your option) any later version.
+
+	solidity is distributed in the hope that it will be useful,
+	but WITHOUT ANY WARRANTY; without even the implied warranty of
+	MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+	GNU General Public License for more details.
+
+	You should have received a copy of the GNU General Public License
+	along with solidity.  If not, see <http://www.gnu.org/licenses/>.
+*/
+/**
+ * @author Federico Bond <federicobond@gmail.com>
+ * @date 2016
+ * Static analyzer and checker.
+ */
+
+#pragma once
+
+#include <libsolidity/analysis/TypeChecker.h>
+#include <libsolidity/ast/Types.h>
+#include <libsolidity/ast/ASTAnnotations.h>
+#include <libsolidity/ast/ASTForward.h>
+#include <libsolidity/ast/ASTVisitor.h>
+
+namespace dev
+{
+namespace solidity
+{
+
+
+/**
+ * The module that performs static analysis on the AST.
+ */
+class StaticAnalyzer: private ASTConstVisitor
+{
+public:
+	/// @param _errors the reference to the list of errors and warnings to add them found during static analysis.
+	explicit StaticAnalyzer(ErrorList& _errors): m_errors(_errors) {}
+
+	/// Performs static analysis on the given source unit and all of its sub-nodes.
+	/// @returns true iff all checks passed. Note even if all checks passed, errors() can still contain warnings
+	bool analyze(SourceUnit const& _sourceUnit);
+
+private:
+	/// Adds a new warning to the list of errors.
+	void warning(SourceLocation const& _location, std::string const& _description);
+
+	virtual bool visit(ContractDefinition const& _contract) override;
+	virtual void endVisit(ContractDefinition const& _contract) override;
+
+	virtual bool visit(FunctionDefinition const& _function) override;
+	virtual void endVisit(FunctionDefinition const& _function) override;
+
+	virtual bool visit(MemberAccess const& _memberAccess) override;
+
+	ErrorList& m_errors;
+
+	/// 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;
+};
+
+}
+}

libsolidity/ast/Types.h

@@ -1117,6 +1117,8 @@ public:
 
 	virtual std::string toString(bool _short) const override;
 
+	Kind kind() const { return m_kind; }
+
 private:
 	Kind m_kind;
 };

libsolidity/interface/CompilerStack.cpp

@@ -32,6 +32,7 @@
 #include <libsolidity/analysis/NameAndTypeResolver.h>
 #include <libsolidity/analysis/TypeChecker.h>
 #include <libsolidity/analysis/DocStringAnalyser.h>
+#include <libsolidity/analysis/StaticAnalyzer.h>
 #include <libsolidity/analysis/SyntaxChecker.h>
 #include <libsolidity/codegen/Compiler.h>
 #include <libsolidity/interface/InterfaceHandler.h>
@@ -202,6 +203,15 @@ bool CompilerStack::parse()
 
 				m_contracts[contract->name()].contract = contract;
 			}
+
+	if (noErrors)
+	{
+		StaticAnalyzer staticAnalyzer(m_errors);
+		for (Source const* source: m_sourceOrder)
+			if (!staticAnalyzer.analyze(*source->ast))
+				noErrors = false;
+	}
+
 	m_parseSuccessful = noErrors;
 	return m_parseSuccessful;
 }

test/libsolidity/SolidityNameAndTypeResolution.cpp

@@ -26,6 +26,7 @@
 #include <libsolidity/parsing/Scanner.h>
 #include <libsolidity/parsing/Parser.h>
 #include <libsolidity/analysis/NameAndTypeResolver.h>
+#include <libsolidity/analysis/StaticAnalyzer.h>
 #include <libsolidity/analysis/SyntaxChecker.h>
 #include <libsolidity/interface/Exceptions.h>
 #include <libsolidity/analysis/GlobalContext.h>
@@ -89,7 +90,11 @@ parseAnalyseAndReturnError(string const& _source, bool _reportWarnings = false,
 					TypeChecker typeChecker(errors);
 					bool success = typeChecker.checkTypeRequirements(*contract);
 					BOOST_CHECK(success || !errors.empty());
-
+				}
+		if (success)
+		{
+			StaticAnalyzer staticAnalyzer(errors);
+			staticAnalyzer.analyze(*sourceUnit);
 		}
 		if (errors.size() > 1 && !_allowMultipleErrors)
 			BOOST_FAIL("Multiple errors found");
@@ -189,6 +194,14 @@ CHECK_ERROR_OR_WARNING(text, Warning, substring, true, false)
 // [checkSuccess(text)] asserts that the compilation down to typechecking succeeds.
 #define CHECK_SUCCESS(text) do { BOOST_CHECK(success((text))); } while(0)
 
+#define CHECK_SUCCESS_NO_WARNINGS(text) \
+do \
+{ \
+	auto sourceAndError = parseAnalyseAndReturnError((text), true); \
+	BOOST_CHECK(sourceAndError.second == nullptr); \
+} \
+while(0)
+
 
 BOOST_AUTO_TEST_SUITE(SolidityNameAndTypeResolution)
 
@@ -4777,6 +4790,81 @@ BOOST_AUTO_TEST_CASE(invalid_mobile_type)
 	CHECK_ERROR(text, TypeError, "");
 }
 
+BOOST_AUTO_TEST_CASE(warns_msg_value_in_non_payable_public_function)
+{
+	char const* text = R"(
+		contract C {
+			function f() {
+				msg.value;
+			}
+		}
+	)";
+	CHECK_WARNING(text, "\"msg.value\" used in non-payable function. Do you want to add the \"payable\" modifier to this function?");
+}
+
+BOOST_AUTO_TEST_CASE(does_not_warn_msg_value_in_payable_function)
+{
+	char const* text = R"(
+		contract C {
+			function f() payable {
+				msg.value;
+			}
+		}
+	)";
+	CHECK_SUCCESS_NO_WARNINGS(text);
+}
+
+BOOST_AUTO_TEST_CASE(does_not_warn_msg_value_in_internal_function)
+{
+	char const* text = R"(
+		contract C {
+			function f() internal {
+				msg.value;
+			}
+		}
+	)";
+	CHECK_SUCCESS_NO_WARNINGS(text);
+}
+
+BOOST_AUTO_TEST_CASE(does_not_warn_msg_value_in_library)
+{
+	char const* text = R"(
+		library C {
+			function f() {
+				msg.value;
+			}
+		}
+	)";
+	CHECK_SUCCESS_NO_WARNINGS(text);
+}
+
+BOOST_AUTO_TEST_CASE(does_not_warn_non_magic_msg_value)
+{
+	char const* text = R"(
+		contract C {
+			struct msg {
+				uint256 value;
+			}
+
+			function f() {
+				msg.value;
+			}
+		}
+	)";
+	CHECK_SUCCESS_NO_WARNINGS(text);
+}
+
+BOOST_AUTO_TEST_CASE(does_not_warn_msg_value_in_modifier_following_non_payable_public_function)
+{
+	char const* text = R"(
+		contract c {
+			function f() { }
+			modifier m() { msg.value; _; }
+		}
+	)";
+	CHECK_SUCCESS_NO_WARNINGS(text);
+}
+
 BOOST_AUTO_TEST_SUITE_END()
 
 }