fix(store): revert if slice bound is invalid [L-10] (#2034)

Co-authored-by: Kevin Ingersoll <[email protected]>
latticexyz · Jan 18, 2024 · 7b28d32 · 7b28d32
1 parent ad4ac44
commit 7b28d32
Show file tree

Hide file tree

Showing 7 changed files with 37 additions and 19 deletions.
diff --git a/.changeset/perfect-windows-reply.md b/.changeset/perfect-windows-reply.md
@@ -0,0 +1,5 @@
+---
+"@latticexyz/store": patch
+---
+
+Added a custom error `Store_InvalidBounds` for when the `start:end` slice in `getDynamicFieldSlice` is invalid (it used to revert with the default overflow error)
diff --git a/docs/pages/store/reference/store.mdx b/docs/pages/store/reference/store.mdx
@@ -124,6 +124,12 @@ error Store_InvalidResourceType(bytes2 expected, ResourceId resourceId, string r
 error Store_InvalidStaticDataLength(uint256 expected, uint256 received);
 ```
 
+### Store_InvalidBounds
+
+```solidity
+error Store_InvalidBounds(uint256 start, uint256 end);
+```
+
 ### Store_IndexOutOfBounds
 
 ```solidity

diff --git a/packages/store/gas-report.json b/packages/store/gas-report.json
@@ -573,25 +573,25 @@
     "file": "test/StoreCoreDynamic.t.sol",
     "test": "testGetDynamicFieldSlice",
     "name": "get field slice (cold, 1 slot)",
-    "gasUsed": 5609
+    "gasUsed": 5569
   },
   {
     "file": "test/StoreCoreDynamic.t.sol",
     "test": "testGetDynamicFieldSlice",
     "name": "get field slice (warm, 1 slot)",
-    "gasUsed": 1711
+    "gasUsed": 1671
   },
   {
     "file": "test/StoreCoreDynamic.t.sol",
     "test": "testGetDynamicFieldSlice",
     "name": "get field slice (semi-cold, 1 slot)",
-    "gasUsed": 3704
+    "gasUsed": 3664
   },
   {
     "file": "test/StoreCoreDynamic.t.sol",
     "test": "testGetDynamicFieldSlice",
     "name": "get field slice (warm, 2 slots)",
-    "gasUsed": 3931
+    "gasUsed": 3891
   },
   {
     "file": "test/StoreCoreDynamic.t.sol",
@@ -1023,7 +1023,7 @@
     "file": "test/StoreHooksColdLoad.t.sol",
     "test": "testGetItem",
     "name": "StoreHooks: get 1 element (cold)",
-    "gasUsed": 8486
+    "gasUsed": 8446
   },
   {
     "file": "test/StoreHooksColdLoad.t.sol",

diff --git a/packages/store/src/IStoreErrors.sol b/packages/store/src/IStoreErrors.sol
@@ -10,6 +10,7 @@ interface IStoreErrors {
   error Store_InvalidResourceType(bytes2 expected, ResourceId resourceId, string resourceIdString);
 
   error Store_InvalidStaticDataLength(uint256 expected, uint256 received);
+  error Store_InvalidBounds(uint256 start, uint256 end);
   error Store_IndexOutOfBounds(uint256 length, uint256 accessedIndex);
   error Store_InvalidKeyNamesLength(uint256 expected, uint256 received);
   error Store_InvalidFieldNamesLength(uint256 expected, uint256 received);

diff --git a/packages/store/src/StoreCore.sol b/packages/store/src/StoreCore.sol
@@ -933,6 +933,10 @@ library StoreCore {
     uint256 start,
     uint256 end
   ) internal view returns (bytes memory) {
+    // Verify the slice bounds are valid
+    if (start > end) {
+      revert IStoreErrors.Store_InvalidBounds(start, end);
+    }
     // Verify the accessed data is within the bounds of the dynamic field.
     // This is necessary because we don't delete the dynamic data when a record is deleted,
     // but only decrease its length.
@@ -945,7 +949,9 @@ library StoreCore {
     // Get the length and storage location of the dynamic field
     uint256 location = StoreCoreInternal._getDynamicDataLocation(tableId, keyTuple, dynamicFieldIndex);
 
-    return Storage.load({ storagePointer: location, offset: start, length: end - start });
+    unchecked {
+      return Storage.load({ storagePointer: location, offset: start, length: end - start });
+    }
   }
 }
 

diff --git a/packages/world-modules/gas-report.json b/packages/world-modules/gas-report.json
@@ -111,7 +111,7 @@
     "file": "test/KeysInTableModule.t.sol",
     "test": "testSetAndDeleteRecordHookCompositeGas",
     "name": "delete a composite record on a table with keysInTableModule installed",
-    "gasUsed": 155971
+    "gasUsed": 155851
   },
   {
     "file": "test/KeysInTableModule.t.sol",
@@ -129,7 +129,7 @@
     "file": "test/KeysInTableModule.t.sol",
     "test": "testSetAndDeleteRecordHookGas",
     "name": "delete a record on a table with keysInTableModule installed",
-    "gasUsed": 85089
+    "gasUsed": 85049
   },
   {
     "file": "test/KeysWithValueModule.t.sol",
@@ -201,31 +201,31 @@
     "file": "test/query.t.sol",
     "test": "testCombinedHasHasValueNotQuery",
     "name": "CombinedHasHasValueNotQuery",
-    "gasUsed": 104878
+    "gasUsed": 104518
   },
   {
     "file": "test/query.t.sol",
     "test": "testCombinedHasHasValueQuery",
     "name": "CombinedHasHasValueQuery",
-    "gasUsed": 53348
+    "gasUsed": 53228
   },
   {
     "file": "test/query.t.sol",
     "test": "testCombinedHasNotQuery",
     "name": "CombinedHasNotQuery",
-    "gasUsed": 131374
+    "gasUsed": 130734
   },
   {
     "file": "test/query.t.sol",
     "test": "testCombinedHasQuery",
     "name": "CombinedHasQuery",
-    "gasUsed": 84497
+    "gasUsed": 84137
   },
   {
     "file": "test/query.t.sol",
     "test": "testCombinedHasValueNotQuery",
     "name": "CombinedHasValueNotQuery",
-    "gasUsed": 85039
+    "gasUsed": 84679
   },
   {
     "file": "test/query.t.sol",
@@ -237,19 +237,19 @@
     "file": "test/query.t.sol",
     "test": "testHasQuery",
     "name": "HasQuery",
-    "gasUsed": 18882
+    "gasUsed": 18802
   },
   {
     "file": "test/query.t.sol",
     "test": "testHasQuery1000Keys",
     "name": "HasQuery with 1000 keys",
-    "gasUsed": 5804585
+    "gasUsed": 5764585
   },
   {
     "file": "test/query.t.sol",
     "test": "testHasQuery100Keys",
     "name": "HasQuery with 100 keys",
-    "gasUsed": 541538
+    "gasUsed": 537538
   },
   {
     "file": "test/query.t.sol",
@@ -261,7 +261,7 @@
     "file": "test/query.t.sol",
     "test": "testNotValueQuery",
     "name": "NotValueQuery",
-    "gasUsed": 46944
+    "gasUsed": 46824
   },
   {
     "file": "test/StandardDelegationsModule.t.sol",

diff --git a/packages/world/gas-report.json b/packages/world/gas-report.json
@@ -57,13 +57,13 @@
     "file": "test/Factories.t.sol",
     "test": "testCreate2Factory",
     "name": "deploy contract via Create2",
-    "gasUsed": 4663033
+    "gasUsed": 4676449
   },
   {
     "file": "test/Factories.t.sol",
     "test": "testWorldFactory",
     "name": "deploy world via WorldFactory",
-    "gasUsed": 12501056
+    "gasUsed": 12514303
   },
   {
     "file": "test/World.t.sol",