Merge pull request #6248 from ethereum/shiftopt-fix-overflow

Fixes u256 overflow in logical shift optimization rule and adds tests.

Changelog.md

@@ -2,6 +2,7 @@
 
 Important Bugfixes:
  * Yul Optimizer: Fix visitation order bug for the structural simplifier.
+ * Optimizer: Fix overflow in optimization rule that simplifies double shift by constant.
 
 Language Features:
  * Allow calldata arrays with dynamically encoded base types with ABIEncoderV2.

docs/bugs.json

@@ -1,4 +1,16 @@
 [
+    {
+        "name": "DoubleShiftSizeOverflow",
+        "summary": "Double bitwise shifts by large constants whose sum overflows 256 bits can result in unexpected values.",
+        "description": "Nested logical shift operations whose total shift size is 2**256 or more are incorrectly optimized. This only applies to shifts by numbers of bits that are compile-time constant expressions.",
+        "introduced": "0.5.5",
+        "fixed": "0.5.6",
+        "severity": "low",
+        "conditions": {
+            "optimizer": true,
+            "evmVersion": ">=constantinople"
+        }
+    },
     {
         "name": "ExpExponentCleanup",
         "summary": "Using the ** operator with an exponent of type shorter than 256 bits can result in unexpected values.",

docs/bugs.rst

@@ -52,9 +52,15 @@ severity
     discoverability in contract tests, likelihood of occurrence and
     potential damage by exploits.
 conditions
-    Conditions that have to be met to trigger the bug. Currently, this
+    Conditions that have to be met to trigger the bug. The following
-    is an object that can contain a boolean value ``optimizer``, which
+    keys can be used:
+    ``optimizer``, Boolean value which
     means that the optimizer has to be switched on to enable the bug.
+    ``evmVersion``, a string that indicates which EVM version compiler
+    settings trigger the bug. The string can contain comparison
+    operators. For example, ``">=constantinople"`` means that the bug
+    is present when the EVM version is set to ``constantinople`` or
+    later.
     If no conditions are given, assume that the bug is present.
 check
     This field contains different checks that report whether the smart contract

docs/bugs_by_version.json

@@ -630,7 +630,9 @@
         "released": "2019-02-12"
     },
     "0.5.5": {
-        "bugs": [],
+        "bugs": [
+            "DoubleShiftSizeOverflow"
+        ],
         "released": "2019-03-05"
     }
 }

libevmasm/RuleList.h

@@ -349,14 +349,26 @@ std::vector<SimplificationRule<Pattern>> simplificationRuleListPart7(
 	rules.push_back({
 		// SHL(B, SHL(A, X)) -> SHL(min(A+B, 256), X)
 		{Instruction::SHL, {{B}, {Instruction::SHL, {{A}, {X}}}}},
-		[=]() -> Pattern { return {Instruction::SHL, {std::min(A.d() + B.d(), u256(256)), X}}; },
+		[=]() -> Pattern {
+			bigint sum = bigint(A.d()) + B.d();
+			if (sum >= 256)
+				return {Instruction::AND, {X, u256(0)}};
+			else
+				return {Instruction::SHL, {u256(sum), X}};
+		},
 		false
 	});
 
 	rules.push_back({
 		// SHR(B, SHR(A, X)) -> SHR(min(A+B, 256), X)
 		{Instruction::SHR, {{B}, {Instruction::SHR, {{A}, {X}}}}},
-		[=]() -> Pattern { return {Instruction::SHR, {std::min(A.d() + B.d(), u256(256)), X}}; },
+		[=]() -> Pattern {
+			bigint sum = bigint(A.d()) + B.d();
+			if (sum >= 256)
+				return {Instruction::AND, {X, u256(0)}};
+			else
+				return {Instruction::SHR, {u256(sum), X}};
+		},
 		false
 	});
 

test/libevmasm/Optimiser.cpp

@@ -238,6 +238,36 @@ BOOST_AUTO_TEST_CASE(cse_associativity2)
 	checkCSE(input, {Instruction::DUP2, Instruction::DUP2, Instruction::ADD, u256(5), Instruction::ADD});
 }
 
+BOOST_AUTO_TEST_CASE(cse_double_shift_right_overflow)
+{
+	if (dev::test::Options::get().evmVersion().hasBitwiseShifting())
+	{
+		AssemblyItems input{
+			Instruction::CALLVALUE,
+			u256(2),
+			Instruction::SHR,
+			u256(-1),
+			Instruction::SHR
+		};
+		checkCSE(input, {u256(0)});
+	}
+}
+
+BOOST_AUTO_TEST_CASE(cse_double_shift_left_overflow)
+{
+	if (dev::test::Options::get().evmVersion().hasBitwiseShifting())
+	{
+		AssemblyItems input{
+			Instruction::DUP1,
+			u256(2),
+			Instruction::SHL,
+			u256(-1),
+			Instruction::SHL
+		};
+		checkCSE(input, {u256(0)});
+	}
+}
+
 BOOST_AUTO_TEST_CASE(cse_storage)
 {
 	AssemblyItems input{

test/libsolidity/SolidityEndToEndTest.cpp

@@ -15009,11 +15009,21 @@ BOOST_AUTO_TEST_CASE(bitwise_shifting_constantinople_combined)
 					c := shl(0xd0, shl(0x40, a))
 				}
 			}
+			function shl_combined_overflow(uint a) public returns (uint c) {
+				assembly {
+					c := shl(0x01, shl(not(0x00), a))
+				}
+			}
 			function shr_combined_large(uint a) public returns (uint c) {
 				assembly {
 					c := shr(0xd0, shr(0x40, a))
 				}
 			}
+			function shr_combined_overflow(uint a) public returns (uint c) {
+				assembly {
+					c := shr(0x01, shr(not(0x00), a))
+				}
+			}
 			function sar_combined_large(uint a) public returns (uint c) {
 				assembly {
 					c := sar(0xd0, sar(0x40, a))
@@ -15052,8 +15062,10 @@ BOOST_AUTO_TEST_CASE(bitwise_shifting_constantinople_combined)
 	BOOST_CHECK(callContractFunction("shl_combined_large(uint256)", u256(0)) == encodeArgs(u256(0)));
 	BOOST_CHECK(callContractFunction("shl_combined_large(uint256)", u256("0xffff")) == encodeArgs(u256(0)));
 	BOOST_CHECK(callContractFunction("shl_combined_large(uint256)", u256("0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff")) == encodeArgs(u256(0)));
+	BOOST_CHECK(callContractFunction("shl_combined_overflow(uint256)", u256(2)) == encodeArgs(u256(0)));
 	BOOST_CHECK(callContractFunction("shr_combined_large(uint256)", u256(0)) == encodeArgs(u256(0)));
 	BOOST_CHECK(callContractFunction("shr_combined_large(uint256)", u256("0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff")) == encodeArgs(u256(0)));
+	BOOST_CHECK(callContractFunction("shr_combined_overflow(uint256)", u256(2)) == encodeArgs(u256(0)));
 	BOOST_CHECK(callContractFunction("sar_combined_large(uint256)", u256(0)) == encodeArgs(u256(0)));
 	BOOST_CHECK(callContractFunction("sar_combined_large(uint256)", u256("0x7fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff")) == encodeArgs(u256(0)));
 	BOOST_CHECK(callContractFunction("sar_combined_large(uint256)", u256("0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff")) == encodeArgs(u256("0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff")));