Fix keccak bug #11134

hrkrshnn · 2021-03-22T09:51:11Z

Closes #11131

I'll prepare bugs.json

test/libsolidity/semanticTests/functionCall/mapping_array_internal_argument.sol

test/libsolidity/semanticTests/structs/struct_copy.sol

test/libsolidity/semanticTests/array/reusing_memory.sol

Changelog.md

test/libevmasm/Optimiser.cpp

docs/bugs.json

chriseth · 2021-03-22T15:04:14Z

docs/bugs.json

+    {
+        "name": "KeccakCaching",
+        "summary": "The bytecode optimizer incorrectly re-used previously evaluated Keccak-256 hashes. You are unlikely to be affected if you do not compute Keccak-256 hashes in inline assembly.",
+        "description": "Solidity's bytecode optimizer has a step that can compute Keccak-256 hashes, if the contents of the memory are known during compilation time. This step also has a mechanism to determine that two Keccak-256 hashes are equal even if the values in memory are not known during compile time. This mechanism had a bug where, Keccak-256 of the same memory content, but different sizes were considered equal. More specifically, ``keccak256(mpos1, length1)`` and ``keccak256(mpos2, length2)`` in some cases were considered equal if ``length1`` and ``length2``, when rounded up to nearest multiple of 32 were the same, and when the memory contents at ``mpos1`` and ``mpos2`` can be deduced to be equal. You maybe affected if you compute multiple Keccak-256 hashes of the same content, but with different length inside inline assembly.",


Suggested change

"description": "Solidity's bytecode optimizer has a step that can compute Keccak-256 hashes, if the contents of the memory are known during compilation time. This step also has a mechanism to determine that two Keccak-256 hashes are equal even if the values in memory are not known during compile time. This mechanism had a bug where, Keccak-256 of the same memory content, but different sizes were considered equal. More specifically, ``keccak256(mpos1, length1)`` and ``keccak256(mpos2, length2)`` in some cases were considered equal if ``length1`` and ``length2``, when rounded up to nearest multiple of 32 were the same, and when the memory contents at ``mpos1`` and ``mpos2`` can be deduced to be equal. You maybe affected if you compute multiple Keccak-256 hashes of the same content, but with different length inside inline assembly.",

"description": "Solidity's bytecode optimizer has a step that can compute Keccak-256 hashes, if the contents of the memory are known during compilation time. This step also has a mechanism to determine that two Keccak-256 hashes are equal even if the values in memory are not known during compile time. This mechanism had a bug where Keccak-256 of the same memory content, but different sizes were considered equal. More specifically, ``keccak256(mpos1, length1)`` and ``keccak256(mpos2, length2)`` in some cases were considered equal if ``length1`` and ``length2``, when rounded up to nearest multiple of 32 were the same, and when the memory contents at ``mpos1`` and ``mpos2`` can be deduced to be equal. You maybe affected if you compute multiple Keccak-256 hashes of the same content, but with different length inside inline assembly.",

Oh maybe also add some exclusion facts: Code is unaffected if it uses keccak256 with a length that is not a compile-time constant or if it is always a multiple of 32.

Fixed the comma and added the exclusion fact as the last line

chriseth · 2021-03-22T15:08:37Z

Code looks good!

...bsolidity/semanticTests/inlineAssembly/keccak256_optimizer_bug_different_memory_location.sol

bshastry

Minor comment about positive test case

test/libsolidity/semanticTests/inlineAssembly/keccak256_optimizer_cache_bug.sol

...bsolidity/semanticTests/inlineAssembly/keccak256_optimizer_bug_different_memory_location.sol

hrkrshnn · 2021-03-22T16:32:30Z

libevmasm/KnownState.h

@@ -100,10 +100,12 @@ class KnownState
 	void resetStorage() { m_storageContent.clear(); }
 	/// Resets any knowledge about storage.
 	void resetMemory() { m_memoryContent.clear(); }
+	/// Resets known Keccak-256 hashes
+	void resetKnownKeccak256Hashes() { m_knownKeccak256Hashes.clear(); }


Added a new function to clear the m_knownKeccak256Hashes whenever memory is reset. Forcefull reset makes the code 'look' safer, even though I think it makes no difference.

I don't think this is needed: loadFromMemory returns "unknown" after a memory clear and the sequence number is correctly incremented.

I think it's not needed, either. But doesn't it make sense to keep it for consistency with the rest of the resets? I also think it feels safer to reset the keccak hashes when memory is reset.

Changelog.md

libevmasm/KnownState.cpp

bshastry

LGTM

docs/bugs.json

hrkrshnn force-pushed the fix-keccak-bug branch 2 times, most recently from 9508d13 to c3becff Compare March 22, 2021 11:04

hrkrshnn commented Mar 22, 2021

View reviewed changes

test/libsolidity/semanticTests/functionCall/mapping_array_internal_argument.sol Outdated Show resolved Hide resolved

hrkrshnn commented Mar 22, 2021

View reviewed changes

test/libsolidity/semanticTests/structs/struct_copy.sol Outdated Show resolved Hide resolved

hrkrshnn commented Mar 22, 2021

View reviewed changes

test/libsolidity/semanticTests/array/reusing_memory.sol Outdated Show resolved Hide resolved

hrkrshnn requested review from chriseth and bshastry March 22, 2021 12:07

hrkrshnn force-pushed the fix-keccak-bug branch from c3becff to 97593f6 Compare March 22, 2021 12:56