From 8a6692b2cfb7cf53db6731acd6a9908bd36b5475 Mon Sep 17 00:00:00 2001 From: wadeAlexC Date: Thu, 5 Oct 2017 09:28:25 -0400 Subject: Improves address literal checksum error message --- Changelog.md | 1 + libdevcore/CommonData.cpp | 25 ++++++++++++++----------- libdevcore/CommonData.h | 4 ++++ libsolidity/analysis/TypeChecker.cpp | 8 +++++--- libsolidity/ast/AST.cpp | 6 ++++++ libsolidity/ast/AST.h | 2 ++ 6 files changed, 32 insertions(+), 14 deletions(-) diff --git a/Changelog.md b/Changelog.md index 45521f3e..2487b87c 100644 --- a/Changelog.md +++ b/Changelog.md @@ -2,6 +2,7 @@ Features: * Syntax Checker: Turn the usage of ``callcode`` into an error as experimental 0.5.0 feature. + * Type Checker: Improve address checksum warning. * Type Checker: More detailed errors for invalid array lengths (such as division by zero). Bugfixes: diff --git a/libdevcore/CommonData.cpp b/libdevcore/CommonData.cpp index db11e61c..85ad685b 100644 --- a/libdevcore/CommonData.cpp +++ b/libdevcore/CommonData.cpp @@ -86,20 +86,23 @@ bool dev::passesAddressChecksum(string const& _str, bool _strict) )) return true; + return _str == dev::getChecksummedAddress(_str); +} + +string dev::getChecksummedAddress(string const& _addr) +{ + string s = _addr.substr(0, 2) == "0x" ? _addr.substr(2) : _addr; h256 hash = keccak256(boost::algorithm::to_lower_copy(s, std::locale::classic())); - for (size_t i = 0; i < 40; ++i) + string ret = "0x"; + + for (size_t i = 0; i < s.length(); ++i) { char addressCharacter = s[i]; - bool lowerCase; - if ('a' <= addressCharacter && addressCharacter <= 'f') - lowerCase = true; - else if ('A' <= addressCharacter && addressCharacter <= 'F') - lowerCase = false; - else - continue; unsigned nibble = (unsigned(hash[i / 2]) >> (4 * (1 - (i % 2)))) & 0xf; - if ((nibble >= 8) == lowerCase) - return false; + if (nibble >= 8) + ret += toupper(addressCharacter); + else + ret += tolower(addressCharacter); } - return true; + return ret; } diff --git a/libdevcore/CommonData.h b/libdevcore/CommonData.h index 765707f8..e76a0949 100644 --- a/libdevcore/CommonData.h +++ b/libdevcore/CommonData.h @@ -209,4 +209,8 @@ bool contains(T const& _t, V const& _v) /// are considered valid. bool passesAddressChecksum(std::string const& _str, bool _strict); +/// @returns the checksummed version of an address +/// @param hex strings that look like an address +std::string getChecksummedAddress(std::string const& _addr); + } diff --git a/libsolidity/analysis/TypeChecker.cpp b/libsolidity/analysis/TypeChecker.cpp index 746e762e..fee60797 100644 --- a/libsolidity/analysis/TypeChecker.cpp +++ b/libsolidity/analysis/TypeChecker.cpp @@ -1122,7 +1122,7 @@ bool TypeChecker::visit(VariableDeclarationStatement const& _statement) var.annotation().type->toString() + ". Try converting to type " + valueComponentType->mobileType()->toString() + - " or use an explicit conversion." + " or use an explicit conversion." ); else m_errorReporter.typeError( @@ -1320,7 +1320,7 @@ bool TypeChecker::visit(TupleExpression const& _tuple) _tuple.annotation().isPure = isPure; if (_tuple.isInlineArray()) { - if (!inlineArrayType) + if (!inlineArrayType) m_errorReporter.fatalTypeError(_tuple.location(), "Unable to deduce common type for array elements."); _tuple.annotation().type = make_shared(DataLocation::Memory, inlineArrayType, types.size()); } @@ -2000,7 +2000,9 @@ void TypeChecker::endVisit(Literal const& _literal) m_errorReporter.warning( _literal.location(), "This looks like an address but has an invalid checksum. " - "If this is not used as an address, please prepend '00'." + "If this is not used as an address, please prepend '00'. " + "Correct checksummed address: '" + _literal.getChecksummedAddress() + "'. " + "For more information please see https://solidity.readthedocs.io/en/develop/types.html#address-literals" ); } if (!_literal.annotation().type) diff --git a/libsolidity/ast/AST.cpp b/libsolidity/ast/AST.cpp index 1048b610..4911f161 100644 --- a/libsolidity/ast/AST.cpp +++ b/libsolidity/ast/AST.cpp @@ -583,3 +583,9 @@ bool Literal::passesAddressChecksum() const solAssert(isHexNumber(), "Expected hex number"); return dev::passesAddressChecksum(value(), true); } + +std::string Literal::getChecksummedAddress() const +{ + solAssert(isHexNumber(), "Expected hex number"); + return dev::getChecksummedAddress(value()); +} diff --git a/libsolidity/ast/AST.h b/libsolidity/ast/AST.h index 733e7c78..5d6763ca 100644 --- a/libsolidity/ast/AST.h +++ b/libsolidity/ast/AST.h @@ -1613,6 +1613,8 @@ public: bool looksLikeAddress() const; /// @returns true if it passes the address checksum test. bool passesAddressChecksum() const; + /// @returns the checksummed version of an address + std::string getChecksummedAddress() const; private: Token::Value m_token; -- cgit From 6ebc094474837d922ac00a92c54c903c5eb78585 Mon Sep 17 00:00:00 2001 From: Alex Beregszaszi Date: Fri, 20 Oct 2017 15:23:28 +0100 Subject: Ensure that non-hex characters are caught in address checksumming --- libdevcore/CommonData.cpp | 8 ++++++-- libdevcore/Exceptions.h | 1 + 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/libdevcore/CommonData.cpp b/libdevcore/CommonData.cpp index 85ad685b..445d11cd 100644 --- a/libdevcore/CommonData.cpp +++ b/libdevcore/CommonData.cpp @@ -21,6 +21,7 @@ #include #include +#include #include #include @@ -92,10 +93,13 @@ bool dev::passesAddressChecksum(string const& _str, bool _strict) string dev::getChecksummedAddress(string const& _addr) { string s = _addr.substr(0, 2) == "0x" ? _addr.substr(2) : _addr; + assertThrow(s.length() == 40, InvalidAddress, ""); + assertThrow(s.find_first_not_of("0123456789abcdefABCDEF") == string::npos, InvalidAddress, ""); + h256 hash = keccak256(boost::algorithm::to_lower_copy(s, std::locale::classic())); - string ret = "0x"; - for (size_t i = 0; i < s.length(); ++i) + string ret = "0x"; + for (size_t i = 0; i < 40; ++i) { char addressCharacter = s[i]; unsigned nibble = (unsigned(hash[i / 2]) >> (4 * (1 - (i % 2)))) & 0xf; diff --git a/libdevcore/Exceptions.h b/libdevcore/Exceptions.h index a3e638bf..cfe72fbf 100644 --- a/libdevcore/Exceptions.h +++ b/libdevcore/Exceptions.h @@ -44,6 +44,7 @@ private: #define DEV_SIMPLE_EXCEPTION(X) struct X: virtual Exception { const char* what() const noexcept override { return #X; } } +DEV_SIMPLE_EXCEPTION(InvalidAddress); DEV_SIMPLE_EXCEPTION(BadHexCharacter); DEV_SIMPLE_EXCEPTION(FileError); -- cgit From 1d5dd909b4ed8625330e9ec859e02dfd067f4006 Mon Sep 17 00:00:00 2001 From: Alex Beregszaszi Date: Tue, 24 Oct 2017 10:54:51 +0100 Subject: Do not try to display checksummed address for too-short/long address literals --- libsolidity/analysis/TypeChecker.cpp | 4 ++-- libsolidity/ast/AST.cpp | 2 ++ libsolidity/ast/AST.h | 2 +- 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/libsolidity/analysis/TypeChecker.cpp b/libsolidity/analysis/TypeChecker.cpp index fee60797..73047e76 100644 --- a/libsolidity/analysis/TypeChecker.cpp +++ b/libsolidity/analysis/TypeChecker.cpp @@ -2000,8 +2000,8 @@ void TypeChecker::endVisit(Literal const& _literal) m_errorReporter.warning( _literal.location(), "This looks like an address but has an invalid checksum. " - "If this is not used as an address, please prepend '00'. " - "Correct checksummed address: '" + _literal.getChecksummedAddress() + "'. " + "If this is not used as an address, please prepend '00'. " + + (!_literal.getChecksummedAddress().empty() ? "Correct checksummed address: '" + _literal.getChecksummedAddress() + "'. " : "") + "For more information please see https://solidity.readthedocs.io/en/develop/types.html#address-literals" ); } diff --git a/libsolidity/ast/AST.cpp b/libsolidity/ast/AST.cpp index 4911f161..62507093 100644 --- a/libsolidity/ast/AST.cpp +++ b/libsolidity/ast/AST.cpp @@ -587,5 +587,7 @@ bool Literal::passesAddressChecksum() const std::string Literal::getChecksummedAddress() const { solAssert(isHexNumber(), "Expected hex number"); + if (value().length != 42) + return string(); return dev::getChecksummedAddress(value()); } diff --git a/libsolidity/ast/AST.h b/libsolidity/ast/AST.h index 5d6763ca..feffde64 100644 --- a/libsolidity/ast/AST.h +++ b/libsolidity/ast/AST.h @@ -1613,7 +1613,7 @@ public: bool looksLikeAddress() const; /// @returns true if it passes the address checksum test. bool passesAddressChecksum() const; - /// @returns the checksummed version of an address + /// @returns the checksummed version of an address (or empty string if not valid) std::string getChecksummedAddress() const; private: -- cgit From 8d26894841d7b5897b7c6f126f6ea1b8293ab5a2 Mon Sep 17 00:00:00 2001 From: Alex Beregszaszi Date: Tue, 24 Oct 2017 11:01:56 +0100 Subject: Show checksummed address always (prepend with 0) --- libsolidity/ast/AST.cpp | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/libsolidity/ast/AST.cpp b/libsolidity/ast/AST.cpp index 62507093..8da6964e 100644 --- a/libsolidity/ast/AST.cpp +++ b/libsolidity/ast/AST.cpp @@ -587,7 +587,10 @@ bool Literal::passesAddressChecksum() const std::string Literal::getChecksummedAddress() const { solAssert(isHexNumber(), "Expected hex number"); - if (value().length != 42) + /// Pad literal to be a proper hex address. + string address = value().substr(2); + if (address.length() > 40) return string(); - return dev::getChecksummedAddress(value()); + address.insert(address.begin(), 40 - address.size(), '0'); + return dev::getChecksummedAddress(address); } -- cgit From f7249abe284b5f29996621f2e19cab7f09f51785 Mon Sep 17 00:00:00 2001 From: Alex Beregszaszi Date: Tue, 24 Oct 2017 11:55:30 +0100 Subject: Extend address checksum tests --- test/libsolidity/SolidityNameAndTypeResolution.cpp | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/test/libsolidity/SolidityNameAndTypeResolution.cpp b/test/libsolidity/SolidityNameAndTypeResolution.cpp index 30624260..88ec58ee 100644 --- a/test/libsolidity/SolidityNameAndTypeResolution.cpp +++ b/test/libsolidity/SolidityNameAndTypeResolution.cpp @@ -5766,7 +5766,7 @@ BOOST_AUTO_TEST_CASE(invalid_address_checksum) } } )"; - CHECK_WARNING(text, "checksum"); + CHECK_WARNING(text, "This looks like an address but has an invalid checksum."); } BOOST_AUTO_TEST_CASE(invalid_address_no_checksum) @@ -5779,10 +5779,10 @@ BOOST_AUTO_TEST_CASE(invalid_address_no_checksum) } } )"; - CHECK_WARNING(text, "checksum"); + CHECK_WARNING(text, "This looks like an address but has an invalid checksum."); } -BOOST_AUTO_TEST_CASE(invalid_address_length) +BOOST_AUTO_TEST_CASE(invalid_address_length_short) { char const* text = R"( contract C { @@ -5792,7 +5792,20 @@ BOOST_AUTO_TEST_CASE(invalid_address_length) } } )"; - CHECK_WARNING(text, "checksum"); + CHECK_WARNING(text, "This looks like an address but has an invalid checksum."); +} + +BOOST_AUTO_TEST_CASE(invalid_address_length_long) +{ + char const* text = R"( + contract C { + function f() pure public { + address x = 0xFA0bFc97E48458494Ccd857e1A85DC91F7F0046E0; + x; + } + } + )"; + CHECK_WARNING_ALLOW_MULTI(text, "This looks like an address but has an invalid checksum."); } BOOST_AUTO_TEST_CASE(address_test_for_bug_in_implementation) -- cgit From 53796c0fe8bfd6ff2707bd12dd3141169489f763 Mon Sep 17 00:00:00 2001 From: Alex Beregszaszi Date: Fri, 17 Nov 2017 00:40:50 +0000 Subject: Add tests for getChecksummedAddress --- test/libdevcore/Checksum.cpp | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/test/libdevcore/Checksum.cpp b/test/libdevcore/Checksum.cpp index 17a17d22..4eedbd99 100644 --- a/test/libdevcore/Checksum.cpp +++ b/test/libdevcore/Checksum.cpp @@ -19,6 +19,8 @@ */ #include +#include + #include "../TestHelper.h" @@ -31,6 +33,38 @@ namespace test BOOST_AUTO_TEST_SUITE(Checksum) +BOOST_AUTO_TEST_CASE(calculate) +{ + BOOST_CHECK(!getChecksummedAddress("0x5aaeb6053f3e94c9b9a09f33669435e7ef1beaed").empty()); + BOOST_CHECK(!getChecksummedAddress("0x0123456789abcdefABCDEF0123456789abcdefAB").empty()); + // too short + BOOST_CHECK_THROW(getChecksummedAddress("0x5aaeb6053f3e94c9b9a09f33669435e7ef1beae"), InvalidAddress); + BOOST_CHECK_THROW(getChecksummedAddress("5aaeb6053f3e94c9b9a09f33669435e7ef1beae"), InvalidAddress); + // too long + BOOST_CHECK_THROW(getChecksummedAddress("0x5aaeb6053f3e94c9b9a09f33669435e7ef1beaed1"), InvalidAddress); + BOOST_CHECK_THROW(getChecksummedAddress("5aaeb6053f3e94c9b9a09f33669435e7ef1beaed1"), InvalidAddress); + // non-hex character + BOOST_CHECK_THROW(getChecksummedAddress("0x5aaeb6053f3e94c9b9a09f33669435e7ef1beaeK"), InvalidAddress); + + // the official test suite from EIP-55 + vector cases { + // all upper case + "0x52908400098527886E0F7030069857D2E4169EE7", + "0x8617E340B3D01FA5F11F306F4090FD50E238070D", + // all lower case + "0xde709f2102306220921060314715629080e2fb77", + "0x27b1fdb04752bbc536007a920d24acb045561c26", + // regular + "0x5aAeb6053F3E94C9b9A09f33669435E7Ef1BeAed", + "0xfB6916095ca1df60bB79Ce92cE3Ea74c37c5d359", + "0xdbF03B407c01E7cD3CBea99509d93f8DDDC8C6FB", + "0xD1220A0cf47c7B9Be7A2E6BA89F429762e7b9aDb" + }; + + for (size_t i = 0; i < cases.size(); i++) + BOOST_REQUIRE_MESSAGE(getChecksummedAddress(cases[i]) == cases[i], cases[i]); +} + BOOST_AUTO_TEST_CASE(regular) { BOOST_CHECK(passesAddressChecksum("0x5aAeb6053F3E94C9b9A09f33669435E7Ef1BeAed", true)); -- cgit