From 75a92b0ffd0946c17a27a58e6e02abe96cd3fa00 Mon Sep 17 00:00:00 2001 From: Erik Kundt Date: Thu, 26 Jul 2018 21:45:24 +0200 Subject: Warns if modifier uses msg.value in non-payable function. --- libsolidity/analysis/ViewPureChecker.cpp | 123 ++++++++++++++++++++++--------- 1 file changed, 87 insertions(+), 36 deletions(-) (limited to 'libsolidity/analysis/ViewPureChecker.cpp') 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 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 static const pureMembers{ - "encode", "encodePacked", "encodeWithSelector", "encodeWithSignature", "decode", "data", "sig", "blockhash" + using MagicMember = pair; + set 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 static const payableMembers{ + {MagicType::Kind::Message, "value"} + }; + + auto const& type = dynamic_cast(*_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(_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(_modifier.name()->annotation().referencedDeclaration), ""); -- cgit