fix(store): add missing FieldLayout and Schema validations [L-07] (#2046

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

Hide file tree

Showing 11 changed files with 94 additions and 31 deletions.
diff --git a/.changeset/shaggy-pianos-fetch.md b/.changeset/shaggy-pianos-fetch.md
@@ -0,0 +1,5 @@
+---
+"@latticexyz/store": patch
+---
+
+Added more validation checks for `FieldLayout` and `Schema`.
diff --git a/docs/pages/store/reference/store.mdx b/docs/pages/store/reference/store.mdx
@@ -118,10 +118,10 @@ error Store_TableNotFound(ResourceId tableId, string tableIdString);
 error Store_InvalidResourceType(bytes2 expected, ResourceId resourceId, string resourceIdString);
 ```
 
-### Store_InvalidDynamicDataLength
+### Store_InvalidStaticDataLength
 
 ```solidity
-error Store_InvalidDynamicDataLength(uint256 expected, uint256 received);
+error Store_InvalidStaticDataLength(uint256 expected, uint256 received);
 ```
 
 ### Store_IndexOutOfBounds
@@ -148,6 +148,18 @@ error Store_InvalidFieldNamesLength(uint256 expected, uint256 received);
 error Store_InvalidValueSchemaLength(uint256 expected, uint256 received);
 ```
 
+### Store_InvalidValueSchemaStaticLength
+
+```solidity
+error Store_InvalidValueSchemaStaticLength(uint256 expected, uint256 received);
+```
+
+### Store_InvalidValueSchemaDynamicLength
+
+```solidity
+error Store_InvalidValueSchemaDynamicLength(uint256 expected, uint256 received);
+```
+
 ### Store_InvalidSplice
 
 ```solidity

diff --git a/packages/store/gas-report.json b/packages/store/gas-report.json
@@ -87,7 +87,7 @@
     "file": "test/FieldLayout.t.sol",
     "test": "testValidate",
     "name": "validate field layout",
-    "gasUsed": 3954
+    "gasUsed": 6925
   },
   {
     "file": "test/Gas.t.sol",
@@ -321,7 +321,7 @@
     "file": "test/KeyEncoding.t.sol",
     "test": "testRegisterAndGetFieldLayout",
     "name": "register KeyEncoding table",
-    "gasUsed": 719023
+    "gasUsed": 727536
   },
   {
     "file": "test/Mixed.t.sol",
@@ -477,7 +477,7 @@
     "file": "test/Schema.t.sol",
     "test": "testValidate",
     "name": "validate schema",
-    "gasUsed": 11497
+    "gasUsed": 13777
   },
   {
     "file": "test/Slice.t.sol",
@@ -765,7 +765,7 @@
     "file": "test/StoreCoreGas.t.sol",
     "test": "testRegisterAndGetFieldLayout",
     "name": "StoreCore: register table",
-    "gasUsed": 640905
+    "gasUsed": 651200
   },
   {
     "file": "test/StoreCoreGas.t.sol",
@@ -1107,7 +1107,7 @@
     "file": "test/Vector2.t.sol",
     "test": "testRegisterAndGetFieldLayout",
     "name": "register Vector2 field layout",
-    "gasUsed": 442408
+    "gasUsed": 451196
   },
   {
     "file": "test/Vector2.t.sol",

diff --git a/packages/store/src/FieldLayout.sol b/packages/store/src/FieldLayout.sol
@@ -28,8 +28,10 @@ library FieldLayoutLib {
   error FieldLayoutLib_TooManyFields(uint256 numFields, uint256 maxFields);
   error FieldLayoutLib_TooManyDynamicFields(uint256 numFields, uint256 maxFields);
   error FieldLayoutLib_Empty();
-  error FieldLayoutLib_StaticLengthIsZero();
-  error FieldLayoutLib_StaticLengthDoesNotFitInAWord();
+  error FieldLayoutLib_InvalidStaticDataLength(uint256 staticDataLength, uint256 computedStaticDataLength);
+  error FieldLayoutLib_StaticLengthIsZero(uint256 index);
+  error FieldLayoutLib_StaticLengthIsNotZero(uint256 index);
+  error FieldLayoutLib_StaticLengthDoesNotFitInAWord(uint256 index);
 
   /**
    * @notice Encodes the given field layout into a single bytes32.
@@ -51,9 +53,9 @@ library FieldLayoutLib {
     for (uint256 i = 0; i < _staticFieldLengths.length; ) {
       uint256 staticByteLength = _staticFieldLengths[i];
       if (staticByteLength == 0) {
-        revert FieldLayoutLib_StaticLengthIsZero();
+        revert FieldLayoutLib_StaticLengthIsZero(i);
       } else if (staticByteLength > WORD_SIZE) {
-        revert FieldLayoutLib_StaticLengthDoesNotFitInAWord();
+        revert FieldLayoutLib_StaticLengthDoesNotFitInAWord(i);
       }
 
       unchecked {
@@ -166,17 +168,30 @@ library FieldLayoutInstance {
     }
 
     // Static lengths must be valid
+    uint256 _staticDataLength;
     for (uint256 i; i < _numStaticFields; ) {
       uint256 staticByteLength = fieldLayout.atIndex(i);
       if (staticByteLength == 0) {
-        revert FieldLayoutLib.FieldLayoutLib_StaticLengthIsZero();
+        revert FieldLayoutLib.FieldLayoutLib_StaticLengthIsZero(i);
       } else if (staticByteLength > WORD_SIZE) {
-        revert FieldLayoutLib.FieldLayoutLib_StaticLengthDoesNotFitInAWord();
+        revert FieldLayoutLib.FieldLayoutLib_StaticLengthDoesNotFitInAWord(i);
       }
+      _staticDataLength += staticByteLength;
       unchecked {
         i++;
       }
     }
+    // Static length sums must match
+    if (_staticDataLength != fieldLayout.staticDataLength()) {
+      revert FieldLayoutLib.FieldLayoutLib_InvalidStaticDataLength(fieldLayout.staticDataLength(), _staticDataLength);
+    }
+    // Unused fields must be zero
+    for (uint256 i = _numStaticFields; i < MAX_TOTAL_FIELDS; i++) {
+      uint256 staticByteLength = fieldLayout.atIndex(i);
+      if (staticByteLength != 0) {
+        revert FieldLayoutLib.FieldLayoutLib_StaticLengthIsNotZero(i);
+      }
+    }
   }
 
   /**

diff --git a/packages/store/src/IStoreErrors.sol b/packages/store/src/IStoreErrors.sol
@@ -9,10 +9,12 @@ interface IStoreErrors {
   error Store_TableNotFound(ResourceId tableId, string tableIdString);
   error Store_InvalidResourceType(bytes2 expected, ResourceId resourceId, string resourceIdString);
 
-  error Store_InvalidDynamicDataLength(uint256 expected, uint256 received);
+  error Store_InvalidStaticDataLength(uint256 expected, uint256 received);
   error Store_IndexOutOfBounds(uint256 length, uint256 accessedIndex);
   error Store_InvalidKeyNamesLength(uint256 expected, uint256 received);
   error Store_InvalidFieldNamesLength(uint256 expected, uint256 received);
   error Store_InvalidValueSchemaLength(uint256 expected, uint256 received);
+  error Store_InvalidValueSchemaStaticLength(uint256 expected, uint256 received);
+  error Store_InvalidValueSchemaDynamicLength(uint256 expected, uint256 received);
   error Store_InvalidSplice(uint40 startWithinField, uint40 deleteCount, uint40 fieldLength);
 }
diff --git a/packages/store/src/Schema.sol b/packages/store/src/Schema.sol
@@ -171,8 +171,11 @@ library SchemaInstance {
     // No static field can be after a dynamic field
     uint256 countStaticFields;
     uint256 countDynamicFields;
+    uint256 _staticDataLength;
     for (uint256 i; i < _numTotalFields; ) {
-      if (schema.atIndex(i).getStaticByteLength() > 0) {
+      uint256 staticByteLength = schema.atIndex(i).getStaticByteLength();
+      if (staticByteLength > 0) {
+        _staticDataLength += staticByteLength;
         // Static field in dynamic part
         if (i >= _numStaticFields) revert SchemaLib.SchemaLib_StaticTypeAfterDynamicType();
         unchecked {
@@ -189,6 +192,10 @@ library SchemaInstance {
         i++;
       }
     }
+    // Static length sums must match
+    if (_staticDataLength != schema.staticDataLength()) {
+      revert SchemaLib.SchemaLib_InvalidLength(schema.staticDataLength());
+    }
 
     // Number of static fields must match
     if (countStaticFields != _numStaticFields) revert SchemaLib.SchemaLib_InvalidLength(countStaticFields);

diff --git a/packages/store/src/StoreCore.sol b/packages/store/src/StoreCore.sol
@@ -205,6 +205,28 @@ library StoreCore {
     if (valueSchema.numFields() != fieldLayout.numFields()) {
       revert IStoreErrors.Store_InvalidValueSchemaLength(fieldLayout.numFields(), valueSchema.numFields());
     }
+    if (valueSchema.numStaticFields() != fieldLayout.numStaticFields()) {
+      revert IStoreErrors.Store_InvalidValueSchemaStaticLength(
+        fieldLayout.numStaticFields(),
+        valueSchema.numStaticFields()
+      );
+    }
+    if (valueSchema.numDynamicFields() != fieldLayout.numDynamicFields()) {
+      revert IStoreErrors.Store_InvalidValueSchemaDynamicLength(
+        fieldLayout.numDynamicFields(),
+        valueSchema.numDynamicFields()
+      );
+    }
+
+    // Verify that static field lengths are consistent between Schema and FieldLayout
+    for (uint256 i; i < fieldLayout.numStaticFields(); i++) {
+      if (fieldLayout.atIndex(i) != valueSchema.atIndex(i).getStaticByteLength()) {
+        revert IStoreErrors.Store_InvalidStaticDataLength(
+          fieldLayout.atIndex(i),
+          valueSchema.atIndex(i).getStaticByteLength()
+        );
+      }
+    }
 
     // Verify there is no resource with this ID yet
     if (ResourceIds._getExists(tableId)) {

diff --git a/packages/store/test/FieldLayout.t.sol b/packages/store/test/FieldLayout.t.sol
@@ -36,12 +36,12 @@ contract FieldLayoutTest is Test, GasReporter {
   }
 
   function testInvalidFieldLayoutStaticTypeIsZero() public {
-    vm.expectRevert(FieldLayoutLib.FieldLayoutLib_StaticLengthIsZero.selector);
+    vm.expectRevert(abi.encodeWithSelector(FieldLayoutLib.FieldLayoutLib_StaticLengthIsZero.selector, 1));
     FieldLayoutEncodeHelper.encode(1, 0, 1);
   }
 
   function testInvalidFieldLayoutStaticTypeDoesNotFitInAWord() public {
-    vm.expectRevert(FieldLayoutLib.FieldLayoutLib_StaticLengthDoesNotFitInAWord.selector);
+    vm.expectRevert(abi.encodeWithSelector(FieldLayoutLib.FieldLayoutLib_StaticLengthDoesNotFitInAWord.selector, 1));
     FieldLayoutEncodeHelper.encode(1, 33, 1);
   }
 

diff --git a/packages/world-modules/gas-report.json b/packages/world-modules/gas-report.json
@@ -75,13 +75,13 @@
     "file": "test/KeysInTableModule.t.sol",
     "test": "testInstallComposite",
     "name": "install keys in table module",
-    "gasUsed": 1419104
+    "gasUsed": 1435380
   },
   {
     "file": "test/KeysInTableModule.t.sol",
     "test": "testInstallGas",
     "name": "install keys in table module",
-    "gasUsed": 1419104
+    "gasUsed": 1435380
   },
   {
     "file": "test/KeysInTableModule.t.sol",
@@ -93,13 +93,13 @@
     "file": "test/KeysInTableModule.t.sol",
     "test": "testInstallSingleton",
     "name": "install keys in table module",
-    "gasUsed": 1419104
+    "gasUsed": 1435380
   },
   {
     "file": "test/KeysInTableModule.t.sol",
     "test": "testSetAndDeleteRecordHookCompositeGas",
     "name": "install keys in table module",
-    "gasUsed": 1419104
+    "gasUsed": 1435380
   },
   {
     "file": "test/KeysInTableModule.t.sol",
@@ -117,7 +117,7 @@
     "file": "test/KeysInTableModule.t.sol",
     "test": "testSetAndDeleteRecordHookGas",
     "name": "install keys in table module",
-    "gasUsed": 1419104
+    "gasUsed": 1435380
   },
   {
     "file": "test/KeysInTableModule.t.sol",
@@ -135,7 +135,7 @@
     "file": "test/KeysWithValueModule.t.sol",
     "test": "testGetKeysWithValueGas",
     "name": "install keys with value module",
-    "gasUsed": 674321
+    "gasUsed": 681695
   },
   {
     "file": "test/KeysWithValueModule.t.sol",
@@ -153,7 +153,7 @@
     "file": "test/KeysWithValueModule.t.sol",
     "test": "testInstall",
     "name": "install keys with value module",
-    "gasUsed": 674321
+    "gasUsed": 681695
   },
   {
     "file": "test/KeysWithValueModule.t.sol",
@@ -165,7 +165,7 @@
     "file": "test/KeysWithValueModule.t.sol",
     "test": "testSetAndDeleteRecordHook",
     "name": "install keys with value module",
-    "gasUsed": 674321
+    "gasUsed": 681695
   },
   {
     "file": "test/KeysWithValueModule.t.sol",
@@ -183,7 +183,7 @@
     "file": "test/KeysWithValueModule.t.sol",
     "test": "testSetField",
     "name": "install keys with value module",
-    "gasUsed": 674321
+    "gasUsed": 681695
   },
   {
     "file": "test/KeysWithValueModule.t.sol",
@@ -303,7 +303,7 @@
     "file": "test/UniqueEntityModule.t.sol",
     "test": "testInstall",
     "name": "install unique entity module",
-    "gasUsed": 694733
+    "gasUsed": 702723
   },
   {
     "file": "test/UniqueEntityModule.t.sol",
@@ -315,7 +315,7 @@
     "file": "test/UniqueEntityModule.t.sol",
     "test": "testInstallRoot",
     "name": "installRoot unique entity module",
-    "gasUsed": 663752
+    "gasUsed": 671743
   },
   {
     "file": "test/UniqueEntityModule.t.sol",

diff --git a/packages/world/gas-report.json b/packages/world/gas-report.json
@@ -63,7 +63,7 @@
     "file": "test/Factories.t.sol",
     "test": "testWorldFactory",
     "name": "deploy world via WorldFactory",
-    "gasUsed": 12386832
+    "gasUsed": 12501056
   },
   {
     "file": "test/World.t.sol",
@@ -129,7 +129,7 @@
     "file": "test/World.t.sol",
     "test": "testRegisterTable",
     "name": "Register a new table in the namespace",
-    "gasUsed": 528266
+    "gasUsed": 537060
   },
   {
     "file": "test/World.t.sol",

diff --git a/packages/world/mud.config.ts b/packages/world/mud.config.ts
@@ -106,7 +106,7 @@ export default mudConfig({
   },
   excludeSystems: [
     // Worldgen currently does not support systems inheriting logic
-    // from other contracts, so all parts of CoreSystem are named
+    // from other contracts, so all parts of CoreRegistrationSystem are named
     // System too to be included in the IBaseWorld interface.
     // However, IStoreRegistrationSystem overlaps with IStore if
     // included in IBaseWorld, so it needs to be excluded from worldgen.