Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 5 commits into from
Jan 18, 2024
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f8c511f
fix(store): emit custom erro if slice bound is invalid
dk1a Dec 6, 2023
af40ae8
Create perfect-windows-reply.md
dk1a Dec 6, 2023
cc67c79
Update .changeset/perfect-windows-reply.md
holic Jan 16, 2024
758bcc1
Merge remote-tracking branch 'origin/main' into dk1a/L-10
holic Jan 16, 2024
8bcee1c
Merge remote-tracking branch 'origin/main' into dk1a/L-10
holic Jan 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/perfect-windows-reply.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@latticexyz/store": patch
---

Added a custom error `Store_IndexOutOfBounds` for when the `start:end` slice in `getDynamicFieldSlice` is invalid (it used to revert with the default overflow error)
holic marked this conversation as resolved.
Show resolved Hide resolved
6 changes: 6 additions & 0 deletions docs/pages/store/reference/store.mdx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,12 @@ error Store_InvalidResourceType(bytes2 expected, ResourceId resourceId, string r
error Store_InvalidDynamicDataLength(uint256 expected, uint256 received);
```

### Store_InvalidBounds

```solidity
error Store_InvalidBounds(uint256 start, uint256 end);
```

### Store_IndexOutOfBounds

```solidity
Expand Down
10 changes: 5 additions & 5 deletions packages/store/gas-report.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -621,25 +621,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",
Expand Down Expand Up @@ -1059,7 +1059,7 @@
"file": "test/StoreHooksColdLoad.t.sol",
"test": "testGetItem",
"name": "StoreHooks: get 1 element (cold)",
"gasUsed": 8486
"gasUsed": 8446
},
{
"file": "test/StoreHooksColdLoad.t.sol",
Expand Down
1 change: 1 addition & 0 deletions packages/store/src/IStoreErrors.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ interface IStoreErrors {
error Store_InvalidResourceType(bytes2 expected, ResourceId resourceId, string resourceIdString);

error Store_InvalidDynamicDataLength(uint256 expected, uint256 received);
error Store_InvalidBounds(uint256 start, uint256 end);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thoughts on Store_InvalidSlice or Store_InvalidSliceBounds?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from @alvrs

no strong opinions, i'm happy with InvalidBounds, or InvalidSliceBounds (think "bounds" in there is nice for consistency with the "out of bounds" error)

error Store_IndexOutOfBounds(uint256 length, uint256 accessedIndex);
error Store_InvalidKeyNamesLength(uint256 expected, uint256 received);
error Store_InvalidFieldNamesLength(uint256 expected, uint256 received);
Expand Down
8 changes: 7 additions & 1 deletion packages/store/src/StoreCore.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -905,6 +905,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.
Expand All @@ -917,7 +921,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, length: end - start, offset: start });
unchecked {
return Storage.load({ storagePointer: location, length: end - start, offset: start });
}
}
}

Expand Down
22 changes: 11 additions & 11 deletions packages/world-modules/gas-report.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@
"file": "test/KeysInTableModule.t.sol",
"test": "testSetAndDeleteRecordHookCompositeGas",
"name": "delete a composite record on a table with keysInTableModule installed",
"gasUsed": 155729
"gasUsed": 155609
},
{
"file": "test/KeysInTableModule.t.sol",
Expand All @@ -129,7 +129,7 @@
"file": "test/KeysInTableModule.t.sol",
"test": "testSetAndDeleteRecordHookGas",
"name": "delete a record on a table with keysInTableModule installed",
"gasUsed": 84971
"gasUsed": 84931
},
{
"file": "test/KeysWithValueModule.t.sol",
Expand Down Expand Up @@ -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",
Expand All @@ -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",
Expand All @@ -261,7 +261,7 @@
"file": "test/query.t.sol",
"test": "testNotValueQuery",
"name": "NotValueQuery",
"gasUsed": 46944
"gasUsed": 46824
},
{
"file": "test/StandardDelegationsModule.t.sol",
Expand Down
Loading