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.

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): fix potential memory corruption [M-04] #1978

Merged
merged 5 commits into from
Dec 4, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 6 additions & 0 deletions .changeset/small-brooms-prove.md
Original file line number Diff line number Diff line change
@@ -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
16 changes: 8 additions & 8 deletions packages/store/gas-report.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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",
Expand Down Expand Up @@ -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",
Expand All @@ -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",
Expand Down Expand Up @@ -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",
Expand Down
7 changes: 6 additions & 1 deletion packages/store/src/Storage.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
37 changes: 37 additions & 0 deletions packages/store/test/Storage.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
Loading