contracts-bedrock: prevent overflows in ResourceMetering + prevent grief #5064
Conversation
|
tynes
force-pushed
the
feat/prevent-overflows
branch
2 times, most recently
from
0ad9161
to
48d014b
Compare
|
Hey @tynes! This PR has merge conflicts. Please fix them before continuing review. |
tynes
force-pushed
the
feat/prevent-overflows
branch
from
7491d80
to
8557bd0
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## develop #5064 +/- ##
===========================================
- Coverage 41.12% 40.57% -0.56%
===========================================
Files 339 318 -21
Lines 20796 20184 -612
Branches 772 661 -111
===========================================
- Hits 8553 8190 -363
+ Misses 11586 11337 -249
Partials 657 657
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
I've added an additional commit that includes a table test for testing the resource params at a bunch of different values and then it writes the output to csv. It includes the USD cost to buy up deposit gas |
tynes
force-pushed
the
feat/prevent-overflows
branch
from
9c46277
to
b550c7d
Compare
|
I'd like to rebase this on #5101 to get more accurate benchmarks |
|
Hey @tynes! This PR has merge conflicts. Please fix them before continuing review. |
tynes
force-pushed
the
feat/prevent-overflows
branch
from
b550c7d
to
4d50b52
Compare
tynes
changed the title
|
Hey @tynes! This PR has merge conflicts. Please fix them before continuing review. |
When multiplying a uint64 and uint128 and assigning to a uint256, the solidity checked math applies to the types of the numbers being multiplied. This means that solidity catches an overflow when the result of larger than a uint128 even though its assigned to a uint256. This PR uses unchecked math to prevent this kind of overflow instead of casting the values to uint256 because it is safe to use unchecked math here. The following inequality always holds true, which shows that it is safe: ``` solidity type(uint64).max * type(uint128).max < type(uint256).max ```
Remove the unchecked arithmetic in favor of lowering the max base fee. This will prevent situations in which the max base fee causes gas consumption greater than the block gas limit.
tynes
force-pushed
the
feat/prevent-overflows
branch
from
f39d4c0
to
83dbf35
Compare
Description
When multiplying a uint64 and uint128 and assigning to a uint256, the solidity checked math applies to the types of the numbers being multiplied. This means that solidity catches an overflow when the result of larger than a uint128 even though its assigned to a uint256.
This PR uses unchecked math to prevent this kind of overflow instead of casting the values to uint256 because it is safe to use unchecked math here. The following inequality always holds true, which shows that it is safe:
Update:
The above unchecked logic was removed in favor of lowering the max deposit base fee. The values are simply type casted to
uint256when doing the math to prevent overflows instead of doing unchecked math.Test coverage is added for max resource consumption with a max deposit base fee as well as some test coverage for invariants that must be held for the system to work properly, such as the max base fee being larger than the min base fee and the initial base fee being in between the min and the max.
The invariant must be held that the user is able to request the max amount of resources when the base fee is at its max value. If the max base fee is too large, then it will result in more L1 gas being consumed than the L1 block gas limit. Test coverage is added to ensure that the block gas limit can compute a deposit when the max base fee is set.
Update 2:
The max resource amount has been updated to 20 million with the same target resources, meaning that the base fee will ratchet up much more quickly. This keeps the gas prices the same for when we are around the target and they will increase much more aggressively when there is high usage. This will make it much more expensive to continuously frontrun a deposit and buy the max resources, causing it to revert.
The CSV file generated can be found in google sheets here
It is a known issue that the deposit base fee can rise to a point where it is impossible to buy more resources on L2. It should be an invariant that you can buy the max resources no matter what the deposit base fee is, but we cannot address that before going to mainnet. We need to eventually add an alternative deposit path where the L2 gas can be purchased by sending ether instead of with a gas burn. That is too large of a project to take on before mainnet, but that is how the invariant can be held.
The following data was taken from the csv file
The amount of gas burnt is dynamic based on the current L1 base fee to hold user fees constant. You can see that it costs $1600 when the eth price is $1600 to buy the max resource limit when the deposit base fee is 50 gwei. You can see when the L1 base fee is 1 gwei and the user requests 100000 gas, it results in 5000370 gas being used. This is a 50:1 ratio.