fix(store): fix potential memory corruption [M-04] (#1978)

latticexyz · Dec 4, 2023 · 5ac4c97 · 5ac4c97
1 parent a4aff73
commit 5ac4c97
Show file tree

Hide file tree

Showing 4 changed files with 57 additions and 9 deletions.
diff --git a/.changeset/small-brooms-prove.md b/.changeset/small-brooms-prove.md
@@ -0,0 +1,6 @@
+---
+"@latticexyz/store": patch
+---
+
+Fixed M-04 Memory Corruption on Load From Storage
+It only affected external use of `Storage.load` with a `memoryPointer` argument
diff --git a/packages/store/gas-report.json b/packages/store/gas-report.json
@@ -285,7 +285,7 @@
     "file": "test/GasStorageLoad.t.sol",
     "test": "testCompareStorageLoadMUD",
     "name": "MUD storage load (cold, 1 word, partial)",
-    "gasUsed": 2460
+    "gasUsed": 2484
   },
   {
     "file": "test/GasStorageLoad.t.sol",
@@ -309,7 +309,7 @@
     "file": "test/GasStorageLoad.t.sol",
     "test": "testCompareStorageLoadMUD",
     "name": "MUD storage load (warm, 1 word, partial)",
-    "gasUsed": 460
+    "gasUsed": 484
   },
   {
     "file": "test/GasStorageLoad.t.sol",
@@ -609,13 +609,13 @@
     "file": "test/Storage.t.sol",
     "test": "testStoreLoad",
     "name": "store 34 bytes over 3 storage slots (with offset and safeTrail))",
-    "gasUsed": 23009
+    "gasUsed": 23013
   },
   {
     "file": "test/Storage.t.sol",
     "test": "testStoreLoad",
     "name": "load 34 bytes over 3 storage slots (with offset and safeTrail))",
-    "gasUsed": 865
+    "gasUsed": 885
   },
   {
     "file": "test/StoreCoreDynamic.t.sol",
@@ -627,19 +627,19 @@
     "file": "test/StoreCoreDynamic.t.sol",
     "test": "testGetDynamicFieldSlice",
     "name": "get field slice (warm, 1 slot)",
-    "gasUsed": 1677
+    "gasUsed": 1711
   },
   {
     "file": "test/StoreCoreDynamic.t.sol",
     "test": "testGetDynamicFieldSlice",
     "name": "get field slice (semi-cold, 1 slot)",
-    "gasUsed": 3680
+    "gasUsed": 3704
   },
   {
     "file": "test/StoreCoreDynamic.t.sol",
     "test": "testGetDynamicFieldSlice",
     "name": "get field slice (warm, 2 slots)",
-    "gasUsed": 3907
+    "gasUsed": 3931
   },
   {
     "file": "test/StoreCoreDynamic.t.sol",
@@ -891,7 +891,7 @@
     "file": "test/StoreCoreGas.t.sol",
     "test": "testSetAndGetField",
     "name": "get static field (overlap 2 slot)",
-    "gasUsed": 1865
+    "gasUsed": 1889
   },
   {
     "file": "test/StoreCoreGas.t.sol",

diff --git a/packages/store/src/Storage.sol b/packages/store/src/Storage.sol
@@ -199,7 +199,12 @@ library Storage {
           wordRemainder = 32 - offset;
         }
 
-        uint256 mask = leftMask(wordRemainder);
+        uint256 mask;
+        if (length < wordRemainder) {
+          mask = leftMask(length);
+        } else {
+          mask = leftMask(wordRemainder);
+        }
         /// @solidity memory-safe-assembly
         assembly {
           // Load data from storage and offset it to match memory

diff --git a/packages/store/test/Storage.t.sol b/packages/store/test/Storage.t.sol
@@ -93,4 +93,41 @@ contract StorageTest is Test, GasReporter {
     Storage.store({ storagePointer: storagePointer, offset: offset, data: abi.encodePacked((data)) });
     assertEq(bytes16(Storage.loadField({ storagePointer: storagePointer, length: 16, offset: offset })), data);
   }
+
+  function testStoreLoadToPointer() public {
+    uint256 memoryCorruptionCheck = 0x0101010101010101010101010101010101010101010101010101010101010101;
+    uint80 prefix = 0x01010101010101010101;
+    uint80 dataBeforeLoad = 0x00000000000000000000;
+    uint80 dataAfterLoad = 0xbeeeeeeeeeeeeeeeeeef;
+    uint256 postfix = 0x010101010101010101010101;
+
+    bytes memory testData = abi.encodePacked(
+      memoryCorruptionCheck,
+      prefix,
+      dataBeforeLoad,
+      postfix,
+      memoryCorruptionCheck
+    );
+    bytes memory expectedData = abi.encodePacked(
+      memoryCorruptionCheck,
+      prefix,
+      dataAfterLoad,
+      postfix,
+      memoryCorruptionCheck
+    );
+    uint256 memoryPointer;
+    /// @solidity memory-safe-assembly
+    assembly {
+      memoryPointer := add(testData, 0x20)
+    }
+    // skip prefixes
+    memoryPointer += 32 + 10;
+
+    uint256 storagePointer = uint256(keccak256("some location"));
+    Storage.store({ storagePointer: storagePointer, offset: 10, data: abi.encodePacked(dataAfterLoad) });
+
+    Storage.load({ storagePointer: storagePointer, length: 10, offset: 10, memoryPointer: memoryPointer });
+
+    assertEq(testData, expectedData);
+  }
 }