Skip to content

Commit f67620e

Browse files
r0qsr0qs
committed
Fix cleanup storage
1 parent 0fa5ac6 commit f67620e

17 files changed

+371
-40
lines changed

‎docs/bugs.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
[
22
{
3-
"uid": "SOL-2025-4",
3+
"uid": "SOL-2025-1",
44
"name": "InconsistentTreatmentOfStorageArraysOnSlotOverflowBoundary",
55
"summary": "Fixed-length storage arrays crossing the 2^256 slot boundary can exhibit unexpected behavior when cleared (using the delete operator) or partially assigned, leading to silent data retention and inconsistent results.",
66
"description": "Large static arrays in storage risk overlapping the 2^256 storage slot boundary. Partial assignments or delete operations may not properly reset all elements in such conditions, causing inconsistency during deletion and unexpected data retention. Although such situations are exceedingly rare in typical contracts, overflow on array deletion become more plausible when arrays are extremely large or the storage is manually positioned close to the storage boundaries. The compiler already warns about potential storage collisions in such scenarios.",

‎libsolidity/codegen/ArrayUtils.cpp

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -290,9 +290,9 @@ void ArrayUtils::copyArrayToStorage(ArrayType const& _targetType, ArrayType cons
290290
_context << Instruction::POP << Instruction::SWAP1 << Instruction::POP;
291291
// stack: target_ref target_data_end target_data_pos_updated
292292
if (targetBaseType->storageBytes() < 32)
293-
utils.clearStorageLoop(TypeProvider::uint256(), false); // TODO: check the boolean
293+
utils.clearStorageLoop(TypeProvider::uint256(), !targetType->isDynamicallySized());
294294
else
295-
utils.clearStorageLoop(targetBaseType, false); // TODO: check the boolean
295+
utils.clearStorageLoop(targetBaseType, !targetType->isDynamicallySized());
296296
_context << Instruction::POP;
297297
}
298298
);
@@ -590,9 +590,9 @@ void ArrayUtils::clearArray(ArrayType const& _typeIn) const
590590
ArrayUtils(_context).convertLengthToSize(_type);
591591
_context << Instruction::ADD << Instruction::SWAP1;
592592
if (_type.baseType()->storageBytes() < 32)
593-
ArrayUtils(_context).clearStorageLoop(TypeProvider::uint256(), !_type.isDynamicallySized()); // TODO: check boolean
593+
ArrayUtils(_context).clearStorageLoop(TypeProvider::uint256(), !_type.isDynamicallySized());
594594
else
595-
ArrayUtils(_context).clearStorageLoop(_type.baseType(), !_type.isDynamicallySized()); // TODO: check boolean
595+
ArrayUtils(_context).clearStorageLoop(_type.baseType(), !_type.isDynamicallySized());
596596
_context << Instruction::POP;
597597
}
598598
solAssert(_context.stackHeight() == stackHeightStart - 2, "");
@@ -631,9 +631,9 @@ void ArrayUtils::clearDynamicArray(ArrayType const& _type) const
631631
<< Instruction::SWAP1;
632632
// stack: data_pos_end data_pos
633633
if (_type.storageStride() < 32)
634-
clearStorageLoop(TypeProvider::uint256(), false); // TODO: check boolean
634+
clearStorageLoop(TypeProvider::uint256(), /* _canOverflow */ false);
635635
else
636-
clearStorageLoop(_type.baseType(), false); // TODO: check boolean
636+
clearStorageLoop(_type.baseType(), /* _canOverflow */ false);
637637
// cleanup
638638
m_context << endTag;
639639
m_context << Instruction::POP;
@@ -771,14 +771,14 @@ void ArrayUtils::popStorageArrayElement(ArrayType const& _type) const
771771
}
772772
}
773773

774-
void ArrayUtils::clearStorageLoop(Type const* _type, bool _assumeEndAfterStart) const
774+
void ArrayUtils::clearStorageLoop(Type const* _type, bool _canOverflow) const
775775
{
776776
solAssert(_type->storageBytes() >= 32, "");
777777
m_context.callLowLevelFunction(
778778
"$clearStorageLoop_" + _type->identifier(),
779779
2,
780780
1,
781-
[_type, _assumeEndAfterStart](CompilerContext& _context)
781+
[_type, _canOverflow](CompilerContext& _context)
782782
{
783783
unsigned stackHeightStart = _context.stackHeight();
784784
if (_type->category() == Type::Category::Mapping)
@@ -794,7 +794,7 @@ void ArrayUtils::clearStorageLoop(Type const* _type, bool _assumeEndAfterStart)
794794
_context <<
795795
Instruction::DUP1 <<
796796
Instruction::DUP3;
797-
if (_assumeEndAfterStart)
797+
if (_canOverflow)
798798
_context << Instruction::EQ;
799799
else
800800
_context << Instruction::GT << Instruction::ISZERO;

‎libsolidity/codegen/ArrayUtils.h

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -73,9 +73,10 @@ class ArrayUtils
7373
/// Stack post:
7474
void popStorageArrayElement(ArrayType const& _type) const;
7575
/// Appends a loop that clears a sequence of storage slots of the given type (excluding end).
76+
/// @param _canOverflow whether the storage is treated as circular when clearing.
7677
/// Stack pre: end_ref start_ref
7778
/// Stack post: end_ref
78-
void clearStorageLoop(Type const* _type, bool _assumeEndAfterStart) const;
79+
void clearStorageLoop(Type const* _type, bool _canOverflow) const;
7980
/// Converts length to size (number of storage slots or calldata/memory bytes).
8081
/// if @a _pad then add padding to multiples of 32 bytes for calldata/memory.
8182
/// Stack pre: length

‎libsolidity/codegen/YulUtilFunctions.cpp

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1431,7 +1431,7 @@ std::string YulUtilFunctions::cleanUpStorageArrayEndFunction(ArrayType const& _t
14311431
)")
14321432
("convertToSize", arrayConvertLengthToSize(_type))
14331433
("dataPosition", arrayDataAreaFunction(_type))
1434-
("clearStorageRange", clearStorageRangeFunction(*_type.baseType(), false))
1434+
("clearStorageRange", clearStorageRangeFunction(*_type.baseType(), !_type.isDynamicallySized()))
14351435
("packed", _type.baseType()->storageBytes() <= 16)
14361436
("itemsPerSlot", std::to_string(32 / _type.baseType()->storageBytes()))
14371437
("storageBytes", std::to_string(_type.baseType()->storageBytes()))
@@ -1483,7 +1483,7 @@ std::string YulUtilFunctions::cleanUpDynamicByteArrayEndSlotsFunction(ArrayType
14831483
)")
14841484
("dataLocation", arrayDataAreaFunction(_type))
14851485
("div32Ceil", divide32CeilFunction())
1486-
("clearStorageRange", clearStorageRangeFunction(*_type.baseType(), false))
1486+
("clearStorageRange", clearStorageRangeFunction(*_type.baseType(), /* _canOverflow */ false))
14871487
.render();
14881488
});
14891489
}
@@ -1523,7 +1523,7 @@ std::string YulUtilFunctions::decreaseByteArraySizeFunction(ArrayType const& _ty
15231523
("functionName", functionName)
15241524
("dataPosition", arrayDataAreaFunction(_type))
15251525
("partialClearStorageSlot", partialClearStorageSlotFunction())
1526-
("clearStorageRange", clearStorageRangeFunction(*_type.baseType(), true))
1526+
("clearStorageRange", clearStorageRangeFunction(*_type.baseType(), /* _canOverflow */ true))
15271527
("transitLongToShort", byteArrayTransitLongToShortFunction(_type))
15281528
("div32Ceil", divide32CeilFunction())
15291529
("encodeUsedSetLen", shortByteArrayEncodeUsedAreaSetLengthFunction())
@@ -1817,7 +1817,7 @@ std::string YulUtilFunctions::partialClearStorageSlotFunction()
18171817
});
18181818
}
18191819

1820-
std::string YulUtilFunctions::clearStorageRangeFunction(Type const& _type, bool _assumeEndAfterStart)
1820+
std::string YulUtilFunctions::clearStorageRangeFunction(Type const& _type, bool _canOverflow)
18211821
{
18221822
if (_type.storageBytes() < 32)
18231823
solAssert(_type.isValueType(), "");
@@ -1834,7 +1834,7 @@ std::string YulUtilFunctions::clearStorageRangeFunction(Type const& _type, bool
18341834
}
18351835
)")
18361836
("functionName", functionName)
1837-
("compare", _assumeEndAfterStart ? "sub" : "lt")
1837+
("compare", _canOverflow ? "sub" : "lt")
18381838
("setToZero", storageSetToZeroFunction(_type.storageBytes() < 32 ? *TypeProvider::uint256() : _type, VariableDeclaration::Location::Unspecified))
18391839
("increment", _type.storageSize().str())
18401840
.render();
@@ -1872,7 +1872,7 @@ std::string YulUtilFunctions::clearStorageArrayFunction(ArrayType const& _type)
18721872
(
18731873
"clearRange",
18741874
_type.baseType()->category() != Type::Category::Mapping ?
1875-
clearStorageRangeFunction((_type.baseType()->storageBytes() < 32) ? *TypeProvider::uint256() : *_type.baseType(), true) :
1875+
clearStorageRangeFunction((_type.baseType()->storageBytes() < 32) ? *TypeProvider::uint256() : *_type.baseType(), /* _canOverflow */ true) :
18761876
""
18771877
)
18781878
("lenToSize", arrayConvertLengthToSize(_type))

‎libsolidity/codegen/YulUtilFunctions.h

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -268,7 +268,8 @@ class YulUtilFunctions
268268
/// @returns the name of a function that will clear the storage area given
269269
/// by the start and end (exclusive) parameters (slots).
270270
/// signature: (start, end)
271-
std::string clearStorageRangeFunction(Type const& _type, bool _assumeEndAfterStart);
271+
/// if _canOverflow is true, it treats the storage as circular and clears by wrapping around.
272+
std::string clearStorageRangeFunction(Type const& _type, bool _canOverflow);
272273

273274
/// @returns the name of a function that will clear the given storage array
274275
/// signature: (slot) ->
@@ -618,7 +619,7 @@ class YulUtilFunctions
618619
std::string cleanUpDynamicByteArrayEndSlotsFunction(ArrayType const& _type);
619620

620621
/// @returns the name of a function that increases size of byte array
621-
/// when we resize byte array frextractUsedSetLenom < 32 elements to >= 32 elements or we push to byte array of size 31 copying of data will occur
622+
/// when we resize byte array from < 32 elements to >= 32 elements or we push to byte array of size 31 copying of data will occur
622623
/// signature: (array, data, oldLen, newLen)
623624
std::string increaseByteArraySizeFunction(ArrayType const& _type);
624625

‎test/libsolidity/semanticTests/array/push/array_push_struct.sol

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,4 +22,4 @@ contract c {
2222
// test() -> 2, 3, 4, 5
2323
// gas irOptimized: 135329
2424
// gas legacy: 147437
25-
// gas legacyOptimized: 146429
25+
// gas legacyOptimized: 148715

‎test/libsolidity/semanticTests/storage/delete_overflow_bug.sol

Lines changed: 0 additions & 19 deletions
This file was deleted.

‎test/libsolidity/semanticTests/storage/storage_boundary_array_assignment.sol

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
contract C {
2+
function getArray() internal pure returns (uint256[10][1] storage _x) {
3+
assembly {
4+
_x.slot := sub(0, 5)
5+
}
6+
}
7+
8+
function assignArray(uint256[10] memory y) public {
9+
uint256[10][1] storage _x = getArray();
10+
_x[0] = y;
11+
}
12+
13+
function x() public view returns (uint256[10] memory) {
14+
return getArray()[0];
15+
}
16+
}
17+
// ----
18+
// x() -> 0, 0, 0, 0, 0, 0, 0, 0, 0, 0
19+
// assignArray(uint256[10]): 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ->
20+
// gas irOptimized: 245236
21+
// gas legacyOptimized: 245441
22+
// x() -> 1, 2, 3, 4, 5, 6, 7, 8, 9, 10
23+
// assignArray(uint256[10]): 10, 20, 30, 40, 50, 60, 70, 80, 90, 100 ->
24+
// x() -> 10, 0x14, 0x1e, 0x28, 0x32, 0x3c, 0x46, 0x50, 0x5a, 0x64
25+
// gas irOptimized: 198025
26+
// gas legacy: 200268
27+
// gas legacyOptimized: 198058

‎test/libsolidity/semanticTests/storage/storage_boundary_array_delete.sol

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
contract C {
2+
function getArray() internal pure returns (uint256[10][1] storage _x) {
3+
assembly {
4+
_x.slot := sub(0, 5)
5+
}
6+
}
7+
8+
function fillArray() public {
9+
uint256[10][1] storage _x = getArray();
10+
for (uint i = 1; i < 10; i++)
11+
_x[0][i] = i;
12+
}
13+
14+
function clearArray() public {
15+
uint256[10][1] storage _x = getArray();
16+
delete _x[0];
17+
}
18+
19+
function x() public view returns (uint256[10] memory) {
20+
return getArray()[0];
21+
}
22+
}
23+
// ----
24+
// x() -> 0, 0, 0, 0, 0, 0, 0, 0, 0, 0
25+
// fillArray()
26+
// gas irOptimized: 220705
27+
// gas legacyOptimized: 220871
28+
// x() -> 0, 1, 2, 3, 4, 5, 6, 7, 8, 9
29+
// clearArray()
30+
// x() -> 0, 0, 0, 0, 0, 0, 0, 0, 0, 0
31+
// gas irOptimized: 181920
32+
// gas legacy: 184143
33+
// gas legacyOptimized: 181899

‎test/libsolidity/semanticTests/storage/storage_boundary_array_delete_overlapping_variable.sol

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
contract C {
2+
uint256 public y = 42;
3+
4+
function getArray() internal pure returns (uint256[10][1] storage _x) {
5+
assembly {
6+
_x.slot := sub(0, 5)
7+
}
8+
}
9+
10+
function fillArray() public {
11+
uint256[10][1] storage _x = getArray();
12+
for (uint i = 1; i < 10; i++)
13+
_x[0][i] = i;
14+
}
15+
16+
function clearArray() public {
17+
uint256[10][1] storage _x = getArray();
18+
delete _x[0];
19+
}
20+
21+
function x() public view returns (uint256[10] memory) {
22+
return getArray()[0];
23+
}
24+
}
25+
26+
// ----
27+
// y() -> 42
28+
// x() -> 0, 0, 0, 0, 0, 0x2a, 0, 0, 0, 0
29+
// fillArray()
30+
// gas irOptimized: 203627
31+
// gas legacyOptimized: 203793
32+
// y() -> 5
33+
// x() -> 0, 1, 2, 3, 4, 5, 6, 7, 8, 9
34+
// clearArray()
35+
// y() -> 0
36+
// x() -> 0, 0, 0, 0, 0, 0, 0, 0, 0, 0
37+
// gas irOptimized: 168243
38+
// gas legacy: 170463
39+
// gas legacyOptimized: 168219

0 commit comments

Comments
 (0)