diff --git a/src/StdStorage.sol b/src/StdStorage.sol index e21dba04..d169d48a 100644 --- a/src/StdStorage.sol +++ b/src/StdStorage.sol @@ -211,7 +211,16 @@ library stdStorageSafe { // Check that value between found offsets is equal to the current call result uint256 curVal = (uint256(prev) & getMaskByOffsets(offsetLeft, offsetRight)) >> offsetRight; - if (!shortBytesFound && uint256(callData.result) != curVal) { + // A getter whose return type is a signed integer narrower than 256 bits + // ABI-encodes its value sign-extended, while storage holds only the field's own + // bits, so a negative value never matches the slot that holds it. Compare the + // return truncated to the field's width. For a full-width field the mask is all + // ones and this is a no-op. + if ( + !shortBytesFound + && (uint256(callData.result) & (getMaskByOffsets(offsetLeft, offsetRight) >> offsetRight)) + != curVal + ) { continue; } @@ -320,8 +329,19 @@ library stdStorageSafe { } /// @notice Reads the found storage slot value as int256. + /// @dev A field narrower than 256 bits is stored as its own bits only, so the value is + /// sign-extended back to `int256` from the field's width. Full-width fields are unchanged. function read_int(StdStorage storage self) internal returns (int256) { - return abi.decode(_read(self), (int256)); + FindData storage data = find(self, false); + uint256 offsetLeft = data.offsetLeft; + uint256 offsetRight = data.offsetRight; + uint256 value = (uint256(vm.load(self._target, bytes32(data.slot))) & getMaskByOffsets(offsetLeft, offsetRight)) + >> offsetRight; + clear(self); + + uint256 shift = offsetLeft + offsetRight; + if (shift == 0) return int256(value); + return (int256(value) << shift) >> shift; } /// @notice Returns the parent mapping slot index and the key used to reach the found slot. @@ -520,10 +540,19 @@ library stdStorage { find(self, false); } FindData storage data = self.finds[who][fsig][keccak256(abi.encodePacked(params, field_depth))]; + uint256 valueToStore = uint256(set); if ((data.offsetLeft + data.offsetRight) > 0) { - uint256 maxVal = 2 ** (256 - (data.offsetLeft + data.offsetRight)); + uint256 width = 256 - (data.offsetLeft + data.offsetRight); + uint256 maxVal = 2 ** width; + // `checked_write_int` sign-extends a negative value to 256 bits, which does not fit + // the field even when the number it represents does. Narrow it back when the whole + // extension is consistent; the getter still returns the sign-extended form, so the + // verification below is unaffected. + if (valueToStore >= maxVal && (int256(valueToStore) >> (width - 1)) == -1) { + valueToStore &= maxVal - 1; + } require( - uint256(set) < maxVal, + valueToStore < maxVal, string( abi.encodePacked( "stdStorage checked_write(StdStorage): Packed slot. We can't fit value greater than ", @@ -533,7 +562,7 @@ library stdStorage { ); } bytes32 curVal = vm.load(who, bytes32(data.slot)); - bytes32 valToSet = stdStorageSafe.getUpdatedSlotValue(curVal, uint256(set), data.offsetLeft, data.offsetRight); + bytes32 valToSet = stdStorageSafe.getUpdatedSlotValue(curVal, valueToStore, data.offsetLeft, data.offsetRight); vm.store(who, bytes32(data.slot), valToSet); diff --git a/test/StdStorage.t.sol b/test/StdStorage.t.sol index e7172a3a..d11b4d16 100644 --- a/test/StdStorage.t.sol +++ b/test/StdStorage.t.sol @@ -296,6 +296,31 @@ contract StdStorageTest is Test { assertEq(val, type(int256).min); } + // A getter returning a signed type narrower than 256 bits ABI-encodes the value + // sign-extended, while the slot holds only the field's own bits. Before this was handled, + // `find` matched neither and reverted with "Slot(s) not found." + function test_StorageReadIntPackedNegative() public { + int256 val = stdstore.enable_packed_slots().target(address(test)).sig(test.tJ.selector).read_int(); + assertEq(val, -5); + } + + function test_StorageReadIntPackedPositiveSibling() public { + uint256 val = stdstore.enable_packed_slots().target(address(test)).sig(test.tK.selector).read_uint(); + assertEq(val, 7); + } + + function test_StorageReadIntSoloNegative() public { + int256 val = stdstore.enable_packed_slots().target(address(test)).sig(test.tSolo.selector).read_int(); + assertEq(val, -1); + } + + function test_StorageWriteIntPackedNegative() public { + stdstore.enable_packed_slots().target(address(test)).sig(test.tJ.selector).checked_write_int(-42); + assertEq(test.tJ(), -42); + // the neighbour sharing the slot is untouched + assertEq(test.tK(), 7); + } + function testFuzz_Packed(uint256 val, uint8 elemToGet) public { // This function tries an assortment of packed slots, shifts meaning number of elements // that are packed. Shiftsizes are the size of each element, i.e. 8 means a data type that is 8 bits, 16 == 16 bits, etc. @@ -533,6 +558,14 @@ contract StorageTest { // Array with length matching values of elements. uint256[] public edgeCaseArray = [3, 3, 3]; + // Signed fields narrower than 256 bits. `tJ`/`tK` share a slot; `tSolo` is pushed into a + // slot of its own by the `uint256` that follows it. + int64 public tJ = -5; + uint64 public tK = 7; + + int8 public tSolo = -1; + uint256 public tAfterSolo = 1; + constructor() { basic = UnpackedStruct({a: 1337, b: 1337});