Commit b6f4caed authored by Mark Tyneway's avatar Mark Tyneway

contracts-bedrock: prevent overflows

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.
parent 2ffabba6
This diff is collapsed.
This diff is collapsed.
This diff is collapsed.
...@@ -54,11 +54,16 @@ abstract contract ResourceMetering is Initializable { ...@@ -54,11 +54,16 @@ abstract contract ResourceMetering is Initializable {
/** /**
* @notice Maximum base fee value, cannot go higher than this. * @notice Maximum base fee value, cannot go higher than this.
* This value must be small enough to allow a user to request the
* MAX_RESOURCE_LIMIT when the base fee is at its max value.
* Setting this value too large can result in more gas being
* consumed than the L1 block gas limit.
*/ */
int256 public constant MAXIMUM_BASE_FEE = int256(uint256(type(uint128).max)); int256 public constant MAXIMUM_BASE_FEE = int256(uint256(type(uint32).max / 7 * 6));
/** /**
* @notice Initial base fee value. * @notice Initial base fee value. This value must be smaller than the
* MAXIMUM_BASE_FEE.
*/ */
uint128 public constant INITIAL_BASE_FEE = 1_000_000_000; uint128 public constant INITIAL_BASE_FEE = 1_000_000_000;
...@@ -134,14 +139,7 @@ abstract contract ResourceMetering is Initializable { ...@@ -134,14 +139,7 @@ abstract contract ResourceMetering is Initializable {
); );
// Determine the amount of ETH to be paid. // Determine the amount of ETH to be paid.
// Safety: _amount is a uint64 uint256 resourceCost = uint256(_amount) * uint256(params.prevBaseFee);
// params.prevBaseFee is a uint128
// resourceCost is a uint256
// type(uint64).max * type(uint128).max < type(uint256).max
uint256 resourceCost;
unchecked {
resourceCost = uint256(_amount) * uint256(params.prevBaseFee);
}
// We currently charge for this ETH amount as an L1 gas burn, so we convert the ETH amount // We currently charge for this ETH amount as an L1 gas burn, so we convert the ETH amount
// into gas by dividing by the L1 base fee. We assume a minimum base fee of 1 gwei to avoid // into gas by dividing by the L1 base fee. We assume a minimum base fee of 1 gwei to avoid
......
...@@ -34,6 +34,27 @@ contract ResourceMetering_Test is Test { ...@@ -34,6 +34,27 @@ contract ResourceMetering_Test is Test {
initialBlockNum = uint64(block.number); initialBlockNum = uint64(block.number);
} }
/**
* @notice The INITIAL_BASE_FEE must be less than the MAXIMUM_BASE_FEE
* and greater than the MINIMUM_BASE_FEE.
*/
function test_meter_initialBaseFee_succeeds() external {
uint256 max = uint256(meter.MAXIMUM_BASE_FEE());
uint256 min = uint256(meter.MINIMUM_BASE_FEE());
uint256 initial = uint256(meter.INITIAL_BASE_FEE());
assertTrue(max > initial);
assertTrue(min < initial);
}
/**
* @notice The MINIMUM_BASE_FEE must be less than the MAXIMUM_BASE_FEE.
*/
function test_meter_minBaseFeeLessThanMaxBaseFee_succeeds() external {
uint256 max = uint256(meter.MAXIMUM_BASE_FEE());
uint256 min = uint256(meter.MINIMUM_BASE_FEE());
assertTrue(max > min);
}
function test_meter_initialResourceParams_succeeds() external { function test_meter_initialResourceParams_succeeds() external {
(uint128 prevBaseFee, uint64 prevBoughtGas, uint64 prevBlockNum) = meter.params(); (uint128 prevBaseFee, uint64 prevBoughtGas, uint64 prevBlockNum) = meter.params();
...@@ -123,8 +144,10 @@ contract ResourceMetering_Test is Test { ...@@ -123,8 +144,10 @@ contract ResourceMetering_Test is Test {
* multiplying against a uint64 _amount can result in an overflow * multiplying against a uint64 _amount can result in an overflow
* even though its assigning to a uint256. The values MUST be casted * even though its assigning to a uint256. The values MUST be casted
* to uint256 when doing the multiplication to prevent overflows. * to uint256 when doing the multiplication to prevent overflows.
* The function is called with the L1 block gas limit to ensure that
* the MAX_RESOURCE_LIMIT can be consumed at the MAXIMUM_BASE_FEE.
*/ */
function test_meter_useMaxWithMaxBaseFee_success() external { function test_meter_useMaxWithMaxBaseFee_succeeds() external {
uint128 _prevBaseFee = uint128(uint256(meter.MAXIMUM_BASE_FEE())); uint128 _prevBaseFee = uint128(uint256(meter.MAXIMUM_BASE_FEE()));
uint64 _prevBoughtGas = 0; uint64 _prevBoughtGas = 0;
uint64 _prevBlockNum = uint64(block.number); uint64 _prevBlockNum = uint64(block.number);
...@@ -141,7 +164,7 @@ contract ResourceMetering_Test is Test { ...@@ -141,7 +164,7 @@ contract ResourceMetering_Test is Test {
assertEq(prevBlockNum, _prevBlockNum); assertEq(prevBlockNum, _prevBlockNum);
uint64 gasRequested = uint64(uint256(meter.MAX_RESOURCE_LIMIT())); uint64 gasRequested = uint64(uint256(meter.MAX_RESOURCE_LIMIT()));
meter.use(gasRequested); meter.use{ gas: 30_000_000 }(gasRequested);
} }
// Demonstrates that the resource metering arithmetic can tolerate very large gaps between // Demonstrates that the resource metering arithmetic can tolerate very large gaps between
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment