Merge pull request #10239 from ethereum/failOnInvalidEncoding

Fail on invalid storage encoding for byte arrays.

Changelog.md

@@ -3,6 +3,7 @@
 Breaking Changes:
  * Assembler: The artificial ASSIGNIMMUTABLE opcode and the corresponding builtin in the "EVM with object access" dialect of Yul take the base offset of the code to modify as additional argument.
  * Code Generator: All arithmetic is checked by default. These checks can be disabled using ``unchecked { ... }``.
+ * Code Generator: Cause a panic if a byte array in storage is accessed whose length is encoded incorrectly.
  * General: Remove global functions ``log0``, ``log1``, ``log2``, ``log3`` and ``log4``.
  * Type Checker: Function call options can only be given once.
  * Type System: Unary negation can only be used on signed integers, not on unsigned integers.

docs/080-breaking-changes.rst

@@ -32,6 +32,9 @@ the compiler notifying you about it.
   This will save gas on errors while it still allows static analysis tools to distinguish
   these situations from a revert on invalid input, like a failing ``require``.
 
+* If a byte array in storage is accessed whose length is encoded incorrectly, a panic is caused.
+  A contract cannot get into this situation unless inline assembly is used to modify the raw representation of storage byte arrays.
+
 New Restrictions
 ================
 

docs/control-structures.rst

@@ -598,6 +598,7 @@ The error code supplied with the error data indicates the kind of panic.
 #. 0x11: If an arithmetic operation results in underflow or overflow outside of an ``unchecked { ... }`` block.
 #. 0x12; If you divide or modulo by zero (e.g. ``5 / 0`` or ``23 % 0``).
 #. 0x21: If you convert a value that is too big or negative into an enum type.
+#. 0x22: If you access a storage byte array that is incorrectly encoded.
 #. 0x31: If you call ``.pop()`` on an empty array.
 #. 0x32: If you access an array, ``bytesN`` or an array slice at an out-of-bounds or negative index (i.e. ``x[i]`` where ``i >= x.length`` or ``i < 0``).
 #. 0x41: If you allocate too much memory or create an array that is too large.

libsolidity/codegen/ABIFunctions.cpp

@@ -683,18 +683,16 @@ string ABIFunctions::abiEncodingFunctionCompactStorageArray(
 				// <readableTypeNameFrom> -> <readableTypeNameTo>
 				function <functionName>(value, pos) -> ret {
 					let slotValue := sload(value)
+					let length := <byteArrayLengthFunction>(slotValue)
+					pos := <storeLength>(pos, length)
 					switch and(slotValue, 1)
 					case 0 {
 						// short byte array
-						let length := and(div(slotValue, 2), 0x7f)
-						pos := <storeLength>(pos, length)
 						mstore(pos, and(slotValue, not(0xff)))
 						ret := add(pos, <lengthPaddedShort>)
 					}
 					case 1 {
 						// long byte array
-						let length := div(slotValue, 2)
-						pos := <storeLength>(pos, length)
 						let dataPos := <arrayDataSlot>(value)
 						let i := 0
 						for { } lt(i, length) { i := add(i, 0x20) } {
@@ -708,6 +706,7 @@ string ABIFunctions::abiEncodingFunctionCompactStorageArray(
 			templ("functionName", functionName);
 			templ("readableTypeNameFrom", _from.toString(true));
 			templ("readableTypeNameTo", _to.toString(true));
+			templ("byteArrayLengthFunction", m_utils.extractByteArrayLengthFunction());
 			templ("storeLength", arrayStoreLengthForEncodingFunction(_to, _options));
 			templ("lengthPaddedShort", _options.padded ? "0x20" : "length");
 			templ("lengthPaddedLong", _options.padded ? "i" : "length");

libsolidity/codegen/ArrayUtils.cpp

@@ -818,30 +818,25 @@ void ArrayUtils::incrementDynamicArraySize(ArrayType const& _type) const
 		// lowest-order byte (we actually use a mask with fewer bits) must
 		// be (31*2+0) = 62
 
+		m_context << Instruction::DUP1 << Instruction::SLOAD << Instruction::DUP1;
+		m_context.callYulFunction(m_context.utilFunctions().extractByteArrayLengthFunction(), 1, 1);
 		m_context.appendInlineAssembly(R"({
-			let data := sload(ref)
-			let shifted_length := and(data, 63)
 			// We have to copy if length is exactly 31, because that marks
 			// the transition between in-place and out-of-place storage.
-			switch shifted_length
+			switch length
-			case 62
+			case 31
 			{
 				mstore(0, ref)
 				let data_area := keccak256(0, 0x20)
 				sstore(data_area, and(data, not(0xff)))
-				// New length is 32, encoded as (32 * 2 + 1)
+				// Set old length in new format (31 * 2 + 1)
-				sstore(ref, 65)
+				data := 63
-				// Replace ref variable by new length
-				ref := 32
 			}
-			default
-			{
 			sstore(ref, add(data, 2))
-				// Replace ref variable by new length
+			// return new length in ref
-				if iszero(and(data, 1)) { data := shifted_length }
+			ref := add(length, 1)
-				ref := add(div(data, 2), 1)
+		})", {"ref", "data", "length"});
-			}
+		m_context << Instruction::POP << Instruction::POP;
-		})", {"ref"});
 	}
 	else
 		m_context.appendInlineAssembly(R"({
@@ -860,28 +855,25 @@ void ArrayUtils::popStorageArrayElement(ArrayType const& _type) const
 
 	if (_type.isByteArray())
 	{
+		m_context << Instruction::DUP1 << Instruction::SLOAD << Instruction::DUP1;
+		m_context.callYulFunction(m_context.utilFunctions().extractByteArrayLengthFunction(), 1, 1);
 		util::Whiskers code(R"({
-			let slot_value := sload(ref)
-			switch and(slot_value, 1)
-			case 0 {
-				// short byte array
-				let length := and(div(slot_value, 2), 0x1f)
 			if iszero(length) {
 				mstore(0, <panicSelector>)
 				mstore(4, <emptyArrayPop>)
 				revert(0, 0x24)
 			}
-
+			switch gt(length, 31)
+			case 0 {
+				// short byte array
 				// Zero-out the suffix including the least significant byte.
 				let mask := sub(exp(0x100, sub(33, length)), 1)
 				length := sub(length, 1)
 				slot_value := or(and(not(mask), slot_value), mul(length, 2))
-				sstore(ref, slot_value)
 			}
 			case 1 {
 				// long byte array
 				mstore(0, ref)
-				let length := div(slot_value, 2)
 				let slot := keccak256(0, 0x20)
 				switch length
 				case 32
@@ -889,7 +881,7 @@ void ArrayUtils::popStorageArrayElement(ArrayType const& _type) const
 					let data := sload(slot)
 					sstore(slot, 0)
 					data := and(data, not(0xff))
-					sstore(ref, or(data, 62))
+					slot_value := or(data, 62)
 				}
 				default
 				{
@@ -905,14 +897,14 @@ void ArrayUtils::popStorageArrayElement(ArrayType const& _type) const
 
 					// Reduce the length by 1
 					slot_value := sub(slot_value, 2)
+				}
+			}
 			sstore(ref, slot_value)
-				}
-			}
 		})");
 		code("panicSelector", util::selectorFromSignature("Panic(uint256)").str());
 		code("emptyArrayPop", to_string(unsigned(util::PanicCode::EmptyArrayPop)));
-		m_context.appendInlineAssembly(code.render(), {"ref"});
+		m_context.appendInlineAssembly(code.render(), {"ref", "slot_value", "length"});
-		m_context << Instruction::POP;
+		m_context << Instruction::POP << Instruction::POP << Instruction::POP;
 	}
 	else
 	{
@@ -1040,16 +1032,7 @@ void ArrayUtils::retrieveLength(ArrayType const& _arrayType, unsigned _stackDept
 		case DataLocation::Storage:
 			m_context << Instruction::SLOAD;
 			if (_arrayType.isByteArray())
-			{
+				m_context.callYulFunction(m_context.utilFunctions().extractByteArrayLengthFunction(), 1, 1);
-				// Retrieve length both for in-place strings and off-place strings:
-				// Computes (x & (0x100 * (ISZERO (x & 1)) - 1)) / 2
-				// i.e. for short strings (x & 1 == 0) it does (x & 0xff) / 2 and for long strings it
-				// computes (x & (-1)) / 2, which is equivalent to just x / 2.
-				m_context << u256(1) << Instruction::DUP2 << u256(1) << Instruction::AND;
-				m_context << Instruction::ISZERO << u256(0x100) << Instruction::MUL;
-				m_context << Instruction::SUB << Instruction::AND;
-				m_context << u256(2) << Instruction::SWAP1 << Instruction::DIV;
-			}
 			break;
 		}
 	}

libsolidity/codegen/YulUtilFunctions.cpp

@@ -1003,25 +1003,6 @@ string YulUtilFunctions::wrappingIntExpFunction(
 	});
 }
 
-string YulUtilFunctions::extractByteArrayLengthFunction()
-{
-	string functionName = "extract_byte_array_length";
-	return m_functionCollector.createFunction(functionName, [&]() {
-		Whiskers w(R"(
-			function <functionName>(data) -> length {
-				// Retrieve length both for in-place strings and off-place strings:
-				// Computes (x & (0x100 * (ISZERO (x & 1)) - 1)) / 2
-				// i.e. for short strings (x & 1 == 0) it does (x & 0xff) / 2 and for long strings it
-				// computes (x & (-1)) / 2, which is equivalent to just x / 2.
-				let mask := sub(mul(0x100, iszero(and(data, 1))), 1)
-				length := div(and(data, mask), 2)
-			}
-		)");
-		w("functionName", functionName);
-		return w.render();
-	});
-}
-
 string YulUtilFunctions::arrayLengthFunction(ArrayType const& _type)
 {
 	string functionName = "array_length_" + _type.identifier();
@@ -1064,6 +1045,29 @@ string YulUtilFunctions::arrayLengthFunction(ArrayType const& _type)
 	});
 }
 
+string YulUtilFunctions::extractByteArrayLengthFunction()
+{
+	string functionName = "extract_byte_array_length";
+	return m_functionCollector.createFunction(functionName, [&]() {
+		Whiskers w(R"(
+			function <functionName>(data) -> length {
+				length := div(data, 2)
+				let outOfPlaceEncoding := and(data, 1)
+				if iszero(outOfPlaceEncoding) {
+					length := and(length, 0x7f)
+				}
+
+				if eq(outOfPlaceEncoding, lt(length, 32)) {
+					<panic>()
+				}
+			}
+		)");
+		w("functionName", functionName);
+		w("panic", panicFunction(PanicCode::StorageEncodingError));
+		return w.render();
+	});
+}
+
 std::string YulUtilFunctions::resizeDynamicArrayFunction(ArrayType const& _type)
 {
 	solAssert(_type.location() == DataLocation::Storage, "");

libsolidity/codegen/YulUtilFunctions.h

@@ -178,6 +178,12 @@ public:
 	/// signature: (array) -> length
 	std::string arrayLengthFunction(ArrayType const& _type);
 
+	/// @returns function name that extracts and returns byte array length from the value
+	/// stored at the slot.
+	/// Causes a Panic if the length encoding is wrong.
+	/// signature: (data) -> length
+	std::string extractByteArrayLengthFunction();
+
 	/// @returns the name of a function that resizes a storage array
 	/// signature: (array, newLen)
 	std::string resizeDynamicArrayFunction(ArrayType const& _type);
@@ -447,10 +453,6 @@ private:
 	/// use exactly one variable to hold the value.
 	std::string conversionFunctionSpecial(Type const& _from, Type const& _to);
 
-	/// @returns function name that extracts and returns byte array length
-	/// signature: (data) -> length
-	std::string extractByteArrayLengthFunction();
-
 	/// @returns the name of a function that reduces the size of a storage byte array by one element
 	/// signature: (byteArray)
 	std::string storageByteArrayPopFunction(ArrayType const& _type);

libsolutil/ErrorCodes.h

@@ -29,6 +29,7 @@ enum class PanicCode
 	UnderOverflow = 0x11, // arithmetic underflow or overflow
 	DivisionByZero = 0x12, // division or modulo by zero
 	EnumConversionError = 0x21, // enum conversion error
+	StorageEncodingError = 0x22, // invalid encoding in storage
 	EmptyArrayPop = 0x31, // empty array pop
 	ArrayOutOfBounds = 0x32, // array out of bounds access
 	ResourceError = 0x41, // resource error (too large allocation or too large array)

test/libsolidity/semanticTests/array/invalid_encoding_for_storage_byte_array.sol

@@ -0,0 +1,92 @@
+contract C {
+    bytes public x = "abc";
+    bytes public y;
+    function invalidateXShort() public {
+        assembly { sstore(x.slot, 64) }
+        delete y;
+    }
+    function invalidateXLong() public {
+        assembly { sstore(x.slot, 5) }
+        delete y;
+    }
+    function abiEncode() public view returns (bytes memory) { return x; }
+    function abiEncodePacked() public view returns (bytes memory) { return abi.encodePacked(x); }
+    function copyToMemory() public view returns (bytes memory m) { m = x; }
+    function indexAccess() public view returns (byte) { return x[0]; }
+    function assignTo() public { x = "def"; }
+    function assignToLong() public { x = "1234567890123456789012345678901234567"; }
+    function copyToStorage() public { y = x; }
+    function copyFromStorageShort() public { y = "abc"; x = y; }
+    function copyFromStorageLong() public { y = "1234567890123456789012345678901234567"; x = y; }
+    function arrayPop() public { x.pop(); }
+    function arrayPush() public { x.push("t"); }
+    function arrayPushEmpty() public { x.push(); }
+    function del() public { delete x; }
+}
+// ----
+// x() -> 0x20, 3, 0x6162630000000000000000000000000000000000000000000000000000000000
+// abiEncode() -> 0x20, 3, 0x6162630000000000000000000000000000000000000000000000000000000000
+// abiEncodePacked() -> 0x20, 3, 0x6162630000000000000000000000000000000000000000000000000000000000
+// copyToMemory() -> 0x20, 3, 0x6162630000000000000000000000000000000000000000000000000000000000
+// indexAccess() -> 0x6100000000000000000000000000000000000000000000000000000000000000
+// arrayPushEmpty()
+// arrayPush()
+// x() -> 0x20, 5, 0x6162630074000000000000000000000000000000000000000000000000000000
+// arrayPop()
+// assignToLong()
+// x() -> 0x20, 0x25, 0x3132333435363738393031323334353637383930313233343536373839303132, 0x3334353637000000000000000000000000000000000000000000000000000000
+// assignTo()
+// x() -> 0x20, 3, 0x6465660000000000000000000000000000000000000000000000000000000000
+// copyFromStorageShort()
+// x() -> 0x20, 3, 0x6162630000000000000000000000000000000000000000000000000000000000
+// copyFromStorageLong()
+// x() -> 0x20, 0x25, 0x3132333435363738393031323334353637383930313233343536373839303132, 0x3334353637000000000000000000000000000000000000000000000000000000
+// copyToStorage()
+// x() -> 0x20, 0x25, 0x3132333435363738393031323334353637383930313233343536373839303132, 0x3334353637000000000000000000000000000000000000000000000000000000
+// y() -> 0x20, 0x25, 0x3132333435363738393031323334353637383930313233343536373839303132, 0x3334353637000000000000000000000000000000000000000000000000000000
+// del()
+// x() -> 0x20, 0x00
+// invalidateXLong()
+// x() -> FAILURE, hex"4e487b71", 0x22
+// abiEncode() -> FAILURE, hex"4e487b71", 0x22
+// abiEncodePacked() -> FAILURE, hex"4e487b71", 0x22
+// copyToMemory() -> FAILURE, hex"4e487b71", 0x22
+// indexAccess() -> FAILURE, hex"4e487b71", 0x22
+// arrayPushEmpty() -> FAILURE, hex"4e487b71", 0x22
+// arrayPush() -> FAILURE, hex"4e487b71", 0x22
+// x() -> FAILURE, hex"4e487b71", 0x22
+// arrayPop() -> FAILURE, hex"4e487b71", 0x22
+// assignToLong() -> FAILURE, hex"4e487b71", 0x22
+// x() -> FAILURE, hex"4e487b71", 0x22
+// assignTo() -> FAILURE, hex"4e487b71", 0x22
+// x() -> FAILURE, hex"4e487b71", 0x22
+// copyFromStorageShort() -> FAILURE, hex"4e487b71", 0x22
+// x() -> FAILURE, hex"4e487b71", 0x22
+// copyFromStorageLong() -> FAILURE, hex"4e487b71", 0x22
+// x() -> FAILURE, hex"4e487b71", 0x22
+// copyToStorage() -> FAILURE, hex"4e487b71", 0x22
+// x() -> FAILURE, hex"4e487b71", 0x22
+// y() -> 0x20, 0x00
+// del() -> FAILURE, hex"4e487b71", 0x22
+// x() -> FAILURE, hex"4e487b71", 0x22
+// invalidateXShort()
+// x() -> FAILURE, hex"4e487b71", 0x22
+// abiEncode() -> FAILURE, hex"4e487b71", 0x22
+// abiEncodePacked() -> FAILURE, hex"4e487b71", 0x22
+// copyToMemory() -> FAILURE, hex"4e487b71", 0x22
+// indexAccess() -> FAILURE, hex"4e487b71", 0x22
+// arrayPushEmpty() -> FAILURE, hex"4e487b71", 0x22
+// arrayPush() -> FAILURE, hex"4e487b71", 0x22
+// x() -> FAILURE, hex"4e487b71", 0x22
+// arrayPop() -> FAILURE, hex"4e487b71", 0x22
+// assignToLong() -> FAILURE, hex"4e487b71", 0x22
+// x() -> FAILURE, hex"4e487b71", 0x22
+// assignTo() -> FAILURE, hex"4e487b71", 0x22
+// x() -> FAILURE, hex"4e487b71", 0x22
+// copyFromStorageShort() -> FAILURE, hex"4e487b71", 0x22
+// x() -> FAILURE, hex"4e487b71", 0x22
+// copyFromStorageLong() -> FAILURE, hex"4e487b71", 0x22
+// x() -> FAILURE, hex"4e487b71", 0x22
+// copyToStorage() -> FAILURE, hex"4e487b71", 0x22
+// x() -> FAILURE, hex"4e487b71", 0x22
+// y() -> 0x20, 0x00