From ad4ac44594f222fdfeca77e4d262eb47ef735836 Mon Sep 17 00:00:00 2001 From: dk1a Date: Thu, 18 Jan 2024 14:05:27 +0300 Subject: [PATCH] fix(store): add missing FieldLayout and Schema validations [L-07] (#2046) Co-authored-by: Kevin Ingersoll --- .changeset/shaggy-pianos-fetch.md | 5 +++++ docs/pages/store/reference/store.mdx | 16 +++++++++++++-- packages/store/gas-report.json | 10 +++++----- packages/store/src/FieldLayout.sol | 27 ++++++++++++++++++++------ packages/store/src/IStoreErrors.sol | 4 +++- packages/store/src/Schema.sol | 9 ++++++++- packages/store/src/StoreCore.sol | 22 +++++++++++++++++++++ packages/store/test/FieldLayout.t.sol | 4 ++-- packages/world-modules/gas-report.json | 22 ++++++++++----------- packages/world/gas-report.json | 4 ++-- packages/world/mud.config.ts | 2 +- 11 files changed, 94 insertions(+), 31 deletions(-) create mode 100644 .changeset/shaggy-pianos-fetch.md diff --git a/.changeset/shaggy-pianos-fetch.md b/.changeset/shaggy-pianos-fetch.md new file mode 100644 index 0000000000..0061d93089 --- /dev/null +++ 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 index 71551f45df..4feae2286d 100644 --- 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 index a519e817cd..22fbdffdc6 100644 --- 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 index bd1ea91a4d..ae5d66ed26 100644 --- 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 index 5a957a2f4e..d948320be1 100644 --- 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 index cdd4c9e05d..8b06f0858c 100644 --- 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 index 144c2349ae..43613fa7b4 100644 --- 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 index aad53cb73d..87d3c72d4b 100644 --- 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 index e3e5dc4b90..df8aa33a65 100644 --- 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 index b8db9e3bb7..914f87918c 100644 --- 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 index bd15594a38..73a79a1393 100644 --- 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.