From 05139500fb44c1ff574756b3a5d10d46674c236d Mon Sep 17 00:00:00 2001 From: Federico Bond Date: Thu, 1 Dec 2016 10:55:02 -0300 Subject: Warn about using msg.value in non-payable function --- libsolidity/analysis/StaticAnalyzer.cpp | 78 +++++++++++++++++++ libsolidity/analysis/StaticAnalyzer.h | 72 +++++++++++++++++ libsolidity/ast/Types.h | 2 + libsolidity/interface/CompilerStack.cpp | 10 +++ test/libsolidity/SolidityNameAndTypeResolution.cpp | 90 +++++++++++++++++++++- 5 files changed, 251 insertions(+), 1 deletion(-) create mode 100644 libsolidity/analysis/StaticAnalyzer.cpp create mode 100644 libsolidity/analysis/StaticAnalyzer.h diff --git a/libsolidity/analysis/StaticAnalyzer.cpp b/libsolidity/analysis/StaticAnalyzer.cpp new file mode 100644 index 00000000..c39f874e --- /dev/null +++ b/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 . +*/ +/** + * @author Federico Bond + * @date 2016 + * Static analyzer and checker. + */ + +#include +#include +#include + +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(_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::Type::Warning); + *err << + errinfo_sourceLocation(_location) << + errinfo_comment(_description); + + m_errors.push_back(err); +} diff --git a/libsolidity/analysis/StaticAnalyzer.h b/libsolidity/analysis/StaticAnalyzer.h new file mode 100644 index 00000000..b6cf783e --- /dev/null +++ b/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 . +*/ +/** + * @author Federico Bond + * @date 2016 + * Static analyzer and checker. + */ + +#pragma once + +#include +#include +#include +#include +#include + +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; +}; + +} +} diff --git a/libsolidity/ast/Types.h b/libsolidity/ast/Types.h index 72640a1c..26e2b8f2 100644 --- a/libsolidity/ast/Types.h +++ b/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; }; diff --git a/libsolidity/interface/CompilerStack.cpp b/libsolidity/interface/CompilerStack.cpp index b4fd6d87..eb588fc2 100644 --- a/libsolidity/interface/CompilerStack.cpp +++ b/libsolidity/interface/CompilerStack.cpp @@ -32,6 +32,7 @@ #include #include #include +#include #include #include #include @@ -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; } diff --git a/test/libsolidity/SolidityNameAndTypeResolution.cpp b/test/libsolidity/SolidityNameAndTypeResolution.cpp index 11c38114..a4e601f7 100644 --- a/test/libsolidity/SolidityNameAndTypeResolution.cpp +++ b/test/libsolidity/SolidityNameAndTypeResolution.cpp @@ -26,6 +26,7 @@ #include #include #include +#include #include #include #include @@ -89,8 +90,12 @@ 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"); for (auto const& currentError: errors) @@ -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() } -- cgit