Fix unchecked overflow in legacy codegen array size computation#16481
Fix unchecked overflow in legacy codegen array size computation#16481
Conversation
r0qs
force-pushed
the
fix-unchecked-mul-storage-array-clear
branch
2 times, most recently
from
3aefba0 to
8dabc96
Compare
r0qs
force-pushed
the
fix-unchecked-mul-storage-array-clear
branch
from
8dabc96 to
43c59b5
Compare
| "summary": "Deleting a dynamic array of large static arrays could silently skip storage clearing due to an unchecked multiplication overflow in the evmasm code generator.", | ||
| "description": "When the legacy (evmasm) code generator computes the total number of storage slots occupied by an array, it multiplies the array length by the storage size of its base type. This multiplication was performed without an overflow check, so when the product exceeded ``2**256``, the result would wrap to a small value (or zero). This caused the subsequent clearing loop to process fewer slots than necessary, leaving stale data in storage. The bug could be triggered by using the ``delete`` operator on a dynamic storage array whose base type is large enough for the product to overflow. The via-IR pipeline was not affected, because it already used overflow-checked arithmetic for this computation. With the fix, the legacy code generator now reverts with an arithmetic overflow panic in this situation, matching the via-IR behavior.", | ||
| "link": "", | ||
| "introduced": "0.1.0", |
There was a problem hiding this comment.
The oldest version I could test using Remix was v0.1.3+commit.028f561d, but the bug likely dates back to the introduction of convertLengthToSize in https://github.com/argotorg/solidity/pull/1/changes#diff-bda18cdea6a1adc5907560e2625cd6d135e40fe81a48d0d9e04978d5f0e0586eR626.
r0qs
force-pushed
the
fix-unchecked-mul-storage-array-clear
branch
4 times, most recently
from
78e7b41 to
e5d0f63
Compare
r0qs
force-pushed
the
fix-unchecked-mul-storage-array-clear
branch
2 times, most recently
from
4ecbda1 to
bee5416
Compare
clonker
left a comment
clonker
left a comment
There was a problem hiding this comment.
Overall looks good to me. Is there any way to check if 0.1.0 was in fact already affected?
There was a problem hiding this comment.
You could think about changing it a bit so that you push individual elements to the data and show that pushing one element + clear doesn't revert, pushing two + clear reverts.
There was a problem hiding this comment.
The elements are so large that even pushing one will revert though. Just for a different reason (out of gas).
There was a problem hiding this comment.
The elements are so large that even pushing one will revert though. Just for a different reason (out of gas).
| else | ||
| m_context << _arrayType.baseType()->storageSize() << Instruction::MUL; | ||
| { | ||
| m_context << _arrayType.baseType()->storageSize(); | ||
| m_context.callYulFunction( | ||
| m_context.utilFunctions().overflowCheckedIntMulFunction(*TypeProvider::uint256()), | ||
| 2, | ||
| 1 | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
This will affect all uses of convertLengthToSize() with storage arrays. Are all of them covered with tests?
There was a problem hiding this comment.
When fixing such issues, we can't stop at fixing just the case that was pointed out in the report. We should take a wider look and consider what other problems of this kind we might have. I think you even mentioned on one of the calls that you've seen places missing overflow checks. This needs a deeper analysis.
There was a problem hiding this comment.
Definitely. Found the same unchecked multiplication in ArrayUtils::accessIndex:
solidity/libsolidity/codegen/ArrayUtils.cpp
Line 777 in 61baed5
And unlike the delete case, this one also affects the via-IR pipeline
solidity/libsolidity/codegen/YulUtilFunctions.cpp
Line 2431 in 61baed5
The function YulUtilFunctions::storageArrayIndexAccessFunction uses unchecked mul(index, storageSize). This causes silent data corruption when accessing elements at indices where the product overflows.
Fixed in both pipelines. And added a test that uses assembly to set the array length directly, since That test was unnecessary, since I managed to reproduce it without inline assembly. So I replaced the test.push() itself goes through accessIndex() and would trigger the overflow check first.
But since the underline issue is the same, I'm unsure if we should have two separate bug entries or only one. Any thoughts on that?
For now I added only one bug entry but two changelog entries. Depending on the preferred approach, I can split the bugs in two PRs or update the title and description of this PR accordingly.
|
r0qs
force-pushed
the
fix-unchecked-mul-storage-array-clear
branch
9 times, most recently
from
4043584 to
98cb60c
Compare
r0qs
force-pushed
the
fix-unchecked-mul-storage-array-clear
branch
from
98cb60c to
406b888
Compare
github-actions
bot
added
the
stale
r0qs
removed
the
stale
3 participants
This PR fixes an unchecked multiplication overflow in ArrayUtils::convertLengthToSize() that could cause
deleteto silently skip storage clearing whenlength * storageSizewraps modulo2^256, leaving stale data in storage.The correct behavior is to revert (as done in the IR pipeline), because clearing
2^256slots is fundamentally impossible within gas limits. Adeletethat returns success but leaves data uncleared is incorrect.Legacy evmasm codegen now uses overflowCheckedIntMulFunction, matching via-ir behavior.